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

Aaron Ballman via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Aug 13 11:29:50 PDT 2021


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.
+  }];
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108029



More information about the llvm-commits mailing list