[PATCH] D89344: Introduce convenience macro ASAN_NO_INSTR_PTR.

[Dan Liew via Phabricator via llvm-commits]

delcypher added a comment.

In D89344#2328787 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89344