Visibility hidden #995
Visibility hidden #995
Conversation
There was a problem hiding this comment.
This needs one more thing before merging--the FAQ entry talking about hidden visibility seems largely obsolete with these changes. Should we shorten it? Amend it? Drop it?
The one about small binaries? I'd leave it as is. It's very informative. What would be nice is a new FAQ entry about the possible visibility warning and how to resolve it.
| @@ -12,7 +12,7 @@ | |||
|
|
|||
| #include "cast.h" | |||
|
|
|||
| NAMESPACE_BEGIN(pybind11) | |||
| NAMESPACE_BEGIN(PYBIND11_NAMESPACE) | |||
There was a problem hiding this comment.
Wouldn't it be simpler to add the visibility attribute to the NAMESPACE_BEGIN macro instead of introducing a new one?
There was a problem hiding this comment.
I'm against that, for selfish reasons :). I have several other projects that use the NAMESPACE_BEGIN macro, where that behavior may not be intended.
There was a problem hiding this comment.
Ah, I guess that's why NAMESPACE_BEGIN/END are the only macros not prefixed with PYBIND11 :)
| # py::module_local). We force it on everything inside the `pybind11` | ||
| # namespace; also turning it on for a pybind module compilation here avoids | ||
| # potential warnings or issues from having mixed hidden/non-hidden types. | ||
| set_target_properties(${target_name} PROPERTIES CXX_VISIBILITY_PRESET "hidden") |
docs/advanced/misc.rst
Outdated
| accessed by multiple extension modules: | ||
| Note that pybind11 code compiled with hidden-by-default symbol visibility (e.g. | ||
| via the command line flag ``-fvisibility=hidden`` on GCC/Clang), which is | ||
| strongly recommended for pybind11 modules, can interfere with the ability to |
There was a problem hiding this comment.
I guess it's required, not just recommended anymore. Maybe simplify to something along the lines of: "By default, pybind11 modules are compiled with hidden symbol visibility (see :ref:...), which can interfere..."
docs/advanced/misc.rst
Outdated
| via the command line flag ``-fvisibility=hidden`` on GCC/Clang), which is | ||
| strongly recommended for pybind11 modules, can interfere with the ability to | ||
| access types defined in another extension module. Working around this requires | ||
| manually export types that are accessed by multiple extension modules; pybind11 |
| symbol names and duplication of code in different translation units. It | ||
| sets default visibility to *hidden*, which is required for some pybind11 | ||
| features and functionality when attempting to load multiple pybind11 modules | ||
| compiled under different pybind11 versions. It also adds additional flags |
There was a problem hiding this comment.
Weird question, but: Why two spaces after a period?
There was a problem hiding this comment.
In monospace fonts, it is visually closer to, e.g., a TeX inter-sentence space. It's also consistent with what you get from tools like man when used in a terminal. And while I do think it looks a bit better, it's mainly habit: I do it without even thinking about it.
There was a problem hiding this comment.
Also (I just noticed) vim does this automatically when rewrapping (e.g. if a line ends with a .). I'd be quite surprised if there isn't an option to control that, but it isn't something I turned on specifically.
docs/compiling.rst
Outdated
| (``-fvisibility=hidden``) and .OBJ files with many sections on Visual Studio | ||
| (``/bigobj``). The :ref:`FAQ <faq:symhidden>` contains an | ||
| and ``/LTCG`` on Visual Studio) and .OBJ files with many sections on Visual | ||
| Studio (``/bigobj``). The :ref:`FAQ <faq:symhidden>` contains an |
There was a problem hiding this comment.
Unlike pybind11_add_module(), the CMake targets haven't been including any visibility flags. It would be good to point this out here.
Alternatively, the visibility property could be added to the targets, but that's somewhat of a breaking change.
There was a problem hiding this comment.
Oh, actually I meant to add that as well but forgot. Why would it be breaking?
There was a problem hiding this comment.
If the library was relying on the default visibility to export symbols, the change would break cross-module types. It would require a one line change in CMake to restore default visibility, so it might not be a big deal.
There was a problem hiding this comment.
It seems it's not so easy to add it; there isn't any sort of INTERFACE_CXX_VISIBILITY_PRESET, which means we'd be back to figuring out when to add it based on compilers ourselves. Better, I think, to just document that target users should set the property themselves.
There was a problem hiding this comment.
The condition should be as simple as if(NOT MSVC), shouldn't it?
After some thinking, I feel that the hidden visibility should be enforced as much as possible. With an increasing number of pybind11 modules on PyPI, it would be quite easy to run into internals versioning issues and/or bindings of common STL containers.
This adds a PYBIND11_NAMESPACE macro that expands to the `pybind11` namespace with hidden visibility under gcc-type compilers, and otherwise to the plain `pybind11`. This then forces hidden visibility on everything in pybind, solving the visibility issues discussed at end end of pybind#949.
…iler flag This should resolve the issues with default visibility causing problems under debug compilations. Moreover using the cmake property makes it easier for a caller to override if absolutely needed for some reason.
jagerman
force-pushed
the
visibility-hidden
branch
from
d959519
to
8b9c6c5
Compare
CMakeLists.txt
Outdated
| @@ -91,6 +91,9 @@ if(NOT (CMAKE_VERSION VERSION_LESS 3.0)) # CMake >= 3.0 | |||
| $<BUILD_INTERFACE:${PYTHON_INCLUDE_DIRS}> | |||
| $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>) | |||
| target_compile_options(pybind11 INTERFACE $<BUILD_INTERFACE:${PYBIND11_CPP_STANDARD}>) | |||
| if(NOT MSVC) | |||
| target_compile_options(pybind11 INTERFACE $<BUILD_INTERFACE:-fvisibility=hidden>) | |||
There was a problem hiding this comment.
No need for build/install interface separation here because the same flag is applied to both. If target_compile_options(pybind11 INTERFACE -fvisibility=hidden) is set here, then nothing needs to be added to pybind11Config.cmake.
|
LGTM! |
This is a few commits to force hidden visibility on the
pybind11namespace and to unconditionally add it into the compilation flags (also this sets the flag through the cmake property rather than setting the actual compilation flags).This needs one more thing before merging--the FAQ entry talking about hidden visibility seems largely obsolete with these changes. Should we shorten it? Amend it? Drop it?