This is an archive of the discontinued LLVM Phabricator instance.

Add iOS/watchOS/tvOS support for ASan (compiler-rt part)
ClosedPublic

Authored by zaks.anna on Dec 17 2015, 12:50 PM.

Details

Summary

This change along with the corresponding changes in clang (http://reviews.llvm.org/D15624) and llvm (http://reviews.llvm.org/D15625) completes ASan support for iOS/watchOS/tvOS.

(Many parts of this were conributed by Kuba Brecka and the build system was rewritten by Chris Bieneman.)

Diff Detail

Event Timeline

zaks.anna updated this revision to Diff 43175.Dec 17 2015, 12:50 PM
zaks.anna retitled this revision from to Add iOS/watchOS/tvOS support for ASan (compiler-rt part).
zaks.anna updated this object.
zaks.anna added subscribers: kubamracek, beanz.
samsonov added inline comments.Dec 17 2015, 1:09 PM
cmake/config-ix.cmake
205

This doesn't seem right: this whole code path is not executed on Apple platforms.

313–315

Are you sure you're ready to enable it by default? Maybe remove "experimental" then?

490

I would really like to see this generalized somehow.

lib/asan/asan_internal.h
36

Let's define ASAN_LOW_MEMORY in a single place

#ifndef ASAN_LOW_MEMORY
#if SANITIZER_OS
#  define ASAN_LOW_MEMORY 1
#elseif SANITIZER_WORDSIZE == 32
#  define ASAN_LOW_MEMORY 1
...
lib/asan/asan_mapping.h
155

See how we deal with SANITIZER_IOS/SANITIZER_IOSSIM above - maybe do the same?

lib/sanitizer_common/sanitizer_mac.cc
360

This is wrong - of course SIGSEGV is deadly, we just don't need to handle it.

lib/sanitizer_common/sanitizer_platform.h
46

You need to make sure SANITIZER_TVOS is *always* defined to 0 or 1. Same with SANITIZER_WATCHOS.

zaks.anna added inline comments.Dec 17 2015, 3:28 PM
lib/asan/asan_mapping.h
155

I think this way of writing it is less error prone. SANITIZER_IOS is set for both the simulator and the device. If the order of elifs above is reversed, the logic would break. I think it's better to rewrite the code above to make it more explicit.

lib/sanitizer_common/sanitizer_mac.cc
360

How about we rename the function into "IsHandledDeadlySignal". It already only returns true if common_flags()->handle_segv.

zaks.anna updated this revision to Diff 43198.Dec 17 2015, 4:51 PM
zaks.anna marked 3 inline comments as done.

Addressed most of Alexey's comments. Chris B. will address the build system refactoring piece.

beanz edited edge metadata.Dec 18 2015, 10:17 AM

Anna,

Can you please split out the CMake changes into a separate patch from the other changes? In general I think having more smaller self-contained patches will make it easier to review.

More comments inline.

Thanks,
-Chris

cmake/config-ix.cmake
313–316

I think at this point we can have iOS be on and not experimental, but can we do that as a separate patch?

tvOS and watchOS should be experimental and Off by default. There are some issues we'll need to discuss and work out to get that working in open source. Having options will be an important step for us to make it work, but the options need to be off to start with.

490

I probably won't have time to look at this until January, but I'll help Anna rework this. I think we can set some values up front and turn the logic into a loop.

zaks.anna marked an inline comment as done.Dec 18 2015, 2:16 PM

Chris B. is going to submit a patch for the cmake changes and I will rebase on top of it after that is excepted.

zaks.anna updated this revision to Diff 45335.Jan 19 2016, 6:57 PM
zaks.anna edited edge metadata.

Chris Bieneman has refactored and landed the cmake changes in r257544. Rebased the patch on top of that commit.

samsonov edited edge metadata.Jan 21 2016, 1:33 PM

Looks pretty good.

cmake/config-ix.cmake
313

Please submit this small change (switching the default) as a separate commit, so that it would be easy revert if iOS build won't work on some setups.

lib/asan/asan_internal.h
39
# if SANITIZER_IOS || (SANITIZER_WORDSIZE == 32)
lib/asan/asan_mapping.h
153–159

Please restore the indentation.

155

Makes sense, let's fix 32-bit variant accordingly then.

168–169

Do you need to remove these now?

lib/sanitizer_common/sanitizer_mac.cc
360

Please make it a plain if() instead of #if?

lib/sanitizer_common/sanitizer_platform.h
45

Suggestion: write this as

  1. if TARGET_OS_IPHONE && TARGET_OS_WATCH
  2. define SANITIZER_WATCHOS 1
  3. else
  4. define SANITIZER_WATCHOS 0
  5. endif

and the same for SANITIZER_TVOS, to minimize the places we define this macro at.

zaks.anna updated this revision to Diff 45757.Jan 22 2016, 2:49 PM
zaks.anna edited edge metadata.

Address all comments from Samson's latest review.

samsonov accepted this revision.Jan 22 2016, 2:57 PM
samsonov edited edge metadata.

LGTM (one small nit below). Thank you!

lib/sanitizer_common/sanitizer_mac.cc
364

remove else

This revision is now accepted and ready to land.Jan 22 2016, 2:57 PM
zaks.anna closed this revision.Feb 2 2016, 1:34 PM

Committed in r259451