Prevent introduction of a dependency of libasan.a on libstdc++

[serge-sans-paille]

Summary

This an attempty to fix an issue introduced by https://reviews.llvm.org/D70662

Function-scope static initialization are guarded in C++, so we should probably not use it because it introduces a dependency on __cxa_guard* symbols.
In the context of clang, libasan is linked statically, and it currently needs to the odd situation where compiling C code with clang and asan requires -lstdc++.

I'm unsure of the portability requirements, providing a potential solution in this review.

[jakubjelinek]

This is certainly wrong. It will happily return 1 when some other thread is changing it, and the store is non-atomic.
Perhaps you want something like:

static atomic_uint32_t  kAltStackSize;
uptr ret = atomic_load(&kAltStackSize, memory_order_relaxed);
if (ret == 0) {
  ret = SIGSTKSZ * 4;
  atomic_store(&kAltStackSize, ret, memory_order_relaxed);
}
return ret;

on the assumption that SIGSTKSZ will always evaluate to the same non-zero constant and it is ok to evaluate it more than once, but faster not to do that if possible.
Would be nice to get rid of the overhead if SIGSTKSZ is a constant expression though, maybe:

if (__builtin_constant_p (SIGSTKSZ * 4))
  return SIGSTKSZ * 4;
else {
  // Above atomic stuff.
}

[serge-sans-paille]

In D102475#2759153, @jakubjelinek wrote:

This is certainly wrong. It will happily return 1 when some other thread is changing it, and the store is non-atomic.

I understand why the store needs to be atomic, but I don't understand why the CAS may return true more than once (assuming that SIGSTKSZ is different from zero) ?

[jakubjelinek]

It will succeed once. But if another thread calls GetAltStackSize() while the first one just returned from __sync_bool_compare_and_swap(&kAltStackSize, 0, 1) but hasn't yet stored the SIGSTKSZ * 4 value (e.g. is inside of sysconf), or even has stored, but the value hasn't propagated to other CPU caches yet, then they can see kAltStackSize of 1 rather than the right value.

[serge-sans-paille]

@eugenis are you fine with that change? the maintenance cost seems low, and the benefit for the end user seems valuable enough (?)

[serge-sans-paille]

Remove clang-formatted noise

[serge-sans-paille]

@eugenis any thought on that one? Without this patch, clang -fsanitize=address main.c fails because it needs C++ symbol.

[serge-sans-paille]

@kcc any thoughts on that one?

[serge-sans-paille]

@kcc @eugenis : up ? I applied the patch to the Fedora package because it ruins the user experience otherwise. I think it's worth considering applying it to main too.

[kcc]

+Vitaly Buka <[e-mail]> +Matt Morehouse <[e-mail]>

[vitalybuka]

LGTM

[jakubjelinek]

compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp

170

As I said earlier, I'd add before this

#ifdef __has_builtin
#if __has_builtin(__builtin_constant_p)
  if (__builtin_constant_p(SIGSTKSZ * 4))
    return SIGSTKSZ * 4;
#endif
#endif

because if SIGSTKSZ * 4 is a constant, there is no need to bother with the atomic loads/stores.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp

170

Thanks, I agree.
Looks like there was a claim that SIGSTKSZ is not constant https://reviews.llvm.org/D100645
I believe unless we have an evidence of that we should threat this as a constant.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp

170

And if we know a platform where it's not constant please use __builtin_constant_p trick

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp

170

Actually how about just static_assert(_builtin_constant_p(SIGSTKSZ * 4)) so we quickly find what is broken?

[jakubjelinek]

The reason why the previous change was done is because it is no longer a constant on Linux with recent glibc (2.34 to be released later this year). But it is still constant for older glibcs, or on non-Linux.

[vitalybuka]

In D102475#2803426, @jakubjelinek wrote:

The reason why the previous change was done is because it is no longer a constant on Linux with recent glibc (2.34 to be released later this year). But it is still constant for older glibcs, or on non-Linux.

__builtin_constant_p then

Can we have a link into glibc source for future reference with a short explanation why we can just "return SIGSTKSZ * 4;" always ?

[jakubjelinek]

https://reviews.llvm.org/D100645 contains the details.

[vitalybuka]

I looked up all calls to this function and we use this function once per thread, I don't see a point in this caching
Following should be enough and solve symbols issues.

static uptr GetAltStackSize() {
  return SIGSTKSZ * 4;
}

[jakubjelinek]

It used to be 3 times per thread, now it is 2 times per thread.

[vitalybuka]

I am fine with up to 10 :)

[serge-sans-paille]

Do not cache the (potential) function call.

compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp

 // run-time libraries and implements libc-dependent POSIX-specific functions
 // from sanitizer_libc.h.
 //===----------------------------------------------------------------------===//
 
 #include "sanitizer_platform.h"
 
 #if SANITIZER_POSIX
 
+#include "sanitizer_atomic.h"
 #include "sanitizer_common.h"
 #include "sanitizer_flags.h"
 #include "sanitizer_platform_limits_netbsd.h"
 #include "sanitizer_platform_limits_posix.h"
 #include "sanitizer_platform_limits_solaris.h"
 #include "sanitizer_posix.h"
 #include "sanitizer_procmaps.h"
 
 #include <errno.h>
 #include <fcntl.h>
 #include <pthread.h>
 #include <signal.h>
 #include <stdlib.h>
 #include <sys/mman.h>
 #include <sys/resource.h>
 #include <sys/stat.h>
 #include <sys/time.h>
 #include <sys/types.h>
 #include <sys/wait.h>
 #include <unistd.h>
 
 #if SANITIZER_FREEBSD
 // The MAP_NORESERVE define has been removed in FreeBSD 11.x, and even before
 // that, it was never implemented.  So just define it to zero.
 #undef MAP_NORESERVE
 #define MAP_NORESERVE 0
 #endif
 
 typedef void (*sa_sigaction_t)(int, siginfo_t *, void *);
 

 
 bool SupportsColoredOutput(fd_t fd) {
   return isatty(fd) != 0;
 }
 
 #if !SANITIZER_GO
 // TODO(glider): different tools may require different altstack size.
 static uptr GetAltStackSize() {
+  static atomic_uintptr_t kAltStackSize{0};
+  uptr ret = atomic_load(&kAltStackSize, memory_order_relaxed);
+  if (ret == 0) {
     // SIGSTKSZ is not enough.
-  static const uptr kAltStackSize = SIGSTKSZ * 4;
-  return kAltStackSize;
+    ret = SIGSTKSZ * 4;
+    atomic_store(&kAltStackSize, ret, memory_order_relaxed);
+  }
+  return ret;
 }
 
 void SetAlternateSignalStack() {
   stack_t altstack, oldstack;
   CHECK_EQ(0, sigaltstack(nullptr, &oldstack));
   // If the alternate stack is already in place, do nothing.
   // Android always sets an alternate stack, but it's too small for us.
   if (!SANITIZER_ANDROID && !(oldstack.ss_flags & SS_DISABLE)) return;