wlav / cppyy

Other
391 stars 40 forks source link

Error while loading library using regex in cppyy==2.4.2 #124

Closed Gabrielcarvfer closed 1 year ago

Gabrielcarvfer commented 1 year ago

Hi. I was retesting ns-3 with cppyy==2.4.2 and found an issue with one of our libraries that use regex.

Trace ``` manylinux-pip-wheel-py3.10 > int CppyyLegacy::TSystem::Load(const char* module, const char* entry = "", CppyyLegacy::Bool_t system = kFALSE) => manylinux-pip-wheel-py3.10 > regex_error: libns3-dev-topology-read-optimized.so manylinux-pip-wheel-py3.10 > TClass::GetClass: Header Parsing - The representation of std::regex_error was not found in the type system. A lookup in the interpreter is about to be tried: this can cause parsing. This can be avoided selecting std::regex_error in the linkdef/selection file. manylinux-pip-wheel-py3.10 > TClass::GetClass: Header Parsing - The representation of std::runtime_error was not found in the type system. A lookup in the interpreter is about to be tried: this can cause parsing. This can be avoided selecting std::runtime_error in the linkdef/selection file. manylinux-pip-wheel-py3.10 > TClass::GetClass: Header Parsing - The representation of std::regex_constants was not found in the type system. A lookup in the interpreter is about to be tried: this can cause parsing. This can be avoided selecting std::regex_constants in the linkdef/selection file. ```

Based on the trace above, I already figured it has something to do with the following regex.

test.cc ``` //test.cc #include "test.h" #define START "^" /// End of a line #define END "$" /// One or more spaces #define SPACE "[ \t]+" /// Zero or more spaces #define MAYSPACE "[ \t]*" /// Regex expression matching a MAP line #define ROCKETFUEL_MAPS_LINE \ START "(-*[0-9]+)" SPACE "(@[?A-Za-z0-9,+-]+)" SPACE "(\\+)*" MAYSPACE "(bb)*" MAYSPACE \ "\\(([0-9]+)\\)" SPACE "(&[0-9]+)*" MAYSPACE "->" MAYSPACE "(<[0-9 \t<>]+>)*" MAYSPACE \ "(\\{-[0-9\\{\\} \t-]+\\})*" SPACE "=([A-Za-z0-9.!-]+)" SPACE "r([0-9])" MAYSPACE END std::regex rocketfuel_map_regex = std::regex(ROCKETFUEL_MAPS_LINE, std::regex::extended); ```
test.h ``` //test.h #include extern std::regex rocketfuel_map_regex; ```

Compiling it with g++ -o test.so test.cc test.h -fpic -shared then loading and calling it from cppyy

import cppyy
cppyy.add_include_path("./")
cppyy.load_library("test.so")

cppyy.cppdef("""
#include<test.h>
int main(){
    std::string test_string = "test_string";
    std::smatch matches;
    auto ret = std::regex_match(test_string, matches, rocketfuel_map_regex);
    return 0;
}
""")
test = cppyy.gbl.main()

Not working with: cppyy==2.4.2 cppyy-backend==1.14.10 cppyy-cling==6.27.1 CPyCppyy==1.12.12

cppyy.gbl.std.regex_error: int CppyyLegacy::TSystem::Load(const char module, const char entry = "", CppyyLegacy::Bool_t system = kFALSE) => regex_error: Unexpected escape character.

Worked just fine with: cppyy==2.4.1 cppyy-backend==1.14.9 cppyy-cling==6.27.0 CPyCppyy==1.12.11

Process finished with exit code 0

wlav commented 1 year ago

I'm afraid that 2.4.2 is correct, though. If I compile that main() as a C++ program with g++, I receive the same exception. (No idea why there's no exception in 2.4.1.)

Gabrielcarvfer commented 1 year ago

Weird. We test this piece of code with several compilers and it works in every single one of them. Just tested locally, and works just fine.

wlav commented 1 year ago

I have gcc 9.4.0 currently. It also fails on cppyy 3.0.0 (Clang13). It works on Mac with clang 10. Let me dig a bit more ... maybe something in the compiler flags.

Gabrielcarvfer commented 1 year ago

I have gcc 11.13, but we test everything (including this regex code) from gcc 8 to 12, clang from 6 to 14, Windows (MinGW) and Macs (AppleClang).

wlav commented 1 year ago

According to gdb, the exception comes from the construction of the global in test.so. I confirmed that also for the python case, the whole main() program is unnecessary: just loading the test.so library raises the exception.

wlav commented 1 year ago

Difference, for gcc anyway, is addition of the --std=c++17 flag. It throws if the flag is present, not if not.

wlav commented 1 year ago

Hmm, only now see that above you already had int CppyyLegacy::TSystem::Load(const char* module, const char* entry = "", CppyyLegacy::Bool_t system = kFALSE), so in your example, too, the exception originates from the loading of the shared library (and hence on construction of the global variable, as that's the only code that executes), i.e. no cppyy involved yet when the exception occurs.

Gabrielcarvfer commented 1 year ago

Yes. I was not sure if it was due to something in cppyy or cling. I believed it had to do something with cling, until you managed to reproduce the issue locally.

We build by default with c++17, so it can't be the issue. I didn't manage to reproduce the issue locally, nor in containers, only when loading with cppyy==2.4.3.

I'm going to try a bit more and try to isolate it a bit further. Sorry to bother you.

wlav commented 1 year ago

Is fine; I was curious myself. Turns out that the standard C++ headers on Linux have a preprocessor flag __STRICT_ANSI__ that controls whether or not to throw said exception. If that flag is defined, then throw. If not then not (although to be more precise, there are 2 locations where that exception can be thrown and only one of them, with gcc9.4 anyway, is protected by the flag, but the protected one is the cause here).

Gabrielcarvfer commented 1 year ago

Oh, I see. Thanks for the help. I'm going to add a new test job with strict c++ to catch these. I'm going to retry cppyy==2.4.2 and then close this issue. :smile:

Sidetracking: is it be possible to set __python_owns__=False in a template proxy? This proxy always returns a refcounted smart pointer that we want to cleanup from C++.

Gabrielcarvfer commented 1 year ago

It really was my fault. My bad. Closing this.

wlav commented 1 year ago

For templates, the design as-is doesn't have any connection between the factory that instantiates them and the instances, because there isn't any in C++. It's not impossible to add that (this is the 2nd use case asking for it), but it's a bit of a project.

As for smart pointers, there's some machinery to deal with them directly. I remember this report: https://github.com/wlav/cppyy/issues/31

All that said, I don't understand why setting __python_owns__=False is the right approach here: the ownership is on the ref-counted smart pointer, not on the underlying C++ object. I'd figure Python should always own its copies?

Gabrielcarvfer commented 1 year ago

Sorry for the late response.

That's a really good question. I just remember having trouble with objects being destroyed in a weird order and causing issues (e.g. use-after-free, double free, not breaking a reference cycle, etc). Address sanitizer was my saviour.

Some of these were totally our fault and got fixed (people trust valgrind more than they should...). Others we just worked around and seem to be working just fine.

Sorry for bothering again. And thanks for your help.