Skip to content

Set symbol visibility to hidden for rclcpp #638

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.

Sign up for GitHub

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

Closed
bhatsach wants to merge 2 commits into from
Closed

Set symbol visibility to hidden for rclcpp #638

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6f15f30
Set symbol visibility to hidden for rclcpp
bhatsach Feb 26, 2019
1459878
Remove inner RCL public macros for each of the modified classes and i…
bhatsach Feb 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions rclcpp/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ ament_target_dependencies(${PROJECT_NAME}
"rosidl_typesupport_cpp"
"rosidl_generator_cpp")

configure_rcl(${PROJECT_NAME})
Copy link

@thomas-moulard thomas-moulard Feb 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to be rclcpp?

Copy link
Author

@bhatsach bhatsach Feb 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@thomas-moulard thomas-moulard Feb 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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

Copy link
Contributor

@traversaro traversaro Feb 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@bhatsach bhatsach Feb 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@traversaro traversaro Feb 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 .

Copy link
Member

@wjwwood wjwwood Feb 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@bhatsach bhatsach Feb 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@traversaro traversaro Feb 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# Causes the visibility macros to use dllexport rather than dllimport,
# which is appropriate when building the dll but not consuming it.
target_compile_definitions(${PROJECT_NAME}
Expand Down
2 changes: 1 addition & 1 deletion rclcpp/include/rclcpp/client.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ namespace node_interfaces
class NodeBaseInterface;
} // namespace node_interfaces

class ClientBase
class RCLCPP_PUBLIC ClientBase
Copy link
Member

@wjwwood wjwwood Feb 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@bhatsach bhatsach Feb 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

Copy link
Member

@wjwwood wjwwood Feb 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@bhatsach bhatsach Feb 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
public:
RCLCPP_SMART_PTR_DEFINITIONS_NOT_COPYABLE(ClientBase)
Expand Down
2 changes: 1 addition & 1 deletion rclcpp/include/rclcpp/executor.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ static inline ExecutorArgs create_default_executor_arguments()
* model.
* See SingleThreadedExecutor and MultiThreadedExecutor for examples of execution paradigms.
*/
class Executor
class RCLCPP_PUBLIC Executor
{
public:
RCLCPP_SMART_PTR_DEFINITIONS_NOT_COPYABLE(Executor)
Expand Down
2 changes: 1 addition & 1 deletion rclcpp/include/rclcpp/intra_process_manager.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ namespace intra_process_manager
*
* This class is neither CopyConstructable nor CopyAssignable.
*/
class IntraProcessManager
class RCLCPP_PUBLIC IntraProcessManager
{
private:
RCLCPP_DISABLE_COPY(IntraProcessManager)
Expand Down
2 changes: 1 addition & 1 deletion rclcpp/include/rclcpp/node.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ namespace rclcpp
{

/// Node is the single point of entry for creating publishers and subscribers.
class Node : public std::enable_shared_from_this<Node>
class RCLCPP_PUBLIC Node : public std::enable_shared_from_this<Node>
{
public:
RCLCPP_SMART_PTR_DEFINITIONS(Node)
Expand Down
2 changes: 1 addition & 1 deletion rclcpp/include/rclcpp/publisher.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ namespace intra_process_manager
class IntraProcessManager;
}

class PublisherBase
class RCLCPP_PUBLIC PublisherBase
{
friend ::rclcpp::node_interfaces::NodeTopicsInterface;

Expand Down
2 changes: 1 addition & 1 deletion rclcpp/include/rclcpp/service.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
namespace rclcpp
{

class ServiceBase
class RCLCPP_PUBLIC ServiceBase
{
public:
RCLCPP_SMART_PTR_DEFINITIONS_NOT_COPYABLE(ServiceBase)
Expand Down
2 changes: 1 addition & 1 deletion rclcpp/include/rclcpp/subscription.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ class IntraProcessManager;

/// Virtual base class for subscriptions. This pattern allows us to iterate over different template
/// specializations of Subscription, among other things.
class SubscriptionBase
class RCLCPP_PUBLIC SubscriptionBase
{
public:
RCLCPP_SMART_PTR_DEFINITIONS_NOT_COPYABLE(SubscriptionBase)
Expand Down
2 changes: 1 addition & 1 deletion rclcpp/include/rclcpp/waitable.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
namespace rclcpp
{

class Waitable
class RCLCPP_PUBLIC Waitable
{
public:
RCLCPP_SMART_PTR_DEFINITIONS_NOT_COPYABLE(Waitable)
Expand Down
1 change: 1 addition & 0 deletions rclcpp_action/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ ament_target_dependencies(${PROJECT_NAME}
"rosidl_generator_c"
"rosidl_generator_cpp")

configure_rcl(${PROJECT_NAME})
# Causes the visibility macros to use dllexport rather than dllimport,
# which is appropriate when building the dll but not consuming it.
target_compile_definitions(${PROJECT_NAME}
Expand Down
2 changes: 1 addition & 1 deletion rclcpp_action/include/rclcpp_action/client.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class ClientBaseImpl;
*
* Internally, this class is responsible for interfacing with the `rcl_action` API.
*/
class ClientBase : public rclcpp::Waitable
class RCLCPP_ACTION_PUBLIC ClientBase : public rclcpp::Waitable
{
public:
RCLCPP_ACTION_PUBLIC
Expand Down
2 changes: 1 addition & 1 deletion rclcpp_action/include/rclcpp_action/server.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ enum class CancelResponse : int8_t
*
* Internally, this class is responsible for interfacing with the `rcl_action` API.
*/
class ServerBase : public rclcpp::Waitable
class RCLCPP_ACTION_PUBLIC ServerBase : public rclcpp::Waitable
{
public:
RCLCPP_ACTION_PUBLIC
Expand Down
2 changes: 1 addition & 1 deletion rclcpp_action/include/rclcpp_action/server_goal_handle.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ namespace rclcpp_action
*
* Internally, this class is responsible for interfacing with the `rcl_action` API.
*/
class ServerGoalHandleBase
class RCLCPP_ACTION_PUBLIC ServerGoalHandleBase
{
public:
/// Indicate if client has requested this goal be cancelled.
Expand Down
1 change: 1 addition & 0 deletions rclcpp_lifecycle/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ ament_target_dependencies(rclcpp_lifecycle
"rosidl_typesupport_cpp"
)

configure_rcl(${PROJECT_NAME})
# Causes the visibility macros to use dllexport rather than dllimport,
# which is appropriate when building the dll but not consuming it.
target_compile_definitions(rclcpp_lifecycle PRIVATE "RCLCPP_LIFECYCLE_BUILDING_DLL")
Expand Down
2 changes: 1 addition & 1 deletion rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ namespace rclcpp_lifecycle
/**
* has lifecycle nodeinterface for configuring this node.
*/
class LifecycleNode : public node_interfaces::LifecycleNodeInterface,
class RCLCPP_LIFECYCLE_PUBLIC LifecycleNode : public node_interfaces::LifecycleNodeInterface,
public std::enable_shared_from_this<LifecycleNode>
{
public:
Expand Down