This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt] Fix Asan build on Android
ClosedPublic

Authored by etienneb on Sep 20 2016, 8:37 AM.

Details

Summary

The dynamic shadow code is not detected correctly on Android.
The android shadow seems to start at address zero.

The bug is introduced here:

https://reviews.llvm.org/D23363

Started here: https://build.chromium.org/p/chromium.fyi/builders/ClangToTAndroidASan/builds/4029
Likely due to an asan runtime change, filed https://llvm.org/bugs/show_bug.cgi?id=30462

From asan_mapping.h:

#if SANITIZER_WORDSIZE == 32
#  if SANITIZER_ANDROID
#    define SHADOW_OFFSET (0)   <<---- HERE
#  elif defined(__mips__)

Shadow address on android is 0.

From asan_rtl.c:

if (shadow_start == 0) {
  [...]
  shadow_start = FindAvailableMemoryRange(space_size, alignment, granularity);
}

We assumed that 0 is dynamic address.

On windows, the address was determined with:

#  elif SANITIZER_WINDOWS64
#   define SHADOW_OFFSET __asan_shadow_memory_dynamic_address
#  else

and __asan_shadow_memory_dynamic_address is initially zero.

Diff Detail

Event Timeline

etienneb updated this revision to Diff 71942.Sep 20 2016, 8:37 AM
etienneb retitled this revision from to [compiler-rt] Fix Asan build on Android.
etienneb updated this object.
etienneb added a reviewer: rnk.
etienneb added subscribers: chrisha, llvm-commits.
etienneb updated this object.Sep 20 2016, 8:39 AM
rnk added inline comments.Sep 20 2016, 8:58 AM
lib/asan/asan_rtl.cc
483–484

I think kcc likes regular if over #if, when possible. Also, isn't this macro undefined usually?

Vitaly, could you double check this.

kcc added a reviewer: eugenis.Sep 20 2016, 9:37 AM
vitalybuka added inline comments.Sep 20 2016, 1:54 PM
lib/asan/asan_rtl.cc
483–484

Maybe ~0 instead of 0?

kcc added a subscriber: kcc.Sep 20 2016, 2:00 PM
kcc added inline comments.
lib/asan/asan_rtl.cc
483–484

Correct, kcc has very strong opinion on #ifs.

  1. When you can avoid an #if -- avoid it.
  2. When you can't avoid it, check #1 once more
  3. If desperate, split the files and put a top-level #if on the entire file.
etienneb marked 3 inline comments as done.Sep 21 2016, 7:34 AM
etienneb added inline comments.
lib/asan/asan_rtl.cc
483–484

I do not like #if and worse "nested" #if :)
I'm do not have a strong opinion on the way to solve this.
I just want to fix Android bots that we (ok, I broke).

Maybe ~0 instead of 0?

We can't use ~0 here because we rely on the way macro are expanded.

Initially, the variable is assigned to zero (this is needed):

__asan_shadow_memory_dynamic_address = 0;

Because the following statement is assigned from a macro:

uptr shadow_start = kLowShadowBeg;

That macro is expanded from other macros, ...

#define kLowShadowBeg   SHADOW_OFFSET

And ends up to be "based" on __asan_shadow_memory_dynamic_address which must be zero.

I'm gonna think a something not to ugly.

etienneb updated this revision to Diff 72053.Sep 21 2016, 8:39 AM
etienneb marked an inline comment as done.

fix without #if

what about this solution?

etienneb updated this revision to Diff 72063.Sep 21 2016, 8:56 AM

fix assert failing

vitalybuka accepted this revision.Sep 21 2016, 9:13 AM
vitalybuka edited edge metadata.

LGTM

This revision is now accepted and ready to land.Sep 21 2016, 9:13 AM
vitalybuka added inline comments.Sep 21 2016, 9:18 AM
lib/asan/asan_rtl.cc
472

How kLowShadowBeg can be 0 if it's kDefaultShadowSentinel

vitalybuka added inline comments.Sep 21 2016, 9:20 AM
lib/asan/asan_rtl.cc
472

I see

etienneb updated this revision to Diff 72070.Sep 21 2016, 9:22 AM
etienneb edited edge metadata.

add comment

Please land it if it fixes the bot.

Tests are fine on linux:

~/llvm/build$ ninja check-asan
[36/37] Running the AddressSanitizer tests
Testing Time: 89.23s

Expected Passes    : 1398
Expected Failures  : 3
Unsupported Tests  : 425

Tests on windows win7 64-bits are fine:

-- Testing: 543 tests, 16 threads --
Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..
Testing Time: 309.71s
  Expected Passes    : 373
  Passes With Retry  : 3
  Expected Failures  : 16
  Unsupported Tests  : 151

Landing...

etienneb closed this revision.Sep 21 2016, 9:40 AM
thakis added a subscriber: thakis.Sep 21 2016, 11:29 AM

this went in at r282085