Set symbol visibility to hidden for rclcpp #638

bhatsach · 2019-02-26T17:04:37Z

Enabling symbol visibility feature in gcc and clang compilers.
This will hep find symbol export related issues in linux and
potentially reduce compile times.

Discourse topic link:
https://discourse.ros.org/t/set-symbol-visibility-to-hidden-for-rmw-and-rcl-packages/7981

Signed-off-by: Sachin Suresh Bhat bhatsach@amazon.com

Connects to ros2/rcl#391

Enabling symbol visibility feature in gcc and clang compilers. This will hep find symbol export related issues in linux and potentially reduce compile times. Discourse topic link: https://discourse.ros.org/t/set-symbol-visibility-to-hidden-for-rmw-and-rcl-packages/7981 Signed-off-by: Sachin Suresh Bhat <bhatsach@amazon.com>

thomas-moulard

LGTM, thanks Sachin!
It's awesome you actually found bugs while doing this.

thomas-moulard · 2019-02-26T17:58:04Z

Linux
Linux-aarch64
macOS
Windows

thomas-moulard · 2019-02-26T19:05:04Z

rclcpp/CMakeLists.txt

@@ -109,6 +109,7 @@ ament_target_dependencies(${PROJECT_NAME}
  "rosidl_typesupport_cpp"
  "rosidl_generator_cpp")

+configure_rcl(${PROJECT_NAME})


Needs to be rclcpp?

I am trying to reuse the configure_rcl macro in ros2/rcl. This is in a way similar to how the configure_rmw_library macro is being reused in rmw_fastrtps and other packages. The single macro takes care of both C and C++ code

This is not possible because they are not part of the same shared library. See doc here: https://gcc.gnu.org/wiki/Visibility

edit:

You have two SOs librcl.so and librcl_cpp.so.
When you build librcl.so, you can access both any public symbol of any dependency and any internal symbol, even private ones (because they are part of the same library).
Same thing when you build librcl_cpp.so. You can access your own symbols and any public one but not private symbols from another lib like e.g. librcl.so.

By re-using the macro, you are actually allowing librcl_cpp.so to access librcl.so private symbols which will not work.

Removing the duplication is doable but you'ld need something like that:

https://github.com/jrl-umi3218/jrl-cmakemodules/blob/bf10c6ca3565b96ab283edaa8f18b5b533e59da9/header.cmake

https://github.com/jrl-umi3218/jrl-cmakemodules/blob/bf10c6ca3565b96ab283edaa8f18b5b533e59da9/config.hh.cmake

This would significantly increase the scope of this change so I won't recommend for this to be done in this PR.

A possible alternative may be to use the CMake standard macro generate_export_header and the DEFINE_SYMBOL target property.

See ros/console_bridge#43 for a use in a ROS-related repo, and https://bitbucket.org/osrf/gazebo/issues/2262/symbol-visibility-issues-in-windows for a related issue in Gazebo.

Quick note: This is the way in which we are sharing the "configure_rcl" macro from rcl to rclcpp: https://github.com/ros2/rmw/blob/master/rmw/CMakeLists.txt#L62-L65
I am not sure if sharing just the macro is allowing librcl_cpp.so to access librcl.so private symbols, I am guessing it is not.

The critical part is in https://github.com/ros2/rcl/pull/391/files#diff-19c13a4e66284bae327c94111b226946R71, in which you are defining the RCL_BUILDING_DLL macro also for compilation units that are not part of the rcl. The effect will be that when including https://github.com/ros2/rcl/blob/aa4ac7d06cc5e59d693408260868edb004154b69/rcl/include/rcl/visibility_control.h as part of a compilation unit outside of rcl, RCL_PUBLIC (in Windows) will have the value __declspec(dllexport) instead of the correct __declspec(dllimport). This is exactly the problem described in https://bitbucket.org/osrf/gazebo/issues/2262/symbol-visibility-issues-in-windows .

A possible alternative may be to use the CMake standard macro generate_export_header and the DEFINE_SYMBOL target property.

I personally don't like the generated code approach (I prefer a single, checked-in boil-plate file) and a custom define.

I am trying to reuse the configure_rcl macro in ros2/rcl. This is in a way similar to how the configure_rmw_library macro is being reused in rmw_fastrtps and other packages. The single macro takes care of both C and C++ code

As the others said, you can do this, but only for the visibility compiler options, not the visibility compiler definition.

Thanks for the inputs @thomas-moulard , @traversaro and @wjwwood. I have removed the target definition specific code in configure_rcl in ros2/rcl#391 . This is because each rcl or rclcpp has its own target definition specified in the CMakeList. For eg:
https://github.com/ros2/rclcpp/blob/master/rclcpp/CMakeLists.txt#L112-L115
https://github.com/ros2/rclcpp/blob/master/rclcpp_action/CMakeLists.txt#L38-L41

Is this a reasonable solution? Do you think we should have a separate configure_rclcpp for rclcpp packages?

I personally don't like the generated code approach (I prefer a single, checked-in boil-plate file) and a custom define.

This is probably a bit OT in this PR, but for the sake of clarity: the generated header file (both CMake's and the example linked by @thomas-moulard ) is useful for supporting the case in which a library is compiled as a static library under Windows. Without that, in any downstream compilation unit RCLCPP_PUBLIC is always defined as __declspec(dllimport) and this annotation will generate a Visual Studio error LNK2019 if the RCLCPP library is a actually compiled as static. A possible alternative without having a generated file is to have an additional logic that defines RCLCPP_PUBLIC as an empty macro if RCLCPP_STATIC is defined, but then it is necessary some kind of build system logic to make sure that RCLCPP_STATIC is always defined (even downstream) if RCLCPP is build as static, for example a conditional target_compile_definitions(rclcpp PUBLIC RCLCPP_STATIC) called only if the library is built is static, or similar.

See ros/console_bridge#40 (comment) for a similar comment.

thomas-moulard

Sorry - let me rescind my vote until the macro gets fixed.

thomas-moulard · 2019-02-26T19:18:05Z

rclcpp/CMakeLists.txt

@@ -109,6 +109,7 @@ ament_target_dependencies(${PROJECT_NAME}
  "rosidl_typesupport_cpp"
  "rosidl_generator_cpp")

+configure_rcl(${PROJECT_NAME})


This is not possible because they are not part of the same shared library. See doc here: https://gcc.gnu.org/wiki/Visibility

edit:

You have two SOs librcl.so and librcl_cpp.so.
When you build librcl.so, you can access both any public symbol of any dependency and any internal symbol, even private ones (because they are part of the same library).
Same thing when you build librcl_cpp.so. You can access your own symbols and any public one but not private symbols from another lib like e.g. librcl.so.

By re-using the macro, you are actually allowing librcl_cpp.so to access librcl.so private symbols which will not work.

Removing the duplication is doable but you'ld need something like that:

https://github.com/jrl-umi3218/jrl-cmakemodules/blob/bf10c6ca3565b96ab283edaa8f18b5b533e59da9/header.cmake

https://github.com/jrl-umi3218/jrl-cmakemodules/blob/bf10c6ca3565b96ab283edaa8f18b5b533e59da9/config.hh.cmake

This would significantly increase the scope of this change so I won't recommend for this to be done in this PR.

wjwwood · 2019-02-26T21:02:17Z

rclcpp/include/rclcpp/client.hpp

@@ -49,7 +49,7 @@ namespace node_interfaces
 class NodeBaseInterface;
 }  // namespace node_interfaces

-class ClientBase
+class RCLCPP_PUBLIC ClientBase


Can you explain why? We explicitly avoided this, choosing to make each method public manually. We've had issues making these public on Windows due to the std data structures which are private members of the class.

Here's a sample error I ran into while running rclcpp package with visibility set to hidden. Is there any other way to handle this instead of making the class visible?

collect2: error: ld returned 1 exit status make[2]: *** [test_service] Error 1 make[1]: *** [CMakeFiles/test_service.dir/all] Error 2 make[1]: *** Waiting for unfinished jobs.... CMakeFiles/test_publisher.dir/test/test_publisher.cpp.o: In function `std::shared_ptr<rclcpp::Publisher<rcl_interfaces::msg::IntraProcessMessage_<std::allocator<void> >, std::allocator<void> > > std::dynamic_pointer_cast<rclcpp::Publisher<rcl_interfaces::msg::IntraProcessMessage_<std::allocator<void> >, std::allocator<void> >, rclcpp::PublisherBase>(std::shared_ptr<rclcpp::PublisherBase> const&)': test_publisher.cpp:(.text._ZSt20dynamic_pointer_castIN6rclcpp9PublisherIN14rcl_interfaces3msg20IntraProcessMessage_ISaIvEEES5_EENS0_13PublisherBaseEESt10shared_ptrIT_ERKS9_IT0_E[_ZSt20dynamic_pointer_castIN6rclcpp9PublisherIN14rcl_interfaces3msg20IntraProcessMessage_ISaIvEEES5_EENS0_13PublisherBaseEESt10shared_ptrIT_ERKS9_IT0_E]+0x30): undefined reference to `typeinfo for rclcpp::PublisherBase' CMakeFiles/test_publisher.dir/test/test_publisher.cpp.o:(.data.rel.ro._ZTIN6rclcpp9PublisherIN14rcl_interfaces3msg20IntraProcessMessage_ISaIvEEES4_EE[_ZTIN6rclcpp9PublisherIN14rcl_interfaces3msg20IntraProcessMessage_ISaIvEEES4_EE]+0x10): undefined reference to `typeinfo for rclcpp::PublisherBase' collect2: error: ld returned 1 exit status make[2]: *** [test_publisher] Error 1 make[1]: *** [CMakeFiles/test_publisher.dir/all] Error 2 CMakeFiles/test_externally_defined_services.dir/test/test_externally_defined_services.cpp.o:(.data.rel.ro._ZTIN6rclcpp7ServiceINS_3srv4MockEEE[_ZTIN6rclcpp7ServiceINS_3srv4MockEEE]+0x10): undefined reference to `typeinfo for rclcpp::ServiceBase' collect2: error: ld returned 1 exit status make[2]: *** [test_externally_defined_services] Error 1 make[1]: *** [CMakeFiles/test_externally_defined_services.dir/all] Error 2 CMakeFiles/test_client.dir/test/test_client.cpp.o:(.data.rel.ro._ZTIN6rclcpp6ClientIN14rcl_interfaces3srv14ListParametersEEE[_ZTIN6rclcpp6ClientIN14rcl_interfaces3srv14ListParametersEEE]+0x10): undefined reference to `typeinfo for rclcpp::ClientBase'

I think adding it to the class may be required, but you need to test it on Windows too, because I think it might be in conflict.

If you do leave the public in the class declaration, then the RCLCPP_PUBLIC for each method in the class is redundant and should be removed.

I removed all the inner RCLCPP_PUBLIC keywords from the modified classes. Will request @thomas-moulard to run this on windows CI for a quick check

…ntroduce a separate configure_rclcpp.cmake Signed-off-by: Sachin Suresh Bhat <bhatsach@amazon.com>

thomas-moulard · 2019-02-27T00:42:04Z

Linux
Linux-aarch64
macOS
Windows

wjwwood · 2019-02-27T05:07:31Z

This is what I was referring to (dll warnings from the linker):

https://ci.ros2.org/job/ci_windows/6118/warnings43Result/new/

bhatsach · 2019-02-27T06:15:02Z

This is what I was referring to (dll warnings from the linker):

https://ci.ros2.org/job/ci_windows/6118/warnings43Result/new/

:( let me look into this tomorrow. @wjwwood, can you help close out the rcl related changes if they are good to go? ros2/rcl#391

bhatsach · 2019-03-01T20:00:24Z

Discarding this PR for now as we need further investigation before getting back onto this task:

Investigate the above windows warnings and identify ways to resolve the same
Investigate the above os x test failures and identify ways to resolve the same
Identify a solution which works well on all the platforms. One of the main objectives here is to identify dllexport issues earlier.

Resolves #638 Co-authored-by: Shane Loretz <shane.loretz@gmail.com>

thomas-moulard approved these changes Feb 26, 2019

View reviewed changes

thomas-moulard reviewed Feb 26, 2019

View reviewed changes

thomas-moulard suggested changes Feb 26, 2019

View reviewed changes

wjwwood reviewed Feb 26, 2019

View reviewed changes

Remove inner RCL public macros for each of the modified classes and i…

1459878

…ntroduce a separate configure_rclcpp.cmake Signed-off-by: Sachin Suresh Bhat <bhatsach@amazon.com>

bhatsach force-pushed the hide-visibility branch from 8c5bbdd to 1459878 Compare February 27, 2019 00:26

thomas-moulard mentioned this pull request Feb 27, 2019

[ROS2 QA] Enable symbol visibility for all RMW and RCL packages ros2/ros2#663

Closed

4 tasks

bhatsach closed this Mar 1, 2019

jacobperron mentioned this pull request Apr 5, 2019

Backport #637 - Wait for action server before sending goal #686

Merged

jacobperron self-assigned this Apr 5, 2019

nuclearsandwich pushed a commit that referenced this pull request Apr 5, 2019

Backport #637 - Wait for action server before sending goal (#686)

f7bb006

Resolves #638 Co-authored-by: Shane Loretz <shane.loretz@gmail.com>

traversaro mentioned this pull request Oct 18, 2019

[Windows] dllexport support for ament_cmake ament/ament_cmake#201

Open

traversaro mentioned this pull request Mar 16, 2021

Symbol Visibility in ROS on Windows ms-iot/ROSOnWindows#322

Merged

traversaro mentioned this pull request Jun 28, 2021

moveit-core: Fixes for Windows, and CI for macOS and Windows moveit/moveit#2604

Merged

6 tasks

traversaro mentioned this pull request Apr 21, 2024

Support shared library building on Windows rerun-io/rerun#6053

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set symbol visibility to hidden for rclcpp #638

Set symbol visibility to hidden for rclcpp #638

bhatsach commented Feb 26, 2019 •

edited

Loading

thomas-moulard left a comment

thomas-moulard commented Feb 26, 2019

thomas-moulard Feb 26, 2019

bhatsach Feb 26, 2019

thomas-moulard Feb 26, 2019 •

edited

Loading

traversaro Feb 26, 2019

bhatsach Feb 26, 2019 •

edited

Loading

traversaro Feb 26, 2019

wjwwood Feb 26, 2019

bhatsach Feb 26, 2019

traversaro Feb 26, 2019

thomas-moulard left a comment

thomas-moulard Feb 26, 2019 •

edited

Loading

wjwwood Feb 26, 2019

bhatsach Feb 26, 2019

wjwwood Feb 26, 2019

bhatsach Feb 27, 2019

thomas-moulard commented Feb 27, 2019

wjwwood commented Feb 27, 2019

bhatsach commented Feb 27, 2019

bhatsach commented Mar 1, 2019

Set symbol visibility to hidden for rclcpp #638

Set symbol visibility to hidden for rclcpp #638

Conversation

bhatsach commented Feb 26, 2019 • edited Loading

thomas-moulard left a comment

Choose a reason for hiding this comment

thomas-moulard commented Feb 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomas-moulard Feb 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhatsach Feb 26, 2019 • edited Loading

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

thomas-moulard left a comment

Choose a reason for hiding this comment

thomas-moulard Feb 26, 2019 • edited Loading

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

thomas-moulard commented Feb 27, 2019

wjwwood commented Feb 27, 2019

bhatsach commented Feb 27, 2019

bhatsach commented Mar 1, 2019

bhatsach commented Feb 26, 2019 •

edited

Loading

thomas-moulard Feb 26, 2019 •

edited

Loading

bhatsach Feb 26, 2019 •

edited

Loading

thomas-moulard Feb 26, 2019 •

edited

Loading