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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
