LGTM

LGTM

In D63736#1556524, @vlad.tsyrklevich wrote:

In D63736#1556463, @hctim wrote:

In D63736#1556349, @eugenis wrote:

Do we need similar protection in deallocate?

We shouldn't need to. GPA::deallocate() may call malloc() and free(), but can only be called with guarded allocations. Recursion is okay (and desirable, as the unwinder's implementation is okay to be sampled) in deallocate().

Don't we hold the Mutex during AllocationMetadata::RecordDeallocation()? Doesn't that mean that the unwinder could cause deadlock during deallocation?

Do we need similar protection in deallocate?

LGTM

LGTM

LGTM nice!

LGTM

More ideas for tests: multi dimensional arrays, two levels of inlining, lexical scopes.

Please check what happens in "weird" cases with dynamic stack realignment or dynamic sp adjustment. If one or both of these cases can not be handled with this debug info format, can we at least avoid getting confused by them? Are we simply missing fp-offset in the debug info if the offset is not statically known? Add a test case.

LGTM
I still think my suggestion for the name is best, but don't care about it too much.

LGTM

LGTM

LGTM

LGTM

remove commented lines

append_list_if(COMPILER_RT_HAS_FPIC_FLAG -fPIC GWP_ASAN_CFLAGS)

We can make the tags undeterministic again by starting each ring buffer at a random index.

In D63300#1543961, @eugenis wrote:

What if we do it the other way around - store SP instead of FP? SP is unusable for locals when there are dynamic allocas; FP is unusable when there is dynamic stack realignment; not sure which case is more common, but both should be rare enough. This could help with the code size at least a little.

What if we do it the other way around - store SP instead of FP? SP is unusable for locals when there are dynamic allocas; FP is unusable when there is dynamic stack realignment; not sure which case is more common, but both should be rare enough. This could help with the code size at least a little.

LGTM

Thanks!
LGTM

The instrumentation pass change looks good.
There is a test for fdiv in msan_basic.ll, you can do something similar.

LGTM

LGTM

It's a refactoring leftover.

+ IR test

comments

@thakis @pcc
LGTM

I think the idea is that implementing our own spinlock is not much harder than having 3 platform-specific implementations (posix, window, fuchsia).
Also, scudo_standalone is doing the same ( @cryptoad, why? ).

Ha, I think it matches LLVM perfectly :)
G, of course, stands for "Galaxy".

LGTM

sorry for the delay, I'll take a look later this week

That's fine, thank you.
I need to submit the change manually anyway.

add a comment

Should this be an error and cause an "unsupported configuration"?

But we don't really know that, because a change that makes this configuration supported needs to be in libc or in platform build files, not in compiler-rt. I'd rather not block anyone from experimenting.

What about makeing it a warning, "untested configuration"? Obviously your call, just seems prudent to me.

In D61337#1487120, @winksaville wrote:

@eugenis, which of the following conditions have tests?

HWASAN_WITH_INTERCEPTORS=OFF, SANITIZER_ANDROID=OFF

This configuration does not exist.

Should this be an error and cause an "unsupported configuration"?

In D61337#1486551, @winksaville wrote:

@eugenis, which of the following conditions have tests?

HWASAN_WITH_INTERCEPTORS=OFF, SANITIZER_ANDROID=OFF

HWASAN_WITH_INTERCEPTORS=OFF is a mode where we rely on libc to call hwasan_thread_enter and hwasan_thread_exit as appropriate. This is particularly important when libc is built with hwasan, because then instrumented code may run on a thread before GetCurrentThread is called. At this moment, only bionic supports this, as far as I know.

See https://reviews.llvm.org/D61337

Sorry I've missed your comment. I've uploaded a different fix for __hwasan_tls problem:
https://reviews.llvm.org/D61337

disable sanitizer_common reallocarray test on darwin

LGTM
Thank you!

This change includes D61155.

Looks fine to me. @kcc PTAL

LGTM

LGTM

I think __hwasan_tls should be defined if !SANITIZER_ANDROID, without the other condition.

IMHO, we could use printf() in gwpasan directly, with a guard against recursive malloc.

I don't think this will work in a signal-safe manner. The only times that GwpAsan calls printf() is in the signal handler, and even if we temporarily disable sampled allocations, we still are exercising non-reentrant functions in a signal handler.

LGTM

If the atomic load ends up on the hot path, we could try moving it to the same cache line as NextSampleCounter. That would make it thread local, but it can be initialized from the global variable on the slow path.

LGTM

In D60617#1467093, @glider wrote:

In D60617#1464971, @eugenis wrote:

Please test the case when alloca can not be found.
See https://bugs.llvm.org/show_bug.cgi?id=41481 for the reference.

Ok, will do. The repro in the bug doesn't produce IR containing a select statement for me, but the idea is clear.

With this change msan will start detecting new bugs, ex.:
for (int i = 0; ...; ++i) {

int x;
if (i == 0) x=0;
read(x);

}

It can also slow down some programs a lot.

Agreed. This also makes less sense without origins - do you think we shall hide it behind track-origins>0 ?

LGTM
Consider extending the test with another alloca which does not participate in any untraceable lifetimes, but has to be poisoned in entry block anyway.