[PATCH] D137346: [-Wunsafe-buffer-usage] Initial commit - Transition away from raw buffer accesses.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM



================
Comment at: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h:34
+// through the handler class.
+void checkUnsafeBufferUsage(const Decl *D, UnsafeBufferUsageHandler &Handler);
+
----------------
NoQ wrote:
> aaron.ballman wrote:
> > Do we need the interface to accept a non-const reference?
> It's same as asking whether the `handle...` methods should be const. Roughly similar to whether `MatchFinder::MatchCallback::run()` should be const. I don't see a reason why not, but also "Why say lot word when few word do trick", could have been a lambda.
> 
> As an opposite example, static analyzer's `Checker::check...` callbacks really needed to be const, because carrying mutable state in the checker is often *tempting* but always *terrible*, in a way that's not immediately obvious to beginners.
Hmmm, I may regret this later (I often do when suggesting laxity with `const`), but I suppose it's reasonable to leave the interface mutable. I usually prefer things to be `const` up front and then relax the restriction later once we have a need, but this is a case where relaxing that later would be about as viral as adding `const` later.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11731-11732
+// Unsafe buffer usage diagnostics.
+def warn_unsafe_buffer_usage : Warning<"unchecked operation on raw buffer in expression">,
+  InGroup<DiagGroup<"unsafe-buffer-usage">>, DefaultIgnore;
 } // end of sema component.
----------------
NoQ wrote:
> aaron.ballman wrote:
> > The diagnostic wording isn't wrong, but I am a bit worried about complex expressions. Consider something like `void func(a, b, c + d, e++, f(&g));` -- if you got this warning on that line of code, how likely would you be to spot what caused the diagnostic? I think we need to be sure that issuing this warning *always* passes in an extra `SourceRange` for the part of the expression that's caused the issue so users will at least get some underlined squiggles to help them out.
> Yeah, I agree. Later we'll additionally specialize this warning to be more specific than "expression" (eg. "pointer arithmetic", "array access", etc.).
> 
> Hmm, is there a way to write the .td file entry so that the source range becomes mandatory?
> Hmm, is there a way to write the .td file entry so that the source range becomes mandatory?

Not to my knowledge; we could maybe thread through enough information to assert if the diagnostic is called without a source range, but I'm not certain we could reasonably get a compile-time error if the range isn't provided.


================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2152-2154
+  void handleUnsafeOperation(const Stmt *Operation) override {
+    S.Diag(Operation->getBeginLoc(), diag::warn_unsafe_buffer_usage);
+  }
----------------
NoQ wrote:
> aaron.ballman wrote:
> > I think this interface needs an additional `SourceRange` parameter that can be passed in as a streaming argument, along these lines. This way you'll get squiggles for the problematic part of the expression.
> `Operation->getSourceRange()` does the trick right? Like this:
> ```
> warning: unchecked operation on raw buffer in expression
> 
>   a[i];
>   ~~~~
> ```
> I suspect that the Operation is forever going to be the exact thing we want to highlight, there's virtually no reason for it to be anything else.
> 
> Or do you think it'd be better to do it like this?
> ```
> warning: unchecked operation on raw buffer in expression
> 
>   a[i];
>   ~
> ```
> Operation->getSourceRange() does the trick right?

Yup!

> Or do you think it'd be better to do it like this?

I prefer the first form where the whole operation is highlighted instead of just an operand of the operation. I think that'll make more sense for situations like:
```
a + i;
```
to highlight the whole expression instead of just `a`. It also helpfully means I don't have to ask for a test like: `i[a]` and see if we highlight the correct "raw buffer" operand. :-D


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

https://reviews.llvm.org/D137346