LGTM, thank you.

Hmm, this is not 100% correct. CFI by design respects caller's settings for -fsanitize-recover and -fsanitize-trap, communicated through the DiagData argument to __cfi_check. A better default implementation would call __cfi_check_fail instead of trap unconditionally.

This is long overdue, I think. Thank you!

I'm far from a C or C++ expert, but FWIW I'm not entirely convinced that this example breaks language semantics.

LGTM

I do not understand the bionic issue. __hwasan_thread_exit is called very late (as the last thing in that function), while the signals are blocked. Why do thread cleanup functions matter?

I'm fine with it in general. Is asan_abi.cpp meant as a temporary stub? It's not even link anywhere in the current version.

LGTM

LGTM, thanks!

Sorry, I do not remember why this requirement is there. Must be related to shadow / allocator placement and kernel mapping conflicts, but hwasan is using dynamic shadow so that should not be an issue... LGTM as long as it works.

Not convinced that this is right. The original fix is for a false postive, not a false negative - i.e., we want to prevent speculation of a memory access that is not provably safe.

I think this should generally work.

LGTM

LGTM but please make a better test case.

LGTM, thank you!

I've uploaded D141978 with a revert. I've opted against introducing a new subtarget feature because that would mean keeping dead code in the repo, without a way of testing it.

LGTM

LGTM with a comment

Perfect, LGTM

Hmm do we allocate the ring buffer when tracking is not required? This would be a waste of a page.

I think using the environment part of the triple is the right choice. We could also just go ahead and flip the current behavior until there is evidence of a platform that configures SCTRL_EL1.BT0 differently, at which point they can implement the triple logic.

LGTM

This change claims that PACIASP has an implicit BTI JC. Is that correct? From my reading of the spec (and I have about 50% certainty that I'm right), the behavior is actually the same as of BTI C. At least this is true on Linux, where SCTRL_EL1.BT0 is set to 1.

LGTM

LGTM

Do you have a stress test to exhaust all gwp-asan slots?

It's a single mprotect across the gwp-asan region. Also no need to bother with recursive guards as no new allocations can reach gwp-asan.

It seems like this could become much simpler if we disable gwp-asan on the first report instead of killing slots one-by-one.

LGTM

If there are no concerns about deprecating 39 and 42 bit VMA support in MSan, then I'm fine with this. I'd love to see performance comparison of static vs dynamic shadow, but this is a separate issue.

addressed Florian's comment

ping

expanded comments

If we are changing the mapping + the ABI, we should be 100% confident the new one covers all ASLR possibilities. Why not just run some binary a lot of times and collect the range of addresses, or even inspect the kernel source for possible executable locations? Also, make it a large binary.

ping

ping

Ah, so is the problem with ASLR randomizing the initial executable mappings over a region larger than 64Gb? We do not care about app allocating memory, heap placement is defined by msan. Could you confirm that the new mapping covers all possible locations? Even with ex. a huge executable binary.

Sorry, I don't follow. What is limited to 64Gb (the sum of all app regions? why does it matter?) and why can't an "invalid" region be mapped?

As explained in D117635, kPageSizeBits is part of the compiler contract and must be a compile-time constant.

LGTM, but I'd merge the two changes. I don't really see the point of submitting a test for the broken behavior first.

LGTM

I like the direction of this change and agree that it needs a test.

I think this is a fix for LazyInitialize in the tsan_rtl.h - the logic there assumes that when preinit_array is always used whenever it can be used, and no further initialization is necessary. This assumption does not hold under -shared-libsan. IMO this should be fixed in LazyInitialize, not here.

LGTM

LGTM

LGTM

stage2 is fine with me, too
LGTM

I think I'd rather keep left/right as internal concepts and only translate them to before/after in the output.

Change description says that the new pass "marks them as tagged". That's not what is happening.

Well compile-time may be hard. Hwasan does some runtime checks in InitShadow, that's ok too

Maybe add a few compile-time checks that mem->shadow->mem returns the original value? At least the sparc equations are complicated enough.

Could you also fix asan and msan for consistency?

LGTM

LGTM

LGTM

LGTM

I think this is a pretty minor change - we are not touching the general structure of the report, nor introducing a new error type (even though the latter happens from time to time).

LGTM

LGTM

Sorry but I had to revert this because of the conflicts with another revert: https://reviews.llvm.org/D131065