Make calls into the pthread library use weak symbols.

[saugustine]

Summary

This makes the formerly hard dependency on libpthread into a weak
dependency.

[ldionne]

What problem are you trying to solve with this?

I think we talked about it already in a different setting, but why not implement a no-op RWMutex instead?

[saugustine]

This is better than a no-op RWMutex because then the same binary works both when the application is multi-threaded and when it isn't. Therefore the build doesn't have to distinguish between the two cases.

[ldionne]

libunwind/src/RWMutex.hpp

62 ↗

(On Diff #193780)

When you redeclare functions like this, are attributes stacked with the attributes of other (previous) declarations?

73 ↗

(On Diff #193780)

Just to make sure I understand: pthread_create != nullptr is only going to be satisfied if pthread_create was linked into the program, _and_ it was used at least once before? I thought it would be non-null whenever it was linked into the program, whether you used it or not.

[saugustine]

libunwind/src/RWMutex.hpp

62 ↗

(On Diff #193780)

The gcc documentation says so.

6.33 Declaring Attributes of Functions
... Compatible attribute specifications on distinct declarations of the same function are merged....

The clang docs aren't specific on this point. But I know of enough uses of this style that I would consider it a bug if it didn't.

73 ↗

(On Diff #193780)

It will be non-null only if it was linked. Whether or not it was used makes no difference.

This is more conservative than strictly necessary, but it saves the complexity of tracking whether or not multiple threads are actually active.

[saugustine]

ping?

[ldionne]

@kledzik

Nick, what do you think about adding weak symbols just for achieving this?

[saugustine]

In D60285#1461209, @ldionne wrote:

@kledzik

Nick, what do you think about adding weak symbols just for achieving this?

Nick any thoughts? I'd like to move forward, for whatever definition of "forward" we may settle on.

[mtrent]

In D60285#1467441, @saugustine wrote:

In D60285#1461209, @ldionne wrote:

@kledzik

Nick, what do you think about adding weak symbols just for achieving this?

Nick any thoughts? I'd like to move forward, for whatever definition of "forward" we may settle on.

Nick is out of the office this week. He will be back next week.

[saugustine]

Hi Nick--hope your vacation was good.

When you get a chance, would you respond to ldionne's question above?

[kledzik]

Can this be conditionalized to not happen on macOS/iOS? I can see needing this on platforms where pthreads is optional, but it looks like this code will apply to macOS too, which introduces risk there.

[ldionne]

Given this, I think implementing a no-op RWMutex is a better solution. And if you need the ability for the same library to work in both cases, maybe this code could be in a WeakRWMutex (or something like that).

[saugustine]

• Add an option to make pthread symbol references weak.

[saugustine]

I've now put the weak-reference version behind a non-default build option. Is this acceptable?

[ldionne]

libunwind/src/CMakeLists.txt

70 ↗

(On Diff #198718)

This is not an existing CMake option, is it? You should add it.

[saugustine]

• Make LIBUNWIND_WEAK_PTHREAD a formal Cmake option.

[saugustine]

Any of the previous reviewers have a comment before I commit?

libunwind/trunk/CMakeLists.txt

 option(LIBUNWIND_ENABLE_ASSERTIONS "Enable assertions independent of build mode." ON)
 option(LIBUNWIND_ENABLE_PEDANTIC "Compile with pedantic enabled." ON)
 option(LIBUNWIND_ENABLE_WERROR "Fail and stop if a warning is triggered." OFF)
 option(LIBUNWIND_ENABLE_SHARED "Build libunwind as a shared library." ON)
 option(LIBUNWIND_ENABLE_STATIC "Build libunwind as a static library." ON)
 option(LIBUNWIND_ENABLE_CROSS_UNWINDING "Enable cross-platform unwinding support." OFF)
 option(LIBUNWIND_ENABLE_ARM_WMMX "Enable unwinding support for ARM WMMX registers." OFF)
 option(LIBUNWIND_ENABLE_THREADS "Build libunwind with threading support." ON)
+option(LIBUNWIND_WEAK_PTHREAD_LIB "Use weak references to refer to pthread functions." OFF)
 option(LIBUNWIND_USE_COMPILER_RT "Use compiler-rt instead of libgcc" OFF)
 option(LIBUNWIND_INCLUDE_DOCS "Build the libunwind documentation." ${LLVM_INCLUDE_DOCS})
 
 set(LIBUNWIND_LIBDIR_SUFFIX "${LLVM_LIBDIR_SUFFIX}" CACHE STRING
     "Define suffix of library directory name (32/64)")
 option(LIBUNWIND_INSTALL_LIBRARY "Install the libunwind library." ON)
 cmake_dependent_option(LIBUNWIND_INSTALL_STATIC_LIBRARY
   "Install the static libunwind library." ON

libunwind/trunk/src/CMakeLists.txt

if (LIBUNWIND_USE_COMPILER_RT)
   list(APPEND libraries "${LIBUNWIND_BUILTINS_LIBRARY}")
 else()
   unwind_append_if(libraries LIBUNWIND_HAS_GCC_S_LIB gcc_s)
   unwind_append_if(libraries LIBUNWIND_HAS_GCC_LIB gcc)
 endif()
 unwind_append_if(libraries LIBUNWIND_HAS_DL_LIB dl)
 if (LIBUNWIND_ENABLE_THREADS)
   unwind_append_if(libraries LIBUNWIND_HAS_PTHREAD_LIB pthread)
+  unwind_append_if(LIBUNWIND_COMPILE_FLAGS LIBUNWIND_WEAK_PTHREAD_LIB -DLIBUNWIND_USE_WEAK_PTHREAD=1)
 endif()
 
 # Setup flags.
 unwind_append_if(LIBUNWIND_CXX_FLAGS LIBUNWIND_HAS_NO_RTTI_FLAG -fno-rtti)
 
 unwind_append_if(LIBUNWIND_LINK_FLAGS LIBUNWIND_HAS_NODEFAULTLIBS_FLAG -nodefaultlibs)
 
 # MINGW_LIBRARIES is defined in config-ix.cmake

libunwind/trunk/src/RWMutex.hpp

bool unlock() {
     ReleaseSRWLockExclusive(&_lock);
     return true;
   }
 
 private:
   SRWLOCK _lock = SRWLOCK_INIT;
 };
 
-#else
+#elif !defined(LIBUNWIND_USE_WEAK_PTHREAD)
 
 class _LIBUNWIND_HIDDEN RWMutex {
 public:
-  bool lock_shared() { return pthread_rwlock_rdlock(&_lock) == 0; }
+  bool lock_shared() { return pthread_rwlock_rdlock(&_lock) == 0;  }
   bool unlock_shared() { return pthread_rwlock_unlock(&_lock) == 0; }
   bool lock() { return pthread_rwlock_wrlock(&_lock) == 0; }
   bool unlock() { return pthread_rwlock_unlock(&_lock) == 0; }
 
 private:
   pthread_rwlock_t _lock = PTHREAD_RWLOCK_INITIALIZER;
 };
 
+#else
+
+extern "C" int __attribute__((weak))
+pthread_create(pthread_t *thread, const pthread_attr_t *attr,
+               void *(*start_routine)(void *), void *arg);
+extern "C" int __attribute__((weak))
+pthread_rwlock_rdlock(pthread_rwlock_t *lock);
+extern "C" int __attribute__((weak))
+pthread_rwlock_wrlock(pthread_rwlock_t *lock);
+extern "C" int __attribute__((weak))
+pthread_rwlock_unlock(pthread_rwlock_t *lock);
+
+// Calls to the locking functions are gated on pthread_create, and not the
+// functions themselves, because the data structure should only be locked if
+// another thread has been created. This is what similar libraries do.
+
+class _LIBUNWIND_HIDDEN RWMutex {
+public:
+  bool lock_shared() {
+    return !pthread_create || (pthread_rwlock_rdlock(&_lock) == 0);
+  }
+  bool unlock_shared() {
+    return !pthread_create || (pthread_rwlock_unlock(&_lock) == 0);
+  }
+  bool lock() {
+    return !pthread_create || (pthread_rwlock_wrlock(&_lock) == 0);
+  }
+  bool unlock() {
+    return !pthread_create || (pthread_rwlock_unlock(&_lock) == 0);
+  }
+
+private:
+  pthread_rwlock_t _lock = PTHREAD_RWLOCK_INITIALIZER;
+};
+
 #endif
 
 } // namespace libunwind
 
 #endif // __RWMUTEX_HPP__