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