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

pydrake: Modules should be compiled with hidden symbols #8433

Open
jamiesnape opened this issue Mar 22, 2018 · 6 comments
Open

pydrake: Modules should be compiled with hidden symbols #8433

jamiesnape opened this issue Mar 22, 2018 · 6 comments
Assignees
Labels
component: distribution Nightly binaries, monthly releases, docker, installation priority: backlog type: feature request

Comments

@jamiesnape
Copy link
Contributor

INFO: From Compiling bindings/pydrake/symbolic_py.cc:
In file included from bindings/pydrake/symbolic_py.cc:13:0:
bazel-out/k8-opt/bin/bindings/pydrake/util/_virtual_includes/wrap_pybind/drake/bindings/pydrake/util/wrap_pybind.h
:17:7: warning: 'drake::pydrake::MirrorDef' declared with greater visibility than the type of its field
'drake::pydrake::MirrorDef::m_' [-Wattributes]
 class MirrorDef {
       ^
bazel-out/k8-opt/bin/bindings/pydrake/util/_virtual_includes/wrap_pybind/drake/bindings/pydrake/util/wrap_pybind.h
:17:7: warning: 'drake::pydrake::MirrorDef' declared with greater visibility than the type of its field
'drake::pydrake::MirrorDef::mirror_' [-Wattributes]
@jwnimmer-tri
Copy link
Collaborator

I suspect that we should also promote Wattributes to Werror=attributes. @EricCousineau-TRI if you do that as part of the fix then great; otherwise I can open it as a new issue later on.

@EricCousineau-TRI
Copy link
Contributor

Gotcha. Seems that I need to inline / hide this class:
http://pybind11.readthedocs.io/en/stable/faq.html#someclass-declared-with-greater-visibility-than-the-type-of-its-field-someclass-member-wattributes

Will see if I can update those flags.

@EricCousineau-TRI EricCousineau-TRI changed the title 'drake::pydrake::MirrorDef' declared with greater visibility than the type of its field pydrake: Modules should be compiled with hidden symbols Dec 18, 2018
@EricCousineau-TRI
Copy link
Contributor

EricCousineau-TRI commented Dec 18, 2018

Relates #8540 on the level of RTTI and all that.
If we are able to resolve this, we should ensure CPS-generated CMake interface propagates visibility as well via its interface (#8644).

@EricCousineau-TRI EricCousineau-TRI added the component: pydrake Python API and its supporting Starlark macros label May 2, 2020
@jwnimmer-tri jwnimmer-tri added component: distribution Nightly binaries, monthly releases, docker, installation and removed component: pydrake Python API and its supporting Starlark macros labels Nov 11, 2021
@jwnimmer-tri
Copy link
Collaborator

Refer to #16756 for a nice trick. We can set hidden visibility on an entire Drake namespace, instead of fighting with -fvisibility=hidden vs the linker.

With the attribute, symbols used within the file but not in the pydrake namespace can still be hidden (or not) per their original declarations. With the -fvisibility build flag all symbols are hidden, which leads to mismatched visibility vs other TUs.

namespace pydrake __attribute__ ((visibility ("hidden"))) {
...
}  // namespace pydrake

@jwnimmer-tri
Copy link
Collaborator

I think this is probably still worth doing, but in terms of scheduling my plan is to do the module flattening first (so that we have fewer python shared libraries -- most likely just one), at which point it should be less of a hassle to get this right.

@jwnimmer-tri
Copy link
Collaborator

See also #18677. It adds the warning flag, and provides a nice macro for the namespace-level visibility control.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: distribution Nightly binaries, monthly releases, docker, installation priority: backlog type: feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants