LGTM

Remove the fix, keep the test.

LGTM

LGTM but please wait for review from Vitaly, too

LGTM

LGTM

LGTM

LGTM

This looks nice. Any idea how much does it improve analysis success rate in practice?

What if the signal arrives before MsanThread::Init?
This is what bionic does for hwasan: https://android-review.googlesource.com/c/platform/bionic/+/1134990/

LGTM

In D105169#3111935, @hyeongyukim wrote:

-  long long res;
+  register long long res __asm__("x0");

Is it okay to commit this change by myself?

The approach looks reasonable to me.

You are absolutely right. X86 variant uses an "=a" constraint (rax register), others pin the output variable to a specific register with asm declaration. It appears we've missed it in Aarch64.

LGTM w/ nit

LGTM, but maybe put the short tag in () or [] to show that it is logically part of the memory tag? And I would remove the "/short" part - it's confusing if you don't know what short tags are, and unnecessary if you do.

LGTM

LGTM

In D111164#3093454, @uabelho wrote:

Hi,

It looks like

LLVM :: tools/llvm-objcopy/MachO/universal-object.test

starts failing with this patch when the compiler is built with ubsan:
http://lab.llvm.org:8011/#/builders/5/builds/13703

I agree it is a little bit scary that noundef can be removed from an argument - msan appears to put more requirements on the attribute than the rest of llvm (it's a call abi attribute for msan, not just optimization).

+llvm-commits

In D111984#3074929, @foobarrior wrote:

Should I fix lint checks from the review? They seem to be incorrect from my point of view

Please apply the clang-format lint changes in the comments.

LGTM

address comment

Right, but a cache for SanitizerArgs is not enough to avoid repeated diagnostics, is it? Ex. if I request a non-existing sanitizer, I think I would get errors from host arg parsing, as well as from each of device1 and device2, because each device will have a unique ArgList.

Don't we want to diagnose the problems in the job's command line? What kind of changes can the driver do there that would affect SanitizerArgs?

This also needs the -mrelax-relocations=no flag to work correctly, right?

LGTM

@hyeongyukim, thank you for the summary. This looks like a great change, and a net positive to me. The test churn in the other patch is unfortunate, but IMHO unavoidable.

You know that you can also use msan_print_shadow, which is better in every way? :)
Also, do you mind updating msan_print_shadow to show the memory address, in addition to the shadow address, in the header?

LGTM

I'm not sure what does this test test.
Do we depend anywhere on the hash function producing these exact values, or is it part of an abi contract?

Hmm, I really like this info.

LGTM

LGTM

LGTM

Sure.
Of course, this is not clearly documented anywhere.
LGTM

LGTM

I wonder if it is better to handle it through hasCopyImplyingStackAdjustment. This way it stays within the x86 target.

LGTM modulo the CET discussion

LGTM

LGTM

Yes, I think I like this version better. What does glibc has at the offset of __mask_was_saved? Is it impossible for the magic value to match that contents, or simply very unlikely?

mention that the digits are *decimal* somewhere?

Also, let's call the target "tsan-dynamic" to match the naming convention established in asan.

From the discussion in D69045, I see that in older glibc versions setjmp in thread creation was intercepted, and we had to intercept this __libc_longjmp to match, but that is no longer the case. If there are no good solutions that work everywhere, then supporting 2.31+ is OK I guess (it's 1.5 years old by now). Could you see if we could detect hwasan vs libc jmpbuf format easily though, and forward to libc in the latter case?

My only concern is that an unsuspecting tool will symbolize this as code, not data location. As I understand, these are mostly equivalent when there is no line info for the address, which is the most common case. See the difference between SymbolizableObjectFile::symbolizeCode and symbolizeData. This change looks an improvement.

This certainly add some coverage, but I'm not sure we want to spend test resources on this configuration. Bootstrap of LLVM with TSan is unlikely to find much because LLVM is single threaded.

LGTM

LGTM with Vitaly's comment about test coverage

still missing test cases for combinations of mixed safe/unsafe accesses

LGTM

Ideas for more analysis tests:

• unsafe alloca with a mix of safe and unsafe accesses
• memcpy that is safe on one side and unsafe on the other. Either between two allocas, or within the same (memmove?). Or between alloca and non-stack memory.

This is pretty cool, I thought it would be more complicated.

LGTM

symbolizer build fixed in D108841.
There is still the asan test failure - you don't need any fancy build to reproduce it, just check-asan in a regular cmake build. It's in TestCases/replaceable_new_delete.cpp.
Sorry I can not chase it right now - it's a very busy week for me. Please do not reland until it is fixed though.