This is an archive of the discontinued LLVM Phabricator instance.

[asan] Allocator support for Fuchsia
ClosedPublic

Authored by mcgrathr on Aug 1 2017, 2:13 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

mcgrathr created this revision.Aug 1 2017, 2:13 PM

Split out from D35865.

cryptoad added inline comments.
lib/asan/asan_allocator.h
122 ↗(On Diff #109219)

Random question:
SANITIZER_CAN_USE_ALLOCATOR64 is false for __aarch64__ by default (https://github.com/llvm-mirror/compiler-rt/blob/master/lib/sanitizer_common/sanitizer_platform.h#L193) except for Android.
I didn't see any change for Fuchsia there (I might have missed it), and as I understand Fuchsia supports AArch64.
Would setting SANITIZER_CAN_USE_ALLOCATOR64 for AArch64 in sanitizer_platform.h make sense as well?

mcgrathr updated this revision to Diff 109248.Aug 1 2017, 4:14 PM
mcgrathr marked an inline comment as done.

Make sure SANITIZER_CAN_USE_ALLOCATOR64 is 1 on aarch64.

alekseyshl added inline comments.Aug 2 2017, 11:20 AM
lib/asan/asan_malloc_linux.cc
87 ↗(On Diff #109248)

How about minimizing the ifdefed section and go with this:

INLINE bool MaybeInDlsym() {
#if SANITIZER_FUCHSIA
  return false;
#else
  return asan_init_is_running;
#endif
}

Wouldn't the compiler be smart enough to optimize out all that unused stuff? And we will have to do the same for IsInDlsymAllocPool to help it to figure that out.

mcgrathr updated this revision to Diff 109395.Aug 2 2017, 12:38 PM
mcgrathr marked an inline comment as done.

Minimal #if'ing out for alloc-inside-dlsym support, let the compiler optimize it away.

vitalybuka added inline comments.Aug 2 2017, 2:32 PM
lib/asan/asan_malloc_linux.cc
56 ↗(On Diff #109395)

Why do you need to ifdef this?
I'd expect that asan_init_is_running will be false on fuchsia anyway

mcgrathr added inline comments.Aug 2 2017, 2:37 PM
lib/asan/asan_malloc_linux.cc
56 ↗(On Diff #109395)

It will always be false at runtime, but the compiler can't tell that statically.
I want the dead code (and its static data bloat) to be compiled out, which it is with this.

vitalybuka added inline comments.Aug 2 2017, 3:01 PM
lib/asan/asan_malloc_linux.cc
56 ↗(On Diff #109395)

could you please remove ifdef then? We don't like them only when no other options.
We can add them later if it cause real problems.

56 ↗(On Diff #109395)

actually just undo MaybeInDlsym change

mcgrathr added inline comments.Aug 2 2017, 3:26 PM
lib/asan/asan_malloc_linux.cc
56 ↗(On Diff #109395)

It sounds like you want all the dead code and data bloat to be there.
It won't cause problems other than marginal slowdown and memory bloat.
But I don't see a good reason to leave it there when eliminating it is easy and noninvasive.
I have a new version where there is no #if but only a single if (constant) that will cause everything to be compiled away.
It required a semantic change to IsInDlsymAllocPool but one that is harmless and lets the compiler grok that it can compile it away.

mcgrathr updated this revision to Diff 109441.Aug 2 2017, 3:27 PM

No more #if to get the alloc-from-dlsym hacks compiled away, but they still get compiled away.

alekseyshl accepted this revision.Aug 2 2017, 4:05 PM
alekseyshl added inline comments.
lib/asan/asan_malloc_linux.cc
46 ↗(On Diff #109441)

Make it inline.

48 ↗(On Diff #109441)

UNLIKELY is not required here.

This revision is now accepted and ready to land.Aug 2 2017, 4:05 PM
mcgrathr marked 2 inline comments as done.Aug 2 2017, 4:12 PM
mcgrathr added inline comments.
lib/asan/asan_malloc_linux.cc
46 ↗(On Diff #109441)

Done, though the compiler does inline it without the keyword.

mcgrathr updated this revision to Diff 109451.Aug 2 2017, 4:13 PM
mcgrathr marked an inline comment as done.

Added INLINE, removed UNLIKELY.

This is ready to land now.
Please land it for me!

Thanks,
Roland

This revision was automatically updated to reflect the committed changes.
vitalybuka added inline comments.Aug 7 2017, 1:47 AM
lib/asan/asan_malloc_linux.cc
35 ↗(On Diff #109441)

Why sizeof is gone here?

mcgrathr added inline comments.Aug 7 2017, 2:22 PM
lib/asan/asan_malloc_linux.cc
35 ↗(On Diff #109441)

Previously it was sizeof(alloc_memory_for_dlsym), the static size of the static buffer.
I changed it to allocated_for_dlsym, which is the dynamic count of how much of the static buffer has been used.

The reason for the change is that in the SANITIZER_FUCHSIA case, the compiler can see statically that nothing ever sets allocated_for_dlsym and so it will always be zero and hence this comparison is always false and it can optimize away all the related dead code entirely.

In theory the change is harmless because the offset of any allocated block will be less than the current count.
However, I failed to notice that the count in allocated_for_dlsym is of sizeof(uptr) units while the comparison is being made against a byte count. Hence I introduced https://bugs.llvm.org/show_bug.cgi?id=34085 with the change. I'll do a follow-up change that gets the comparison arithmetic right, so that bug won't happen but Fuchsia will get back the dead code elimination I was going for with the change.