User Details
- User Since
- Oct 3 2012, 3:00 AM (432 w, 3 d)
Yesterday
LGTM
Thu, Jan 14
@vitalybuka FYI
Wed, Jan 13
LGTM
Tue, Jan 12
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.
Mon, Jan 11
LGTM
Thanks!
Fri, Jan 8
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.
Tue, Jan 5
LGTM
Wed, Dec 30
LGTM
Tue, Dec 29
LGTM
Tue, Dec 22
LGTM
LGTM
LGTM
Mon, Dec 21
Dec 14 2020
LGTM
Dec 11 2020
- Find llvm-symbolizer in /usr/bin, same as addr2line before?
- Support something like $ORIGIN substitution in llvm_symbolizer_path.
Dec 10 2020
Dec 9 2020
LGTM
Are there any tests that need updating?
Dec 8 2020
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,
Dec 7 2020
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
Dec 4 2020
It is probably a good idea to fuzz the option parser before using it in prod.
LGTM with nit
Dec 3 2020
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.
Dec 1 2020
LGTM
Yes, the TLS slot is owned by the platform heap allocator (scudo/asan/hwasan). Tests should not touch it.
Nov 30 2020
LGTM
Nov 23 2020
Nov 20 2020
LGTM
The unpoison_file stuff is very incomplete, but this seems like an improvement.
Nov 19 2020
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)
Nov 18 2020
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?
Nov 17 2020
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?
Nov 13 2020
LGTM
Thank you.
Nov 12 2020
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.
Nov 10 2020
LGTM
Oct 30 2020
LGTM
Oct 29 2020
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
Oct 21 2020
LGTM
Oct 20 2020
LGTM
Oct 19 2020
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