Add support for pthread-based thread local storage #80

esteve · 2016-12-08T09:55:55Z

This PR adds support for pthread-based thread local storage on platforms that don't have compiler builtin support for it. I only enabled it for iOS < 10.0, but it can be extended to any other platform via CMake flags.

mikaelarguedas · 2017-04-05T18:24:15Z

@ros2/team review this please

wjwwood

I had a few questions but otherwise it looks solid to me.

I'll start some CI.

wjwwood · 2017-04-07T01:54:09Z

rmw/include/rmw/macros.h

 #else
-  #define RMW_THREAD_LOCAL __thread
+  #define RMW_THREAD_LOCAL _Thread_local


Why this change for the non-iOS<10 and non-windows case?

C11 added support for TLS via _Thread_local, __thread is a GCC extension that Clang supports for compatibility reasons. I can revert it to __thread, but I thought that given that IIRC ROS2 now requires a C11-capable compiler, it might be nice to use _Thread_local instead.

The MSVC compiler is far from C11 compliant, AFAIK. So I think that we do not require a C11 compiler. It has been turned on in a few of the packages, but I had explicitly tried to avoid it in rmw. That being said, we already have to special case here for Windows, and the other compilers are likely C11 compliant. I'll either leave this as-is or try to detect when _Thread_local is not available and fallback further to __thread.

wjwwood · 2017-04-07T01:58:34Z

rmw/include/rmw/macros.h

+    // iOS 10 added support for thread local storage
+    #if __IPHONE_OS_VERSION_MAX_ALLOWED < 100000
+      #define RMW_THREAD_LOCAL_PTHREAD 1
+      #define RMW_THREAD_LOCAL


So in this case the PTHREAD alternative is used?

Exactly. If we happen to be targeting iOS < 10.0, the pthread-based implementation of TLS will be used (RMW_THREAD_LOCAL_PTHREAD is defined). This can be expanded to other platforms that have an implementation of pthread, but for now it's only for iOS < 10.0.

After thinking about it, I think this might be somewhat dangerous because if someone is using RMW_THREAD_LOCAL, but doesn't special case it for RMW_THREAD_LOCAL_PTHREAD then they might end up with something that assumes it is thread local, but actually isn't because RMW_THREAD_LOCAL is just empty. I think I'll do #undef RMW_THREAD_LOCAL instead.

wjwwood · 2017-04-07T02:02:47Z

Oh, actually, it looks like it needs to be rebased. I'll do that too.

wjwwood · 2017-04-07T02:05:58Z

rmw/CMakeLists.txt

@@ -21,13 +21,17 @@ set(rmw_sources
 set_source_files_properties(
  ${rmw_sources}
  PROPERTIES LANGUAGE "C")
-add_library(${PROJECT_NAME} SHARED ${rmw_sources})
+add_library(${PROJECT_NAME} ${rmw_sources})


@dirk-thomas this is ok now that we have the "default to SHARED" turned on with https://github.com/ros2/ament_cmake_ros/blob/master/ament_cmake_ros/cmake/build_shared_libs.cmake, right?

No, this package does currently not use ament_cmake_ros since #81 was reverted. Therefore the SHARED needs to stay.

Should we just wait for #81 or should I try this change without removing SHARED? I guess it's not relevant to the rest of the thread local related changes.

@esteve I suppose that's up to you. I can easily remove this part of the patch and rerun CI for you.

I've readded the SHARED keyword in 911bb77 until I can figure out why the ament_cmake_ros changes don't work.

wjwwood · 2017-04-07T02:48:04Z

Looks like rmw is already defaulting to non-shared and causing issues:

02:10:57 /usr/bin/ld: /home/rosbuild/ci_scripts/ws/install/rmw/lib/librmw.a(error_handling.c.o): relocation R_X86_64_TPOFF32 against `__rmw_error_state' can not be used when making a shared object; recompile with -fPIC

-- http://ci.ros2.org/job/ci_linux/2410/consoleFull

@dirk-thomas what is blocking us from removing SHARED from the add_library call and still getting dynamic libraries by default?

@esteve since you're probably wanting to use this in the static linking mode at some point, I guess we should wait to merge this until the above issue is investigated?

wjwwood · 2017-04-07T02:50:20Z

All of the CI I just ran, in case it's helpful for debugging:

Linux:
Linux-aarch64:
macOS:
Windows:

esteve · 2017-04-07T09:17:13Z

@wjwwood thanks for rebasing this PR. I've readded the explicit SHARED keyword, until #81 is resolved so this PR can be merged sooner.

wjwwood · 2017-04-08T01:19:06Z

Ok @esteve, here's new CI with your latest commit:

Linux:
Linux-aarch64:
macOS:
Windows:

…10.0)

…ent_cmake_ros

also undef RMW_THREAD_LOCAL to cause compiler error in the case that pthread must be used

wjwwood · 2017-04-13T02:10:56Z

CI:

Linux:
Linux-aarch64:
macOS:
- (known compiler warning)
Windows:

wjwwood · 2017-04-13T02:11:11Z

Thanks @esteve!

esteve · 2017-04-13T09:56:30Z

@wjwwood thanks for fleshing out this PR. Just a minor comment, I remember deciding not using TargetConditionals.h (df1500b), but unfortunately I forgot why. I think it was because TargetConditionals.h was not available in certain versions of the iOS SDK or because some of its definitions are deprecated, but nevertheless, if any issue shows up, it can be addressed in the future.

Thanks again for testing this!

wjwwood · 2017-04-13T22:17:57Z

@wjwwood thanks for fleshing out this PR. Just a minor comment, I remember deciding not using TargetConditionals.h (df1500b), but unfortunately I forgot why. I think it was because TargetConditionals.h was not available in certain versions of the iOS SDK or because some of its definitions are deprecated, but nevertheless, if any issue shows up, it can be addressed in the future.

Doh. That's what I get for trying to make it better. Ok we can address it if we need to in the future. Hopefully it works for some cases now.

Thanks again for testing this!

No worries, I wanted to get it in before #101 because this whole code block is getting moved to another package for reuse in other places.

esteve · 2017-04-14T10:54:11Z

@wjwwood oh sorry, I hope I didn't sound ungrateful, the code definitely looks much better now. It's actually my fault for not documenting why I didn't use TargetConditionals.h, now I forgot why I opted not to use it 😄

wjwwood · 2017-04-14T20:15:24Z

@wjwwood oh sorry, I hope I didn't sound ungrateful

Not at all.

wjwwood · 2018-10-25T00:14:12Z

@esteve do you know if pthread is still required for iOS? I thought I read that it's no longer required.

The reason I'm asking is that I'm refactoring the error handling code to no longer allocate memory when setting an error (trying to improve memory management) and the pthread specific code is making things really difficult.

I'm simultaneously looking into whether or not other potential platforms need pthread support still as well.

esteve · 2018-10-29T16:34:33Z

I recall needing pthread for TLS with a Linaro toolchain a while ago, but I imagine that's no longer a problem. As for iOS, C11-based TLS is supported since 10. If this code needs to be revived in the future, I can take a look at it, but for now... KILL IT WITH FIRE

ros-discourse · 2025-06-13T05:17:03Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/announcing-objective-c-on-macos-and-ios-for-ros2/946/1

esteve added in progress Actively being worked on (Kanban column) in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Dec 8, 2016

esteve changed the title ~~Added support for pthread-based thread local storage~~ Add support for pthread-based thread local storage Dec 8, 2016

wjwwood approved these changes Apr 7, 2017

View reviewed changes

wjwwood reviewed Apr 7, 2017

View reviewed changes

esteve and others added 5 commits April 11, 2017 22:16

Added support for pthread-based thread local storage (e.g. for iOS < …

5a180b0

…10.0)

Readd SHARED to add_library until ROS2 can be built statically via am…

2c80634

…ent_cmake_ros

change detection of apple OS's

df1500b

also undef RMW_THREAD_LOCAL to cause compiler error in the case that pthread must be used

line length fix

942343f

remove SHARED again now that #98 has been merged

5256729

wjwwood merged commit 3121262 into ros2:master Apr 13, 2017

wjwwood removed the in review Waiting for review (Kanban column) label Apr 13, 2017

esteve deleted the thread-local-ios branch April 13, 2017 09:50

wjwwood mentioned this pull request Apr 13, 2017

Control shared/static linking via BUILD_SHARED_LIBS and ament_cmake_ros ros2/rmw_fastrtps#87

Merged

wjwwood mentioned this pull request Oct 29, 2018

avoid dynamic memory allocation during error handling ros2/rcutils#121

Merged

Add support for pthread-based thread local storage #80

Add support for pthread-based thread local storage #80

Uh oh!

Conversation

esteve commented Dec 8, 2016

Uh oh!

mikaelarguedas commented Apr 5, 2017

Uh oh!

wjwwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wjwwood commented Apr 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wjwwood commented Apr 7, 2017

Uh oh!

wjwwood commented Apr 7, 2017

Uh oh!

esteve commented Apr 7, 2017

Uh oh!

wjwwood commented Apr 8, 2017

Uh oh!

wjwwood commented Apr 13, 2017

Uh oh!

wjwwood commented Apr 13, 2017

Uh oh!

esteve commented Apr 13, 2017

Uh oh!

wjwwood commented Apr 13, 2017

Uh oh!

esteve commented Apr 14, 2017

Uh oh!

wjwwood commented Apr 14, 2017

Uh oh!

wjwwood commented Oct 25, 2018

Uh oh!

esteve commented Oct 29, 2018

Uh oh!

ros-discourse commented Jun 13, 2025

Uh oh!

Uh oh!