[-Wunsafe-buffer-usage] Add T* -> span<T> Fix-Its for function parameters

[ziqingluo-90]

Summary

Generate fix-its for function parameters. Currently, the analyzer fixes one parameter at a time.
Fix-its for a function parameter includes:

1. Fix the parameter declaration of the definition, result in a new overload of the function. We call the function with the original signature the old overload.
2. For any other existing declaration of the old overload, mark it with the [[unsafe_buffer_usage]] attribute and generate a new overload declaration next to it.
3. Creates a new definition for the old overload, which is simply defined by a call to the new overload.

Example,

void foo(int *p);

void foo(int *p) {
   int x = p[5];
}

will be like the following after applying fix-its

[[unsafe_buffer_usage]] void foo(int * p);
void foo(std::span<int> p);

void foo(std::span<int> p) {
...
}
[[unsafe_buffer_usage]] void foo(int * p) {
  return foo(std::span<int>{p, <# size #>});
}

[jkorous]

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

10

Oh, yes, this is fun. I hope I will eventually convince FileCheck this C++ attributes are not substitution blocks 😅

https://llvm.org/docs/CommandGuide/FileCheck.html#filecheck-string-substitution-blocks

[jkorous]

• rebased
• moved compatibility decl/def below original

still see crash in tests

[jkorous]

• fixed all crashes in our tests
• added cases of functions where we should not emit Fix-Its for parameters

[jkorous]

Started working on conflict detection for added compatibility overload.

[jkorous]

Note: One thing that I have little confidence the current patch handles correctly would be qualified function names. Definitely need tests.

[jkorous]

clang/lib/Analysis/UnsafeBufferUsage.cpp

918

We should not use the attribute directly - instead we should have a macro that gates it with __has_cpp_attribute.
https://clang.llvm.org/docs/LanguageExtensions.html#has-cpp-attribute

[jkorous]

clang/lib/Analysis/UnsafeBufferUsage.cpp

918

Also, we need to think about how do we vend the macro.

[jkorous]

clang/lib/Analysis/UnsafeBufferUsage.cpp

899

Sorry, the example below is unrelated. What I meant is that we shouldn't for example use \n on Windows.
EDIT: We also realized during an offline discussion that we should have our tests reflect that (somehow).

[jkorous]

As we discussed offline - we should add tests that our overload conflict detection would notice overloads that are declared below the function that's being analyzed.
Since our analysis callback is invoked after a body function has been parsed (but before the whole TU is parsed) it is not immediately clear if all function declarations in the TU will already be represented in the AST at that point.

[ziqingluo-90]

Taking care of this patch on behalf of @jkorous

[ziqingluo-90]

Still work in progress (but comments are very welcomed) ---there are several problems need to be solved:

• To conservatively detect any overload of the function whose parameter is being fixed. Thus far, the analysis cannot see any overload declared after the analyzing function.
• Fix-its conflict. Insertions of #include<span> and [[clang::unsafe_buffer_usage]] can conflict if both are inserted at the beginning of a file.
• There are many unsupported cases.
• Do we really want to fix parameters one at a time?
• As the fix-its become more complicated, tests now are harder to read.

[jkorous]

I just discovered my original prototype had yet another bug - parameters that use "array syntax" would get spanified but their name would be missing.

E. g.
void foo(int param[]) {
would get transformed to (notice the missing param name:
[clang::unsafe_buffer_usage] void foo(std::span<int>) {

I am wondering what is the source range of the parameter type in this case - the original code likely expects parameter type to be left of parameter name which might or might not be true in case.

It is possible that @ziqingluo-90 has fixed the bug in the meantime but I don't see a test - can we add one just to be sure?

It is likely that Transformer might solve this problem out of the box - maybe we should use it for this patch after all.

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

63 ↗

(On Diff #502893)

Since you mention main in the tests...
We can't change its signature. Not emitting Fix-Its (and not emitting the note to change param type) might be sufficient for now.
Long-term we might want a special Fix-It for argv - maybe construct std::span argv{argv_raw, argc}; local variable but that should be done in another patch.
Can you please add a test that we don't emit Fix-It for argv?

[jkorous]

I found one more bug.

The prototype mishandles const qualifier of the parameter type which means it transforms const int* to const span<const int> which is incorrect. That would correspond to const int* const.

Can we add a test for this case?

[ziqingluo-90]

Fix issues in transforming parameter declarations of the form const T * var and T * var[].
Add test for overload declarations appearing after the analyzing function with unsafe parameters.
Add more tests for unsupported cases so that we don't forget.

[jkorous]

I just realized one more problem we have to address with this patch - default arguments.
ParmVarDecl::getDefaultArg should be able to tell us if in any of the declarations of the function there is a default argument for the parameter we want to transform. Then we need to figure out if we can fix it/how do we fix it and we shouldn't emit the fixit if we can't fix the default argument. This problem is pretty similar to local variable initialization and we should try to reuse the solution.

clang/lib/Analysis/UnsafeBufferUsage.cpp

875

This TODO will hopefully no longer be true after we land.
https://reviews.llvm.org/D146342

883

IMO we should wrap this in some macro to make it user-friendlier.

Something like:

#ifndef UNSAFE_BUFFER_USAGE
#if __has_cpp_attribute(clang::unsafe_buffer_usage)
#define UNSAFE_BUFFER_USAGE [[clang::unsafe_buffer_usage]]
#else
#define UNSAFE_BUFFER_USAGE 
#endif
#endif

so the code (from our fix-its or written by the users) can look like this:

UNSAFE_BUFFER_USAGE void unsafe_foo(int* ptr, unsigned size);

...and we need to figure where exactly should the macro live (or if there should be multiple definitions, etc.).

I would love to hear @NoQ's thoughts on this topic.

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

883

That's, uh, interesting. We want users to use a macro of this kind and we need the macro to be defined, just like we want the users to use span and we want <span> to be included. But unlike #include <span>, we *really* don't want users to define the macro manually every time they need it. Instead we want it defined manually in some centralized place, and have the fixit machine pick it up from there.

So I think the fixit machine should never suggest *definining* the macro. That's the opposite of what we want.

On the other hand, there's a moderately famous precedent of compiler picking up an existing macro, namely the attribute [[fallthrough]]:

• example: https://godbolt.org/z/vvcW76cPE
• implementation: https://github.com/llvm/llvm-project/blob/llvmorg-16.0.0/clang/lib/Sema/AnalysisBasedWarnings.cpp#L1224)

So I think we can try to build something similar.

[NoQ]

In D143048#4256855, @jkorous wrote:

I just realized one more problem we have to address with this patch - default arguments.
ParmVarDecl::getDefaultArg should be able to tell us if in any of the declarations of the function there is a default argument for the parameter we want to transform. Then we need to figure out if we can fix it/how do we fix it and we shouldn't emit the fixit if we can't fix the default argument. This problem is pretty similar to local variable initialization and we should try to reuse the solution.

Oh right.

The problem with default arguments is that their AST lives outside of function body. It's the job of the *caller* to invoke this code. So that's a separate isolated piece of code that our matchers need to traverse together with the function's body.

I recommend giving up for the purposes of this patch (just make sure we don't try to fix such code) and see if we can address this later.

Which reminds me, we also need to handle CXXCtorInitializers correctly (or at least give up when we see them) - they're yet another piece of code that lives outside of function body, and they may access function parameters.

[ziqingluo-90]

To conservatively give up on fixing function parameters if they have default arguments.

The following improvement will be added by follow-up patches:

• Improve the insertion of [[unsafe_buffer_usage]]
• Fix function parameters with default arguments
• Group fix-its of multiple parameters

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

873–874

I suspect that it's always going to be "the whole TU", given that Sema is currently at the end of TU parsing (because that's where we put our analysis).

I recommend adding some tests with namespaces and other situations when functions are nested into something. Esp. when the out-of-line definition may be lexically available outside of the scope, eg.

namespace N {
  void foo();
}

void N::foo() {
  // implementation
}

where the compatibility overload has to go into the right namespace and the implementation of the compatibility overload has to be correctly prefixed with N::.

883

I think we have to go with raw [[clang::unsafe_buffer_usage]] for now. We can implement macro detection later, but we shouldn't insert preprocessor directives ourselves, because that's absolutely not how we want the final code to look.

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

26 ↗

(On Diff #515500)

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

11

Is there a semicolon at the end?

Also, do we want to preserve the parameter name p here, given that it's available on the respective redeclaration?

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

20 ↗

(On Diff #515500)

Why, what's wrong with it?

[ziqingluo-90]

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

11

No. There is no semicolon. The idea is that the fix-it tries to append a new function decl A; to a function decl B; but there is no obvious way to get the end location of B;. We only have the end location of B. So the fix-it inserts ;A to the end of B.

11

I did think about whether preserving the parameter name is necessary and didn't find a strong reason to do so.
A minor reason could be that if contracts are introduced in C++ one day, parameters in declarations need to have names so that contracts can refer to them.

I will add names to fix-its. Also I will be happy to hear any other benefits of doing that.

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

20 ↗

(On Diff #515500)

For const int * const x (or int * const x), we transform it to const std::span<const int> x (or const std::span<int> x). The left-most const are missing in the fix-its.

[ziqingluo-90]

Fix issues in overload lookup. Thanks to @NoQ 's comment.
Fix issues in handling qualified function or parameter names.
Add parameter names back for inserted overload declarations if the names exist in the original function declaration.

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

873–874

This is very valuable comment that points out a big hole in my patch. Now it should be fixed. Thank you.

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

878–880

Wait, did we decide to emit the #if check at every function after all?

I'd much rather temporarily emit the raw attribute without the check, than temporarily emit an #if, because the raw attribute is desired at least in some cases, but #if next to the function is never desired.

891

Need to test this with various anonymous functions:

• constructors/destructors of anonymous classes? https://godbolt.org/z/876d9MY1v
• overloaded operators, most importantly lambda call operators?

959–960

What happens if these are in different FileIDs? Do we end up with a pair of pointers into unrelated buffers? (Eg., macro expansions, or #include in the middle of the function declaration.)

1062

Does this work correctly if the pointee type is an anonymous struct? It probably does (given that anonymous structs can't be used directly, and typedefs aren't skipped here).

I suspect it's more reliable to copy-paste the original spelling of the type from the source code than to rely on pretty-printing, but it could have other problems (eg., there's a typedef that covers the *).

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

11

Parameter name could be a valuable hint to the user about the purpose of the parameter. The user doesn't necessarily see the implementation, and it's not like we're copying over the comments before the function. So out of the few ways the caller can figure out what to put there, this could be the most important one.

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

20 ↗

(On Diff #515500)

Oh, right, yes, makes sense in both cases!

This doesn't really lead to broken code and it doesn't really sacrifice much const-correctness because const in this position applies only to the parameter's stack storage, which people don't usually overwrite anyway, given that it reliably disappears at end of function. In particular, it's completely immaterial to the caller.

(It could matter when the class does something non-trivial, like have weird global side effects in overloaded assignment operator, so if it's const it could act as a guarantee that these side effects don't happen no matter what the function does; but nobody writes code like this, and std::span specifically is definitely not written like this.)

But I totally agree that in the spirit of preserving maximum similarity to the original code, it's valuable to preserve const here.

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

1062

We should definitely add a test for this even if it's already working correctly, because people love their .getCanonicalType() and I can easily see somebody adding it to the code later for some unrelated reason. They need to know that typedefs can't be dropped here.

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

878–880

oh yes, I should have made the raw attribute the default case here. More advanced cases of dealing with attributes are addressed in this patch.

891

These cases are not supported in this patch. (see the guard at lines 1358--1366).

959–960

This is a good point! I should have just obtained text representation of each part (i.e., return type and function name) separately.

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

20 ↗

(On Diff #515500)

Yes and thanks for the extra note.

Do you think const could also matter for pass-by-reference parameters? e.g.,
void f(int * const &p) -> void f(const std::span<int> &p)

I will add some tests for such cases

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

1062

I hadn't thought of anonymous structs. They can be a problem.
But I think the problem is that we cannot getPointeeType() from a pointer-to an anonymous type.
For example,

typedef struct {int x;} * ANON_PTR;
void foo(ANON_PTR p) {
  ... p[5];
}

One of the generated fix-its looks like this
std::span<struct (unnamed struct at clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span.cpp:7:9)> q

Actually, getCanonicalType() is not a problem in the cases that we can handle. For example,

typedef struct {int x;} ANON_S;
typedef ANON_S * ANON_PTR;

void foo(ANON_PTR p) {  ... }

getCanonicalType() returns ANON_S for the pointee type of the type of p. If ANON_S is defined by a named struct, getCanonicalType() returns the named struct.

[ziqingluo-90]

Address comments and made the following changes to the patch:

1. Stop using SourceManager::getCharacterData to obtain source file texts. Because it is possible that the start and end pointer point to different buffers. Instead, a helper function getRangeText is created.
2. Stop using TypePrinter. Calling getAsString on a QualType uses the printer under the hood. The printer does not necessarily print a type t in the exact same text form as t appearing in the source file. Especially when macros or typedefs are used. We prefer to generate texts for types without any change to their original literal forms. A helper function getPointeeTypeText is created to generate text for types using TypeLocs to obtain source ranges of types. However, there are a few limitations:
   • TypeLoc does not provide source ranges of qualifiers. (There is a QualifiedTypeLoc that looks like an outlier to the rest of the TypeLoc sub-classes and seems fishy to me). We have to generate qualifier texts using a printer separately.
   • Currently, we cannot obtain the text of a pointee type from a parameter declaration, if the declarator is too complicated. It now only supports two forms of parameter declarations: T identifier or T identifier[], where T is any type.
   • If a pointee type is unnamed or has unnamed sub-types, we cannot guarantee that we can refer to that type. Although there are many cases where we still can refer to such an unnamed type, we give up for now.

[NoQ]

Hi, looks great to me now. It looks like it's at a point where there could be more cornercases for us to tackle, but the rough idea looks correct and we can address the rest of them incrementally. Great job!!

A few microscopic nitpicks but generally I think this is ok to land.

clang/lib/Analysis/UnsafeBufferUsage.cpp

920

I'm a big fan of Present Simple all the way, unless there's a really good reason.

958–959

Slightly more C++-y this way.

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

47–49

[t-rasmud]

LGTM.

clang/lib/Analysis/UnsafeBufferUsage.cpp

1053

Minor nit: Changing function name to createOverloadsForFixedParams would need to reflect here as well.

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 "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Type.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/SmallVector.h"
 #include <memory>
 #include <optional>
 
 using namespace llvm;
 using namespace clang;
static FixItList fixVarDeclWithSpan(const VarDecl *D, const ASTContext &Ctx,
   raw_svector_ostream OS(Replacement);
 
   OS << "std::span<" << SpanEltT.getAsString() << "> " << D->getName();
   FixIts.push_back(FixItHint::CreateReplacement(
       SourceRange{D->getBeginLoc(), ReplacementLastLoc}, OS.str()));
   return FixIts;
 }
 
+static FixItList fixParamWithSpan(const ParmVarDecl *PVD,
+                                  const ASTContext &Ctx) {
+  assert(PVD->getType()->isPointerType() && "Only PointerType ParmVarDecl can be changed to std::span.");
+  FixItList FixIts{};
+  auto * FD = dyn_cast<clang::FunctionDecl>(PVD->getDeclContext());
+  if (!FD)
+    return {};
+
+  // TODO It looks like testLambdaCaptureAndGlobal in warn-unsafe-buffer-usage.cpp doesn't have a body and we crash below.
+  if (!FD->getBody())
+    return {};
+
+  // FIXME Respect indentation of the original code.
+
+  // TODO don't hardcode line delimiter - follow what the source file uses
+  // example:
+  //  void foo(int* ptr) { ... }
+  // goal:
+  //  [[clang::unsafe_buffer_usage]] void foo(int* ptr) { std::span(ptr, <# size #>) }
+  //  void foo(int* ptr) { ... }
+
+  const SourceManager &SM = Ctx.getSourceManager();
+
+  // TODO desugaring might be undesirable in some cases
+  // e.g. std::string -> std::basic_string<...>
+  // Maybe this is predominantly specific to std?
+  const std::string newSpanTypeSpelling = "std::span<" + PVD->getType().getDesugaredType(Ctx)->getPointeeType().getAsString() + ">";
+
+  const std::string compatOverloadNameAndSignature = [&]() -> std::string {
+    std::string result =
+      "[[clang::unsafe_buffer_usage]] "
+      + std::string(
+          SM.getCharacterData(FD->getBeginLoc()),
+          SM.getCharacterData(FD->getBody()->getBeginLoc())
+        );
+    while(!result.empty()) {
+      if (isHorizontalWhitespace(result.back()))
+        result.pop_back();
+      else
+        break;
+    }
+    return result;
+  }();
+
+  const std::string compatOverloadDef = [&]() -> std::string {
+    // create compat overload definition
+    std::string compatOverloadDef =
+      compatOverloadNameAndSignature + "\n" +
+      "{\n" +
+      "  return " + FD->getNameInfo().getAsString() + "(";
+    for (ParmVarDecl * Param : FD->parameters()) {
+      if (Param->isImplicit())
+        return std::string{}; // TODO error handling
+
+      // FIXME: if a param doesn't have a name - add a name in the signature of compat overload and use it here to pass to the original overload
+      if (Param->getNameAsString().empty())
+        return std::string{}; // TODO error handling
+
+      if (Param == PVD) {
+        // This is our spanified paramter!
+        // TODO don't hardcode the placeholder
+        compatOverloadDef += newSpanTypeSpelling + "("+Param->getNameAsString() + ", <# size #>)";
+      } else {
+        compatOverloadDef += Param->getNameAsString();
+      }
+
+      compatOverloadDef += ", ";
+    }
+    // remove the last ", "
+    compatOverloadDef.pop_back();
+    compatOverloadDef.pop_back();
+    // finish call
+    compatOverloadDef += ");\n";
+    // finish body
+    compatOverloadDef += "}\n";
+    // FIXME: 80-char line formatting?
+    return compatOverloadDef;
+  }();
+
+  if (compatOverloadDef.empty())
+    return {};
+
+  for (FunctionDecl *FReDecl : FD->redecls()) {
+    if (FReDecl->isThisDeclarationADefinition()) {
+      // define compatibility overload
+      // We need to call the origin overload from the compatibility overload.
+      // Let's make sure it has been defined before the call-site by adding the definition
+      // below the definition of the original overload.
+      FixIts.emplace_back(
+        FixItHint::CreateInsertion(
+          FReDecl->getEndLoc().getLocWithOffset(1),
+          "\n" + compatOverloadDef)
+      );
+    } else {
+      // declare compatibility overload
+      // Declarations commonly have Doxygen comments or other similar form of documentation.
+      // In order to not break the documentation we'll add the compatibility overload below the orginal one.
+      // FIXME: I failed to get location of the ';' after the declaration so I am adding one before the compatiblity overload.
+      FixIts.emplace_back(
+        FixItHint::CreateInsertion(FReDecl->getEndLoc().getLocWithOffset(1),  ";\n" + compatOverloadNameAndSignature)
+      );
+    }
+
+    // get ParmVar redecl in current function redecl that corresponds to PVD
+    ParmVarDecl* const PVReDecl = FReDecl->getParamDecl(PVD->getFunctionScopeIndex());
+
+    // FIXME: At least in some special cases we should be able to emit a Fix-It for the default argument (at least with a placeholder).
+    if (PVReDecl->hasDefaultArg())
+      return {}; // TODO test
+
+    const Type *PVReDeclTy = PVReDecl->getType().getTypePtrOrNull();
+    if (!PVReDeclTy)
+      return {}; // TODO test
+
+    // FIXME: We should use std::move() to forward the argument.
+    if (isa<RValueReferenceType>(PVReDeclTy))
+      return {}; // TODO test
+
+    if (const auto* PVReDeclRecTy = dyn_cast<RecordType>(PVReDeclTy)) {
+      if (CXXRecordDecl * PVReDeclCXXRecDe = PVReDeclRecTy->getAsCXXRecordDecl()) {
+        // FIXME: We should check if we can move the argument.
+        if (!PVReDeclCXXRecDe->isTriviallyCopyable())
+          return {}; // TODO test
+      }
+    }
+
+    // change parameter type in the original overload
+    FixIts.emplace_back(
+      FixItHint::CreateReplacement(
+        clang::CharSourceRange::getCharRange(
+          PVReDecl->getTypeSourceInfo()->getTypeLoc().getBeginLoc(),
+          PVReDecl->getTypeSourceInfo()->getTypeLoc().getEndLoc().getLocWithOffset(1)
+        ),
+        newSpanTypeSpelling + " "
+      )
+    );
+  }
+
+  return FixIts;
+}
+
 static FixItList fixVariableWithSpan(const VarDecl *VD,
                                      const DeclUseTracker &Tracker,
                                      const ASTContext &Ctx,
                                      UnsafeBufferUsageHandler &Handler) {
   const DeclStmt *DS = Tracker.lookupDecl(VD);
   assert(DS && "Fixing non-local variables not implemented yet!");
   if (!DS->isSingleDecl()) {
     // FIXME: to support handling multiple `VarDecl`s in a single `DeclStmt`
     return{};
   }
   // 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
   return fixVarDeclWithSpan(VD, Ctx, Handler.getUserFillPlaceHolder());
 }
 
 static FixItList fixVariable(const VarDecl *VD, Strategy::Kind K,
                              const DeclUseTracker &Tracker,
                              const ASTContext &Ctx,
                              UnsafeBufferUsageHandler &Handler) {
+  if (const auto *PVD = dyn_cast<ParmVarDecl>(VD)) {
+    auto * FD = dyn_cast<clang::FunctionDecl>(PVD->getDeclContext());
+    if (!FD)
+      return {};
+
+    // TODO If function has a try block we can't change params unless we check also its catch block for their use.
+    // FIXME We might support static class methods, some select methods, operators and possibly lamdas.
+    if (FD->isMain()
+        || FD->isConstexpr()
+        || FD->getTemplatedKind() != FunctionDecl::TemplatedKind::TK_NonTemplate
+        || FD->isVariadic()
+        || isa<CXXMethodDecl>(FD) // also covers call-operator of lamdas
+        || FD->isOverloadedOperator()
+        )
+      return {}; // TODO test all these cases
+  }
+
   switch (K) {
   case Strategy::Kind::Span: {
-    if (VD->getType()->isPointerType() && VD->isLocalVarDecl())
+    if (VD->getType()->isPointerType()) {
+      if (const auto *PVD = dyn_cast<ParmVarDecl>(VD))
+        return fixParamWithSpan(PVD, Ctx);
+
+      if (VD->isLocalVarDecl())
         return fixVariableWithSpan(VD, Tracker, Ctx, Handler);
+    }
+
     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!");
   }
 }
 
 // Returns true iff there exists a `FixItHint` 'h' in `FixIts` such that the
void clang::checkUnsafeBufferUsage(const Decl *D,
 
   {
     auto [FixableGadgets, WarningGadgets, TrackerRes] = findGadgets(D);
     UnsafeOps = groupWarningGadgetsByVar(std::move(WarningGadgets));
     FixablesForUnsafeVars = groupFixablesByVar(std::move(FixableGadgets));
     Tracker = std::move(TrackerRes);
   }
 
-  // Filter out non-local vars and vars with unclaimed DeclRefExpr-s.
+  // Filter out vars with unclaimed DeclRefExpr-s.
   for (auto it = FixablesForUnsafeVars.byVar.cbegin();
        it != FixablesForUnsafeVars.byVar.cend();) {
-    // FIXME: Support ParmVarDecl as well.
-    if (!it->first->isLocalVarDecl() || Tracker.hasUnclaimedUses(it->first)) {
+    if (Tracker.hasUnclaimedUses(it->first)) {
       it = FixablesForUnsafeVars.byVar.erase(it);
     } else {
       ++it;
     }
   }
 
   llvm::SmallVector<const VarDecl *, 16> UnsafeVars;
   for (const auto &[VD, ignore] : FixablesForUnsafeVars.byVar)

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

+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+// TODO test with #include-d file
+// TODO test if there's not a single character in the file after a decl or def
+
+void local_array_subscript_simple(int *p);
+// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:35-[[@LINE-1]]:40}:"std::span<int> "
+// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:42-[[@LINE-2]]:42}:";\n{{..}}clang::unsafe_buffer_usage{{..}} void local_array_subscript_simple(int *p)"
+
+void local_array_subscript_simple(int *a);
+// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:35-[[@LINE-1]]:40}:"std::span<int> "
+// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:42-[[@LINE-2]]:42}:";\n{{..}}clang::unsafe_buffer_usage{{..}} void local_array_subscript_simple(int *p)"
+
+void local_array_subscript_simple(int *p) {
+// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:35-[[@LINE-1]]:40}:"std::span<int> "
+  int tmp;
+  tmp = p[5];
+}
+// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{..}}clang::unsafe_buffer_usage{{..}} void local_array_subscript_simple(int *p)\n{\n  return local_array_subscript_simple(std::span<int>(p, <# size #>));\n}\n"