This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by serge-sans-paille on May 14 2021, 2:59 AM.

Details

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.

Diff Detail

Event Timeline

serge-sans-paille requested review of this revision.May 14 2021, 2:59 AM
serge-sans-paille created this revision.

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 retitled this revision from Prevent introduction of a dependecy of libasan.a on libstdc++ to Prevent introduction of a dependency of libasan.a on libstdc++.May 14 2021, 5:23 AM

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) ?

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.

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

Remove clang-formatted noise

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

@kcc any thoughts on that one?

@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.

+Vitaly Buka <vitalybuka@google.com> +Matt Morehouse <mascasa@google.com>

This revision is now accepted and ready to land.Jun 7 2021, 10:53 AM
jakubjelinek added inline comments.Jun 7 2021, 10:56 AM
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 requested changes to this revision.Jun 7 2021, 11:11 AM
vitalybuka added inline comments.
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.

This revision now requires changes to proceed.Jun 7 2021, 11:11 AM
vitalybuka added inline comments.Jun 7 2021, 11:13 AM
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 added inline comments.Jun 7 2021, 11:14 AM
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?

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.

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 ?

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;
}

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

I am fine with up to 10 :)

Do not cache the (potential) function call.

vitalybuka accepted this revision.Jun 8 2021, 10:04 AM
This revision is now accepted and ready to land.Jun 8 2021, 10:04 AM
This revision was landed with ongoing or failed builds.Jun 9 2021, 12:40 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2021, 12:40 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript