Page MenuHomePhabricator

[lsan] Enable LSan for x86 Linux.
ClosedPublic

Authored by m.ostapenko on Jan 12 2017, 7:57 AM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

m.ostapenko retitled this revision from to [lsan] Enable LSan for x86 Linux..
m.ostapenko updated this object.
m.ostapenko added reviewers: kcc, eugenis, ygribov.
m.ostapenko set the repository for this revision to rL LLVM.
m.ostapenko added a project: Restricted Project.
m.ostapenko added a subscriber: llvm-commits.
ygribov added inline comments.Jan 12 2017, 8:03 AM
lib/lsan/lsan_interceptors.cc
166 ↗(On Diff #84120)

Why didn't you like uptrs?

lib/sanitizer_common/sanitizer_flags.inc
65 ↗(On Diff #84120)

Redundant ().

lib/sanitizer_common/sanitizer_linux.h
53 ↗(On Diff #84120)

I thought we had implementation for arm as well?

test/lsan/TestCases/large_allocation_leak.cc
6 ↗(On Diff #84120)

This may deserve explanatory comment.

test/lsan/TestCases/use_tls_dynamic.cc
18 ↗(On Diff #84120)

Looks like a hack to get test passing...

m.ostapenko added inline comments.Jan 12 2017, 8:18 AM
lib/lsan/lsan_interceptors.cc
166 ↗(On Diff #84120)

Because for 32 bit arches uptr doesn't match size_t.

lib/sanitizer_common/sanitizer_linux.h
53 ↗(On Diff #84120)

Yeah, but I wanted to post it as a separate patch.

test/lsan/TestCases/large_allocation_leak.cc
6 ↗(On Diff #84120)

Ok.

test/lsan/TestCases/use_tls_dynamic.cc
18 ↗(On Diff #84120)

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.
Do you have any suggestion how we can avoid test failure in another way?

ygribov added inline comments.Jan 12 2017, 8:30 AM
lib/lsan/lsan_interceptors.cc
166 ↗(On Diff #84120)

Hm, I thought it's defined to unsigned long on 32-bits...

test/lsan/TestCases/use_tls_dynamic.cc
18 ↗(On Diff #84120)

I'd use XFAIL. The issue sounds quite important (and not really related to your change).

m.ostapenko added inline comments.Jan 12 2017, 8:44 AM
lib/lsan/lsan_interceptors.cc
166 ↗(On Diff #84120)

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 ↗(On Diff #84120)

Ok. Right now I don't know how can we properly mitigate the issue (fortunately it affects memory leaks from main for standalone LSan).

Updating according to Yura's nits.

m.ostapenko marked 13 inline comments as done.Jan 13 2017, 6:21 AM
vitalybuka added inline comments.Jan 17 2017, 10:34 AM
lib/lsan/lsan_common.h
29 ↗(On Diff #84293)

Can you explain in the comment why?

lib/sanitizer_common/sanitizer_linux.cc
1185 ↗(On Diff #84293)

Could this be more readable with
struct Header {

uptr flags;
uptr ???;
uptr fn;
uptr args;  
....

};
Header* header = stattic_cast(Header*)child_stack - 1;
header = {....}
or just
header.flags = flags
...

m.ostapenko added inline comments.Jan 18 2017, 1:03 AM
lib/sanitizer_common/sanitizer_linux.cc
1185 ↗(On Diff #84293)

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.

vitalybuka added inline comments.Jan 18 2017, 10:46 AM
lib/sanitizer_common/sanitizer_linux.cc
1185 ↗(On Diff #84293)

thanks, we should keep same way for consistency.

m.ostapenko added inline comments.Jan 19 2017, 5:13 AM
lib/lsan/lsan_common.h
29 ↗(On Diff #84293)

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.

ygribov added inline comments.Jan 19 2017, 6:29 AM
lib/lsan/lsan_common.h
29 ↗(On Diff #84293)

Probly add a comment that to enable other platforms one needs to implement clone?

m.ostapenko added inline comments.Jan 19 2017, 6:36 AM
lib/lsan/lsan_common.h
29 ↗(On Diff #84293)

Hm, ok. We also need (probably) update TLS machinery as well.

m.ostapenko marked 4 inline comments as done.
This revision is now accepted and ready to land.Jan 19 2017, 10:47 AM
This revision was automatically updated to reflect the committed changes.
eugenis edited edge metadata.Jan 23 2017, 10:52 AM

clang-5.0: error: unsupported option '-fsanitize=leak' for target
'i386-unknown-linux-gnu'

http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/712/steps/64-bit%20check-sanitizer/logs/stdio

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

Reverted in r292844

clang-5.0: error: unsupported option '-fsanitize=leak' for target
'i386-unknown-linux-gnu'

Oh, just missed my clang change :(

http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/712/steps/64-bit%20check-sanitizer/logs/stdio

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.

Reverted in r292844

So, OK to reapply after https://reviews.llvm.org/D29077 is committed?

Yes, absolutely!