[BUG] pybind11_add_module forcing -fvisiblity=hidden is a bit too aggressive?

[EricCousineau-TRI]

Issue description

In 97aa54fefa, -fvisibility=hidden was introduced to pybind11_add_module as a means to effectively avoid warnings (by means of setting CXX_VISIBILITY_PRESET to "hidden").

I ran into this on our downstream project (which uses a pybind11 fork) here:
RobotLocomotion/drake#14047
In the fork, I had previously stripped out that CMake code, then restored it when merging in the changes from more active development. (In this issue, I've provided the workaround, which is to re-set the visibility to "default".)

The long and short is, we (unfortunately) have very template-heavy public API and bindings, and setting -fvisibility=hidden in downstream units mean that some of the bound symbols may get new RTTI information, which we don't want in our project.

From reading the comments in the commit (and the surround commit on current master), it looks like this preset isn't really necessary; it's really only to suppress errors in user's code who aren't sufficiently careful to curate their symbol visibility.

That's fine, but I would still love to not have to reverse that in code and try to track changes.

Reproducible example code

See downstream issue.

Possible solutions

From most conservative (keep non-expert users happy) to least conservative (require users be cognizant):

1. No change. Maybe add a docstring to pybind11_add_module talking about templates, RTTI, and visibility with bindings, and mention the workaround (which is now coupled to internal implementation details, albeit publicly documented).
2. Add a flag to pybind11_add_module to not foist hidden visibility on downstream code (instantiations).
3. Remove the flag. Update the docs to tell users to add it themselves (not super suggested) or add more explicit visibility annotations (heavily suggested).

@rwgk @YannickJadoul @bstaletic @wjakob Thoughts?

[EricCousineau-TRI]

Hm... Thinking about this more, option (3) may be a bit hypocritical on my part given that we don't explicitly annotate public visibility on those templates 😅

So perhaps (2) is the best for us (though I guess it's really to enable laziness), and (1) is still the simplest.

[wjakob]

We used to enforce "-fvisibility=hidden" globally via the pybind11_add_module(). Then, at some point, we added it to the pybind11 namespace, which basically resolved the binary size problem and removed the need for a global flag. I don't remember all the historical context why it had to be added back to pybind11_add_module, but it sounds like there were some tricky edge cases related to user code rather than pybind11 code bloat.

(2) would be fine with me.

[henryiii]

Can't you just as easily unset it?

pybind11_add_module(stuff files.cpp)
set_property(TARGET stuff PROPERTY CXX_VISIBILITY_PRESET "") # or whatever the default is

Then you don't need a new flag. And this is one of the benefits of using properties instead of piling flags on.

As a minor improvement, we could respect CMAKE_CXX_VISIBILITY_PRESET if that is set, and only add this if that is not set? That's what we do for INTERPROCEDURAL_OPTIMIZATION.

[henryiii]

Also, "advanced" users should be able to use the targets and helper function(s) - you have a lot more control that way.

[YannickJadoul]

I'd also argue to try and keep the default way of using pybind11 (which we describe in the docs) the one that's most likely to be correct for the standard user. (It also would not be a backwards compatible change, right?)

@henryiii's suggestion seems to suggest that adding an option to pybind11_add_module might not be much more economic than just the suggested set_property line? So I'd be inclined to follow that reasoning (but I'm also rather neutral about this), and maybe add this line to the docs. Adding a few extra sentences to the docs about the dangers does seem like a good plan, anyway,

[sarnold]

This issue also affects cythonized extension modules and breaks importing the built module with the following error:

Python 3.8.7rc1 (default, Dec 15 2020, 20:13:07) 
[GCC 10.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import re2
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: dynamic module does not define module export function (PyInit_re2)
>>> 

If I change the flag to -fvisibility=default (or just remove it) the module loads and runs fine, ie, all the tests pass, consumer python packages work properly, etc.

When I finally figured out why that ^^ error was happening, I did try the VISIBILITY_PRESET trick (among others) but nothing worked in my case except not linking to pybind11::module at all (at least on Linux). On macos I get a giant spew of missing symbols if I don't link to the same module.

At the least I think it really needs a well-documented flag to un-force setting -fvisibility=hidden when needed.

[YannickJadoul]

@sarnold Something else must be wrong. pybind11's PyInit_... gets annotated with PYBIND11_EXPORT:

#if !defined(PYBIND11_EXPORT)
#  if defined(WIN32) || defined(_WIN32)
#    define PYBIND11_EXPORT __declspec(dllexport)
#  else
#    define PYBIND11_EXPORT __attribute__ ((visibility("default")))
#  endif
#endif

So I'm not sure what you're doing that somehow hides this symbol? If it's a Cython library that produces PyInit_re2, then why are you compiling it with pybind11's CMake recipe?

Meanwhile, not hiding these symbols could be quite tricky when it comes to libraries interacting, and ODR.

[sarnold]

It's a fork of the original converted to cmake, originally based on the cmake_example repo, and it uses pybind11 for the cmake python/extension/linker helpers on all 3 OS platforms (mainly trying to make it cleaner/compatible with all 3). Maybe not the primary use case, but if you're telling me not to do that, then that should probably also be clearly documented up front. Are you saying I should not use pybind11 at all?

The branch I'm working on is here: https://github.com/freepn/py-re2/tree/pybind11

Sorry I can't really answer your question since I am not the author of the extension code, or else I would file a bug with more detail...

[bstaletic]

@sarnold What you're doing is fine. The only problem is that, by circumbenting the PYBIND11_MODULE macro, you've taken the responsibility of managing the visibility of PyInit_re2. Usually, PYBIND11_MODULE would use PYBIND11_PLUGIN_IMPL and declare the init function with the appropriate visibility. If Cython is generating init functions with the assumption that the visibility of symbols on unix systems is default (which is the wrong default), it really sounds like a bug in Cython.

[YannickJadoul]

Well, I don't think it's that weird that pybind11's build system and instructions are crafted to build pybind11 extensions? As far as I can see, that project is not using pybind11 anywhere (the library, which is the core of the project, not the build system).
Of course, if the build system is useful for other projects, that's great, but pybind11 nowhere claims that this will build any CMake Python project, and I don't believe it was designed with that goal, so expecting things to just work out of the box is maybe slightly unrealistic. Why not just use CMake's generic FindPython and its Python_add_library(... MODULE ...)?

[henryiii]

Just to second that, FindPython is very reasonable, and very similar to our tools (in fact, we pretty much can "just use it" and don't do any workarounds to speak of when you FindPython first). You need at least 3.18.2+, but with pyproject.toml, you can ensure this, or for a non-proper Python project, you can copy in FindPython* from CMake, it's designed to work standalone with CMake 3.7 (at least last I checked).

Cython should probably fix their visibility issues, as setting this is not unreasonable at all.

Finally, I don't think we ever added the ability to ignore adding this flag if CMAKE_CXX_VISIBILITY_PRESET is set. That probably would be consistent and sensible.

I did try the VISIBILITY_PRESET trick (among others) but nothing worked in my case

Do you have an example PR or something that shows what you tried? This really should work?