Page MenuHomePhabricator

Introduce convenience macro ASAN_NO_INSTR_PTR.
Needs ReviewPublic

Authored by delcypher on Oct 13 2020, 2:32 PM.

Details

Summary

This macro makes it convenient to annotate pointer types in source code
to avoid ASan instrumentation.

Previously __attribute__((address_space(1))) had to be applied to
pointer types to avoid instrumentation. However, this leaks
implementation details and is cumbersome to write.

The intended use case here is to annotate accesses to regions of memory
on Darwin that are outside of normal user memory. These regions do not
have a corresponding shadow memory and so when instrumentation tries to
look to access the shadow bytes it causes non-deterministic behaviour.

A test case is included to test various aspects of the macro.

rdar://problem/70234898

Diff Detail

Unit TestsFailed

TimeTest
4,150 mswindows > Clang-Unit.DirectoryWatcher/_/DirectoryWatcherTests_exe::DirectoryWatcherTest.AddFiles
Note: Google Test filter = DirectoryWatcherTest.AddFiles [==========] Running 1 test from 1 test case.
4,019 mswindows > Clang-Unit.DirectoryWatcher/_/DirectoryWatcherTests_exe::DirectoryWatcherTest.DeleteFile
Note: Google Test filter = DirectoryWatcherTest.DeleteFile [==========] Running 1 test from 1 test case.
4,059 mswindows > Clang-Unit.DirectoryWatcher/_/DirectoryWatcherTests_exe::DirectoryWatcherTest.ModifyFile
Note: Google Test filter = DirectoryWatcherTest.ModifyFile [==========] Running 1 test from 1 test case.

Event Timeline

delcypher created this revision.Oct 13 2020, 2:32 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 13 2020, 2:32 PM
Herald added subscribers: Restricted Project, hiraditya. · View Herald Transcript
delcypher requested review of this revision.Oct 13 2020, 2:32 PM
yln added a comment.Oct 13 2020, 3:38 PM

Previously attribute((address_space(1))) had to be applied to pointer types to avoid instrumentation.

I wasn't aware of this. Is this a principled way to tell ASan to skip instrumentation of a local var, or is it used because "it works" (but it depends on implementation details)?
Note that I don't know the actual meaning/semantics of address_space.

Would generalizing __attribute__((no_sanitize("address"))) to also apply to local vars (of pointer type) be a cleaner way?
https://clang.llvm.org/docs/AddressSanitizer.html#disabling-instrumentation-with-attribute-no-sanitize-address

In D89344#2328787, @yln wrote:

Previously attribute((address_space(1))) had to be applied to pointer types to avoid instrumentation.

I wasn't aware of this. Is this a principled way to tell ASan to skip instrumentation of a local var, or is it used because "it works" (but it depends on implementation details)?
Note that I don't know the actual meaning/semantics of address_space.

My understanding is that attribute is used "because it works", not because it's principled in anyway.

IIUC correctly the address_space attribute maps to LLVM IR's notion of address space. Here's a snippet from the LLVM language reference

A global variable may be declared to reside in a target-specific numbered address space. For targets that support them, address spaces may affect how optimizations are performed and/or what target instructions are used to access the variable. The default address space is zero. The address space qualifier must precede any other attributes.

This suggests that there might be downsides to using this attribute (e.g. it might inhibit optimizations)

Would generalizing __attribute__((no_sanitize("address"))) to also apply to local vars (of pointer type) be a cleaner way?
https://clang.llvm.org/docs/AddressSanitizer.html#disabling-instrumentation-with-attribute-no-sanitize-address

I don't think that will work because I think no_sanitize is a function level attribute.

If there is a more principled attribute then I'm happy to change this patch to use it. If such an attribute doesn't exist then I do not think that this patch should be blocked on having a more principled attribute . The whole point of this patch is to introduce a macro that hides the current implementation details. Having the macro introduced in this patch means that we can change the underlying implementation details and users of the macro will not need to care.

In D89344#2328787, @yln wrote:

Previously attribute((address_space(1))) had to be applied to pointer types to avoid instrumentation.

I wasn't aware of this. Is this a principled way to tell ASan to skip instrumentation of a local var, or is it used because "it works" (but it depends on implementation details)?
Note that I don't know the actual meaning/semantics of address_space.

My understanding is that attribute is used "because it works", not because it's principled in anyway.

IIUC correctly the address_space attribute maps to LLVM IR's notion of address space. Here's a snippet from the LLVM language reference

A global variable may be declared to reside in a target-specific numbered address space. For targets that support them, address spaces may affect how optimizations are performed and/or what target instructions are used to access the variable. The default address space is zero. The address space qualifier must precede any other attributes.

This suggests that there might be downsides to using this attribute (e.g. it might inhibit optimizations)

Would generalizing __attribute__((no_sanitize("address"))) to also apply to local vars (of pointer type) be a cleaner way?
https://clang.llvm.org/docs/AddressSanitizer.html#disabling-instrumentation-with-attribute-no-sanitize-address

I don't think that will work because I think no_sanitize is a function level attribute.

If there is a more principled attribute then I'm happy to change this patch to use it. If such an attribute doesn't exist then I do not think that this patch should be blocked on having a more principled attribute . The whole point of this patch is to introduce a macro that hides the current implementation details. Having the macro introduced in this patch means that we can change the underlying implementation details and users of the macro will not need to care.

@yln I tried using the attribute on a local variable. It's not supported.

Volumes/data/dev/llvm/llvm.org/master/llvm/compiler-rt/test/asan/TestCases/no_instrument_pointer.cpp:35:3: error: 'no_sanitize' attribute only applies to functions, Objective-C methods, and global variables
  ASAN_NO_INSTR_PTR(int *)
  ^
/Volumes/data/dev/llvm/llvm.org/master/builds/RelWithDebInfo/lib/clang/12.0.0/include/sanitizer/asan_interface.h:343:54: note: expanded from macro 'ASAN_NO_INSTR_PTR'
#      define ASAN_NO_INSTR_PTR(TYPE) __attribute__((no_sanitize("address"))) TYPE
                                                     ^
/Volumes/data/dev/llvm/llvm.org/master/llvm/compiler-rt/test/asan/TestCases/no_instrument_pointer.cpp:36:10: warning: 'no_sanitize' attribute ignored when parsing type [-Wignored-attributes]
  ptr = (ASAN_NO_INSTR_PTR(int *))source();
         ^~~~~~~~~~~~~~~~~~~~~~~~
/Volumes/data/dev/llvm/llvm.org/master/builds/RelWithDebInfo/lib/clang/12.0.0/include/sanitizer/asan_interface.h:343:54: note: expanded from macro 'ASAN_NO_INSTR_PTR'
#      define ASAN_NO_INSTR_PTR(TYPE) __attribute__((no_sanitize("address"))) TYPE
                                                     ^~~~~~~~~~~~~~~~~~~~~~
1 warning and 1 error generated.

I recommend abandon interface change, add a test for llvm/lib/Transforms/Instrumentation if there is no any yet, and document this trick in https://clang.llvm.org/docs/AddressSanitizer.html

compiler-rt/include/sanitizer/asan_interface.h
328

I don't think it should be here
we don't put here defines for attribute((no_sanitize("address")))

Also this header is interface for runtime, and define controls compiler side.

Ubsan ignores volatile vars https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
maybe better to make sure that asan does the same here as well?

I recommend abandon interface change, add a test for llvm/lib/Transforms/Instrumentation if there is no any yet, and document this trick in https://clang.llvm.org/docs/AddressSanitizer.html

I don't think comparing __attribute__((address_space(1))) with attribute((no_sanitize("address"))) is a fair comparison. They are very different.

In the case of no_sanitize("address"), this attribute was built for the sanitizers and it is actually clear that this attribute disables ASan in someway. This seems like an acceptable (it's somewhat clear what it does) interface to use and hence why we have documentation telling users that they can use this.

We cannot say the same about address_space(1). It was not built for the sanitizers and it is not clear at all that this attribute has anything to with the sanitizers at all. The fact that changing the address space on a pointer type opts loads/stores out of ASan is an implementation detail that I don't think we should be exposing to users.
Therefore I do not see asking users to use address_space(1) directly as an acceptable solution.

compiler-rt/include/sanitizer/asan_interface.h
328

Also this header is interface for runtime, and define controls compiler side.

This header already provides defines for use at compile time so I don't agree with this objection.

Alternatively we could introduce a new header file (e.g. santizer/asan_util.h) for convenience macros that are used at compile time.

Ubsan ignores volatile vars https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
maybe better to make sure that asan does the same here as well?

Why would we want that behaviour for ASan? I checked and it looks like volatile pointers do get instrumented by ASan and I see no reason to change that.

Ubsan ignores volatile vars https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
maybe better to make sure that asan does the same here as well?

Why would we want that behaviour for ASan? I checked and it looks like volatile pointers do get instrumented by ASan and I see no reason to change that.

To have consistent way to disable instrumentation on variable.

compiler-rt/include/sanitizer/asan_interface.h
328

Either this or asan_util.h are going to be used by external code which we can't see. In future it's going to be hard to remove or change anything that here so we like to keep as simple as possible and avoid adding unnecessary convenience stuff here.

Defines you mentioned exist from 2011 and probably we didn't remove them for the reason above. But I don't see recent convenience stuff here.

I don't know if it's official but I assume we expect such tools inside of clients code. @kcc ?

343

Why does it need (TYPE) as parameter?