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

Mon Aug 16 02:19:08 PDT 2021

glider marked 2 inline comments as done.
glider 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:
> 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.

================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:524
+bool CodeGenFunction::ShouldSkipSanitizerInstrumentation() {
+  if (!CurFuncDecl)
+    return false;
----------------
dvyukov wrote:
> When is CurFuncDecl nullptr? Maybe we should look at definition in this case?
I borrowed this check from ShouldInstrumentFunction(), it fires e.g. in the following case (borrowed from an existing regtest):

```
namespace rdar9445102 {
  class Rdar9445102 {
    public:
      Rdar9445102();
  };
}

static rdar9445102::Rdar9445102 s_rdar9445102Initializer;
```

We don't need to distinguish between the declaration and the definition, it happens automatically.
E.g. if we apply the attribute to either of them, the resulting function will have the disable_sanitizer_instrumentation LLVM attribute.

================
Comment at: llvm/include/llvm/AsmParser/LLToken.h:222
   kw_nosanitize_coverage,
+  kw_no_sanitizer_instrumentation,
   kw_null_pointer_is_valid,
----------------
melver wrote:
> LLVM seems to just call them nofoo i.e. here it'd be nosanitizer_instrumentation.
> I think Clang and LLVM are a bit inconsistent in their naming (no_foo vs nofoo) . See e.g. nosanitize_coverage which is similarly inconsistent yet consistent with other LLVM attrs.
Renamed the attribute to disable_sanitizer_instrumentation per Evgenii's comment.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108029