People keep asking LSan to be available on 32 bit targets (e.g. https://github.com/google/sanitizers/issues/403) despite the fact that false negative ratio might be huge (up to 85%). This happens for big real world applications that may contain random binary data (e.g. browser), but for smaller apps situation is not so terrible and LSan still might be useful.
This patch adds initial support for x86 Linux (disabled by default), ARM32 is in TODO list.
We used this patch (well, ported to GCC) on our 32 bit mobile emulators and it worked pretty fine thus I'm posting it here to initiate further discussion.
Details
- Reviewers
kcc vitalybuka ygribov eugenis - Commits
- rG651cfe3cfaca: [lsan] Renable LSan for x86 Linux
rG2523faf677b2: [lsan] Enable LSan for x86 Linux.
rCRT293610: [lsan] Renable LSan for x86 Linux
rCRT292775: [lsan] Enable LSan for x86 Linux.
rL293610: [lsan] Renable LSan for x86 Linux
rL292775: [lsan] Enable LSan for x86 Linux.
Diff Detail
- Repository
- rL LLVM
Event Timeline
lib/lsan/lsan_interceptors.cc | ||
---|---|---|
166 | Why didn't you like uptrs? | |
lib/sanitizer_common/sanitizer_flags.inc | ||
65–66 | Redundant (). | |
lib/sanitizer_common/sanitizer_linux.h | ||
53 | I thought we had implementation for arm as well? | |
test/lsan/TestCases/large_allocation_leak.cc | ||
6 | This may deserve explanatory comment. | |
test/lsan/TestCases/use_tls_dynamic.cc | ||
18 | Looks like a hack to get test passing... |
lib/lsan/lsan_interceptors.cc | ||
---|---|---|
166 | Because for 32 bit arches uptr doesn't match size_t. | |
lib/sanitizer_common/sanitizer_linux.h | ||
53 | Yeah, but I wanted to post it as a separate patch. | |
test/lsan/TestCases/large_allocation_leak.cc | ||
6 | Ok. | |
test/lsan/TestCases/use_tls_dynamic.cc | ||
18 | Yeah, for x86 the frame for main may contain spilled address for TLS chunk thus leak is not detected. I used the outlining hack to avoid this issue. |
lib/lsan/lsan_interceptors.cc | ||
---|---|---|
166 | Yeah, and size_t is unsigned int. ASan uses size_t as well in new/delete interceptors. I need this just to avoid build errors like: /home/max/src/llvm/projects/compiler-rt/lib/lsan/lsan_interceptors.cc:166:7: error: 'operator new' takes type size_t ('unsigned int') as first parameter | |
test/lsan/TestCases/use_tls_dynamic.cc | ||
18 | Ok. Right now I don't know how can we properly mitigate the issue (fortunately it affects memory leaks from main for standalone LSan). |
lib/lsan/lsan_common.h | ||
---|---|---|
29 | Can you explain in the comment why? | |
lib/sanitizer_common/sanitizer_linux.cc | ||
1185 | Could this be more readable with uptr flags; uptr ???; uptr fn; uptr args; .... }; |
lib/sanitizer_common/sanitizer_linux.cc | ||
---|---|---|
1185 | Hm, this is quite low level C code (mainly copied from Glibc sources), is it worth to C++ise it? For all other architectures we don't use C++ style thus we'll probably need to update them to C++ style or just use C style for all arches in order to preserve consistency. |
lib/sanitizer_common/sanitizer_linux.cc | ||
---|---|---|
1185 | thanks, we should keep same way for consistency. |
lib/lsan/lsan_common.h | ||
---|---|---|
29 | I'm sorry, I don't understand what exactly I should explain here? Why I enable LSan for x86 Linux only? FWIW, I wanted to combine the new condition with already existing one at lines 25-26, but found it to messy thus I decided to use separate #elif line. |
lib/lsan/lsan_common.h | ||
---|---|---|
29 | Probly add a comment that to enable other platforms one needs to implement clone? |
lib/lsan/lsan_common.h | ||
---|---|---|
29 | Hm, ok. We also need (probably) update TLS machinery as well. |
clang-5.0: error: unsupported option '-fsanitize=leak' for target
'i386-unknown-linux-gnu'
Also, Maxim, your llvm-commits messages use an invalid reply-to address @partner.samsung.com which says:
550 5.1.1 Recipient address rejected: User unknown
Oh, just missed my clang change :(
Also, Maxim, your llvm-commits messages use an invalid reply-to address @partner.samsung.com which says:
550 5.1.1 Recipient address rejected: User unknown
Yeah, I've asked Chris Lattner to change my email, but he hasn't answered yet (perhaps he is too busy now). I'll ask him again.
You can kick me at m.ostapenko@samsung anyway and thanks for taking care of revert. I'll post my clang changes shortly.
Can you explain in the comment why?