[-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[ziqingluo-90]

Summary

Use clang fix-its to transform declarations of local variables, which are used for buffer access , to be of std::span type.

We placed a few limitations to keep the solution simple:

1. it only transforms local variable declarations (no parameter declaration)
2. it only considers single level pointers, i.e., pointers of type T * regardless of whether T is again a pointer.
3. it only transforms to std::span types (no std::array, or std::span::iterator, or ...)
4. it can only transform a VarDecl that belongs to a DeclStmt whose has a single child.

One of the purposes of keeping this patch simple enough is to first evaluate if fix-it is an appropriate approach to do the transformation.

[NoQ]

Ok so at the conference @ymandel suggested us to use libTooling's Transformers API to make our lives easier (https://clang.llvm.org/docs/ClangTransformerTutorial.html). I'm fairly certain we should at least consider using them, but I haven't looked into it deeply enough yet. "Changing the type of a variable" is a problem that ideally should be solved only once, because it's a problem that's easy to formulate but difficult to "get right". Transformers appear to be a collection of solutions to problems of this kind. We should see if they already provide an out-of-the-box solution, or if they have bits and pieces of machinery we can reuse in our solution.

[ymandel]

In D139737#3989927, @NoQ wrote:

Ok so at the conference @ymandel suggested us to use libTooling's Transformers API to make our lives easier (https://clang.llvm.org/docs/ClangTransformerTutorial.html). I'm fairly certain we should at least consider using them, but I haven't looked into it deeply enough yet. "Changing the type of a variable" is a problem that ideally should be solved only once, because it's a problem that's easy to formulate but difficult to "get right". Transformers appear to be a collection of solutions to problems of this kind. We should see if they already provide an out-of-the-box solution, or if they have bits and pieces of machinery we can reuse in our solution.

1. Transformer is good when you are basing your changes around AST matchers. It is a collection of combinators over the MatchResult type. From what I'm seeing here, though, you don't seem to be basing it on AST matchers, in which case the libraries are not as useful.
2. We (google) have a tool for changing the type of a variable which takes into account all cascading changes to other declarations that result from that type change. It is built on Transformer and another library that builds a graph of relationships in the AST, relevant to the type-rewriting problem. Unfortunately, that's all internal -- not for IP reasons but simply because we never had reason to upstream it (nor an obvioius place to put it). So, I'd say that this advanced tooling is useful here if you want to _selectively_ change type T to type S, so you need tooling to tell you specifically which S's to update. If you're changing all T to S, then the problem is simpler.

[NoQ]

In D139737#3991699, @ymandel wrote:

2. We (google) have a tool for changing the type of a variable which takes into account all cascading changes to other declarations that result from that type change. It is built on Transformer and another library that builds a graph of relationships in the AST, relevant to the type-rewriting problem. Unfortunately, that's all internal -- not for IP reasons but simply because we never had reason to upstream it (nor an obvioius place to put it). So, I'd say that this advanced tooling is useful here if you want to _selectively_ change type T to type S, so you need tooling to tell you specifically which S's to update. If you're changing all T to S, then the problem is simpler.

Yeah we want to be selective, so in our case there's an extra analysis step between noticing that the variable is of a certain type and realizing that we want to change it to a different type.

I guess I'm curious if there are smaller building blocks of your machine that we could reuse. Like, for instance, the procedure of updating the type of the variable (without even updating all uses): it's a somewhat self-contained problem, and it's difficult enough on its own (you need to split up multi-variable DeclStmts in two or three, find the right source locations for everything, and so on). I wonder if we could avoid reinventing the wheel there somehow.

[ymandel]

In D139737#3993813, @NoQ wrote:

In D139737#3991699, @ymandel wrote:

2. We (google) have a tool for changing the type of a variable which takes into account all cascading changes to other declarations that result from that type change. It is built on Transformer and another library that builds a graph of relationships in the AST, relevant to the type-rewriting problem. Unfortunately, that's all internal -- not for IP reasons but simply because we never had reason to upstream it (nor an obvioius place to put it). So, I'd say that this advanced tooling is useful here if you want to _selectively_ change type T to type S, so you need tooling to tell you specifically which S's to update. If you're changing all T to S, then the problem is simpler.

Yeah we want to be selective, so in our case there's an extra analysis step between noticing that the variable is of a certain type and realizing that we want to change it to a different type.

I guess I'm curious if there are smaller building blocks of your machine that we could reuse. Like, for instance, the procedure of updating the type of the variable (without even updating all uses): it's a somewhat self-contained problem, and it's difficult enough on its own (you need to split up multi-variable DeclStmts in two or three, find the right source locations for everything, and so on). I wonder if we could avoid reinventing the wheel there somehow.

Sure. I'll try to get you some sample code later today.

[ziqingluo-90]

did a rebase

[jkorous]

Thanks for the rebase!

Nit: I'd just replace std::function with auto as it saves us repeating the parameter types (and #include <functional>).

clang/lib/Analysis/UnsafeBufferUsage.cpp

676

680

684

[ziqingluo-90]

To follow LLVM's convention that global variables better have types that do NOT require construction, I change the type of the global variable from std::string to constexpr const char * const.

[ziqingluo-90]

Rebase the patch with respect to the re-architecture: https://reviews.llvm.org/D140062.

Since we do not have any FixableGadget to trigger fix-its at this point, I let fix-its of local variable declarations always be emitted for the purpose of testing.

[jkorous]

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

56

Should we rather pick something that is syntactically incorrect in C++ in order to prevent accidental silent corruption of the sources?
FWIW Xcode uses <#placeholder#> syntax.

clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.cpp

1 ↗

(On Diff #487527)

I am starting to think we should split up our tests to allow for less conflicts between patches.
Could we please rename the file to a more specific name e. g. warn-unsafe-buffer-usage-fixits-local-var-span.cpp?

1 ↗

(On Diff #487527)

Also, let's check only correctness of the FixIts in this test.
I would remove RUN lines with -verify and expected-warnings.

[NoQ]

In D139737#4037455, @ziqingluo-90 wrote:

Since we do not have any FixableGadget to trigger fix-its at this point, I let fix-its of local variable declarations always be emitted for the purpose of testing.

It sounds to me as if by landing the patch we'll temporarily make the compiler emit incorrect fixes. I think we should avoid doing that. Is it possible to wait until our first proof-of-concept FixableGadget lands before landing this patch? I think there shouldn't be too much conflict between such patches.

[jkorous]

In D139737#4042093, @NoQ wrote:

It sounds to me as if by landing the patch we'll temporarily make the compiler emit incorrect fixes. I think we should avoid doing that. Is it possible to wait until our first proof-of-concept FixableGadget lands before landing this patch? I think there shouldn't be too much conflict between such patches.

Any patch with FixableGadget will face the same problem - i. e. we still won't emit the fixit until this patch has landed.
We decided to include a simple Fixable in this patch to unlock testing.

[jkorous]

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

56

Correction - it is not Xcode, it is clang itself.
https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/CodeCompleteConsumer.cpp#L323

[ziqingluo-90]

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

56

It looks like we could put different messages in between <# and #> to hint users in different situations. I'll make this member a function then.

[ziqingluo-90]

Rebased the code w.r.t. a series of refactoring in [-Wunsafe-buffer-usage].
Added a FixableGadget for array subscripts of the form DRE[*] in the context of lvalue-to-rvalue casting.

Also did a refactoring at the place where matchers are applied: Matchers of a FixableGadget and of a WarningGadget cat match for the same AST node. So they should not be put in an anyOf group, otherwise they can disable each other due to short-circuiting.

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

385

There's already a forward declaration on line 144!

412

Subscripts [0] might be safe, but this doesn't mean we should avoid emitting fixits when we see them. I think this clause should be dropped here. (Might be worth adding a test).

723

Uhh, I'm worried about this approach. I'm not sure these pretty-printers are even correct. They try to look similar to the underlying code, but I don't think they're required to. And even if they were, It only takes one forgotten override in StmtPrinter to produce incorrect pretty-prints that won't compile. So it's very unreliable technology.

I think the intended way to do these things is to simply avoid overwriting code that you want to preserve. Instead, modify code around it. Eg., if you want to change

int *x = foo();

to

std::span<int> x { foo(), N };

then you add std::span< before int, then don't touch int, then replace * with > , then replace = with {, then don't touch foo(), then add , N } immediately after.

If you actually need to move code around (eg., make preserved chunks appear in a different order), I think this should go through a facility like Lexer::getSourceText() which would give you the exact source code as written.

[ziqingluo-90]

Refactored the fix-it generation code to stop using the pretty-printer.

[ziqingluo-90]

Addressed the minor comments

[NoQ]

Awesome, we're getting closer to producing our first fix!

I think the most important thing right now is to add a lot of SourceLocation::isMacroID() checks everywhere, so that to *never* try to fix code that's fully or partially expanded from macros. I suspect that we can do that "after the fact": just scan the list of emitted fixits for any source ranges that start or end in macros, and if even one such range is found, abandon the fix. Might be worth double-checking that the compiler doesn't already do that automatically for all emitted fixits.

I see that the patch doesn't touch handleFixableVariable(). Do we still attach fixits to the warning, or did we already change it to be attached to a note associated with the warning? Because fixits with placeholders aren't allowed on warnings, we should make sure it's attached to note before landing this patch.

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

42

Do we really want this method in the public interface? If the idea is to let people the user override it, maybe let's actually allow that by dropping static and adding virtual? I think this is actually a good idea, maybe if somebody uses our analysis outside of the clang binary, they may want a different placeholder.

43

const StringRef & seems redundant given that StringRef is already a "Ref". Just pass by value.

clang/lib/Analysis/UnsafeBufferUsage.cpp

407

A bit more concise.

637

Hmm, did this need to be moved? I don't think you're calling this function from the new code.

729

(https://www.oxfordinternationalenglish.com/passed-vs-past-whats-the-difference/, looks like "Passed` is always a verb)

771

Excellent observation!

774

I wonder how do we end up in such situation. If it's not array new, this means it's also not a buffer of multiple elements. But I guess the pointer can be reassigned later. Yeah I think you're right, this is the correct solution. Maybe let's leave a comment explaining how this could happen?

776–777

That's another case where .getTypePtr() is unnecessary: dyn_cast<T>(QT.getTypePtr()) is equivalent to`QT->getAs<T>() where -> is QualType`'s overloaded operator and getAs is Type::getAs.

Additionally, array types are special - see doxygen comments for ASTContext::getAsArrayType(). For some reason you're suppposed to consult ASTContext when casting them, which I did in the suggested fix. I'm not sure if it matters in this case when we just want to extract the size.

783–784

int x = 1;
char *ptr = &x; // std::span<char> ptr { &x, 4 };

This is valid code. I suspect we want to check types as well, to see that type sizes match.

Most of the time code like this violates strict aliasing, but char is exceptional, and even if it did violate strict aliasing, people can compile with -fno-strict-aliasing to define away the UB, so we have to respect that.

821

getDesugaredType() looks redundant, getPointeeType() probably already does that.

852

Interesting, so you don't need to discriminate between situation "fix is impossible" and situation "fix is not necessary" because the latter is impossible because the variable type definitely needs fixing? Which is different from the behavior of FixableGadget::getFixits() that returns an empty optional when the fix is impossible and a non-empty optional with zero fixes when the fix is not necessary. I guess let's leave a comment at fixVariable() documenting this behavior?

I also suspect that eventually we'll want variable declarations to simply act as another fixable gadget. In this case we might have to undo this solution and go back to optionals.

859–861

I suspect this code will crash whenever such declarations are encountered, so this is probably not something we want to commit.

Typically assert(..., "not implemented yet") makes sense only when the crashing codepath is also unreachable because no other facility in the compiler exercises it.

It probably make sense to return {} in these cases: the function is still responsible for them, we just haven't implemented them yet.

Let's also add a FIXME test to demonstrate that there's no fix yet in such situations.

clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp

56

In the new int[n] case it also technically doesn't have a constant value. The code correctly checks for side effects, the comment should also probably say something about side effects.

[jkorous]

clang/lib/Analysis/UnsafeBufferUsage.cpp

126

Nit: I think we could simplify this to just

static auto isInUnspecifiedLvalueContext(internal::Matcher<Expr> innerMatcher) {
  return implicitCastExpr(hasCastKind(CastKind::CK_LValueToRValue),
                          castSubExpr(innerMatcher));
}

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

637

it does look like I moved it. Will change it back.

783–784

This code is not valid in C++. An explicit cast is needed in front of &x. I will add a test to show that

int x = 1;
char * ptr = (char *)&x;

will have a place holder for the span size.

[ziqingluo-90]

Addressing comments

[NoQ]

Thank you!! I think we're almost ready to commit but this concern is still hanging:

I see that the patch doesn't touch handleFixableVariable(). Do we still attach fixits to the warning, or did we already change it to be attached to a note associated with the warning? Because fixits with placeholders aren't allowed on warnings, we should make sure it's attached to note before landing this patch.

clang/lib/Analysis/UnsafeBufferUsage.cpp

783–784

Yes you're right! It's only valid in C where these fixits don't apply.

[jkorous]

I am sorry I haven't notice this earlier - let's fix this before we land the patch.

clang/lib/Analysis/UnsafeBufferUsage.cpp

695

We either need a zero to terminate the string or pass the size of Txt to the std::string constructor here. (While toString's name might sound like it'll take care of that it does not.)

Simplified testcase:

void local_ptr_to_array() {
  int tmp;
  int a[10];
  int *p = a;
  tmp = p[5];
}

what I get is (something like this):

void local_ptr_to_array() {
  int tmp;
  int a[10];
  std::span<int> p {a, 10�o
};
  tmp = p[5];
}

The problem is that APInt::toString stores '1' and '0' to Txt but is missing the terminating \0 character that std::string constructor expects.

695

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

699

[ziqingluo-90]

To attach fix-its to notes instead of warnings.
Fix the ending '\0' issue raised by @jkorous

[ziqingluo-90]

rebase

[jkorous]

I got a test failure in SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp which I believe is caused solely by the fact that we emit the diagnostics in different order.
I am not sure it matters and since the Fix-Its clearly specify what source lines they apply to I suggest we simply replace every // CHECK: with // CHECK-DAG:.
That fixed the problem for me.

https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-dag-directive

[jkorous]

clang/lib/Analysis/UnsafeBufferUsage.cpp

871

I believe we should add another condition here: VD->isLocalVarDecl() as we don't support globals (yet?).
We run the matcher with any_ds tag only on function bodies so we won't discover globals anyway and the assert(It != Defs.end() && "Definition never discovered!"); would fail.

[jkorous]

clang/lib/Analysis/UnsafeBufferUsage.cpp

680

I am afraid I might have found one more problem :(
I believe that for span strategy we have to make sure the index is > 0. Otherwise
That means either an unsigned integer or signed or unsigned literal that is greater than 0.
For the literal you can take inspiration here:
https://reviews.llvm.org/D142795

[ziqingluo-90]

address comments

[NoQ]

As far as I'm concerned, I think this is great for the initial implementation! Let's commit as soon as Jan confirms that his problem is addressed.

clang/lib/Analysis/UnsafeBufferUsage.cpp

871

I think this check should happen much earlier. Like, we shouldn't define a strategy for globals, and we shouldn't build fixables out of them. And then assert() here, just to double-check.

[jkorous]

clang/lib/Analysis/UnsafeBufferUsage.cpp

680

@ziqingluo-90 Sorry, looks like I wasn't clear here.
One case (that you've already addressed) is ptr[-5] - for that we can't use std::span::operator[] as it would immediately trap.
But there's the other case of:

uint8_t foo(uint8_t *ptr, int idx) {
  return ptr[idx]
}

If anyone uses a value that's both signed and not a compile-time constant then our compile-time analysis can not prove that the index is always >= 0 and consequently we can't use std::span::operator[] as a replacement.
That's why I think we really need to make sure that the index is ether a) positive literal or b) unsigned.
WDYT?

[jkorous]

clang/lib/Analysis/UnsafeBufferUsage.cpp

680

And yes ... I was wrong - literal 0 is totally fine. Thanks for spotting that!

871

Fair enough, we can do that. Maybe a follow-up patch?

[ziqingluo-90]

Fixed a bug in manipulating source locations:

Using SourceLocation::getLocWithOffset to get a new source location L with a relative offset is convenient. But it requires extra care to make sure that L has the same file ID as the location where L is originated. Otherwise, L may be associated with a wrong file ID (or macro ID). Feeding such an L to a Lexer could cause failures.

[ziqingluo-90]

To avoid emitting incorrect fix-its for array subscripts on span objects:

• For unsafe operations of the form s[e] where s is being transformed to be a span, we only emit fix-its for s[e] when e is a non-negative integer literal or e has an unsigned integer type.

clang/lib/Analysis/UnsafeBufferUsage.cpp

680

I think you are right. Fixed it.

[jkorous]

LGTM
Thank you!

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

public:
 
   /// Invoked when an unsafe operation over raw pointers is found.
   virtual void handleUnsafeOperation(const Stmt *Operation,
                                      bool IsRelatedToDecl) = 0;
 
   /// Invoked when a fix is suggested against a variable.
   virtual void handleFixableVariable(const VarDecl *Variable,
                                      FixItList &&List) = 0;
+
+  /// Returns the text indicating that the user needs to provide input there:
+  static std::string
+  getUserFillPlaceHolder(const StringRef &HintTextToUser = "placeholder") {
+    std::string s = std::string("<# ");
+    s += HintTextToUser;
+    s += " #>";
+    return s;
+  }
 };
 
 // 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/Analysis/Analyses/UnsafeBufferUsageGadgets.def

 #ifndef FIXABLE_GADGET
 #define FIXABLE_GADGET(name) GADGET(name)
 #endif
 
 WARNING_GADGET(Increment)
 WARNING_GADGET(Decrement)
 WARNING_GADGET(ArraySubscript)
 WARNING_GADGET(PointerArithmetic)
+FIXABLE_GADGET(ULCArraySubscript)
 
 #undef FIXABLE_GADGET
 #undef WARNING_GADGET
 #undef GADGET

clang/lib/Analysis/UnsafeBufferUsage.cpp

 };
 
 AST_MATCHER_P(Stmt, forEveryDescendant, internal::Matcher<Stmt>, innerMatcher) {
   const DynTypedMatcher &DTM = static_cast<DynTypedMatcher>(innerMatcher);
   
   MatchDescendantVisitor Visitor(&DTM, Finder, Builder, ASTMatchFinder::BK_All);  
   return Visitor.findMatch(DynTypedNode::create(Node));
 }
+
+AST_MATCHER_P(CastExpr, castSubExpr, internal::Matcher<Expr>, innerMatcher) {
+  return innerMatcher.matches(*Node.getSubExpr(), Finder, Builder);
+}
+
+// Returns a matcher that matches any expression 'e' such that `innerMatcher`
+// matches 'e' and 'e' is in an Unspecified Lvalue Context.
+static auto isInUnspecifiedLvalueContext(internal::Matcher<Expr> innerMatcher) {
+  auto isLvalueToRvalueCast = [](internal::Matcher<Expr> M) {
+    return implicitCastExpr(hasCastKind(CastKind::CK_LValueToRValue),
+                            castSubExpr(M));
+  };
+  //FIXME: add assignmentTo context...
+  return isLvalueToRvalueCast(innerMatcher);
+}
 } // namespace clang::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.
DeclUseList getClaimedVarUseSites() const override {
 
     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 WarningGadget {
-  static constexpr const char *const ArraySubscrTag = "arraySubscr";
+  static constexpr const char *const ArraySubscrTag = "ArraySubscript";
   const ArraySubscriptExpr *ASE;
 
 public:
   ArraySubscriptGadget(const MatchFinder::MatchResult &Result)
       : WarningGadget(Kind::ArraySubscript),
         ASE(Result.Nodes.getNodeAs<ArraySubscriptExpr>(ArraySubscrTag)) {}
 
   static bool classof(const Gadget *G) {
if (const auto *DRE =
       return {DRE};
     }
 
     return {};
   }
   // FIXME: pointer adding zero should be fine
   //FIXME: this gadge will need a fix-it
 };
+
+class Strategy;
+
+// Represents expressions of the form `DRE[*]` in the Unspecified Lvalue
+// Context (see `isInUnspecifiedLvalueContext`).
+// Note here `[]` is the built-in subscript operator.
+class ULCArraySubscriptGadget : public FixableGadget {
+private:
+  static constexpr const char *const ULCArraySubscriptTag = "ULCArraySubscript";
+  const ArraySubscriptExpr *Node;
+
+public:
+  ULCArraySubscriptGadget(const MatchFinder::MatchResult &Result)
+      : FixableGadget(Kind::ULCArraySubscript),
+        Node(Result.Nodes.getNodeAs<ArraySubscriptExpr>(ULCArraySubscriptTag)) {
+    assert(Node != nullptr && "Expecting a non-null matching result");
+  }
+
+  static bool classof(const Gadget *G) {
+    return G->getKind() == Kind::ULCArraySubscript;
+  }
+
+  static Matcher matcher() {
+    auto ArrayOrPtr = anyOf(hasPointerType(), hasArrayType());
+    auto BaseIsArrayOrPtrDRE =
+        hasBase(ignoringParenImpCasts(allOf(declRefExpr(), ArrayOrPtr)));
+    auto Target =
+        arraySubscriptExpr(BaseIsArrayOrPtrDRE,
+                           unless(hasIndex(integerLiteral(equals(0)))))
+            .bind(ULCArraySubscriptTag);
+
+    return expr(isInUnspecifiedLvalueContext(Target));
+  }
+
+  virtual Optional<FixItList> getFixits(const Strategy &S) const override;
+
+  virtual const Stmt *getBaseStmt() const override { return Node; }
+
+  virtual DeclUseList getClaimedVarUseSites() const override {
+    if (const auto *DRE = dyn_cast<DeclRefExpr>(Node->getBase()->IgnoreImpCasts())) {
+      return {DRE};
+    }
+    return {};
+  }
+};
 } // namespace
 
 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>;
#include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
   };
 
   MatchFinder M;
   GadgetFinderCallback CB;
 
   // clang-format off
   M.addMatcher(
     stmt(forEveryDescendant(
+      eachOf(
+      // A `FixableGadget` matcher and a `WarningGadget` matcher should not disable
+      // each other (they could if they were put in the same `anyOf` group).
+      // We also should make sure no two `FixableGadget` (resp. `WarningGadget`) matchers
+      // match for the same node, so that we can group them
+      // in one `anyOf` group (for better performance via short-circuiting).
       stmt(anyOf(
-        // Add Gadget::matcher() for every gadget in the registry.
-#define GADGET(x)                                                              \
-        x ## Gadget::matcher().bind(#x),
+#define FIXABLE_GADGET(x)                                                              \
+        x ## Gadget::matcher(),
 #include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
-        // In parallel, match all DeclRefExprs so that to find out
-        // whether there are any uncovered by gadgets.
-        declRefExpr(anyOf(hasPointerType(), hasArrayType()),
-                    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?
+      )),
+      stmt(anyOf(
+        // Add Gadget::matcher() for every gadget in the registry.
+#define WARNING_GADGET(x)                                                              \
+        x ## Gadget::matcher().bind(#x),
+#include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
+        // In parallel, match all DeclRefExprs so that to find out
+        // whether there are any uncovered by gadgets.
+        declRefExpr(anyOf(hasPointerType(), hasArrayType()), to(varDecl())).bind("any_dre")
+      )))
     )),
     &CB
   );
   // clang-format on
 
   M.match(*D->getBody(), D->getASTContext());
 
   // Gadgets "claim" variables they're responsible for. Once this loop finishes,
for (const DeclRefExpr *DRE : DREs) {
       if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) {
         FixablesForUnsafeVars.byVar[VD].emplace(std::move(F));
       }
     }
   }
   return FixablesForUnsafeVars;
 }
 
+static Strategy
+getNaiveStrategy(const llvm::SmallVectorImpl<const VarDecl *> &UnsafeVars) {
+  Strategy S;
+  for (const VarDecl *VD : UnsafeVars) {
+    S.set(VD, Strategy::Kind::Span);
+  }
+  return S;
+}
+
+Optional<FixItList>
+ULCArraySubscriptGadget::getFixits(const Strategy &S) const {
+  if (const auto *DRE = dyn_cast<DeclRefExpr>(Node->getBase()->IgnoreImpCasts()))
+    if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) {
+      switch (S.lookup(VD)) {
+      case Strategy::Kind::Span:
+        return FixItList{};
+      case Strategy::Kind::Wontfix:
+      case Strategy::Kind::Iterator:
+      case Strategy::Kind::Array:
+      case Strategy::Kind::Vector:
+        llvm_unreachable("unsupported strategies for FixableGadgets");
+      }
+    }
+  return std::nullopt;
+}
+
+// For a non-null initializer `Init` of `T *` type, this function writes
+// `{Init,  S}` as a part of a fix-it to output stream . The list-initializer
+// `{Init, S}` belongs to a span constructor, where `Init` specifies the
+// address of the data and `S` is the extent.  In many cases, this function
+// cannot figure out the actual extent `S`.  It then will use a place holder to
+// replace `S` to ask users to fill `S` in.  The span object being constructed
+// will have type `span<T, S>`.
+//
+// FIXME: This function so far only works for spanify "single-level" pointers.
+// When it comes to multi-level pointers, the data address is not necessarily
+// identical to `Init` and the element type of the constructing span object is
+// not necessarily `T`.
+//
+// Parameters:
+//   `Init` a pointer to the initializer expression
+//   `Ctx` a reference to the ASTContext
+//   `OS` the output stream where fix-it texts being written to
+static void populateInitializerFixItWithSpan(const Expr *Init,
+                                     const ASTContext &Ctx, raw_ostream &OS) {
+  const PrintingPolicy &PP = Ctx.getPrintingPolicy();
+
+  // Prints the begin address of the span constructor:
+  OS << "{";
+  Init->printPretty(OS, nullptr, PP);
+  OS << ", ";
+
+  bool ExtKnown = false; // true iff the extent can be obtained
+  // Printers that print extent into OS and sets ExtKnown to true:
+  std::function PrintExpr = [&ExtKnown, &OS, &PP](const Expr *Size) {
+    Size->printPretty(OS, nullptr, PP);
+    ExtKnown = true;
+  };
+  std::function PrintAPInt = [&ExtKnown, &OS](const APInt &Size) {
+    Size.print(OS, false);
+    ExtKnown = true;
+  };
+  std::function PrintOne = [&ExtKnown, &OS](void) {
+    OS << "1";
+    ExtKnown = true;
+  };
+
+  // Try to get the data extent. Break into different cases:
+  if (auto CxxNew = dyn_cast<CXXNewExpr>(Init->IgnoreImpCasts())) {
+    // In cases `Init` is `new T[n]` and there is no explicit cast over
+    // `Init`, we know that `Init` must evaluates to a pointer to `n` objects
+    // of `T`. So the extent is `n` unless `n` has side effects.  Similar but
+    // simpler for `Init` is `new T`.
+    if (const Expr *Ext = CxxNew->getArraySize().value_or(nullptr)) {
+      if (!Ext->HasSideEffects(Ctx))
+        PrintExpr(Ext);
+    } else if (!CxxNew->isArray())
+      PrintOne();
+  } else if (auto CArrTy = dyn_cast<ConstantArrayType>(
+                 Init->IgnoreImpCasts()->getType().getTypePtr())) {
+    // In cases `Init` is of an array type after stripping off implicit casts,
+    // `PointeeTy` must besimilar to the element type of `CArrTy`, so the
+    // extent is the array size of `CArrTy`.  Note that if the array size is
+    // not a constant, we cannot use it as the extent.
+    PrintAPInt(CArrTy->getSize());
+    // FIXME: local static array may be converted to std:array.  Is there any
+    // dependency among these fix-its?
+  } else {
+    // In cases `Init` is of the form `&Var` after stripping of implicit
+    // casts, where `&` is the built-in operator, `PointeeTy` must be similar
+    // to the type of `Var`, so the extent is 1.
+    if (auto AddrOfExpr = dyn_cast<UnaryOperator>(Init->IgnoreImpCasts()))
+      if (AddrOfExpr->getOpcode() == UnaryOperatorKind::UO_AddrOf &&
+          isa_and_present<DeclRefExpr>(AddrOfExpr->getSubExpr()))
+        PrintOne();
+    // TODO: we can handle more cases, e.g., `&a[0]`, `&a`, std::addressof,...
+    // etc.
+  }
+  if (!ExtKnown)
+    OS << UnsafeBufferUsageHandler::getUserFillPlaceHolder();
+  OS << "}";
+}
+
+// For a `VarDecl` of the form `T  * var (= Init)?`, this
+// function generates a fix-it for the declaration, which re-declares `var` to
+// be of `span<T>` type and transforms the initializer, if present, to a span
+// constructor---`span<T> var {Init, Extent}`, where `Extent` may need the user
+// to fill in.
+//
+// FIXME:
+// For now, we only consider single level pointer types, i.e., given `T` is
+// `int **`, the converted type will be `span<int*>`.
+// We need to handle multi-level pointers correctly later.
+//
+// Parameters:
+//   `D` a pointer the variable declaration node
+//   `Ctx` a reference to the ASTContext
+// Returns:
+//    the generated fix-it
+static FixItHint fixVarDeclWithSpan(const VarDecl *D,
+                                          const ASTContext &Ctx) {
+  const QualType &T = D->getType().getDesugaredType(Ctx);
+  const QualType &SpanEltT = T->getPointeeType();
+  assert(!SpanEltT.isNull() && "Trying to fix a non-pointer type variable!");
+
+  SmallString<32> Replacement;
+  raw_svector_ostream OS(Replacement);
+
+  // Simply make `D` to be of span type:
+  OS << "std::span<" << SpanEltT.getAsString() << "> " << D->getName();
+  if (const Expr *Init = D->getInit())
+    populateInitializerFixItWithSpan(Init, Ctx, OS);
+
+  return FixItHint::CreateReplacement(
+      SourceRange{D->getInnerLocStart(), D->getEndLoc()}, OS.str());
+}
+
+static FixItList fixVariableWithSpan(const VarDecl *VD,
+                                     const DeclUseTracker &Tracker,
+                                     const ASTContext &Ctx) {
+  const DeclStmt *DS = Tracker.lookupDecl(VD);
+  assert(DS && "Fixing non-local variables not implemented yet!");
+  assert(DS->getSingleDecl() == VD &&
+         "Fixing non-single declarations not implemented yet!");
+  // Currently DS is an unused variable but we'll need it when
+  // non-single decls are implemented, where the pointee type name
+  // and the '*' are spread around the place.
+  (void)DS;
+
+  // FIXME: handle cases where DS has multiple declarations
+  FixItHint Fix = fixVarDeclWithSpan(VD, Ctx);
+  return {Fix};
+}
+
+static FixItList fixVariable(const VarDecl *VD, Strategy::Kind K,
+                             const DeclUseTracker &Tracker,
+                             const ASTContext &Ctx) {
+  switch (K) {
+  case Strategy::Kind::Span: {
+    if (VD->getType()->isPointerType())
+      return fixVariableWithSpan(VD, Tracker, Ctx);
+    return {};
+  }
+  case Strategy::Kind::Iterator:
+  case Strategy::Kind::Array:
+  case Strategy::Kind::Vector:
+    llvm_unreachable("Strategy not implemented yet!");
+  case Strategy::Kind::Wontfix:
+    llvm_unreachable("Invalid strategy!");
+  }
+}
+
 static std::map<const VarDecl *, FixItList>
-getFixIts(FixableGadgetSets &FixablesForUnsafeVars, const Strategy &S) {
+getFixIts(FixableGadgetSets &FixablesForUnsafeVars, const Strategy &S, const DeclUseTracker &Tracker, const ASTContext &Ctx) {
   std::map<const VarDecl *, FixItList> FixItsForVariable;
   for (const auto &[VD, Fixables] : FixablesForUnsafeVars.byVar) {
-    // TODO fixVariable - fixit for the variable itself
+    FixItsForVariable[VD] = fixVariable(VD, S.lookup(VD), Tracker, Ctx);
+    // If we fail to produce Fix-It for the declaration we have to skip the variable entirely.
+    if (FixItsForVariable[VD].empty()) {
+      FixItsForVariable.erase(VD);
+      continue;
+    }
+
     bool ImpossibleToFix = false;
     llvm::SmallVector<FixItHint, 16> FixItsForVD;
     for (const auto &F : Fixables) {
       llvm::Optional<FixItList> Fixits = F->getFixits(S);
       if (!Fixits) {
         ImpossibleToFix = true;
         break;
       } else {
         const FixItList CorrectFixes = Fixits.value();
         FixItsForVD.insert(FixItsForVD.end(), CorrectFixes.begin(),
                            CorrectFixes.end());
       }
     }
     if (ImpossibleToFix)
       FixItsForVariable.erase(VD);
     else
       FixItsForVariable[VD].insert(FixItsForVariable[VD].end(),
                                    FixItsForVD.begin(), FixItsForVD.end());
   }
   return FixItsForVariable;
 }
 
-static Strategy
-getNaiveStrategy(const llvm::SmallVectorImpl<const VarDecl *> &UnsafeVars) {
-  Strategy S;
-  for (const VarDecl *VD : UnsafeVars) {
-    S.set(VD, Strategy::Kind::Span);
-  }
-  return S;
-}
-
 void clang::checkUnsafeBufferUsage(const Decl *D,
                                    UnsafeBufferUsageHandler &Handler) {
   assert(D && D->getBody());
 
   WarningGadgetSets UnsafeOps;
   FixableGadgetSets FixablesForUnsafeVars;
   DeclUseTracker Tracker;
 
void clang::checkUnsafeBufferUsage(const Decl *D,
   }
 
   llvm::SmallVector<const VarDecl *, 16> UnsafeVars;
   for (const auto &[VD, ignore] : FixablesForUnsafeVars.byVar)
     UnsafeVars.push_back(VD);
 
   Strategy NaiveStrategy = getNaiveStrategy(UnsafeVars);
   std::map<const VarDecl *, FixItList> FixItsForVariable =
-      getFixIts(FixablesForUnsafeVars, NaiveStrategy);
+      getFixIts(FixablesForUnsafeVars, NaiveStrategy, Tracker, D->getASTContext());
 
   // FIXME Detect overlapping FixIts.
 
   for (const auto &G : UnsafeOps.noVar) {
     Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/false);
   }
 
   for (const auto &[VD, WarningGadgets] : UnsafeOps.byVar) {

clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp

+// RUN: cp %s %t.cpp
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fixit %t.cpp
+// RUN: grep -v CHECK %t.cpp | FileCheck %s
+typedef int * Int_ptr_t;
+typedef int Int_t;
+
+void local_array_subscript_simple() {
+  int tmp;
+// CHECK: std::span<int> p{new int [10], 10};
+// CHECK: std::span<const int> q{new int [10], 10};
+// CHECK: tmp = p[5];
+// CHECK: tmp = q[5];
+  int *p = new int[10];
+  const int *q = new int[10];
+  tmp = p[5];
+  tmp = q[5];
+
+// CHECK: std::span<int> x{new int [10], 10};
+// CHECK: std::span<int> y{new int, 1};
+// CHECK: std::span<Int_t> z{new int [10], 10};
+// CHECK: std::span<Int_t> w{new Int_t [10], 10};
+
+  Int_ptr_t x = new int[10];
+  Int_ptr_t y = new int;
+  Int_t * z = new int[10];
+  Int_t * w = new Int_t[10];
+
+  // CHECK: tmp = x[5];
+  tmp = x[5];
+  // CHECK: tmp = y[5];
+  tmp = y[5]; // y[5] will crash after being span
+  // CHECK: tmp = z[5];
+  tmp = z[5];
+  // CHECK: tmp = w[5];
+  tmp = w[5];
+}
+
+void local_array_subscript_auto() {
+  int tmp;
+// CHECK: std::span<int> p{new int [10], 10};
+// CHECK: tmp = p[5];
+  auto p = new int[10];
+  tmp = p[5];
+}
+
+void local_array_subscript_variable_extent() {
+  int n = 10;
+  int tmp;
+  //FIXME: need to think it twice about side effects in the expressions
+  //used to initialize span objects
+  // CHECK: std::span<int> p{new int [n], n};
+  // CHECK: std::span<int> q{new int [n++], <# placeholder #>};
+  // CHECK: tmp = p[5];
+  // CHECK: tmp = q[5];
+  int *p = new int[n];
+  // If the extent expression does not have a constant value, we cannot fill the extent for users...
+  int *q = new int[n++];
+  tmp = p[5];
+  tmp = q[5];
+}
+
+
+void local_ptr_to_array() {
+  int tmp;
+  int n = 10;
+  int a[10];
+  int b[n];  // If the extent expression does not have a constant value, we cannot fill the extent for users...
+  // CHECK: std::span<int> p{a, 10};
+  // CHECK: std::span<int> q{b, <# placeholder #>};
+  // CHECK: tmp = p[5];
+  // CHECK: tmp = q[5];
+  int *p = a;
+  int *q = b;
+  tmp = p[5];
+  tmp = q[5];
+}
+
+void local_ptr_addrof_init() {
+  int var;
+// CHECK: std::span<int> q{&var, 1};
+// CHECK: var = q[5];
+  int * q = &var;
+  // This expression involves unsafe buffer accesses, which will crash
+  // at runtime after applying the fix-it,
+  var = q[5];
+}