See ongoing disscussion in https://reviews.llvm.org/D87149
I believe TLDR is that unless these miscompiles are for volatile stores,
it is GWP-ASan that needs to be fixed.

Warning: I don't know anything about GWP-ASan, and I'm not experienced in the UB subtleties exposed by my patch...
but a first grep of that dir shows the likely problematic function:

void GuardedPoolAllocator::trapOnAddress(uintptr_t Address, Error E) {
State.FailureType = E;
State.FailureAddress = Address;

// Raise a SEGV by touching first guard page.
volatile char *p = reinterpret_cast<char *>(State.GuardedPagePool);
*p = 0;
__builtin_unreachable();
}

IIUC, we are probably going to change LLVM to allow this usage. But would it be safer/better to use __builtin_trap() or __builtin_debugtrap() in this function?

cc'ing people that I hopefully correctly cross-referenced from:
https://reviews.llvm.org/D60593

Warning: I don't know anything about GWP-ASan, and I'm not experienced in
the UB subtleties exposed by my patch...
but a first grep of that dir shows the likely problematic function:

void GuardedPoolAllocator::trapOnAddress(uintptr_t Address, Error E) {
State.FailureType = E;
State.FailureAddress = Address;

// Raise a SEGV by touching first guard page.
volatile char *p = reinterpret_cast<char *>(State.GuardedPagePool);
*p = 0;
__builtin_unreachable();
}

IIUC, we are probably going to change LLVM to allow this usage. But would it
be safer/better to use __builtin_trap() or __builtin_debugtrap() in this
function?

Nice catch, thanks!
Using __builtin_trap() would be better indeed. unreachable is quite nasty.

Warning: I don't know anything about GWP-ASan, and I'm not experienced in
the UB subtleties exposed by my patch...
but a first grep of that dir shows the likely problematic function:

void GuardedPoolAllocator::trapOnAddress(uintptr_t Address, Error E) {
State.FailureType = E;
State.FailureAddress = Address;

// Raise a SEGV by touching first guard page.
volatile char *p = reinterpret_cast<char *>(State.GuardedPagePool);
*p = 0;
__builtin_unreachable();
}

IIUC, we are probably going to change LLVM to allow this usage. But would it
be safer/better to use __builtin_trap() or __builtin_debugtrap() in this
function?

Nice catch, thanks!
Using __builtin_trap() would be better indeed. unreachable is quite nasty.

I can confirm that changing the __builtin_unreachable() to __builtin_trap() does seem to fix the problem!

While the "problem" will be address either way, I would generally think __builtin_trap() is the better way to cause a trap.

Note that we have at least temporarily restored the failing tests to passing:
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/29534

...with this patch:
4e413e1

But it sounds like changing the GWP-ASan source would be safer.

The volatile store in GWP-ASan is (by designed) supposed to crash the program with a segv at a specific address.

The __builtin_unreachable() is there to guide the compiler that it can tail-call optimise this function.

It seems to me that the root cause is that the __builtin_unreachable() shouldn't be allowed to dead-code eliminate things with side effects (like the volatile store)...

The volatile store in GWP-ASan is (by designed) supposed to crash the
program with a segv at a specific address.

The __builtin_unreachable() is there to guide the compiler that it can
tail-call optimise this function.

It seems to me that the root cause is that the __builtin_unreachable()
shouldn't be allowed to dead-code eliminate things with side effects (like
the volatile store)...

As we discuss in the thread https://reviews.llvm.org/D87149, the LLVM LangRef needs to be updated to provide the above guarantees. That said, generic "side effects", in the LLVM-IR sense, will not be preserved if they are inevitable followed by an unreachable. That is, a regular store to a global for example is a side effect but it won't be preserved. volatile accesses are different and you will probably get what you expect here, we'll update this once the discussion is settled.

The volatile store in GWP-ASan is (by designed) supposed to crash the
program with a segv at a specific address.

The __builtin_unreachable() is there to guide the compiler that it can
tail-call optimise this function.

It seems to me that the root cause is that the __builtin_unreachable()
shouldn't be allowed to dead-code eliminate things with side effects (like
the volatile store)...

As we discuss in the thread https://reviews.llvm.org/D87149, the LLVM
LangRef needs to be updated to provide the above guarantees. That said,
generic "side effects", in the LLVM-IR sense, will not be preserved if
they are inevitable followed by an unreachable. That is, a regular store to
a global for example is a side effect but it won't be preserved. volatile
accesses are different and you will probably get what you expect here, we'll
update this once the discussion is settled.

FYI looks like GWP-ASan still hasn't been fixed,
nor did anybody submit an RFC to change LangRef.
Chances are, these tests will break again tomorrow,
once the temporary patch expires as part of https://reviews.llvm.org/D105338

Sent https://reviews.llvm.org/D105654 to add a workaround in GWP-ASan, but __builtin_unreachable() optimising away code with side effects still needs to be fixed.

Sent https://reviews.llvm.org/D105654 to add a workaround in GWP-ASan, but __builtin_unreachable() optimising away code with side effects still needs to be fixed.

trap is the correct fix. Deleting side effects before unreachable is not wrong