Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not compile helpers with extension by default. #310

Open
fangerer opened this issue Apr 7, 2022 · 4 comments
Open

Do not compile helpers with extension by default. #310

fangerer opened this issue Apr 7, 2022 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@fangerer
Copy link
Contributor

fangerer commented Apr 7, 2022

Right now, we are adding source files argparse.c, buildvalue.c, and helpers.c to the source file list of the HPy extension to be built and then they are compiled together with the extension's sources.

We do so because the goal is that the helpers binary code should be in the HPy extension's library.
While this works in most cases, we recently discovered a problem with that strategy on Mac (using LLVM toolchain). The extension was written in C++ and added:

extra_compile_args=['-std=c++11', '-stdlib=libc++']
extra_link_args=['-stdlib=libc++']

The extension failed to build argparse.c with following error:

gcc -Wno-unused-result -Wsign-compare -g -O0 -Wall -DHPY -DHPY_UNIVERSAL_ABI -I. -I/path/to/lib/python3.8/site-packages/hpy/devel/include -I/path/to/include -I/Users/fa/work/repos/Python-3.8.5/Include -I/Users/fa/work/repos/Python-3.8.5 -c /path/to/lib/python3.8/site-packages/hpy/devel/src/runtime/argparse.c -o build/temp.macosx-12.0-x86_64-3.8-pydebug/path/to/lib/python3.8/site-packages/hpy/devel/src/runtime/argparse.o -std=c++11 -stdlib=libc++
error: invalid argument '-std=c++11' not allowed with 'C'
error: command 'gcc' failed with exit status 1

IMO, the underlying issue is that we are building these sources with the flags of the target. I think we should build them in the same way as we do with the rest of HPy. So, I suggest that we build an archive for the helpers and statically link them into the extension.

Optionally, if someone really wants to compile the helpers with the extension, we can provide an option to do so.

@fangerer fangerer added the bug Something isn't working label Apr 7, 2022
@fangerer fangerer self-assigned this Apr 7, 2022
@hodgestar
Copy link
Contributor

Would adding

    #ifdef __cplusplus
    extern "C" {
    #endif

blocks to the relevant .c files help at all?

@fangerer
Copy link
Contributor Author

fangerer commented Apr 7, 2022

Unfortunately, this does not help (just tried it out).

@hodgestar
Copy link
Contributor

Unfortunately, this does not help (just tried it out).

Pity! Thanks for trying it out.

@steve-s
Copy link
Contributor

steve-s commented Apr 7, 2022

We should perhaps remove the extern "C" blocks in ctx_*.c files. I searched around and afaics it just disables names mangling, but the C++ semantics stays the same. I'd say that compiling those files with C++ compiler is just not supported. We should try to keep the header files, especially the inline functions, in some common subset of C and C++ if possible though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants