Page MenuHomePhabricator

[asan] Add CMake hook to override shadow scale in compiler_rt
ClosedPublic

Authored by waltl on Oct 31 2017, 1:53 PM.

Details

Summary

Allow user to override shadow scale in compiler_rt by passing
-DCOMPILER_RT_ASAN_SHADOW_SCALE=n to CMake. Propagate the override
shadow scale value via a compiler define to compiler-rt and asan
tests. Tests will use the define to partially disable unsupported
tests. Set "-mllvm -asan-mapping-scale=<n>" for compiler_rt tests.

Diff Detail

Repository
rL LLVM

Event Timeline

waltl created this revision.Oct 31 2017, 1:53 PM
waltl updated this revision to Diff 121058.Oct 31 2017, 2:05 PM

Add "differential revision" line.

waltl updated this revision to Diff 121060.Oct 31 2017, 2:08 PM

Revert mistaken update.

kosarev added a subscriber: kosarev.Nov 1 2017, 1:37 AM
waltl updated this revision to Diff 122001.Nov 7 2017, 3:41 PM

Miscellaneous cleanups.

waltl retitled this revision from [asan] Add cmake hook to override default shadow scale to [asan] Add CMake hook to override default shadow scale.Nov 7 2017, 3:48 PM
waltl edited the summary of this revision. (Show Details)
vitalybuka edited edge metadata.

Can you explain or point to use-case where 5 is better than 3?

waltl added a comment.Nov 8 2017, 12:36 PM

Can you explain or point to use-case where 5 is better than 3?

See http://lists.llvm.org/pipermail/llvm-dev/2017-October/118679.html for background. The end-goal for me is to port ASan to an embedding platform that has no virtual memory and limited memory, where reserving 12.5% of the memory for shadow memory is too high. Getting this to work for x86 is only a small amount of extra work and gives me a way to test/validate the changes in the community.

I think LLVM_ASAN_SHADOW_SCALE is not best approach.
Seems unusual that compiler which can generate code for multiple platforms needs to be reconfigure and rebuild.

@kcc already suggested using regular compile flag: http://lists.llvm.org/pipermail/llvm-dev/2017-October/118685.html
So clang will be able to generate both versions of instrumented code. Obviously we need to have multiple version of run-time so driver will pick proper version based on -fsanitize-address-granularity value. (or somehow we can init the same runtime correctly, not sure yet if it's possible)

Just talked with @eugenis
Maybe simpler approach is following:

  1. we have llvm flag in AddressSanitizer.cpp which deppends on platform, 8 or 32
  2. runtime also will pick 8 or 32 from platfor
  3. if you'd like to setup build bot on e.g. x86 just to test 32, than you call add override flag like this. but it should be in compiler-rt only: e.g. COMPILER_RT_ASAN_SHADOW_GRANULARITY

This flag can switch granularity of runtime and also add -mllvm -asan-shadow-granularity=32 for tests

waltl added a comment.Nov 8 2017, 2:16 PM

Just talked with @eugenis
Maybe simpler approach is following:

  1. we have llvm flag in AddressSanitizer.cpp which deppends on platform, 8 or 32
  2. runtime also will pick 8 or 32 from platfor
  3. if you'd like to setup build bot on e.g. x86 just to test 32, than you call add override flag like this. but it should be in compiler-rt only: e.g. COMPILER_RT_ASAN_SHADOW_GRANULARITY This flag can switch granularity of runtime and also add -mllvm -asan-shadow-granularity=32 for tests

That sounds good; I will work on it. One followup question: the current flag is called -asan-shadow-scale. Is that ok or should I change that to -asan-shadow-granularity? I don't know if there are any users who depend on the existing name.

waltl added a comment.Nov 8 2017, 2:18 PM

That sounds good; I will work on it. One followup question: the current flag is called -asan-shadow-scale. Is that ok or should I change that to -asan-shadow-granularity? I don't know if there are any users who depend on the existing name.

Correction: current flag is called -asan-mapping-scale; there is also an -asan-mapping-offset flag for changing the shadow offset.

I'd keep it as is --asan-mapping-scale. I noticed that you are using this in other changes, but didn't realize that it's already there.

kcc edited edge metadata.Nov 8 2017, 2:47 PM

--asan-maping-scale is an -mmlv flag.
Such flags should not be given to users -- they are internal testing only flags.

All this #ifdef stuff really sucks. I don't claim I immediately see a better way to do this, but pleeeeeease, try to avoid such ifdefs when possible.

kcc added a comment.Nov 8 2017, 2:47 PM

--asan-maping-scale is an -mmlv flag.

-mllvm flag

vitalybuka added a comment.EditedNov 8 2017, 3:41 PM
In D39469#919883, @kcc wrote:

--asan-maping-scale is an -mmlv flag.

-mllvm flag

Users do not need this flag. Platform will control granularity. So this flag is going to be used by bots (llvm and compiler-rt tests), when we enforce granularity unusual for host platform.

waltl updated this revision to Diff 122458.Nov 10 2017, 9:17 AM
waltl edited the summary of this revision. (Show Details)

Change approach to only override shadow scale in compiler_rt

waltl retitled this revision from [asan] Add CMake hook to override default shadow scale to [asan] Add CMake hook to override shadow scale in compiler_rt.Nov 10 2017, 9:30 AM
waltl edited the summary of this revision. (Show Details)
vitalybuka added inline comments.Nov 10 2017, 10:23 AM
compiler-rt/CMakeLists.txt
51 ↗(On Diff #122458)
if (COMPILER_RT_ASAN_SHADOW_SCALE GREATER 8 OR
         COMPILER_RT_ASAN_SHADOW_SCALE LESS 0)
58 ↗(On Diff #122458)

I'd expect following should work:
-mllvm -asan-mapping-scale=${COMPILER_RT_ASAN_SHADOW_SCALE}

compiler-rt/lib/asan/asan_mapping.h
135 ↗(On Diff #122458)

I assume that default for your platform will be changed in separate CL?

compiler-rt/test/lit.common.configured.in
28 ↗(On Diff #122458)

can you just just use target_cflags and COMPILER_RT_TEST_COMPILER_CFLAGS

waltl marked an inline comment as done.Nov 10 2017, 1:09 PM
waltl added inline comments.
compiler-rt/CMakeLists.txt
51 ↗(On Diff #122458)

Note, however, that this will also accept non-numbers like "foo".

58 ↗(On Diff #122458)

You are right. I'll make the change.

compiler-rt/lib/asan/asan_mapping.h
135 ↗(On Diff #122458)

Yes -- down the road when I upstream my port, which doesn't exist yet.

compiler-rt/test/lit.common.configured.in
28 ↗(On Diff #122458)

Maybe? Currently COMPILER_RT_TEST_COMPILER_CFLAGS doesn't get propagated to target_cflags because individual sanitizers have their own definitions of target_cflags that supercede it. Is this intended? If the right thing to do is to append the target_cflags together I can do that and it'd better serve our purpose here.

vitalybuka added inline comments.Nov 10 2017, 2:40 PM
compiler-rt/test/lit.common.configured.in
28 ↗(On Diff #122458)

They append content of COMPILER_RT_TEST_COMPILER_CFLAGS, it's in get_test_cc_for_arch implementation

waltl marked an inline comment as done.Nov 10 2017, 3:28 PM
waltl added inline comments.
compiler-rt/test/lit.common.configured.in
28 ↗(On Diff #122458)

Sorry if I'm missing something, but it looks to me that in get_test_cc_for_arch, COMPILER_RT_TEST_COMPILER_CFLAGS is only used for ANDROID, arm, and aarch64. For others, the target CFLAGS comes from TARGET_${arch}_CFLAGS which is constructed by test_targets(), but I am not seeing COMPILER_RT_TEST_COMPILER_CFLAGS referenced in there. This is also supported by experimentation.

waltl updated this revision to Diff 122549.Nov 10 2017, 4:22 PM
waltl edited the summary of this revision. (Show Details)

Address CR comments.

waltl marked 3 inline comments as done.Nov 10 2017, 4:27 PM

I partially addressed this issue by setting config.target_flags in compiler-rt/test/lit.common.cfg. This still requires config.asan_shadow_scale to be set but we need that to set a feature anyways (https://reviews.llvm.org/D39772).

vitalybuka accepted this revision.Nov 10 2017, 4:41 PM
This revision is now accepted and ready to land.Nov 10 2017, 4:41 PM
waltl edited the summary of this revision. (Show Details)Nov 13 2017, 5:48 AM
This revision was automatically updated to reflect the committed changes.