- User Since
- Oct 3 2012, 3:00 AM (477 w, 3 d)
Tue, Nov 23
Tue, Nov 16
Remove the fix, keep the test.
Fri, Nov 12
Thu, Nov 11
LGTM but please wait for review from Vitaly, too
Wed, Nov 10
Tue, Nov 9
Mon, Nov 8
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/
Fri, Nov 5
Wed, Nov 3
The approach looks reasonable to me.
Tue, Nov 2
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.
Mon, Nov 1
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.
Fri, Oct 29
Oct 28 2021
Oct 27 2021
Oct 26 2021
Oct 21 2021
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).
Oct 20 2021
Oct 19 2021
Please apply the clang-format lint changes in the comments.
Oct 18 2021
Oct 15 2021
Oct 14 2021
Oct 13 2021
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.
Oct 11 2021
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?
Oct 7 2021
This also needs the -mrelax-relocations=no flag to work correctly, right?
@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?
Oct 5 2021
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?
Sep 30 2021
Hmm, I really like this info.
Sep 28 2021
Sep 22 2021
Sep 21 2021
Of course, this is not clearly documented anywhere.
Sep 20 2021
Sep 17 2021
I wonder if it is better to handle it through hasCopyImplyingStackAdjustment. This way it stays within the x86 target.
Sep 16 2021
Sep 15 2021
LGTM modulo the CET discussion
Sep 14 2021
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?
Sep 13 2021
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.
Sep 9 2021
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 with Vitaly's comment about test coverage
Sep 8 2021
still missing test cases for combinations of mixed safe/unsafe accesses
Sep 7 2021
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.
Sep 3 2021
This is pretty cool, I thought it would be more complicated.
Sep 2 2021
Sep 1 2021
Aug 27 2021
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.