[PATCH] D108029: [clang][Codegen] Introduce the no_sanitizer_instrumentation attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:2601-2602
+
+This is not the same as ``__attribute__((no_sanitize(...)))``, which depending
+on the tool may still insert instrumentation to prevent false positive reports.
+  }];
----------------
melver wrote:
> aaron.ballman wrote:
> > glider wrote:
> > > aaron.ballman wrote:
> > > > Has there been agreement that this isn't actually a bug? My understanding of `no_sanitize` is that it disables sanitizer support for a function or global. The documentation for that attribute says:
> > > > ```
> > > > Use the ``no_sanitize`` attribute on a function or a global variable
> > > > declaration to specify that a particular instrumentation or set of
> > > > instrumentations should not be applied.
> > > > ```
> > > > That sure reads like "do not instrument this at all" to me, and makes me think we don't need a second attribute that says "no, really, I mean it this time."
> > > No, this isn't a bug, but might need to be better clarified in the docs.
> > > ThreadSanitizer and MemorySanitizer do insert some instrumentation into ignore functions to prevent false positives:
> > > 
> > > > ThreadSanitizer still instruments such functions to avoid false positives and provide meaningful stack traces.
> > > 
> > > (https://clang.llvm.org/docs/ThreadSanitizer.html#attribute-no-sanitize-thread)
> > > 
> > > and 
> > > 
> > > > MemorySanitizer may still instrument such functions to avoid false positives.
> > > 
> > > (https://clang.llvm.org/docs/MemorySanitizer.html#attribute-no-sanitize-memory)
> > > 
> > > This is the behavior people rely onto, although at this point I don't think `no_sanitize(...)` is the best name for attribute not disabling instrumentation completely.
> > Thank you for the information!
> > 
> > Having two attributes with basically the same name to perform this functionality is confusing because users (understandably) will reach for the succinctly named one and make assumptions about what it does from the name.
> > 
> > One possibility would be to change `no_sanitize` to take an additional argument, as in: `__attribute__((no_sanitize(fully_disable, "thread")))`. Perhaps another solution would be to give the proposed attribute a more distinct name, like `disable_sanitizer_instrumentation`, `sanitizer_instrumentation_disabled`, or something else.
> Last I looked at `no_sanitize`, it's quite awkward that it is an attribute that accepts arguments, because it makes it very hard to query for existence of attribute additions/changes with `__has_attribute()`. Given this new attribute is meant to be semantically quite different, the cleaner and less intrusive way with that in mind is to create a new attribute. Unless of course there's a nice way to make `__has_attribute()` work.
> 
> Here's another suggestion for name, which actually makes the difference between `no_sanitize` and the new one obvious: `no_sanitize_any_permit_false_positives`
> 
> At least it would semantically tell a user what might happen, which in turn would hopefully make them avoid this attribute (also because it's hard enough to type) unless they are absolutely sure.
> Given this new attribute is meant to be semantically quite different, the cleaner and less intrusive way with that in mind is to create a new attribute. Unless of course there's a nice way to make __has_attribute() work.

That sounds like good rationale for a separate attribute.

> Here's another suggestion for name, which actually makes the difference between no_sanitize and the new one obvious: no_sanitize_any_permit_false_positives
>
> At least it would semantically tell a user what might happen, which in turn would hopefully make them avoid this attribute (also because it's hard enough to type) unless they are absolutely sure.

That would certainly solve my concerns! Do you envision this being used far less often than `no_sanitize`? (That's my intuition, so I'm just double-checking that this isn't expected to be a popular replacement or something where the long name may be really undesirable.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108029