aaron.ballman added inline comments.
================ Comment at: clang/include/clang/Basic/Attr.td:2945 + let Spellings = [Clang<"disable_sanitizer_instrumentation">]; + let Subjects = SubjectList<[Function]>; + let Documentation = [DisableSanitizerInstrumentationDocs]; ---------------- Do we want this to also appertain to `GlobalVar` and `ObjCMethod` so it's got the same subjects as `no_sanitize`? ================ 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: > > 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.) > Yes, right now we only want to apply this attribute to a couple of critical > places in the Linux kernel. > Other users may pop up in e.g. other kernels (NetBSD is using KMSAN) and > maybe userspace programs, but we don't expect it to be popular, so having a > lengthy attribute name is fine. Fantastic, thank you for the confirmation! ================ Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:526-528 + if (CurFuncDecl->hasAttr<DisableSanitizerInstrumentationAttr>()) + return true; + return false; ---------------- 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
