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 all commits
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
2 changes: 2 additions & 0 deletions rclcpp/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
endif()

include_directories(include)
include(cmake/configure_rclcpp.cmake)

set(${PROJECT_NAME}_SRCS
src/rclcpp/any_executable.cpp
Expand Down Expand Up @@ -109,6 +110,7 @@ ament_target_dependencies(${PROJECT_NAME}
"rosidl_typesupport_cpp"
"rosidl_generator_cpp")

configure_rclcpp(${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
66 changes: 66 additions & 0 deletions rclcpp/cmake/configure_rclcpp.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
# Copyright 2019 Open Source Robotics Foundation, Inc.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#
# Configures ros client library with custom settings.
# The custom settings are all related to library symbol visibility, see:
# https://gcc.gnu.org/wiki/Visibility
# http://www.ibm.com/developerworks/aix/library/au-aix-symbol-visibility/
#
# Below code is heavily referenced from a similar functionality in rmw:
# https://github.com/ros2/rmw/blob/master/rmw/cmake/configure_rmw_library.cmake
#
# :param library_target: the library target
# :type library_target: string
# :param LANGUAGE: Optional flag for the language of the library.
# Allowed values are "C" and "CXX". The default is "CXX".
# :type LANGUAGE: string
#
# @public
#
macro(configure_rclcpp library_target)
cmake_parse_arguments(_ARG "" "LANGUAGE" "" ${ARGN})
if(_ARG_UNPARSED_ARGUMENTS)
message(FATAL_ERROR "configure_rclcpp() called with unused arguments: ${_ARG_UNPARSED_ARGUMENTS}")
endif()

if(NOT _ARG_LANGUAGE)
set(_ARG_LANGUAGE "CXX")
endif()

if(_ARG_LANGUAGE STREQUAL "C")
# Set the visibility to hidden by default if possible
if(CMAKE_C_COMPILER_ID STREQUAL "GNU" OR CMAKE_C_COMPILER_ID MATCHES "Clang")
# Set the visibility of symbols to hidden by default for gcc and clang
# (this is already the default on Windows)
set_target_properties(${library_target}
PROPERTIES
COMPILE_FLAGS "-fvisibility=hidden"
)
endif()

elseif(_ARG_LANGUAGE STREQUAL "CXX")
# Set the visibility to hidden by default if possible
if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU" OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
# Set the visibility of symbols to hidden by default for gcc and clang
# (this is already the default on Windows)
set_target_properties(${library_target}
PROPERTIES
COMPILE_FLAGS "-fvisibility=hidden -fvisibility-inlines-hidden"
)
endif()

else()
message(FATAL_ERROR "configure_rclcpp() called with unsupported LANGUAGE: '${_ARG_LANGUAGE}'")
endif()
endmacro()
11 changes: 1 addition & 10 deletions rclcpp/include/rclcpp/client.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,32 +49,26 @@ 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)

RCLCPP_PUBLIC
ClientBase(
rclcpp::node_interfaces::NodeBaseInterface * node_base,
rclcpp::node_interfaces::NodeGraphInterface::SharedPtr node_graph);

RCLCPP_PUBLIC
virtual ~ClientBase();

RCLCPP_PUBLIC
const char *
get_service_name() const;

RCLCPP_PUBLIC
std::shared_ptr<rcl_client_t>
get_client_handle();

RCLCPP_PUBLIC
std::shared_ptr<const rcl_client_t>
get_client_handle() const;

RCLCPP_PUBLIC
bool
service_is_ready() const;

Expand All @@ -96,15 +90,12 @@ class ClientBase
protected:
RCLCPP_DISABLE_COPY(ClientBase)

RCLCPP_PUBLIC
bool
wait_for_service_nanoseconds(std::chrono::nanoseconds timeout);

RCLCPP_PUBLIC
rcl_node_t *
get_rcl_node_handle();

RCLCPP_PUBLIC
const rcl_node_t *
get_rcl_node_handle() const;

Expand Down
27 changes: 1 addition & 26 deletions rclcpp/include/rclcpp/executor.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,18 +92,16 @@ 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)

/// Default constructor.
// \param[in] ms The memory strategy to be used with this executor.
RCLCPP_PUBLIC
explicit Executor(const ExecutorArgs & args = ExecutorArgs());

/// Default destructor.
RCLCPP_PUBLIC
virtual ~Executor();

/// Do work periodically as it becomes available to us. Blocking call, may block indefinitely.
Expand All @@ -119,12 +117,10 @@ class Executor
* the executor is blocked at the rmw layer while waiting for work and it is notified that a new
* node was added, it will wake up.
*/
RCLCPP_PUBLIC
virtual void
add_node(rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node_ptr, bool notify = true);

/// Convenience function which takes Node and forwards NodeBaseInterface.
RCLCPP_PUBLIC
virtual void
add_node(std::shared_ptr<rclcpp::Node> node_ptr, bool notify = true);

Expand All @@ -135,12 +131,10 @@ class Executor
* This is useful if the last node was removed from the executor while the executor was blocked
* waiting for work in another thread, because otherwise the executor would never be notified.
*/
RCLCPP_PUBLIC
virtual void
remove_node(rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node_ptr, bool notify = true);

/// Convenience function which takes Node and forwards NodeBaseInterface.
RCLCPP_PUBLIC
virtual void
remove_node(std::shared_ptr<rclcpp::Node> node_ptr, bool notify = true);

Expand Down Expand Up @@ -180,12 +174,10 @@ class Executor
/**
* \param[in] node Shared pointer to the node to add.
*/
RCLCPP_PUBLIC
void
spin_node_some(rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node);

/// Convenience function which takes Node and forwards NodeBaseInterface.
RCLCPP_PUBLIC
void
spin_node_some(std::shared_ptr<rclcpp::Node> node);

Expand All @@ -200,11 +192,9 @@ class Executor
* Note that spin_some() may take longer than this time as it only returns once max_duration has
* been exceeded.
*/
RCLCPP_PUBLIC
virtual void
spin_some(std::chrono::nanoseconds max_duration = std::chrono::nanoseconds(0));

RCLCPP_PUBLIC
virtual void
spin_once(std::chrono::nanoseconds timeout = std::chrono::nanoseconds(-1));

Expand Down Expand Up @@ -269,7 +259,6 @@ class Executor

/// Cancel any running spin* function, causing it to return.
/* This function can be called asynchonously from any thread. */
RCLCPP_PUBLIC
void
cancel();

Expand All @@ -279,12 +268,10 @@ class Executor
* unintended consequences.
* \param[in] memory_strategy Shared pointer to the memory strategy to set.
*/
RCLCPP_PUBLIC
void
set_memory_strategy(memory_strategy::MemoryStrategy::SharedPtr memory_strategy);

protected:
RCLCPP_PUBLIC
void
spin_node_once_nanoseconds(
rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node,
Expand All @@ -294,53 +281,41 @@ class Executor
/** \param[in] any_exec Union structure that can hold any executable type (timer, subscription,
* service, client).
*/
RCLCPP_PUBLIC
void
execute_any_executable(AnyExecutable & any_exec);

RCLCPP_PUBLIC
static void
execute_subscription(
rclcpp::SubscriptionBase::SharedPtr subscription);

RCLCPP_PUBLIC
static void
execute_intra_process_subscription(
rclcpp::SubscriptionBase::SharedPtr subscription);

RCLCPP_PUBLIC
static void
execute_timer(rclcpp::TimerBase::SharedPtr timer);

RCLCPP_PUBLIC
static void
execute_service(rclcpp::ServiceBase::SharedPtr service);

RCLCPP_PUBLIC
static void
execute_client(rclcpp::ClientBase::SharedPtr client);

RCLCPP_PUBLIC
void
wait_for_work(std::chrono::nanoseconds timeout = std::chrono::nanoseconds(-1));

RCLCPP_PUBLIC
rclcpp::node_interfaces::NodeBaseInterface::SharedPtr
get_node_by_group(rclcpp::callback_group::CallbackGroup::SharedPtr group);

RCLCPP_PUBLIC
rclcpp::callback_group::CallbackGroup::SharedPtr
get_group_by_timer(rclcpp::TimerBase::SharedPtr timer);

RCLCPP_PUBLIC
void
get_next_timer(AnyExecutable & any_exec);

RCLCPP_PUBLIC
bool
get_next_ready_executable(AnyExecutable & any_executable);

RCLCPP_PUBLIC
bool
get_next_executable(
AnyExecutable & any_executable,
Expand Down
10 changes: 1 addition & 9 deletions 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,19 +119,17 @@ namespace intra_process_manager
*
* This class is neither CopyConstructable nor CopyAssignable.
*/
class IntraProcessManager
class RCLCPP_PUBLIC IntraProcessManager
{
private:
RCLCPP_DISABLE_COPY(IntraProcessManager)

public:
RCLCPP_SMART_PTR_DEFINITIONS(IntraProcessManager)

RCLCPP_PUBLIC
explicit IntraProcessManager(
IntraProcessManagerImplBase::SharedPtr state = create_default_impl());

RCLCPP_PUBLIC
virtual ~IntraProcessManager();

/// Register a subscription with the manager, returns subscriptions unique id.
Expand All @@ -147,7 +145,6 @@ class IntraProcessManager
* \param subscription the Subscription to register.
* \return an unsigned 64-bit integer which is the subscription's unique id.
*/
RCLCPP_PUBLIC
uint64_t
add_subscription(SubscriptionBase::SharedPtr subscription);

Expand All @@ -157,7 +154,6 @@ class IntraProcessManager
*
* \param intra_process_subscription_id id of the subscription to remove.
*/
RCLCPP_PUBLIC
void
remove_subscription(uint64_t intra_process_subscription_id);

Expand Down Expand Up @@ -206,7 +202,6 @@ class IntraProcessManager
*
* \param intra_process_publisher_id id of the publisher to remove.
*/
RCLCPP_PUBLIC
void
remove_publisher(uint64_t intra_process_publisher_id);

Expand Down Expand Up @@ -342,17 +337,14 @@ class IntraProcessManager
}

/// Return true if the given rmw_gid_t matches any stored Publishers.
RCLCPP_PUBLIC
bool
matches_any_publishers(const rmw_gid_t * id) const;

/// Return the number of intraprocess subscriptions to a topic, given the publisher id.
RCLCPP_PUBLIC
size_t
get_subscription_count(uint64_t intra_process_publisher_id) const;

private:
RCLCPP_PUBLIC
static uint64_t
get_next_unique_id();

Expand Down
Loading