Remove cmake `PROPERTIES CXX_VISIBILITY_PRESET "hidden"` #4072

rwgk · 2022-07-16T16:34:44Z

Description

Remove cmake PROPERTIES CXX_VISIBILITY_PRESET "hidden"
Add PYBIND11_NS_VISIBILITY in pybind11/detail/common.h
Use to resolve all "... declared with greater visibility than ..." warnings across all platforms.

TODO:

Decide if it is actually desirable to remove PROPERTIES CXX_VISIBILITY_PRESET "hidden" from the cmake files.
If not, revert the cmake changes but keep the test changes (so that the tests are compatible with default visibility).
Clean up the documentation including FAQ.

Background: Issue #2479

This PR was extracted from PR #4069 [smart_holder]. See commits there.

Suggested changelog entry:

…11_NS_VISIBILITY`, use to resolve all "... declared with greater visibility than ..." warnings across all platforms.

rwgk · 2022-07-16T17:38:11Z

@henryiii
@eacousineau

This PR proves that we do not need PROPERTIES CXX_VISIBILITY_PRESET "hidden" if we do not want to. Note that nothing is changed in the core library, except a tiny factoring to break out PYBIND11_NS_VISIBILITY(...) from PYBIND11_NAMESPACE. All other non-cmake/setup_helpers.py changes are to the tests only.

What is your opinion regarding this question (currently a TODO in the PR description):

Decide if it is actually desirable to remove PROPERTIES CXX_VISIBILITY_PRESET "hidden" from the cmake files.

From my perspective, pushing "hidden" as strongly as it is done currently in the documentation and cmake files is more on the harmful than helpful side. Google-internally, after significant back & forth and confusion, we removed -fvisibility=hidden from our BUILD files, for reasons very similar to what @eacousineau reported under #2479.

EricCousineau-TRI · 2022-07-16T18:03:58Z

I'm down for this!
Any chance you were able to check binary size produced before/after this PR for one platform+Python version+impl?
I would expect it might increase, but ideally it only increases due to other non-pybind11 symbols being (correctly) exposed.

EricCousineau-TRI

First pass done, PTAL!

EricCousineau-TRI · 2022-07-16T18:05:25Z

include/pybind11/detail/common.h

@@ -25,8 +25,10 @@
 // on the main `pybind11` namespace.
 #if !defined(PYBIND11_NAMESPACE)
 #    ifdef __GNUG__


nit VISIBILITY sounds like what is being changed, but this doesn't provide an option to choose.

Consider using HIDDEN to make intent more clear.

EricCousineau-TRI · 2022-07-16T18:05:27Z

include/pybind11/detail/common.h

@@ -25,8 +25,10 @@
 // on the main `pybind11` namespace.
 #if !defined(PYBIND11_NAMESPACE)
 #    ifdef __GNUG__
-#        define PYBIND11_NAMESPACE pybind11 __attribute__((visibility("hidden")))
+#        define PYBIND11_NS_VISIBILITY(...) __VA_ARGS__ __attribute__((visibility("hidden")))


nit While NS is a good abbreviation, perhaps it is still good to full expand NAMESPACE?

Or if this is agnostic to the type of symbol, consider just removing the NS qualifier?

EricCousineau-TRI · 2022-07-16T18:07:04Z

tests/test_custom_type_setup.cpp

@@ -13,14 +13,19 @@

 namespace py = pybind11;

+PYBIND11_NAMESPACE_BEGIN(PYBIND11_NS_VISIBILITY(pybind11_tests))


Not sure if I understand need for applying hidden when it only contains an anonymous namespace.

Is it possible to revert this (possibly redundant) change?

Thanks Eric, I'll wait for Henry's opinion before investing more time, but to explain quickly, this is purely to appease some sad compilers that don't "realize" that the unnamed namespace is already hidden. I could add comments, or remove the unnamed namespace, I'll figure out something after we decided the big question.

EricCousineau-TRI · 2022-07-16T18:07:35Z

tests/test_exceptions.cpp

@@ -15,6 +15,8 @@
 #include <stdexcept>
 #include <utility>

+PYBIND11_NAMESPACE_BEGIN(PYBIND11_NS_VISIBILITY(pybind11_tests))


Same as above - why not just use anonymous namespace {}?

EricCousineau-TRI · 2022-07-16T18:07:56Z

tests/test_exceptions.h

@@ -5,9 +5,13 @@

 // shared exceptions for cross_module_tests

+PYBIND11_NAMESPACE_BEGIN(PYBIND11_NS_VISIBILITY(pybind11_tests))


Same as above - why not just use anonymous namespace {}?

EricCousineau-TRI · 2022-07-16T18:08:01Z

tests/test_numpy_array.cpp

@@ -15,6 +15,8 @@
 #include <cstdint>
 #include <utility>

+PYBIND11_NAMESPACE_BEGIN(PYBIND11_NS_VISIBILITY(pybind11_tests))


Same as above - why not just use anonymous namespace {}?

EricCousineau-TRI · 2022-07-16T18:09:04Z

tests/test_pytypes.cpp

@@ -11,7 +11,7 @@

 #include <utility>

-namespace external {
+PYBIND11_NAMESPACE_BEGIN(PYBIND11_NS_VISIBILITY(external))


nit Er, hiding a namespace external smells odd.

Can we use a better name here? e.g. feaux_external, with a comment of what is going on?

EricCousineau-TRI · 2022-07-16T18:09:21Z

tools/pybind11NewTools.cmake

@@ -203,19 +203,6 @@ function(pybind11_add_module target_name)
    target_link_libraries(${target_name} PRIVATE pybind11::windows_extras)
  endif()

-  # -fvisibility=hidden is required to allow multiple modules compiled against


BTW yay! deleting code makes me 10000% happier!

henryiii · 2022-08-08T14:00:27Z

Let's wait till after 2.10.1 to decide on this. I know the original design was to do the right thing for most users, and most users are not using the ABI compat, and are not that familiar with symbol visibility. So suddenly asking everyone to start hiding their symbols or the binaries expand is not a very great thing to do. There's also a very easy workaround - a user can simply unset it, which is making the more advanced use case take more configuration, but that's nicer.

I think the corrections on the visibility are good, and we can see how much this tends to affect user code. We could also update the docs recommending users set visibility for a release before we actually pull the plug on the default.

Remove cmake PROPERTIES CXX_VISIBILITY_PRESET "hidden", add `PYBIND…

3d1a7f3

…11_NS_VISIBILITY`, use to resolve all "... declared with greater visibility than ..." warnings across all platforms.

rwgk mentioned this pull request Jul 16, 2022

[smart_holder]: trampoline_self_life_support -fvisibility question #3927

Open

3 tasks

rwgk requested a review from EricCousineau-TRI July 16, 2022 17:24

rwgk marked this pull request as ready for review July 16, 2022 17:37

rwgk requested a review from henryiii as a code owner July 16, 2022 17:37

EricCousineau-TRI requested changes Jul 16, 2022

View reviewed changes

This was referenced Oct 24, 2022

[BUG]: ABI incompatibility between 2.9.2 and 2.10 #4105

Closed

docs: prepare for 2.10.1 release #4279

Merged

rwgk mentioned this pull request Dec 12, 2022

Systematically add -DCMAKE_VERBOSE_MAKEFILE=ON in ci.yml #4398

Merged

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11clif#4072

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove cmake `PROPERTIES CXX_VISIBILITY_PRESET "hidden"` #4072

Remove cmake `PROPERTIES CXX_VISIBILITY_PRESET "hidden"` #4072

rwgk commented Jul 16, 2022 •

edited

Loading

rwgk commented Jul 16, 2022

EricCousineau-TRI commented Jul 16, 2022

EricCousineau-TRI left a comment

EricCousineau-TRI Jul 16, 2022

EricCousineau-TRI Jul 16, 2022

EricCousineau-TRI Jul 16, 2022

rwgk Jul 16, 2022

EricCousineau-TRI Jul 16, 2022

EricCousineau-TRI Jul 16, 2022

EricCousineau-TRI Jul 16, 2022

EricCousineau-TRI Jul 16, 2022

EricCousineau-TRI Jul 16, 2022

henryiii commented Aug 8, 2022 •

edited

Loading

		@@ -13,14 +13,19 @@

		namespace py = pybind11;

		PYBIND11_NAMESPACE_BEGIN(PYBIND11_NS_VISIBILITY(pybind11_tests))

		@@ -5,9 +5,13 @@

		// shared exceptions for cross_module_tests

		PYBIND11_NAMESPACE_BEGIN(PYBIND11_NS_VISIBILITY(pybind11_tests))

Remove cmake PROPERTIES CXX_VISIBILITY_PRESET "hidden" #4072

Are you sure you want to change the base?

Remove cmake PROPERTIES CXX_VISIBILITY_PRESET "hidden" #4072

Conversation

rwgk commented Jul 16, 2022 • edited Loading

Description

Suggested changelog entry:

rwgk commented Jul 16, 2022

EricCousineau-TRI commented Jul 16, 2022

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henryiii commented Aug 8, 2022 • edited Loading

Remove cmake `PROPERTIES CXX_VISIBILITY_PRESET "hidden"` #4072

Remove cmake `PROPERTIES CXX_VISIBILITY_PRESET "hidden"` #4072

rwgk commented Jul 16, 2022 •

edited

Loading

henryiii commented Aug 8, 2022 •

edited

Loading