[PATCH] D89344: Introduce convenience macro ASAN_NO_INSTR_PTR.

[Dan Liew via Phabricator via llvm-commits]

delcypher added a comment.

In D89344#2329256 <https://reviews.llvm.org/D89344#2329256>, @vitalybuka wrote:

> 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.



================
Comment at: compiler-rt/include/sanitizer/asan_interface.h:328
+// by ASan. Loads and stores on pointers of this type will not be
+// instrumented.
+//
----------------
vitalybuka wrote:
> 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.
> 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.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89344/new/

https://reviews.llvm.org/D89344