LGTM

@vitalybuka FYI

LGTM

Hmm this code is a lot more approximate than I remember. The case where src % 4 != dst % 4 is quite broken, but you are right that it is not very common, and also ambiguous - when two poisoned 4-byte chunks with different origins contribute to the same 4-byte chunk in the output, the result can not be represented.

LGTM
Thanks!

Can we have a setting where the secondary ring buffer is used for primary allocations, too? The primary record could be pushed when the chunk is about to be reused.

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

In D85788#2449299, @aqjune wrote:

In D85788#2444136, @guiand wrote:

IMO it's better to just one-and-done programatically add -Xclang -disable-noundef-analysis to all the tests' RUN lines. That way there are no hidden flags.

If a script for this can be written and people who are working for the downstream project are happy with the script, this is the best approach IMO. It is clear and explicit.

LGTM

1. Find llvm-symbolizer in /usr/bin, same as addr2line before?
2. Support something like $ORIGIN substitution in llvm_symbolizer_path.

LGTM

Are there any tests that need updating?

I wonder if we could just disable unused argument warning for both

-disable-noundef-analysis
-Xclang -disable-noundef-analysis

I don't see any precedents for this kind of thing in the code.

simply naming it as %clang_noundef would be clearer,

This change looks fine to me, but I'm slightly concerned about https://reviews.llvm.org/D85788 - see my last comment on that revision.

LGTM

Don't you want to similarly align down PoisonEnd?

LGTM

It is probably a good idea to fuzz the option parser before using it in prod.

LGTM with nit

This code needs to call the interceptor to unpoison the output buffer.

I don't mind this change. If anyone feels strongly against it, please let us know.

LGTM

Yes, the TLS slot is owned by the platform heap allocator (scudo/asan/hwasan). Tests should not touch it.

LGTM

LGTM
The unpoison_file stuff is very incomplete, but this seems like an improvement.

D91827 should do it, but I can't test it on high addresses
It does not update the compiler side, but that one would need to be sparc-specific anyway to avoid adding an extra instruction on aarch64

It's actually not necessary to copy the error message as long as error_message_ptr_ is reset under the lock.

remove unnecessary copy

You can notice that this change is moving SetAbortMessage call out from under error_message_lock_.
I'm not sure why exactly I'm doing this, other than the fact that ASan does it this way.
This will allow Printf() from within the callback, which is quite unlikely to happen, but I guess possible in verbose mode or something like that.
All of this is still under the wider error_report_lock_, so a hwasan error in the callback will result in the "nested bug" message and an immediate exit.

(please upload diffs with full context next time, see https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface)

reduced the number of threads in the test to 2

Very interesting, thank you for the explanation.
It looks like this can be fixed by applying sign extension to the masked address?

Remove the implementation of erase(), which is no longer necessary.
PTAL.

faster live list removal

Does it mean CompactRingBuffer is broken, or just the test?
Is this ADI? I don't understand why bits 52-56 (and below) are 0xff in your example.
Could you tell me more about the 64-bit address space in solaris/sparcv9?

LGTM
Thank you.

This time with a better test using pthread barriers.

Hi Sam,

This is strictly a compilation time optimization, right?

Any idea how this is possible?
We only call this if GetModuleNameAndOffsetForPC succeeds, which should only happen when an address is within a shared object.

LGTM

LGTM

LGTM

LGTM
I was confused about the ABI statement in the description at the first glance - could you reword it to make it clear that HWASan ABI is not affected?

LGTM

LGTM

In D89615#2345429, @vitalybuka wrote:

In D89615#2345350, @jyknight wrote:

In D89615#2342857, @vitalybuka wrote:

Why we can't just switch to elf TLS?

It seems to me that ideally the NDK clang should be defaulting to -fno-emulated-tls for android with a new enough target platform version, and should also be defaulting to use lld. I think that's been the plan for a while -- do you know if it's going to actually happen soon?

That might then make some of these changes unnecessary.

@eugenis WDYT?

LGTM

I think it's good to have a limit so that allocation attempts with obviously wrong sizes are rejected with a clear message.

replaced attribute((packed)) with split storage

.

attribute((packed))

This costs 4 bytes per allocation.
We could save some memory by splitting primary and secondary allocator metadata types, but that would require significant code refactoring.

Reverted in 7ecd60bb7022bb681b9dc01a9c232fd93b4b169c

But it still seems fine to follow the default setting of
no_huge_pages_for_shadow. If time is an issue, and users are fine with
high RSS, this flag can be set to false selectively.

ping