This is an archive of the discontinued LLVM Phabricator instance.

Enable LSAN for Android
ClosedPublic

Authored by oontvoo on Aug 13 2020, 12:53 PM.

Details

Summary

Make use of the newly added thread-properties API (available since 31)

Side-effect change: set -fno-emulated-tls for lsan/asan libraries and skip their tests on pre-S Android.

Diff Detail

Event Timeline

oontvoo created this revision.Aug 13 2020, 12:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2020, 12:53 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
oontvoo requested review of this revision.Aug 13 2020, 12:53 PM
oontvoo added a subscriber: enh.Aug 13 2020, 1:17 PM
eugenis added inline comments.Aug 13 2020, 2:13 PM
compiler-rt/lib/lsan/lsan_common.cpp
34

I think this needs runtime detection (dlsym or a weak symbol, there are precedents in sanitizer_common), otherwise the NDK build of the runtime library will never enable this.

36

typo: ANDROILD

FYI https://reviews.llvm.org/D85930 moves standalone LSan on Android/aarch64 to the 64-bit allocator, I don't know if you care about this.
LSan-in-ASan is already there.

oontvoo updated this revision to Diff 285504.Aug 13 2020, 2:57 PM
oontvoo marked an inline comment as done.

Updated diff

oontvoo marked an inline comment as not done.Aug 13 2020, 2:58 PM
oontvoo added inline comments.
compiler-rt/lib/lsan/lsan_common.cpp
34

Sorry, runtime detection for the _iterate_ function (used below)? Or do you mean the new header?

oontvoo marked an inline comment as done.Aug 13 2020, 3:01 PM

I think there needs to be something to disable automatic leak detection on exit on older platform, otherwise existing users of ASan will start getting false leak reports.

compiler-rt/lib/lsan/lsan_common.cpp
34

For the function, like you've done already.

compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
92

It seems easier to make this declaration unconditional (under SANITIZER_ANDROID of course) and get rid of the header include.

435

nit: I'd prefer start_addr and end_addr to be void*, and the casts moved to the assignments below.

oontvoo marked 3 inline comments as done.Aug 13 2020, 6:32 PM

FYI https://reviews.llvm.org/D85930 moves standalone LSan on Android/aarch64 to the 64-bit allocator, I don't know if you care about this.
LSan-in-ASan is already there.

I'm not familiar with this. How would it change the way static and dynamic TLS are allocated on android? Specifically, would it still eventually go to the bionic allocator?

compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
92

It seems easier to make this declaration unconditional (under SANITIZER_ANDROID of course) and get rid of the header include.

How would it know where to look for the function in the case where it does exist ?

I'm guessing somebody needs to include the new header?

oontvoo updated this revision to Diff 285540.Aug 13 2020, 6:33 PM

Updated diff

oontvoo updated this revision to Diff 285544.Aug 13 2020, 7:03 PM

Dont run leakcheck for android if the thread-properties API is not present

This change does not enable standalone LSan. It probably should, as a test vehicle if nothing else. See COMPILER_RT_HAS_LSAN.

It would be great to have some testing, too. Unfortunately with the pandemic our testing rig is locked in the office and most of the devices fell off over time... There used to be aarch64 and arm testing steps here:
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-android/builds/34766

It's pretty hard to setup, but could you check that check-asan check-lsan check-sanitizer pass both with old and new aosp images? I'm concerned about __lsan_disable which involves a thread-local variable built with -fno-emulated-tls and it's probably going to crash before Android Q.

You can peek at the buildbot setup: https://github.com/llvm/llvm-zorg/blob/master/zorg/buildbot/builders/sanitizers/buildbot_android_functions.sh The interesting parts are configure_android and test_arch_on_device.

Any plans to add hwasan+lsan support? Android is generally moving in that direction, and both standalone lsan and asan+lsan are unlikely to see a lot of use.

compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
67

why assert.h?

92

the header does not really add anything useful if you already have the declaration :)

437

Why +1? Looking at the implementation, end seems to be one-past-the-end, no adjustment should be needed.

oontvoo updated this revision to Diff 285812.Aug 14 2020, 9:07 PM
oontvoo marked 4 inline comments as done.

updated diff

This change does not enable standalone LSan. It probably should, as a test vehicle if nothing else. See COMPILER_RT_HAS_LSAN.

Done

It would be great to have some testing, too. Unfortunately with the pandemic our testing rig is locked in the office and most of the devices fell off over time... There used to be aarch64 and arm testing steps here:
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-android/builds/34766

It's pretty hard to setup, but could you check that check-asan check-lsan check-sanitizer pass both with old and new aosp images? I'm concerned about __lsan_disable which involves a thread-local variable built with -fno-emulated-tls and it's probably going to crash before Android Q.

Maybe moving the check for existence of the new API before the call to __lsan_disable would prevent the crash? (because it wouldn't to that) ?

You can peek at the buildbot setup: https://github.com/llvm/llvm-zorg/blob/master/zorg/buildbot/builders/sanitizers/buildbot_android_functions.sh The interesting parts are configure_android and test_arch_on_device.

I've grabbed a pixel3 test phone from the office. Is there a one-step setup script that I could run and point it at the device? Or would I have to set it up manually ?
I could also do the manual tests where I copy the new bionic libc and this patch's asan-aarch64-android.so. to the phone and run some binaries ... see if they work

Any plans to add hwasan+lsan support?

Yes, not sure where to start. :-) Any pointers? (I was under the impression it'd "just work" ... what's still missing? )

MaskRay added inline comments.
compiler-rt/lib/lsan/lsan_common.cpp
306

The convention is just size_t /*dso_id*/, no unused.

I've grabbed a pixel3 test phone from the office. Is there a one-step setup script that I could run and point it at the device? Or would I have to set it up manually ?
I could also do the manual tests where I copy the new bionic libc and this patch's asan-aarch64-android.so. to the phone and run some binaries ... see if they work

Try this one, it's the same script that runs on the buildbot. Be careful, it can mess up anything in the current directory (it expect source/build trees there for a possible incremental run).
Uncomment the line at the bottom to enable arm/aarch64 testing.
https://github.com/llvm/llvm-zorg/blob/master/zorg/buildbot/builders/sanitizers/buildbot_android.sh

Any plans to add hwasan+lsan support?

Yes, not sure where to start. :-) Any pointers? (I was under the impression it'd "just work" ... what's still missing? )

There are a few small things, search for "lsan" in compiler-rt/lib/asan. The main one is an allocator interface for scanning, and a few hooks for initializing lsan and passing the runtime flags.

oontvoo updated this revision to Diff 286449.Aug 18 2020, 6:09 PM

upated diff

oontvoo marked an inline comment as done.Aug 18 2020, 6:09 PM
oontvoo updated this revision to Diff 288500.Aug 27 2020, 6:44 PM

Avoid accessing TLS variables for android when the thread-properties API is not present.

oontvoo updated this revision to Diff 289347.Sep 1 2020, 9:04 PM

updated diff: skipped lsan/asan tests on pre-S images

oontvoo edited the summary of this revision. (Show Details)Sep 1 2020, 9:07 PM
oontvoo updated this revision to Diff 293259.EditedSep 21 2020, 2:44 PM

Updated a few tests to work on Android.

@eugenis: I think this is ready to be reviewed now.
Here are the check-lsan and check-asan result on aarch64:

i686:

(Note: The i686 needs some fix in Bionic or it'd sigsev https://android-review.googlesource.com/c/platform/bionic/+/1419435)

oontvoo updated this revision to Diff 295033.Sep 29 2020, 10:24 AM

(minor cleanup)

eugenis added inline comments.Oct 2 2020, 11:48 AM
compiler-rt/cmake/config-ix.cmake
167

Why is android special here?

295

Please remind me why i686? I don't like that this list contains both i386 and i686, it's hard to tell what can break.

Does it affect the library name? That would definitely break a number of things.

If this is about the test script matching build with a device, the fix should be limited to the the tests.

compiler-rt/lib/lsan/lsan_common.cpp
84

This looks like it would suppress everything in libc++. Could it be refined somehow?

compiler-rt/test/asan/lit.cfg.py
253

ASan is certainly supported before android-31.

compiler-rt/test/lit.common.cfg.py
375

should not this be android-30?

compiler-rt/test/lsan/TestCases/Linux/log-path_test.cpp
16

Do you mean it does not work with the android test harness?

compiler-rt/test/lsan/TestCases/Linux/use_tls_dynamic.cpp
6

why are you removing the glibc-2.27 check?

compiler-rt/test/lsan/lit.common.cfg.py
26

This is better done in cmake, otherwise you can end up with two ASan configs.

But I'd rather fix the segfault problem instead.

oontvoo updated this revision to Diff 295920.Oct 2 2020, 3:04 PM
oontvoo marked 3 inline comments as done.

updated per review comments

compiler-rt/cmake/config-ix.cmake
295

Please remind me why i686? I don't like that this list contains both i386 and i686, it's hard to tell what can break.
If this is about the test script matching build with a device, the fix should be limited to the the tests.

The emulator is i686, so none of the tests would run if i686 is not in the list of supported archs.

Additionally from the thread I started awhile ago with you, enh@, et al, the options then were:

  1. change x86 to mean i686
  2. try to paint i686 as i386 in the test scripts
  3. add i686 to the list of supported things

IIUC, the consensus was "no" to (1) because of a long tail of things.
And I think (2) would be misleading given bionic publicly state that they don't support i386 ABI. (And it'll make it's harder to fix (1) when we need to, which supposedly would happen at some point?)

That leaves us with (3)

Does it affect the library name? That would definitely break a number of things.

Yes, if the test script sees that it's building for i686, it'll change .so file names to have i686. The library itself is still called LeakSanitizer-AddressSanitizer-i386, though.

How do I produce the breakages? Is it better to fix those breakages?

compiler-rt/test/asan/lit.cfg.py
253

Done - removed

compiler-rt/test/lit.common.cfg.py
375

Technically, the thread_properties API is declared to be introduced in 31.

In practice, on master build (which is 30), it's already available ... and there's no 31 builds yet.

Should we change the introduction version to 30?

compiler-rt/test/lsan/TestCases/Linux/log-path_test.cpp
16

Do you mean it does not work with the android test harness?

Right, because we need to pull the file down to lit-check.
Anyway, updated the test so it will work.

compiler-rt/test/lsan/TestCases/Linux/use_tls_dynamic.cpp
6

Done - put it back.

compiler-rt/test/lsan/lit.common.cfg.py
26

It didn't produce any stacktrace - I was only able to track it to somewhere *before* __libc_init_main*

( I suspect it may have to do with emulated-tls )

Any pointer?

eugenis added inline comments.Oct 2 2020, 4:08 PM
compiler-rt/cmake/config-ix.cmake
295

The breakage I'd expect are in android platform build system, and in asan/scripts/asan_device_setup hardcoding library names. Probably something in chromium, too. I'd prefer not to change the existing library names.

But, looking at the prebuilt android toolchains, asan library names already use i686 for android targets (and i386 for the host). Not sure how that happens, could have something to do with the build flags. Could you check if what they do works for you, as well?

The reason I don't like this change is because you add a second alias for x86, and now, potentially, every place in cmake that references ${X86} needs to look for i686, too.

compiler-rt/test/lit.common.cfg.py
375

Yeah that's a problem.
API 30 does not have thread properties. We should assume that the test device is a release build by default.

  • we could set an environment variable
  • pull libc.so from the device and look at the symbols
  • look for "master" in ro.build.fingerprint + api level 30

... then add a feature "thread-properties-api".

Even better, look at ro.build.version.codename.

compiler-rt/test/lsan/lit.common.cfg.py
26

try attaching a debugger, or running under strace. Try disabling the segv handler in lsan (if it is enabled).

oontvoo updated this revision to Diff 296361.Oct 5 2020, 10:30 PM
oontvoo marked 8 inline comments as done.

Updated diff

compiler-rt/cmake/config-ix.cmake
295

But, looking at the prebuilt android toolchains, asan library names already use i686 for android targets (and i386 for the host).

Yes, even the buildbot already uses i686 (And all I was doing here was running the buildbot_android.sh script.)

So apparently it's already skipping all the tests on i686 right now.
But looks like we can change the bot script to use i386 and run these tests as usual.

Ok, I think I will stop poking at this can of worms for now. :-)

Reverted all the i686s.

compiler-rt/test/lit.common.cfg.py
375

Done: use codename when api level is lower than 31.

compiler-rt/test/lsan/lit.common.cfg.py
26

Done.

Issue was lsan not being initted

eugenis added inline comments.Oct 7 2020, 2:30 PM
compiler-rt/lib/asan/CMakeLists.txt
240

Same as before, why is Android special here? The cmake check should have found that the linker does not support this option. Is it because we are forcing LLD at line 90, and the check has run against a different linker?

I guess it's fine as a workaround, but mention the reason in the comment.

compiler-rt/lib/lsan/lsan_common.cpp
85

A bit strange that these suppression is needed on android, and not everywhere with libc++.
What does the leak look like?

compiler-rt/test/asan/lit.cfg.py
212

You are enabling the tests on non-android arm32/arm64.
They might just work there, but I would not do that in this change.
For simplicity, move the android conditions to leak_detection_android.

compiler-rt/test/lsan/TestCases/Linux/use_tls_dynamic.cpp
6

This condition looks weird, because glibc-2.27 refers to the host system, and android refers to the testing device.

It's probably better to make sure that glibc-2.27 is not set when config.android==true.

compiler-rt/test/lsan/TestCases/strace_test.cpp
6

What does this error look like, and why can't it be captured?
Is it something that can be fixed in the adb scripts?

oontvoo updated this revision to Diff 296841.Oct 7 2020, 6:18 PM
oontvoo marked 3 inline comments as done.

updated diff

compiler-rt/lib/asan/CMakeLists.txt
240

Yeah, you'll get a build warning saying /usr/bin/ld: -Wl,-z,gnu-version-script-compat is ignored
Added the explanation.

compiler-rt/lib/lsan/lsan_common.cpp
85

There were leaks in ignore_object.c, cleanup_in_tsd_destructor.c, disabler_in_tsd_destructor.c, and disabler.c

Leak reports (for ignore_object.c):

Test alloc: 0xc5600750.

=================================================================
==31243==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 1337 byte(s) in 1 object(s) allocated from:
    #0 0xe7a116d5 in malloc /usr/local/google/home/vyng/repo/i686_buildbot/rundir/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0x59a33cb2 in main /usr/local/google/home/vyng/repo/i686_buildbot/rundir/llvm-project/compiler-rt/test/lsan/TestCases/ignore_object.c:17:22
    #2 0xe7df5e43 in __libc_init (/system/lib/libc.so+0x4fe43)
    #3 0x59a33ba8 in _start (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0x2ba8)

Objects leaked above:
0xc5003680 (1337 bytes)

Direct leak of 24 byte(s) in 1 object(s) allocated from:
    #0 0xe7a116d5 in malloc /usr/local/google/home/vyng/repo/i686_buildbot/rundir/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0xe7e89183 in __register_atfork (/system/lib/libc.so+0xe3183)
    #2 0xe7de8c9a in pthread_atfork (/system/lib/libc.so+0x42c9a)
    #3 0xe7e2546c in __libc_init_fork_handler() (/system/lib/libc.so+0x7f46c)
    #4 0xe7df5d78 in __libc_preinit_impl() (/system/lib/libc.so+0x4fd78)
    #5 0xe7df5cdb in __libc_preinit() (/system/lib/libc.so+0x4fcdb)
    #6 0xe86d0728  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0x60728)
    #7 0xe86d09cc  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0x609cc)
    #8 0xe86d083a  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0x6083a)
    #9 0xe86d083a  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0x6083a)
    #10 0xe86d083a  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0x6083a)
    #11 0xe871cbf7  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0xacbf7)
    #12 0xe871b582  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0xab582)
    #13 0xe86d435a  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0x6435a)

Objects leaked above:
0xc5a00b80 (24 bytes)

Direct leak of 4 byte(s) in 1 object(s) allocated from:
    #0 0xe7a116d5 in malloc /usr/local/google/home/vyng/repo/i686_buildbot/rundir/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0xe7e7bdc1 in operator new(unsigned int) (/system/lib/libc.so+0xd5dc1)
    #2 0xe7e25b92 in newlocale (/system/lib/libc.so+0x7fb92)
    #3 0xe7d0a1eb  (/system/lib/libc++.so+0x851eb)
    #4 0xe7d100fd  (/system/lib/libc++.so+0x8b0fd)
    #5 0xe7d101f3 in std::__1::locale::locale() (/system/lib/libc++.so+0x8b1f3)
    #6 0xe7ce077f in std::__1::basic_streambuf<char, std::__1::char_traits<char> >::basic_streambuf() (/system/lib/libc++.so+0x5b77f)
    #7 0xe7ced54b in std::__1::ios_base::Init::Init() (/system/lib/libc++.so+0x6854b)
    #8 0xe7cee9fa  (/system/lib/libc++.so+0x699fa)
    #9 0xe86d0728  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0x60728)
    #10 0xe86d09cc  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0x609cc)
    #11 0xe86d083a  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0x6083a)
    #12 0xe86b4303  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0x44303)
    #13 0xe86ae912  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0x3e912)
    #14 0xe86ae9f8  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0x3e9f8)
    #15 0xe8320a76 in dlopen (/apex/com.android.runtime/lib/bionic/libdl.so+0x1a76)
    #16 0xe7ded93c in netdClientInitImpl() (/system/lib/libc.so+0x4793c)
    #17 0xe7e8c9c0 in pthread_once (/system/lib/libc.so+0xe69c0)
    #18 0xe7ded8ab in netdClientInit (/system/lib/libc.so+0x478ab)
    #19 0xe7df5d7d in __libc_preinit_impl() (/system/lib/libc.so+0x4fd7d)
    #20 0xe7df5cdb in __libc_preinit() (/system/lib/libc.so+0x4fcdb)
    #21 0xe86d0728  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0x60728)
    #22 0xe86d09cc  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0x609cc)
    #23 0xe86d083a  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0x6083a)
    #24 0xe86d083a  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0x6083a)
    #25 0xe86d083a  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0x6083a)
    #26 0xe871cbf7  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0xacbf7)
    #27 0xe871b582  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0xab582)
    #28 0xe86d435a  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0x6435a)

Objects leaked above:
0xc56007b0 (4 bytes)

Direct leak of 4 byte(s) in 1 object(s) allocated from:
    #0 0xe7a1e189 in operator new(unsigned int) /usr/local/google/home/vyng/repo/i686_buildbot/rundir/llvm-project/compiler-rt/lib/asan/asan_new_delete.cpp:99:3
    #1 0xe7c03462  (/data/local/tmp/Output/libc++_shared.so+0xd7462)
    #2 0xe7ba2aee  (/data/local/tmp/Output/libc++_shared.so+0x76aee)
    #3 0xe7baac6d in std::__ndk1::locale::__global() (/data/local/tmp/Output/libc++_shared.so+0x7ec6d)
    #4 0xe7baad0a in std::__ndk1::locale::locale() (/data/local/tmp/Output/libc++_shared.so+0x7ed0a)
    #5 0xe7b7eb74  (/data/local/tmp/Output/libc++_shared.so+0x52b74)
    #6 0xe7b7e7dd in std::__ndk1::ios_base::Init::Init() (/data/local/tmp/Output/libc++_shared.so+0x527dd)
    #7 0xe7b6008a  (/data/local/tmp/Output/libc++_shared.so+0x3408a)
    #8 0xe86d0728  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0x60728)
    #9 0xe86d09cc  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0x609cc)
    #10 0xe86d083a  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0x6083a)
    #11 0xe86d083a  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0x6083a)
    #12 0xe871cbf7  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0xacbf7)
    #13 0xe871b582  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0xab582)
    #14 0xe86d435a  (/data/local/tmp/Output/usr/local/google/home/vyng/repo/i686_buildbot/rundir/compiler_rt_build_android_i386/test/lsan/I386LsanConfig/TestCases/Output/ignore_object.c.tmp+0x6435a)

Objects leaked above:
0xc5600790 (4 bytes)

-----------------------------------------------------
Suppressions used:
  count      bytes template
      2        144 *tls_get_addr*
-----------------------------------------------------

SUMMARY: AddressSanitizer: 1369 byte(s) leaked in 4 allocation(s).
compiler-rt/test/lsan/TestCases/strace_test.cpp
6

Running adb shell "strace ..." on a terminal gets you the expected error ("leaksanitizer doesn't work under ptrace").

Unfortunately, my python-fu was limited and I couldn't figure out why the android_run.py wrapper(line 26) would not get that error into either device_stderr or its own stderr.

So this test would die (expectedly) but no error message to check against.

--
Exit Code: 2

Command Output (stderr):
--
FileCheck error: '<stdin>' is empty.
FileCheck command line:  FileCheck /usr/local/google/home/vyng/repo/i686_buildbot/rundir/llvm-project/compiler-rt/test/lsan/TestCases/strace_test.cpp

--
oontvoo updated this revision to Diff 296858.Oct 7 2020, 8:26 PM

updated diff

eugenis added inline comments.Oct 8 2020, 2:12 PM
compiler-rt/lib/lsan/lsan_common.cpp
85
#1 0xe7e89183 in __register_atfork (/system/lib/libc.so+0xe3183)

This is a global in libc.so.

#3 0xe7baac6d in std::__ndk1::locale::__global()

This is a global (function-local static) in libc++.so

I don't know why they are not seen as reachable, but something must be wrong.
Try figuring it out? Those pointers should have been discovered when scanning the module mappings. LSAN_OPTIONS report_objects, log_pointers, etc could help.

oontvoo added inline comments.Oct 8 2020, 5:58 PM
compiler-rt/lib/lsan/lsan_common.cpp
85

Oh wait, these tests explicitly exclude globals (use_globals=0), so obviously they won't scan it.

I guess we'll just have to set use_globals=1 by default for android?

oontvoo updated this revision to Diff 297096.Oct 8 2020, 6:15 PM

Make test set use_global=1 for Android and add warnings if use_global is turnt off.

oontvoo marked an inline comment as done.Oct 8 2020, 6:16 PM
eugenis accepted this revision.Oct 9 2020, 11:58 AM

LGTM with 1 comment

compiler-rt/lib/lsan/lsan_common.cpp
85

Better to remove use_globals from the tests on all platforms. I don't see why its needed there, must be to narrow down the test scope as much as possible, but it is clearly not working on android.

This revision is now accepted and ready to land.Oct 9 2020, 11:58 AM
oontvoo updated this revision to Diff 297306.Oct 9 2020, 12:18 PM
oontvoo marked 2 inline comments as done.

remove use_globals=0 from tests

oontvoo closed this revision.Oct 9 2020, 12:25 PM
dmajor added a subscriber: dmajor.Oct 12 2020, 8:16 AM

This caused a build break in our compiler-rt build for Android: use of undeclared identifier '__interceptor_memalign'

The name is referenced from https://github.com/llvm/llvm-project/blob/85e578f53ad1ba21771470dc9516068a259d29cf/compiler-rt/lib/asan/asan_malloc_linux.cpp#L274 but the definition is no longer provided now that SANITIZER_INTERCEPT_MEMALIGN has excluded Android.

For anyone playing along at home: this landed in https://github.com/llvm/llvm-project/commit/a2291a58bf1c860d026581fee6fe96019dc25440, not sure why Phab didn't automatically mention that here.

dmajor added a comment.EditedOct 12 2020, 9:04 AM

And later we also get use of undeclared identifier 'dl_iterate_phdr' in (EDIT:) lsan_common_linux.cpp. We compile for API 16, which doesn't implement that.

Thanks for the report!

This caused a build break in our compiler-rt build for Android: use of undeclared identifier '__interceptor_memalign'

Are you also building with API 16? (From your second comment)

The name is referenced from https://github.com/llvm/llvm-project/blob/85e578f53ad1ba21771470dc9516068a259d29cf/compiler-rt/lib/asan/asan_malloc_linux.cpp#L274 but the definition is no longer provided now that SANITIZER_INTERCEPT_MEMALIGN has excluded Android.

For anyone playing along at home: this landed in https://github.com/llvm/llvm-project/commit/a2291a58bf1c860d026581fee6fe96019dc25440, not sure why Phab didn't automatically mention that here.

I landed it with git push instead of arc land ... maybe that's the issue?

Thanks for the report!

This caused a build break in our compiler-rt build for Android: use of undeclared identifier '__interceptor_memalign'

Are you also building with API 16? (From your second comment)

Yes, that's right. The memalign error is still present after D89251.

Hmm, I did not notice the memalign chunk. Why was it removed?

Thanks for the report!

This caused a build break in our compiler-rt build for Android: use of undeclared identifier '__interceptor_memalign'

Are you also building with API 16? (From your second comment)

Yes, that's right. The memalign error is still present after D89251.

Included the fix now.

Hmm, I did not notice the memalign chunk. Why was it removed?

memalign is callled during lsan init for Android, causing the "ENSURE_LSAN_INITED" check and the stack-trace to fail.
(See comment on the other patch for explanation)

vitalybuka added inline comments.Oct 13 2020, 2:46 AM
compiler-rt/lib/sanitizer_common/sanitizer_platform_interceptors.h
504–507
vitalybuka reopened this revision.Oct 13 2020, 3:02 AM
This revision is now accepted and ready to land.Oct 13 2020, 3:02 AM
oontvoo closed this revision.Nov 5 2020, 6:19 AM

(relanded)