[PATCH] D146351: sanitizer_common: Use plain thread_local for __sancov_lowest_stack definition.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

In D146351#4207181 <https://reviews.llvm.org/D146351#4207181>, @yln wrote:

> Hi, thanks Vitaly!
>
> AFAIR, `thread_local` (and TLS in general) require macOS 10.12 / iOS 10 / watchOS 3 or greater.
>
> Therefore, can we hold off on this a bit?
>
> @thetruestblue is looking into bumping the minimal sanitizer deployment targets for Apple platforms.  Blue, when you do that, can you circle back here and apply this patch?  This change is a good example for a sanity check: it doesn't work right now but should work afterwards.

`__sancov_lowest_stack` is only used by the sancov instrumentation pass when `-fsanitize-coverage=stack-depth` is passed, which is not the default. This option can't possibly work if SANITIZER_TLS_INITIAL_EXEC_ATTRIBUTE was defined to an empty string, because the instrumentation pass will unconditionally define the variable with TLS: https://github.com/llvm/llvm-project/blob/f67b481098cc30567d0f50a2b21f8f57b92052bd/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp#L489

So I think this is broken on the platforms where SANITIZER_TLS_INITIAL_EXEC_ATTRIBUTE is an empty string. That means we can wrap the definition of `__sancov_lowest_stack` in an `#if !SANITIZER_APPLE` and the code will work on Apple platforms as well as it did before (i.e. the runtime will build, but the compiler flag won't work). I'll update the patch to do that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146351/new/

https://reviews.llvm.org/D146351