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