In D95184#2517508, @clemenswasser wrote:

You should also change compiler-rt/lib/asan/asan_win.cpp like this:

diff --git a/compiler-rt/lib/asan/asan_win.cpp b/compiler-rt/lib/asan/asan_win.cpp
index 8044ae16ff9b..1577c83cf994 100644
--- a/compiler-rt/lib/asan/asan_win.cpp
+++ b/compiler-rt/lib/asan/asan_win.cpp
@@ -134,7 +134,7 @@ INTERCEPTOR(int, _except_handler4, void *a, void *b, void *c, void *d) {
 static thread_return_t THREAD_CALLING_CONV asan_thread_start(void *arg) {
   AsanThread *t = (AsanThread *)arg;
   SetCurrentThread(t);
-  return t->ThreadStart(GetTid(), /* signal_thread_is_registered */ nullptr);
+  return t->ThreadStart(GetTid());
 }

 INTERCEPTOR_WINAPI(HANDLE, CreateThread, LPSECURITY_ATTRIBUTES security,

Address second round of feedback.

Address first round of feedback.

Address first round of feedback.

Tweak condition to make it easier to read.

@vitalybuka This patch implements the "update the frontier" approach you outlined in the https://reviews.llvm.org/D94210 . The patch depends on https://reviews.llvm.org/D95183 .

This is superseded by https://reviews.llvm.org/D95183 and https://reviews.llvm.org/D95184 .

This change isn't needed anymore.

This change isn't needed anymore.

Thanks for addressing my feedback. LGTM

This seems reasonable but I do have a few nits.

LGTM. @ostannard Thanks for investigating!

I'm not entirely satisfied with this patch -- if folks think we should implement not as an in-process builtin, let me know and I can look into that.

In D89344#2329258, @vitalybuka wrote:

Ubsan ignores volatile vars https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
maybe better to make sure that asan does the same here as well?

In D89344#2329256, @vitalybuka wrote:

I recommend abandon interface change, add a test for llvm/lib/Transforms/Instrumentation if there is no any yet, and document this trick in https://clang.llvm.org/docs/AddressSanitizer.html

In D89344#2328898, @delcypher wrote:

In D89344#2328787, @yln wrote:

Previously attribute((address_space(1))) had to be applied to pointer types to avoid instrumentation.

I wasn't aware of this. Is this a principled way to tell ASan to skip instrumentation of a local var, or is it used because "it works" (but it depends on implementation details)?
Note that I don't know the actual meaning/semantics of address_space.

My understanding is that attribute is used "because it works", not because it's principled in anyway.

IIUC correctly the address_space attribute maps to LLVM IR's notion of address space. Here's a snippet from the LLVM language reference

A global variable may be declared to reside in a target-specific numbered address space. For targets that support them, address spaces may affect how optimizations are performed and/or what target instructions are used to access the variable. The default address space is zero. The address space qualifier must precede any other attributes.

This suggests that there might be downsides to using this attribute (e.g. it might inhibit optimizations)

Would generalizing __attribute__((no_sanitize("address"))) to also apply to local vars (of pointer type) be a cleaner way?
https://clang.llvm.org/docs/AddressSanitizer.html#disabling-instrumentation-with-attribute-no-sanitize-address

I don't think that will work because I think no_sanitize is a function level attribute.

If there is a more principled attribute then I'm happy to change this patch to use it. If such an attribute doesn't exist then I do not think that this patch should be blocked on having a more principled attribute . The whole point of this patch is to introduce a macro that hides the current implementation details. Having the macro introduced in this patch means that we can change the underlying implementation details and users of the macro will not need to care.

In D89344#2328787, @yln wrote:

Previously attribute((address_space(1))) had to be applied to pointer types to avoid instrumentation.

I wasn't aware of this. Is this a principled way to tell ASan to skip instrumentation of a local var, or is it used because "it works" (but it depends on implementation details)?
Note that I don't know the actual meaning/semantics of address_space.

LGTM.

Ah I see. That seems pretty reasonable. I had forgotten that lit_config.params is set from the command line. I'd suggest adding the comment I've suggested to make this clearer.
It would also be helpful to update the commit message to expand on the reason why you are doing this with the reason you just gave me in this review.

@abidh Can you explain why this is needed? The emulator is set at CMake configure time and its value depends on the target being tested. Why do you want to override this with a value from the lit_config object?

Fix typo.

Address first round of feedback.

This patch is superseded by https://reviews.llvm.org/D89020.

In D88807#2312194, @yln wrote:

Flaky tests are the worst! Thanks for working on this :)

I think the principled way here would be to have two separate lit invocations for timeout fail/pass:

not lit --timeout=1 infinite_hang.py
# Will hang for 1 second, then aborted by timeout.

lit --timeout=999 short.py
# Will take however long it takes to do `short.py`, then pass. 999 just means "very long timeout"
# This will be immediate on normal hosts, and whatever time it takes on resource constrained hosts.

Or am I missing something?

LGTM provided that you explain in the commit message why this change is being made.

In D86379#2248114, @kubamracek wrote:

Also, I understand that the code in question isn't best already, but I don't have the bandwidth to refactor and test any more extensive changes. I'm only trying to address the problem of getting -arch arm64 / arm64e passed into tests, otherwise we're testing the wrong thing. Let's focus on that, please.

Side note: This get_test_cc_for_arch function is bad in multiple ways.

Could you use get_test_cflags_for_apple_platform platform instead at all the call site?

You should probably refer to https://reviews.llvm.org/D85803 or the commit that removed -mmacosx-version-min=10.12 in the commit message. Other than that LGTM. Thanks for cleaning this up.

@yln Are you sure this actually works for all platforms, in particular the watchOS simulator? The code you link (https://github.com/llvm/llvm-project/blob/master/compiler-rt/lib/tsan/CMakeLists.txt#L122) is not the minimum deploy target. That code just checks the SDK version used to build which is different from the minimum deploy target used.

LGTM. Other than the nit.

@vitalybuka Do I need to worry about overflow here, i.e. kHighMemEnd is ULONG_MAX on some platform in which case we'd overflow and therefore ask for a shadow size of 0?

@tejohnson Side comment about the refactor. The comments on MapDynamicShadow(...) say that is function is actually supposed to mmap the found memory as NO_ACCESS. The current implementation doesn't do this. It just finds a space but doesn't mmap it. Is this a problem?

@yln Note there are other bugs here but I plan to fix this in future commits (e.g. if (size < gap_size) should be if (size <= gap_size)).

In D85378#2200349, @tejohnson wrote:

In D85378#2198320, @delcypher wrote:

@tejohnson Looks like this bug is present on Linux too

uptr FindDynamicShadowStart() {
  uptr shadow_size_bytes = MemToShadowSize(kHighMemEnd);
#if ASAN_PREMAP_SHADOW          
  if (!PremapShadowFailed())
    return FindPremappedShadowStart(shadow_size_bytes);
#endif 

Thanks. Added @vitalybuka as reviewer since he may have a better understanding of the implications, I only just started looking at sanitizer code recently. Similar question to @yln - does this manifest now given the page size alignment? Could you fix the linux code as well?

In D85378#2200000, @yln wrote:

This latent bug was likely hidden because the shadow memory size is
always page aligned due to being allocated by mmap.

Is this bug still latent, or does it manifest now? Can you describe what happens when it is triggered?

@tejohnson Looks like this bug is present on Linux too

LGTM

LGTM except for the lint checks failing. Please fix those before landing.

Looks really good. I just have a few nits.

Fix failing test due to LSan.