[-Wunsafe-buffer-usage] NFC: Implement fix-strategies and variable-use-claiming.

[NoQ]

Summary

This patch adds more abstractions that we'll need later for emitting -Wunsafe-buffer-usage fixits. It doesn't emit any actual fixits, so no change is observed behavior, but it introduces a way to emit fixits, and existing tests now verify that the compiler still emits no fixits, despite knowing how to do so.

The purpose of our code transformation analysis is to fix variable types in the code from raw pointer types to C++ standard collection/view types.

The analysis has to decide on its own which specific type is the most appropriate for every variable. This patch introduces the Strategy class that maps variables to their most appropriate types.

In D137348 we've introduced the Gadget abstraction, which describes a rigid AST pattern that the analysis "fully understands" and may need to fix. Which specific fix is actually necessary for a given Gadget, and whether it's necessary at all, and whether it's possible in the first place, depends on the Strategy. So, this patch adds a virtual method which every gadget can implement in order to teach the analysis how to fix that gadget:

Gadget->getFixits(Strategy)

However, even if the analysis knows how to fix every Gadget, doesn't necessarily mean it can fix the variable. Some uses of the variable may have never been covered by Gadgets, which corresponds to the situation that the analysis doesn't fully understand how the variable is used. This patch introduces a Tracker class that tracks all variable uses (i.e. DeclRefExprs) in the function. Additionally, each Gadget now provides a new virtual method

Gadget->getClaimedVarUseSites()

that the Tracker can call to see which DeclRefExprs are "claimed" by the Gadget. In order to fix the variable with a certain Strategy, the Tracker needs to confirm that there are no unclaimed uses, and every Gadget has to provide a fix for that Strategy.

This "conservative" behavior guarantees that fixes emitted by our analysis are correct by construction. We can now be sure that the analysis won't attempt to emit a fix if it doesn't understand the code. Later, as we implement more getFixits() methods in individual Gadget classes, we'll start progressively emitting more and more fixits.

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

236

This extra payload wasn't advertised but it's very useful because it's hard to jump from VarDecl to DeclStmt without it, and we need to do such jump because our analysis starts from unsafe *uses* of a variable, which only give us a VarDecl.

447–451

We're safe for now because none of the currently implemented Gadget classes "claim" more than one DeclRefExpr. We need to address this FIXME before we implement such gadgets.

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

236

Another way to look at this, is to notice that currently DeclStmt of the variable isn't a Gadget on its own, and ask whether it *should* be a Gadget.

I currently believe that yes, it should be a Gadget, because it can potentially be a "multi-variable" Gadget if, for example, the initializer-expression is itself another raw pointer DeclRefExpr. So there needs to be a way to communicate that the fix for the variable declaration may depend on Strategy for more than one variable, and that the Strategy itself should be chosen carefully, considering that choices for different variables may interact this way. And from the point of view of the *other* variable, this definitely needs to be a Gadget, it's no different from any other Gadget.

So, this part needs some more work.

[xazax.hun]

clang/lib/Analysis/UnsafeBufferUsage.cpp

236

What about DeclStmts that declare multiple variables, some safe some unsafe. Do you plan to generate fixits in that case exploding the DeclStmt?

[steakhal]

clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h

30

Unless we have a well-formed idea of how many elements we are going to have, we should probably omit it.

clang/lib/Analysis/UnsafeBufferUsage.cpp

83

90

I wonder if we should prefer std::optional as we are c++17.
IDK if it was ever covered on the community forum.
I'm just leaving this here without expecting any action.

231–233

259–262

Maybe Uses should refer to the canonical decls. If that would be the case we could just call .contains(VD) here.

408

428–430

[steakhal]

clang/lib/Analysis/UnsafeBufferUsage.cpp

90

Per https://discourse.llvm.org/t/deprecating-llvm-optional-x-hasvalue-getvalue-getvalueor/63716/10, it seems like we should prefer std::optional.

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

90

Oooo nice.

236

Yes absolutely.

They're annoying because if the variable is in the middle, you'll get 3 DeclStmts instead of one, but yeah, we'll have to handle those.

259–262

That's up to the AST, not up to us. I'd rather be safe here, because such bugs are very hard to notice.

428–430

I can get used to that!

[aaron.ballman]

clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h

30

Better yet, use a SmallVectorImpl (since this shows up in an interface)

clang/include/clang/Basic/DiagnosticSemaKinds.td

11654–11657

clang/lib/Analysis/UnsafeBufferUsage.cpp

17–19

Do we have to care about data member accesses (which would be a MemberExpr and not a DeclRefExpr)?

Also, because this shows up in interfaces, it should probably be a SmallVectorImpl so the size doesn't matter.

266–271

Use a specific_decl_iterator<VarDecl> instead?

329–335

clang/lib/Sema/AnalysisBasedWarnings.cpp

2161–2163

Hmmm... llvm::for_each instead?

[NoQ]

A cleaner way to implement the debug facility.

[NoQ]

Whoops, sorry, wrong patch. I was trying to say, "Address review comments".

clang/lib/Analysis/UnsafeBufferUsage.cpp

17–19

Do we have to care about data member accesses (which would be a MemberExpr and not a DeclRefExpr)?

Unlikely. Auto-fixing member variables without breaking source or binary compatibility is next to impossible.

Also, because this shows up in interfaces, it should probably be a SmallVectorImpl so the size doesn't matter.

This doesn't seem to work on return types, which is where these classes show up in my case (SmallVectorImpl has deleted copy-move constructors so it's only suitable for passing by reference).

266–271

Hmm DeclStmt doesn't seem to provide a neat accessor, would you like me to add it?

[aaron.ballman]

clang/lib/Analysis/UnsafeBufferUsage.cpp

17–19

Do we have to care about data member accesses (which would be a MemberExpr and not a DeclRefExpr)?

Unlikely. Auto-fixing member variables without breaking source or binary compatibility is next to impossible.

Okie dokie!

Also, because this shows up in interfaces, it should probably be a SmallVectorImpl so the size doesn't matter.

This doesn't seem to work on return types, which is where these classes show up in my case (SmallVectorImpl has deleted copy-move constructors so it's only suitable for passing by reference).

Ahhh I had forgotten that we didn't allow it as a move-only type.

266–271

Eh, I don't insist. Feel free to do it as an NFC change after landing this patch, if you want to.

[NoQ]

Because we're approaching a stack of ~15 patches and it's starting to become unmanageable, I'm really interested in landing these older patches quickly. I think I addressed the feedback, and I'm definitely committed to addressing more feedback as it comes in, but it's likely that I'd ask for a chance to address it in follow-up patches anyway. Because otherwise the rest of our team will have to undergo annoying rebases, and it's probably also super confusing to review when all patches in the stack are out of sync from each other. So I'll try to land this tomorrow.

[jkorous]

LGTM

[uabelho]

Hello,

I noticed that the following crashes with this patch:

clang -Weverything bbi-77071.c

Result:

clang-16: ../../clang/lib/Analysis/UnsafeBufferUsage.cpp:248: void (anonymous namespace)::DeclUseTracker::discoverDecl(const clang::DeclStmt *): Assertion `Defs.count(VD) == 0 && "Definition already discovered!"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.	Program arguments: /repo/uabelho/main-github/llvm/build-all/bin/clang-16 -cc1 -triple x86_64-unknown-linux-gnu -emit-obj -mrelax-all --mrelax-relocations -disable-free -clear-ast-before-backend -main-file-name bbi-77071.c -mrelocation-model pic -pic-level 2 -pic-is-pie -mframe-pointer=all -fmath-errno -ffp-contract=on -fno-rounding-math -mconstructor-aliases -funwind-tables=2 -target-cpu x86-64 -tune-cpu generic -mllvm -treat-scalable-fixed-error-as-warning -debugger-tuning=gdb -fcoverage-compilation-dir=/repo/uabelho/llvm-dev2/llvm -resource-dir /repo/uabelho/main-github/llvm/build-all/lib/clang/16 -internal-isystem /repo/uabelho/main-github/llvm/build-all/lib/clang/16/include -internal-isystem /usr/local/include -internal-isystem /usr/lib/gcc/x86_64-redhat-linux/4.8.5/../../../../x86_64-redhat-linux/include -internal-externc-isystem /include -internal-externc-isystem /usr/include -Weverything -fdebug-compilation-dir=/repo/uabelho/llvm-dev2/llvm -ferror-limit 19 -fgnuc-version=4.2.1 -fcolor-diagnostics -faddrsig -D__GCC_HAVE_DWARF2_CFI_ASM=1 -o /tmp/bbi-77071-6173ad.o -x c bbi-77071.c
1.	<eof> parser at end of file
2.	bbi-77071.c:5:9: parsing function body 'fn1'
 #0 0x0000000002fdf843 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/repo/uabelho/main-github/llvm/build-all/bin/clang-16+0x2fdf843)
 #1 0x0000000002fdd56e llvm::sys::RunSignalHandlers() (/repo/uabelho/main-github/llvm/build-all/bin/clang-16+0x2fdd56e)
 #2 0x0000000002fdfbc6 SignalHandler(int) Signals.cpp:0:0
 #3 0x00007fcfa502a630 __restore_rt sigaction.c:0:0
 #4 0x00007fcfa2771387 raise (/lib64/libc.so.6+0x36387)
 #5 0x00007fcfa2772a78 abort (/lib64/libc.so.6+0x37a78)
 #6 0x00007fcfa276a1a6 __assert_fail_base (/lib64/libc.so.6+0x2f1a6)
 #7 0x00007fcfa276a252 (/lib64/libc.so.6+0x2f252)
 #8 0x0000000005520daf findGadgets(clang::Decl const*)::GadgetFinderCallback::run(clang::ast_matchers::MatchFinder::MatchResult const&) UnsafeBufferUsage.cpp:0:0
 #9 0x000000000554c862 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::MatchVisitor::visitMatch(clang::ast_matchers::BoundNodes const&) ASTMatchFinder.cpp:0:0
#10 0x000000000557d27b clang::ast_matchers::internal::BoundNodesTreeBuilder::visitMatches(clang::ast_matchers::internal::BoundNodesTreeBuilder::Visitor*) (/repo/uabelho/main-github/llvm/build-all/bin/clang-16+0x557d27b)
#11 0x000000000554be73 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::matchWithFilter(clang::DynTypedNode const&) ASTMatchFinder.cpp:0:0
#12 0x0000000005524f9c clang::ast_matchers::MatchFinder::match(clang::DynTypedNode const&, clang::ASTContext&) (/repo/uabelho/main-github/llvm/build-all/bin/clang-16+0x5524f9c)
#13 0x000000000551eb53 clang::checkUnsafeBufferUsage(clang::Decl const*, clang::UnsafeBufferUsageHandler&) (/repo/uabelho/main-github/llvm/build-all/bin/clang-16+0x551eb53)
#14 0x00000000053f48e5 clang::sema::AnalysisBasedWarnings::IssueWarnings(clang::sema::AnalysisBasedWarnings::Policy, clang::sema::FunctionScopeInfo*, clang::Decl const*, clang::QualType) (/repo/uabelho/main-github/llvm/build-all/bin/clang-16+0x53f48e5)
#15 0x0000000004b91a53 clang::Sema::PopFunctionScopeInfo(clang::sema::AnalysisBasedWarnings::Policy const*, clang::Decl const*, clang::QualType) (/repo/uabelho/main-github/llvm/build-all/bin/clang-16+0x4b91a53)
#16 0x0000000004d14f6f clang::Sema::ActOnFinishFunctionBody(clang::Decl*, clang::Stmt*, bool) (/repo/uabelho/main-github/llvm/build-all/bin/clang-16+0x4d14f6f)
#17 0x0000000004b29135 clang::Parser::ParseFunctionStatementBody(clang::Decl*, clang::Parser::ParseScope&) (/repo/uabelho/main-github/llvm/build-all/bin/clang-16+0x4b29135)
#18 0x0000000004a50d95 clang::Parser::ParseFunctionDefinition(clang::ParsingDeclarator&, clang::Parser::ParsedTemplateInfo const&, clang::Parser::LateParsedAttrList*) (/repo/uabelho/main-github/llvm/build-all/bin/clang-16+0x4a50d95)
#19 0x0000000004a6d637 clang::Parser::ParseDeclGroup(clang::ParsingDeclSpec&, clang::DeclaratorContext, clang::ParsedAttributes&, clang::SourceLocation*, clang::Parser::ForRangeInit*) (/repo/uabelho/main-github/llvm/build-all/bin/clang-16+0x4a6d637)
#20 0x0000000004a4f9a7 clang::Parser::ParseDeclOrFunctionDefInternal(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec&, clang::AccessSpecifier) (/repo/uabelho/main-github/llvm/build-all/bin/clang-16+0x4a4f9a7)
#21 0x0000000004a4f238 clang::Parser::ParseDeclarationOrFunctionDefinition(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec*, clang::AccessSpecifier) (/repo/uabelho/main-github/llvm/build-all/bin/clang-16+0x4a4f238)
#22 0x0000000004a4e35e clang::Parser::ParseExternalDeclaration(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec*) (/repo/uabelho/main-github/llvm/build-all/bin/clang-16+0x4a4e35e)
#23 0x0000000004a4b973 clang::Parser::ParseTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&, clang::Sema::ModuleImportState&) (/repo/uabelho/main-github/llvm/build-all/bin/clang-16+0x4a4b973)
#24 0x0000000004a45da5 clang::ParseAST(clang::Sema&, bool, bool) (/repo/uabelho/main-github/llvm/build-all/bin/clang-16+0x4a45da5)
#25 0x0000000003acb486 clang::FrontendAction::Execute() (/repo/uabelho/main-github/llvm/build-all/bin/clang-16+0x3acb486)
#26 0x0000000003a3d534 clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) (/repo/uabelho/main-github/llvm/build-all/bin/clang-16+0x3a3d534)
#27 0x0000000003b8cd92 clang::ExecuteCompilerInvocation(clang::CompilerInstance*) (/repo/uabelho/main-github/llvm/build-all/bin/clang-16+0x3b8cd92)
#28 0x00000000009f8516 cc1_main(llvm::ArrayRef<char const*>, char const*, void*) (/repo/uabelho/main-github/llvm/build-all/bin/clang-16+0x9f8516)
#29 0x00000000009f53b0 ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) driver.cpp:0:0
#30 0x00000000009f4e66 clang_main(int, char**) (/repo/uabelho/main-github/llvm/build-all/bin/clang-16+0x9f4e66)
#31 0x00007fcfa275d555 __libc_start_main (/lib64/libc.so.6+0x22555)
#32 0x00000000009f0fbb _start (/repo/uabelho/main-github/llvm/build-all/bin/clang-16+0x9f0fbb)
clang-16: error: unable to execute command: Aborted (core dumped)
clang-16: error: clang frontend command failed due to signal (use -v to see invocation)
clang version 16.0.0
Target: x86_64-unknown-linux-gnu
Thread model: posix

bbi-77071.c110 BDownload

[NoQ]

Ooo thanks I'll fix asap.

[NoQ]

Also if you're compiling your code with -Weverything by default, I do recommend explicitly disabling -Wunsafe-buffer-usage for at least a month or so, until we land all the basic functionality and you can make an informed decision whether you actually need it (more reading in https://discourse.llvm.org/t/rfc-c-buffer-hardening/65734 and D136811).

But, yeah, we still shouldn't break users who simply want to enable -Weverything temporarily just to see what warnings are there, that's really bad and I hope I'll patch this up today.

[uabelho]

In D138253#4006197, @NoQ wrote:

Also if you're compiling your code with -Weverything by default, I do recommend explicitly disabling -Wunsafe-buffer-usage for at least a month or so, until we land all the basic functionality and you can make an informed decision whether you actually need it (more reading in https://discourse.llvm.org/t/rfc-c-buffer-hardening/65734 and D136811).

But, yeah, we still shouldn't break users who simply want to enable -Weverything temporarily just to see what warnings are there, that's really bad and I hope I'll patch this up today.

Thanks for the warning. In our fuzzy testing we sometimes randomly use -Weverything just to make sure it doesn't crash. And well, it did crash here, so I guess the testing was succesful :)
So it's no panic for us, but good to sort out the crash.

[NoQ]

I suppressed the assertion in rG8fd62e7. It looks like we'll need a deeper investigation into this.

[uabelho]

It crashes on this as well:

void f(int a[])
{
    int b = {a[1]};
}

with

clang -cc1 foo.c -Weverything

I get

clang: ../../clang/lib/Analysis/UnsafeBufferUsage.cpp:233: void (anonymous namespace)::DeclUseTracker::claimUse(const clang::DeclRefExpr *): Assertion `Uses->count(DRE) && "DRE not found or claimed by multiple matchers!"' failed.

But now I will indeed add -Wno-unsafe-buffer-usage. :)

clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h

 
 /// The interface that lets the caller handle unsafe buffer usage analysis
 /// results by overriding this class's handle... methods.
 class UnsafeBufferUsageHandler {
 public:
   UnsafeBufferUsageHandler() = default;
   virtual ~UnsafeBufferUsageHandler() = default;
 
+  /// This analyses produces large fixits that are organized into lists
+  /// of primitive fixits (individual insertions/removals/replacements).
+  using FixItList = llvm::SmallVector<FixItHint, 2>;
+
   /// Invoked when an unsafe operation over raw pointers is found.
   virtual void handleUnsafeOperation(const Stmt *Operation) = 0;
+
+  /// Invoked when a fix is suggested against a variable.
+  virtual void handleFixableVariable(const VarDecl *Variable,
+                                     FixItList &&List) = 0;
 };
 
 // This function invokes the analysis and allows the caller to react to it
 // through the handler class.
 void checkUnsafeBufferUsage(const Decl *D, UnsafeBufferUsageHandler &Handler);
 
 } // end namespace clang
 
 #endif /* LLVM_CLANG_ANALYSIS_ANALYSES_UNSAFEBUFFERUSAGE_H */

clang/include/clang/Basic/DiagnosticGroups.td

def PedanticMacros : DiagGroup<"pedantic-macros",
                      RestrictExpansionMacro,
                      FinalMacro]>;
 
 def BranchProtection : DiagGroup<"branch-protection">;
 
 // HLSL diagnostic groups
 // Warnings for HLSL Clang extensions
 def HLSLExtension : DiagGroup<"hlsl-extensions">;
+
+def UnsafeBufferUsage : DiagGroup<"unsafe-buffer-usage">;

clang/include/clang/Basic/DiagnosticSemaKinds.td

 
 // Layout randomization diagnostics.
 def err_non_designated_init_used : Error<
   "a randomized struct can only be initialized with a designated initializer">;
 def err_cast_from_randomized_struct : Error<
   "casting from randomized structure pointer type %0 to %1">;
 
 // Unsafe buffer usage diagnostics.
-def warn_unsafe_buffer_usage : Warning<"unchecked operation on raw buffer in expression">,
-  InGroup<DiagGroup<"unsafe-buffer-usage">>, DefaultIgnore;
+def warn_unsafe_buffer_expression : Warning<"unchecked operation on raw buffer in expression">,
+  InGroup<UnsafeBufferUsage>, DefaultIgnore;
+def warn_unsafe_buffer_variable : Warning<"variable %0 participates in unchecked buffer operations">,
+  InGroup<UnsafeBufferUsage>, DefaultIgnore;
 } // end of sema component.

clang/lib/Analysis/UnsafeBufferUsage.cpp

 //===- UnsafeBufferUsage.cpp - Replace pointers with modern C++ -----------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Analysis/Analyses/UnsafeBufferUsage.h"
 #include "llvm/ADT/SmallVector.h"
 
 using namespace llvm;
 using namespace clang;
 using namespace ast_matchers;
 
+namespace {
+// Because the analysis revolves around variables and their types, we'll need to
+// track uses of variables (aka DeclRefExprs).
+using DeclUseList = SmallVector<const DeclRefExpr *, 1>;
+
+// Convenience typedef.
+using FixItList = UnsafeBufferUsageHandler::FixItList;
+
+// Defined below.
+class Strategy;
+} // namespace
+
 // Because we're dealing with raw pointers, let's define what we mean by that.
 static auto hasPointerType() {
   return anyOf(hasType(pointerType()),
                hasType(autoType(hasDeducedType(
                    hasUnqualifiedDesugaredType(pointerType())))));
 }
 
 namespace {
#undef SAFE_GADGET
 
   Gadget(Kind K) : K(K) {}
 
   Kind getKind() const { return K; }
 
   virtual bool isSafe() const = 0;
   virtual const Stmt *getBaseStmt() const = 0;
 
+  /// Returns the list of pointer-type variables on which this gadget performs
+  /// its operation. Typically there's only one variable. This isn't a list
+  /// of all DeclRefExprs in the gadget's AST!
+  virtual DeclUseList getClaimedVarUseSites() const = 0;
+
+  /// Returns a fixit that would fix the current gadget according to
+  /// the current strategy. Returns None if the fix cannot be produced;
+  /// returns an empty list if no fixes are necessary.
+  virtual Optional<FixItList> getFixits(const Strategy &) const {
+    return None;
+  }
+
   virtual ~Gadget() {}
 
 private:
   Kind K;
 };
 
 using GadgetList = std::vector<std::unique_ptr<Gadget>>;
 
public:
   static Matcher matcher() {
     return stmt(unaryOperator(
       hasOperatorName("++"),
       hasUnaryOperand(ignoringParenImpCasts(hasPointerType()))
     ).bind("op"));
   }
 
   const Stmt *getBaseStmt() const override { return Op; }
+
+  DeclUseList getClaimedVarUseSites() const override {
+    if (const auto *DRE =
+            dyn_cast<DeclRefExpr>(Op->getSubExpr()->IgnoreParenImpCasts())) {
+      return {DRE};
+    }
+
+    return {};
+  }
 };
 
 /// A decrement of a pointer-type value is unsafe as it may run the pointer
 /// out of bounds.
 class DecrementGadget : public UnsafeGadget {
   const UnaryOperator *Op;
 
 public:
   DecrementGadget(const MatchFinder::MatchResult &Result)
       : UnsafeGadget(Kind::Decrement),
         Op(Result.Nodes.getNodeAs<UnaryOperator>("op")) {}
 
   static bool classof(const Gadget *G) {
     return G->getKind() == Kind::Decrement;
   }
 
   static Matcher matcher() {
     return stmt(unaryOperator(
       hasOperatorName("--"),
       hasUnaryOperand(ignoringParenImpCasts(hasPointerType()))
     ).bind("op"));
   }
 
   const Stmt *getBaseStmt() const override { return Op; }
+
+  DeclUseList getClaimedVarUseSites() const override {
+    if (const auto *DRE =
+            dyn_cast<DeclRefExpr>(Op->getSubExpr()->IgnoreParenImpCasts())) {
+      return {DRE};
+    }
+
+    return {};
+  }
 };
 
 /// Array subscript expressions on raw pointers as if they're arrays. Unsafe as
 /// it doesn't have any bounds checks for the array.
 class ArraySubscriptGadget : public UnsafeGadget {
   const ArraySubscriptExpr *ASE;
 
 public:
static Matcher matcher() {
     // FIXME: What if the index is integer literal 0? Should this be
     // a safe gadget in this case?
     return stmt(
         arraySubscriptExpr(hasBase(ignoringParenImpCasts(hasPointerType())))
             .bind("arraySubscr"));
   }
 
   const Stmt *getBaseStmt() const override { return ASE; }
+
+  DeclUseList getClaimedVarUseSites() const override {
+    if (const auto *DRE =
+            dyn_cast<DeclRefExpr>(ASE->getBase()->IgnoreParenImpCasts())) {
+      return {DRE};
+    }
+
+    return {};
+  }
 };
 } // namespace
 
-// Scan the function and return a list of gadgets found with provided kits.
-static GadgetList findGadgets(const Decl *D) {
+namespace {
+// An auxiliary tracking facility for the fixit analysis. It helps connect
+// declarations to its and make sure we've covered all uses with our analysis
+// before we try to fix the declaration.
+class DeclUseTracker {
+  using UseSetTy = SmallSet<const DeclRefExpr *, 16>;
+  using DefMapTy = DenseMap<const VarDecl *, const DeclStmt *>;
+
+  // Allocate on the heap for easier move.
+  std::unique_ptr<UseSetTy> Uses{std::make_unique<UseSetTy>()};
+  DefMapTy Defs{};
+
+public:
+  DeclUseTracker() = default;
+  DeclUseTracker(const DeclUseTracker &) = delete; // Let's avoid copies.
+  DeclUseTracker(DeclUseTracker &&) = default;
+
+  // Start tracking a freshly discovered DRE.
+  void discoverUse(const DeclRefExpr *DRE) { Uses->insert(DRE); }
+
+  // Stop tracking the DRE as it's been fully figured out.
+  void claimUse(const DeclRefExpr *DRE) {
+    assert(Uses->count(DRE) &&
+           "DRE not found or claimed by multiple matchers!");
+    Uses->erase(DRE);
+  }
+
+  // A variable is unclaimed if at least one use is unclaimed.
+  bool hasUnclaimedUses(const VarDecl *VD) const {
+    // FIXME: Can this be less linear? Maybe maintain a map from VDs to DREs?
+    return any_of(*Uses, [VD](const DeclRefExpr *DRE) {
+      return DRE->getDecl()->getCanonicalDecl() == VD->getCanonicalDecl();
+    });
+  }
+
+  void discoverDecl(const DeclStmt *DS) {
+    for (const Decl *D: DS->decls()) {
+      if (const auto *VD = dyn_cast<VarDecl>(D)) {
+        assert(Defs.count(VD) == 0 && "Definition already discovered!");
+        Defs[VD] = DS;
+      }
+    }
+  }
+
+  const DeclStmt *lookupDecl(const VarDecl *VD) const {
+    auto It = Defs.find(VD);
+    assert(It != Defs.end() && "Definition never discovered!");
+    return It->second;
+  }
+};
+} // namespace
+
+namespace {
+// Strategy is a map from variables to the way we plan to emit fixes for
+// these variables. It is figured out gradually by trying different fixes
+// for different variables depending on gadgets in which these variables
+// participate.
+class Strategy {
+public:
+  enum class Kind {
+    Wontfix,    // We don't plan to emit a fixit for this variable.
+    Span,       // We recommend replacing the variable with std::span.
+    Iterator,   // We recommend replacing the variable with std::span::iterator.
+    Array,      // We recommend replacing the variable with std::array.
+    Vector      // We recommend replacing the variable with std::vector.
+  };
+
+private:
+  using MapTy = llvm::DenseMap<const VarDecl *, Kind>;
 
-  class GadgetFinderCallback : public MatchFinder::MatchCallback {
-    GadgetList &Output;
+  MapTy Map;
 
-  public:
-    GadgetFinderCallback(GadgetList &Output) : Output(Output) {}
+  Strategy() = default;
+  Strategy(const Strategy &) = delete; // Let's avoid copies.
+  Strategy(Strategy &&) = default;
+
+  void set(const VarDecl *VD, Kind K) {
+    Map[VD] = K;
+  }
+
+  Kind lookup(const VarDecl *VD) const {
+    auto I = Map.find(VD);
+    if (I == Map.end())
+      return Kind::Wontfix;
+
+    return I->second;
+  }
+};
+} // namespace
+
+/// Scan the function and return a list of gadgets found with provided kits.
+static std::pair<GadgetList, DeclUseTracker> findGadgets(const Decl *D) {
+
+  struct GadgetFinderCallback : MatchFinder::MatchCallback {
+    GadgetList Gadgets;
+    DeclUseTracker Tracker;
 
     void run(const MatchFinder::MatchResult &Result) override {
+      if (const auto *DRE = Result.Nodes.getNodeAs<DeclRefExpr>("any_dre")) {
+        Tracker.discoverUse(DRE);
+      }
+
+      if (const auto *DS = Result.Nodes.getNodeAs<DeclStmt>("any_ds")) {
+        Tracker.discoverDecl(DS);
+      }
+
       // Figure out which matcher we've found, and call the appropriate
       // subclass constructor.
       // FIXME: Can we do this more logarithmically?
 #define GADGET(x)                                                              \
       if (Result.Nodes.getNodeAs<Stmt>(#x)) {                                  \
-        Output.push_back(std::make_unique<x ## Gadget>(Result));               \
+        Gadgets.push_back(std::make_unique<x ## Gadget>(Result));              \
         return;                                                                \
       }
 #include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
 #undef GADGET
     }
   };
 
-  GadgetList G;
   MatchFinder M;
-  GadgetFinderCallback CB(G);
+  GadgetFinderCallback CB;
 
   // clang-format off
   M.addMatcher(
     stmt(forEachDescendant(
       stmt(anyOf(
         // Add Gadget::matcher() for every gadget in the registry.
 #define GADGET(x)                                                              \
         x ## Gadget::matcher().bind(#x),
 #include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
 #undef GADGET
-        // FIXME: Is there a better way to avoid hanging comma?
-        unless(stmt())
+        // In parallel, match all DeclRefExprs so that to find out
+        // whether there are any uncovered by gadgets.
+        declRefExpr(hasPointerType(), to(varDecl())).bind("any_dre"),
+        // Also match DeclStmts because we'll need them when fixing
+        // their underlying VarDecls that otherwise don't have
+        // any backreferences to DeclStmts.
+        declStmt().bind("any_ds")
       ))
       // FIXME: Idiomatically there should be a forCallable(equalsNode(D))
       // here, to make sure that the statement actually belongs to the
       // function and not to a nested function. However, forCallable uses
       // ParentMap which can't be used before the AST is fully constructed.
       // The original problem doesn't sound like it needs ParentMap though,
       // maybe there's a more direct solution?
     )),
     &CB
   );
   // clang-format on
 
   M.match(*D->getBody(), D->getASTContext());
 
-  return G; // NRVO!
+  // Gadgets "claim" variables they're responsible for. Once this loop finishes,
+  // the tracker will only track DREs that weren't claimed by any gadgets,
+  // i.e. not understood by the analysis.
+  for (const auto &G : CB.Gadgets) {
+    for (const auto *DRE : G->getClaimedVarUseSites()) {
+      CB.Tracker.claimUse(DRE);
+    }
+  }
+
+  return {std::move(CB.Gadgets), std::move(CB.Tracker)};
 }
 
 void clang::checkUnsafeBufferUsage(const Decl *D,
                                    UnsafeBufferUsageHandler &Handler) {
   assert(D && D->getBody());
 
-  GadgetList Gadgets = findGadgets(D);
+  SmallSet<const VarDecl *, 8> WarnedDecls;
+
+  auto [Gadgets, Tracker] = findGadgets(D);
+
+  DenseMap<const VarDecl *, std::vector<const Gadget *>> Map;
+
+  // First, let's sort gadgets by variables. If some gadgets cover more than one
+  // variable, they'll appear more than once in the map.
   for (const auto &G : Gadgets) {
+    DeclUseList DREs = G->getClaimedVarUseSites();
+
+    // Populate the map.
+    bool Pushed = false;
+    for (const DeclRefExpr *DRE : DREs) {
+      if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) {
+        Map[VD].push_back(G.get());
+        Pushed = true;
+      }
+    }
+
+    if (!Pushed && !G->isSafe()) {
+      // We won't return to this gadget later. Emit the warning right away.
+      Handler.handleUnsafeOperation(G->getBaseStmt());
+      continue;
+    }
+  }
+
+  Strategy S;
+
+  for (const auto &Item : Map) {
+    const VarDecl *VD = Item.first;
+    const std::vector<const Gadget *> &VDGadgets = Item.second;
+
+    // If the variable has no unsafe gadgets, skip it entirely.
+    if (!any_of(VDGadgets, [](const Gadget *G) { return !G->isSafe(); }))
+      continue;
+
+    Optional<FixItList> Fixes = None;
+
+    // Avoid suggesting fixes if not all uses of the variable are identified
+    // as known gadgets.
+    // FIXME: Support parameter variables as well.
+    if (!Tracker.hasUnclaimedUses(VD) && VD->isLocalVarDecl()) {
+      // Choose the appropriate strategy. FIXME: We should try different
+      // strategies.
+      S.set(VD, Strategy::Kind::Span);
+
+      // Check if it works.
+      // FIXME: This isn't sufficient (or even correct) when a gadget has
+      // already produced a fixit for a different variable i.e. it was mentioned
+      // in the map twice (or more). In such case the correct thing to do is
+      // to undo the previous fix first, and then if we can't produce the new
+      // fix for both variables, revert to the old one.
+      Fixes = FixItList{};
+      for (const Gadget *G : VDGadgets) {
+        Optional<FixItList> F = G->getFixits(S);
+        if (!F) {
+          Fixes = None;
+          break;
+        }
+
+        for (auto &&Fixit: *F)
+          Fixes->push_back(std::move(Fixit));
+      }
+    }
+
+    if (Fixes) {
+      // If we reach this point, the strategy is applicable.
+      Handler.handleFixableVariable(VD, std::move(*Fixes));
+    } else {
+      // The strategy has failed. Emit the warning without the fixit.
+      S.set(VD, Strategy::Kind::Wontfix);
+      for (const Gadget *G : VDGadgets) {
+        if (!G->isSafe()) {
           Handler.handleUnsafeOperation(G->getBaseStmt());
         }
       }
+    }
+  }
+}

clang/lib/Sema/AnalysisBasedWarnings.cpp

 
 class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
   Sema &S;
 
 public:
   UnsafeBufferUsageReporter(Sema &S) : S(S) {}
 
   void handleUnsafeOperation(const Stmt *Operation) override {
-    S.Diag(Operation->getBeginLoc(), diag::warn_unsafe_buffer_usage);
+    S.Diag(Operation->getBeginLoc(), diag::warn_unsafe_buffer_expression);
+  }
+
+  void handleFixableVariable(const VarDecl *Variable,
+                             FixItList &&Fixes) override {
+    const auto &D =
+        S.Diag(Variable->getBeginLoc(), diag::warn_unsafe_buffer_variable);
+    D << Variable;
+    for (const auto &F: Fixes) {
+      D << F;
+    }
   }
 };
 
 
 //===----------------------------------------------------------------------===//
 // AnalysisBasedWarnings - Worker object used by Sema to execute analysis-based
 //  warnings on a function, method, or block.
 //===----------------------------------------------------------------------===//
void clang::sema::AnalysisBasedWarnings::IssueWarnings(
 
   // Check for throw out of non-throwing function.
   if (!Diags.isIgnored(diag::warn_throw_in_noexcept_func, D->getBeginLoc()))
     if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D))
       if (S.getLangOpts().CPlusPlus && isNoexcept(FD))
         checkThrowInNonThrowingFunc(S, FD, AC);
 
   // Emit unsafe buffer usage warnings and fixits.
-  if (!Diags.isIgnored(diag::warn_unsafe_buffer_usage, D->getBeginLoc())) {
+  if (!Diags.isIgnored(diag::warn_unsafe_buffer_expression, D->getBeginLoc()) ||
+      !Diags.isIgnored(diag::warn_unsafe_buffer_variable, D->getBeginLoc())) {
     UnsafeBufferUsageReporter R(S);
     checkUnsafeBufferUsage(D, R);
   }
 
   // If none of the previous checks caused a CFG build, trigger one here
   // for the logical error handler.
   if (LogicalErrorHandler::hasActiveDiagnostics(Diags, D->getBeginLoc())) {
     AC.getCFG();