[PATCH] D103304: Update and improve compiler-rt tests for -mllvm -asan_use_after_return=(never|[runtime]|always).

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka accepted this revision.
vitalybuka added a comment.
This revision is now accepted and ready to land.

Other then comment at asan_rtl.cpp:39 LGTM

In D103304#2794683 <https://reviews.llvm.org/D103304#2794683>, @eugenis wrote:

> `__asan_detect_use_after_return`, any variant of it, is not entirely correct. First, using the presence of the global is better than its value, because the linker will pick a random instance in case they disagree, while `&(...) != nullptr` gives reliable OR semantics.
>
> Second, this does not handle `dlopen` out of the box. It can be almost fixed by calling something from a library constructor (like __asan_init) and passing the address/value of the UAR setting, but even that is not 100% correct as code from a library may run before any of that library's constructors. It will also require late-initialization of fake stack on all existing threads at the time of `dlopen`.
>
> Lazy init would work, but need to make sure that fake stack init is async signal safe, because the first use in a thread may be in a signal context. Another option is to make sure that unused fake stack is cheap, and initialize it always. I don't know if that is the case right now.
>
> Having said all this, the implementation in this revision will kind of work in most cases, and the worst consequence of a mistake is some performance loss, so I'm fine with the change as is.

We discussed this disadvantages with @kda already offline.
I guess patch like this does not add any difficulties to move Lazy one after we we investigate signal safety or other possible implications.



================
Comment at: compiler-rt/lib/asan/asan_rtl.cpp:402
+  if (&__asan_detect_use_after_return_always &&
+      __asan_detect_use_after_return_always) {
+    __asan_option_detect_stack_use_after_return = 1;
----------------
kda wrote:
> morehouse wrote:
> > According to the comment above, we should not override the runtime flag when this is 1.
> Check the code in AddressSanitizer, this global (__asan_detect_use_after_return_always) is only created when ClUseAfterReturn is set to 2.
I guess @morehouse comment is about comment on the line 39.
It makes impression that value stored makes a difference, but as we discussed before only presence matters.
I would put there just:
if  __asan_detect_use_after_return_always is defined, runtime must not check detect_stack_use_after_return and create FakeStack


We don't expect 0 there
```
if (&__asan_detect_use_after_return_always) {
  CHECK_EQ(1, __asan_detect_use_after_return_always);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103304