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

pybind11 incorrectly adds "-fvisibility" flag to nvcc #1710

Closed
supervacuus opened this issue Feb 25, 2019 · 5 comments · Fixed by #2338
Closed

pybind11 incorrectly adds "-fvisibility" flag to nvcc #1710

supervacuus opened this issue Feb 25, 2019 · 5 comments · Fixed by #2338

Comments

@supervacuus
Copy link

supervacuus commented Feb 25, 2019

Since nvcc is a compiler driver and not a compiler in itself, it doesn't accept flags like -fvisibility directly, but passes them on via phase-options like -Xcompiler=. If you pass the flag directly, nvcc will return with an error (nvcc fatal : Unknown option 'fvisibility').

When you set up a typical pybind11 project that uses a mix of C++ and CUDA with

cmake_minimum_required(VERSION 3.9)
project(py_cuda LANGUAGES CXX CUDA)
add_subdirectory(pybind11)
pybind11_add_module(...)

and the module consists of C++ (cpp/h) and CUDA files (cu/cuh), nvcc will return the error mentioned above during the build.

I know that pybind11 doesn't officially support nvcc, but the fix for this issue probably very small. The culprit for the behaviour is the following line: https://github.com/pybind/pybind11/blob/master/CMakeLists.txt#L108

target_compile_options doesn't discriminate on the language being compiled or the compiler in use, which is probably why you placed it inside NOT MSVC condition. Interestingly pybind11_add_module already implements this correctly using set_target_properties and the respective target-property: https://github.com/pybind/pybind11/blob/master/tools/pybind11Tools.cmake#L158

Maybe those approaches could be merged? I would simply replace the line in the CMakeLists.txt with

set_target_properties(module PROPERTIES CXX_VISIBILITY_PRESET "hidden")
set_target_properties(module PROPERTIES CUDA_VISIBILITY_PRESET "hidden")

Am I missing something?

This happens in the following environment:
cmake version 3.10.2
The CXX compiler identification is GNU 7.3.0
The CUDA compiler identification is NVIDIA 9.1.85

But i think it will happen in all Linux environments and maybe even on MacOS.

@eacousineau
Copy link
Contributor

eacousineau commented Mar 8, 2019

That approach seems correct, but, er, dumb question: Why do want to compile your bindings with nvcc, vs. just compiling your core algorithms in nvcc, but then compiling your bindings in a normal C++ compiler?
(This is what some of my coworkers do.)

@supervacuus
Copy link
Author

I am very much in agreement. This scenario is from a project I didn't set up myself and I suggested the same thing. But since the delta to make this work is so small, i thought i'd ask if there is an aspect that i didn't really consider.

@carterbox
Copy link

carterbox commented Nov 5, 2019

I had a similar problem with -flto.

I found it non-trivial to figure out how to do compile the CUDA and CXX parts separately and combine them, so I would appreciate it if this was made easier. The problems I was having were related to the fact that successfully statically linking the CUDA part to the CXX parts requires setting some target properties. I found these on stack overflow. Otherwise, dynamically linking the two parts is easy, but then you have to make sure the cuda part is located by the linker at runtime.

@bstaletic
Copy link
Collaborator

@YannickJadoul
Copy link
Collaborator

Nvidia's docs have this section that might be interesting.

https://docs.nvidia.com/cuda/cuda-compiler-driver-nvcc/index.html#options-for-guiding-compiler-driver-forward-host-compiler

Interesting, indeed. Does that mean we could add --forward-unknown-to-host-linker to the CMake target's COMPILE_OPTIONS property, before adding all the rest?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants