[-Wunsafe-buffer-usage] Group parameter fix-its

[ziqingluo-90]

Summary

For a function F whose parameters need to be fixed, we group fix-its of its' parameters together so that either all of the parameters get fixed or none of them gets fixed.

The reason for doing so is to avoid generating low-quality fix-its. For example,

void f(int * p, int * q) {
   int i = p[5];
   int j = q[5];
}

We would like to fix both parameters p and q with std::span. If we do not group them together, they will be fixed in two steps: 1) fixing p first; and then 2) fixing q. Step 1 yields a new overload of f:

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

The new overload of f is, however, still an unsafe function. Then we apply step 2 and have:

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

The "intermediate" overload void f(std::span<int>, int *) stays there but usually is not useful. If there are more than two parameters need to be fixed, there will be more such useless "intermediate" overloads.

With this patch, p and q will be fixed together: either selecting p or q to fix will result in a safe function void f(std::span<int> p, std::span<int> q in one step.

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

2209–2210

I changed the rest of this function drastically so let me explain what I did.

The table FixItsForVariable is initiated with variables whose declarations and associated Fixables can be fixed. So if a variable is not in the table, either 1) its declaration cannot be fixed or 2) one of its Fixables cannot be fixed. Then, the table is further reduced by removing elements such that at least one of its' group members satisfies 1) or 2).
Eventually, the table FixItsForVariable can be used to determine if a variable is ImpossibleToFix (so we no longer need this flag). FixItsForVariable[V], if V exists there, is a collection of fix-its for V's declaration and all Fixables associated to V.

With parameters being fixed, we also generate function overloads as fix-its for the parameters. These overload fix-its are "shared" by the parameters. It means that these fix-its will be added for the group where the parameters belong to, instead of being added to FixItsForVariable[PV] for each such parameter PV.

So finally, for a variable V, FinalFixItsForVariable[V] is a collection of fix-its for the whole group where V is at.

2371–2378

How about changing the variable name to PtrImplicationGraph? For two variables V and W, if W is in PtrImplicationGraph[V], it means fixing V implicates fixing W, right?

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

2371–2378

@t-rasmud what do you think?

[t-rasmud]

clang/lib/Analysis/UnsafeBufferUsage.cpp

2371–2378

Sounds good! PtrImplicationGraph conveys the same meaning as PtrAssignmentGraph and is more generic.

[NoQ]

I'll just leave this tiny bit of analysis here, that we did on the whiteboard a while ago.

The "intermediate" overload void f(std::span<int>, int *) stays there but usually is not useful. If there are more than two parameters need to be fixed, there will be more such useless "intermediate" overloads.

This isn't even the biggest problem. The bigger problem is that the process doesn't actually stop at Step 2.

Let me rewrite this more slowly.

Step 0 - the initial code:

// original body, original signature
void foo(int *p, int *q, size_t sz) {
   int i = p[5]; // warning: unsafe buffer usage over 'p'
   int j = q[5]; // warning: unsafe buffer usage over 'q'
}

Step 1 - fix parameter p:

// FIXED: original body, new signature
void foo(span<int> p, int *q, size_t sz) {
   int i = p[5];
   int j = q[5]; // warning: unsafe buffer usage over 'q'
}

// NEW: compatibility overload from step 1 - original signature
[[unsafe_buffer_usage]]
void f(int *p, int *q, size_t sz) {
   foo(span<int>(p, <#size#>), q, sz);
}

Step 2 - fix parameter q as well:

// FIXED: original code, even newer signature
void foo(span<int> p, span<int> q, size_t sz) {
   int i = p[5];
   int j = q[5];
}

// NEW: compatibility overload from step 2
[[unsafe_buffer_usage]]
void foo(span<int> p, int *q, size_t sz) {
   foo(p, span<int>(q, <#size#>), sz);
}

// UNCHANGED: compatibility overload from step 1 - still carries original signature
[[unsafe_buffer_usage]]
void foo(int *p, int *q, size_t sz) {
   foo(span<int>(p, sz), q, sz); // warning: unsafe buffer usage over 'q'
}

Step 3 - now we have a warning inside the overload from step 1 because it's no longer calling the newest safe function, but it's now calling the compatibility overload from step 2. So we need to change the parameter q of the bottom-most function to span.

// UNCHANGED: original code, even newer signature - unchanged
void foo(span<int> p, span<int> q, size_t sz) {
   int i = p[5];
   int j = q[5];
}

// UNCHANGED: compatibility overload from step 2
[[unsafe_buffer_usage]]
void foo(span<int> p, int *q, size_t sz) {
   foo(p, span<int>(q, sz), sz);
}

// FIXED: compatibility overload from step 1 - new safe signature
[[unsafe_buffer_usage]]
void foo(int *p, span<int> q, size_t sz) {
   foo(span<int>(p, sz), q, sz);
}

// NEW: compatibility overload from step 3 - original signature of the overload from step 1, aka simply original signature
[[unsafe_buffer_usage]]
[[unsafe_buffer_usage]] // sic!
void foo(int *p, int *q, size_t sz) {
   foo(p, span<int>(q, <#size#>), sz); // warning: unsafe buffer usage over 'p'
}

There are a few things that look wrong here.

The set of overloads still makes sense; we get ourselves 4 overloads in all combinations of spans and raw pointers. This on its own makes sense.

We've also somehow got the new raw overload (with two raw pointers) carry two instances of [[unsafe_buffer_usage]]. It lowkey makes sense because the function is unsafe with respect to both parameters, but we probably don't want people to write such code.

Now, here's where it gets really weird. You'd probably expect that new raw overload to call foo(span<int>(p, sz), span<int>(q, sz), sz). If it just did that, it'd be perfectly reasonable code. But it doesn't do that! Indeed, by design the autogenerated compatibility overload calls the freshly-fixed function. Which in case of Step 3 was foo(int *p, span<int> q, size_t sz).

Also, the new raw overload already has a warning in it! This is completely unacceptable. This happened because the function under fix was already annotated as [[unsafe_buffer_usage]]. The autofix wouldn't remove the attribute; the attribute is still completely appropriate there.

Finally, because there's a new warning, the process doesn't stop after Step 3 either. I'll leave imagining what Step 4 would look like as an easy exercise to the reader ^.^

---

So one way or another, we really don't want to be in the business of incrementally fixing parameters one-by-one. Fixing all parameters at once is much easier than fixing them incrementally and discarding all analysis state on each step. Analysis of "partially spanified" code is much harder than analysis of raw C-style code, so we prefer single-step transformation in most cases.

Another direction that we can explore is to avoid auto-fixing functions that already carry [[unsafe_buffer_usage]]. Indeed, it's already a compatibility overload and we know it; it's not *supposed* to be safe! This will prevent Step 3 from breaking things, so it'll leave us with 3 overloads - not ideal, but not incorrect either. It'll also eliminate the "double attribute" problem.

And we ultimately need to explore this direction because no matter how hard we avoid automatic partial transformations, people will still do manual partial transformations! And when they do, we need to demonstrate some reasonable behavior.

But for now let's focus on the basic use case.

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

2201

There's a bug in variable naming: FixablesForUnsafeVarsactually contains fixables for *all* variables. Rashmi correctly renames it in D150489 to reflect its actual behavior. So IIUC you're generating fixits for all variables here, which is probably not what you want.

2392–2395

Does this really go in both directions? Shouldn't it be just one direction?

void foo(int *p) {
  int *q = new int[10];
  p = q;
  q[5] = 7;
}

In this case q is an unsafe local buffer, but p is a (mostly) safe single-object pointer that doesn't need fixing, despite implication from q to p. Of course this example is somewhat unrealistic (people don't usually overwrite their parameters), but it is also the only situation where the other direction matters.

2407

What about the situation where params aren't seen as unsafe yet, but they're discovered to be unsafe later? Eg.

void foo(int *p, int *q) {
  int *p2 = p;
  p2[5] = 7;
  int *q2 = q;
  q2[5] = 7;
}

Will we notice that p and q need to be fixed simultaneously?

---

I suspect that the problem of parameter grouping can't be solved by pre-populating strategy implications between all parameters. It'll either cause you to implicate all parameters (even the ones that will never need fixing), or cause you to miss connections between parameters that do need fixing (as the connection is discovered later).

Connection between parameters needs to be added *after* the implications algorithm has finished. And once it's there (might be already there if I missed something), then this part of code probably becomes unnecessary.

2471–2473

Note: This is why FixablesForUnsafeVars is actually FixablesForAllVars.

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

14–16

QoL thing: "to propagate bounds information between them" isn't the correct reason in this case. Bounds information doesn't propagate between p and q and a. Each of them carries its own, completely unrelated bounds information.

We need to come up with a different reason. Or it might be better to combine the warnings into one warning here, so that we didn't need a reason:

warning: 'p', 'q' and 'a' are unsafe pointers used for buffer access
note: change type of 'p', 'q' and 'a' to 'std::span' to preserve bounds information

This gets a bit worse when parameters are implicated indirectly, but it still mostly works fine, for example:

void foo(int *p, int *q) {
  int *p2 = p;
  p2[5] = 7;
  int *q2 = q;
  q2[5] = 7;
}

would produce:

warning: 'p2' and 'q2' are unsafe pointers used for buffer access
note: change type of 'p2' and 'q2' to 'std::span' to preserve bounds information, and
      change 'p' and 'q' to 'std::span' to propagate bounds information between them

---

In any case, this shows that the relationship between parameters (grouped together simply for being parameters) is very different from the relationship within groups of variables implicated by assignments (even if two or more of them are parameters). All the way down to the warning/note printer, we probably need to continue giving the parameter relationship a special treatment.

[NoQ]

Ok whew that's a big patch! I *think* I found a couple of bugs. In any case, this code is quickly becoming very complicated, and the functions we're editing are rapidly growing out of control. Chopping them up into smaller functions with clear separation of concerns would probably help the reader a lot 😅

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

2201

Thanks for pointing out the incorrectness of the variable name. Actually, whether a variable is unsafe is irrelevant to this function. The function only cares about all variables that need fixes. So I think the incorrect name of the variable does not affect the functionality of my changes here.

2392–2395

You are right! It should be one-direction.

2407

Yes. They will be noticed. Here is why:

The code here first forms a ring over p and q in the assignment (directed) graph:

p <--> q

Then the two initialization statements (implemented in D150489) add two more edges to the graph:

p2 --> p <--> q <-- q2

The algorithm later searches the graph starting from unsafe variables p2 and q2, respectively, Starting from p2, the algorithm discovers that p2 and p, as well as p and q depend on each other. Starting from q2, the algorithm discovers that q2 and q, as well as p and q depend on each other. The dependency graph is sort of undirected. So eventually, the four variables p2, p, q, q2 are in the same group.

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

14–16

This is a good argument! An edge from p to q in an assignment graph stands for not only that fixing p implicates fixing q but also that the bounds information is propagated from q to p. In this sense, the way I connect parameters is inappropriate.

But my concern is that although treating parameters specially could improve the quality of the diagnostic messages, the code may be more complex and less re-used.

[ziqingluo-90]

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

14–16

So for such an example

int f(int *p, int *q) {
int * a = p;
int * b = q;

a[5] = 0;
b[5] = 0;
}

We will fix all four variables a, b, p, q together. But bounds information propagation only happens between a and p, and b and q, respectively.
Then what should the diagnostic message be? How about something like

change type of 'a' to 'std::span' to preserve bounds information, and change 'p' to propagate bounds information between them. 
To ensure parameters are changed together, also
change type of 'b' to 'std::span' to preserve bounds information, and change 'q' to propagate bounds information between them;
...

for variable a.

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

2407

The code here first forms a ring over p and q in the assignment (directed) graph

I don't think it does. The ring is formed over the ParamsNeedFix list, which is empty because none of these parameters are listed in UnsafeOps.byVar.

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

14–16

I think the ideal narrative is to emit a single warning against the entire function, which covers all parameters at once, together with all related variables.

int f(int *p, int *q) {  // warning: function 'f' performs unsafe buffer access over parameters 'p' and 'q'
                         // note: change type of 'p' and 'q' to 'std::span' to receive bounds information from the call site,
                         //       and change local variables 'a' and 'b' to 'std::span' to propagate it to the access site

  int * a = p;           // note: bounds information needs to propagate from 'p' to 'a' here
  int * b = q;           // note: bounds information needs to propagate from 'q' to 'b' here

  a[5] = 0;              // note: used in buffer access here
  b[5] = 0;              // note: used in buffer access here
}

This makes a lot of sense given that we're fixing the entire function. This is quite a major change though, we need to think this through.

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

2407

Actually p and q will be in ParamsNeedFix. Because they are implicated by p2 and q2, who are unsafe variables. ParamsNeedFix include parameters that need fix, either because they are used in unsafe operations or they are implicated by some Gadgets.

But anyway, I now think it's not a good idea to lump parameters with variables grouped by implication, as you pointed out that variable implication also implies bounds propagation.

Instead, I think it might be more appropriate to make a variable group structured:

• Let's call it a connected components for a set of variables that need to be fixed together and sharing bounds information. (The term connected components has been used in the code to refer to a group of variables. It keeps its meaning here.)
• Let's redefine a Variable Group to be a set of mutually disjoint connected componentss (CCs). If there are more than one CCs in a variable group, each of the CCs contains at least one parameter.

For generating fix-its for a variable group, this patch still works as it only requires that variables in a variable group are either all fixed or given up as a whole.
For generating diagnostic messages, we now have more information about relations among grouped variables.

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

2407

Actually p and q will be in ParamsNeedFix. Because they are implicated by p2 and q2, who are unsafe variables. ParamsNeedFix include parameters that need fix, either because they are used in unsafe operations or they are implicated by some Gadgets.

Ohh, right, it's populated twice, I even commented on the other part but didn't notice it populates the same list!

But in this case you'll probably join p and q in

void foo(int *p, int *q) {
  int *p2 = p;
  p2[5] = 7;
  int *q2 = q;
}

right? (In this case q is implicated by q2 so it gets added to the list, but q2 itself isn't unsafe or implicated by anything, so it doesn't need fixing, but connecting it to p would cause it to be fixed unnecessarily). So I still think this can't be correct unless you do it after the crawler finishes.

But anyway, I now think it's not a good idea to lump parameters with variables grouped by implication, as you pointed out that variable implication also implies bounds propagation.

Yeah, I think it makes a lot more sense. Just look at variable groups already built by the existing crawler, and if more than one of them contains parameters, report all groups that contain parameters through one combined warning.

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

2407

Though I'm going to remove this part, achieving some agreement on this algorithm may help me understand the problem better.

right? (In this case q is implicated by q2 so it gets added to the list, but q2 itself isn't unsafe or implicated by anything, so it doesn't need fixing, but connecting it to p would cause it to be fixed unnecessarily)

Right. the code in this patch does not form ParmsNeedFix correctly. ParmsNeedFix should be the set of parameters that are either unsafe or being implicated (transitively) by unsafe variables. Then the group would be "correct" I think.

[t-rasmud]

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

6

I have used a workaround for non-determinism by using regular expressions to match on the expected-note. See https://github.com/llvm/llvm-project/commit/db3dcedb9cedcec4a9570fda7406490c642df8ae#diff-8486f0b4ae37871fc0a186f40a41adbc70e5a0ca0134dc9bba762e1f17994460R176.
This is definitely a temporary fix (but helps with passing llvm builedbot tests upon landing on llvm-project\main) and we should probably discuss and put in a more reliable fix (say, using ordered sets) in the future.

[t-rasmud]

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

252

Does this patch handle virtual methods? Ideally we'd like the fixed methods to have the same subtyping relations as the original method. Does it make sense to have test cases with virtual methods as part of this patch?

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

2407

Aha ok, whew, gotcha, makes sense then!

[ziqingluo-90]

• a major change to the patch: Parameters are now grouped after the grouping of all variables. The groups that contain parameters of the function will form a union group.

• changes to the grouping algorithm: Groups will reside in the vector Groups and being accessed through their indexes so that we do not have to copy groups. The GrpsUnionForParms is the union group over groups containing parameters. It holds a separate copy. These data structures are wrapped in VariableGroupsManager, which returns the proper group given any variable that needs fix.

• add a new diagnostic note kind: diag::note_unsafe_buffer_variable_fixit_together. It will replace diag::note_unsafe_buffer_variable_fixit_group when we do not want to explain how a group of variables gets formed.

• add a "Comparator" to FixableGadgetSets and replace some std::sets with llvm::SetVectors for deterministic diagnostic messages.

clang/lib/Sema/AnalysisBasedWarnings.cpp

2168

The printing process was extracted as a method with some refactoring.

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

252

Sorry for the late response.
That's a good point. Virtual methods could get things more complicated. This is one of the reasons that we do not fix any member functions at this point.

[t-rasmud]

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

306

Can we have a test case with qualifiers on parameters and maybe another with a qualifier on the function itself?

[NoQ]

Alright, I think I managed to fully chew through this patch and I think it's beautiful and everything makes sense to me!

I have a tiny complaint though: It is very large though, 500 lines of code is very hard to digest all at once. Because we aren't coming in with all the context you had when you wrote this code, it's often very hard for us readers to guess the intention behind every section of the diff, and how they are supposed to work together.

In this patch there are a few refactoring steps that weren't responsible for any change in behavior:

• Introduce the VariableGroupsManager abstraction;
• Extract eraseVarsForUnfixableGroupMates() into a function;
• Extract listVariableGroupAsString into a function;

But because I didn't know that at first, I had to carefully look at every line of code to confirm that it actually doesn't change anything.

So it'd help tremendously if these refactorings were extracted into a parallel "no functional change" ("NFC") patch (with zero tests: no failures on existing tests => the patch is indeed NFC) that doesn't do anything other than "prepare the codebase" for this patch. Then in the NFC patch we'd discuss architecture and confirm that it truly doesn't change anything, whereas in this patch every line of code would be filled with meaning and purpose! ^.^

I'm not sure it makes sense split it up now, probably depends on whether Jan and Rashmi have the same troubles as me ^.^

I also has nitpicks.

clang/lib/Analysis/UnsafeBufferUsage.cpp

2130–2131

(.contains() is unfortunately C++20)

2156–2157

Would it make sense to make createFunctionOverloadsForParms accept a FunctionDecl *D from the start?

2162–2163

Another good use for .count().

2224

2430

Why SetVector? Do we care about order? Maybe just SmallSet?

clang/lib/Sema/AnalysisBasedWarnings.cpp

2171

Does the empty string actually ever make sense for the caller? Maybe just assert that this never happens, and force the caller to check that?

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

37–39

The fix is emitted 3 times right? Once for each warning?

Would it make sense to duplicate the three CHECK: lines three times to confirm that?

[t-rasmud]

I'm not sure it makes sense split it up now, probably depends on whether Jan and Rashmi have the same troubles as me ^.^

I have to admit it is a difficult patch to follow. It also has a lot of clever ideas that deserve attention but might be lost because of the complexity of the patch. I would really appreciate if this I can be refactored and split up!

[NoQ]

I just want to add to this, that the NFC part is actually insanely valuable (despite technically not doing anything). This patch is so complex primarily because UnsafeBufferUsage.cpp already has 1300 lines of code in unstructured static functions - that's more than half of our code! I really appreciate every bit of effort to separate it into components with clear boundaries and contracts, and even the new comments really help 🥺

[jkorous]

I vote for splitting the patch to make both the review and any future debugging or git archeology easier.

[ziqingluo-90]

Thanks for the suggestion of splitting this patch to smaller ones.

I have one such smaller patch ready here: https://reviews.llvm.org/D156474. It does make it easier in describing and discussing about the changes.

clang/lib/Analysis/UnsafeBufferUsage.cpp

2430

We'd like to keep the order of elements in this container deterministic. Otherwise, the diagnostic message that lists those elements could be non-deterministic.

clang/lib/Sema/AnalysisBasedWarnings.cpp

2171

The branch deals with the case where the group has only one member, which is the VD. In that case, we do not list group mates of VD as there is none.
This case is actually possible and conforms to the contract of this method.

The case where the group is empty is not suppose to happen.

[ziqingluo-90]

Carved out NFC changes from the original diff.

[ziqingluo-90]

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

37–39

I wish there are better ways to test this. I tried CHECK-COUNT-3 but it doesn't work because it expects each line repeats 3 times consecutively.

306

good point. I recently added such tests for parameters and local variables in following open patches:

• https://reviews.llvm.org/D156189 : clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp:49--62

• https://reviews.llvm.org/D156188 : clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span.cpp:150--198

[NoQ]

OOo down to ±300, I love this!! I'll take a closer look tomorrow.

clang/lib/Analysis/UnsafeBufferUsage.cpp

1855–1856

Speaking of my comment on the other patch.

Can we simply ask Strategy about the strategy for FD->getParamDecl(i) instead of passing a mask?

Eventually we'll have to do that anyway, given how different parameters can be assigned different strategies.

1899–1903

Arguably not a great use of auto, because the return type isn't literally Identifier *.

2430

Ooo right gotcha!

clang/lib/Sema/AnalysisBasedWarnings.cpp

2171

Gotcha, nice!

[ziqingluo-90]

Address comments.

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

1855–1856

Yes. I think using Strategy is the correct way to do it.

Can I make it a follow-up patch? I noticed that this line needs to be fixed:

Strategy NaiveStrategy = getNaiveStrategy(UnsafeVars);

The UnsafeVars is the set of variables associated to all matched FixableGadgets. There can be variables that is safe and do not need fix. Their strategy is set to std::span currently which is not correct. With this line being fixed, we can have examples like

void f(int * p) {
  int * q;
   q = p;
   p[5] = 5;
}

where q's strategy is WONTFIX and p's strategy is std::span. The expression q = p can be fixed to q = p.data(). It currently has an empty fix-it, which is incorrect.

[NoQ]

Ok I think this looks great and almost good to go. I love how the core change in the grouping algorithm is actually tiny and simple!

clang/lib/Analysis/UnsafeBufferUsage.cpp

1855–1856

Yes sure!

2293–2295

The usual idiom to avoid double lookup: use find() to extract an iterator, compare it to end() to see if the item was found, and if it was then dereference the iterator.

I wouldn't use at() given that the whole point of .at() is to throw exceptions, while LLVM is compiled with -fno-exceptions.

clang/lib/Sema/AnalysisBasedWarnings.cpp

2271–2272

IIRC we're supposed to stuff ND directly into the stream and it'll figure out on its own how to name it, or how to put quotes around it. See how there's no quotes around %2!

[ziqingluo-90]

Address comments

[NoQ]

Everything looks great now, let's commit!

Again, thank you Ziqing for simplifying and splitting up the patch. It looks like we're able to keep code complexity from exploding. That makes me feel very optimistic about the future of -Wunsafe-buffer-usage!

clang/lib/Analysis/UnsafeBufferUsage.cpp

1371

Formatting is a bit off (clang-format probably needed some human help here).

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

 #ifndef LLVM_CLANG_ANALYSIS_ANALYSES_UNSAFEBUFFERUSAGE_H
 #define LLVM_CLANG_ANALYSIS_ANALYSES_UNSAFEBUFFERUSAGE_H
 
 #include "clang/AST/Decl.h"
 #include "clang/AST/Stmt.h"
 
 namespace clang {
 
-using DefMapTy = llvm::DenseMap<const VarDecl *, std::vector<const VarDecl *>>;
+using VarGrpTy = std::vector<const VarDecl *>;
+using VarGrpRef = ArrayRef<const VarDecl *>;
+
+class VariableGroupsManager {
+public:
+  VariableGroupsManager() = default;
+  virtual ~VariableGroupsManager() = default;
+  /// Returns the set of variables (including `Var`) that need to be fixed
+  /// together in one step.
+  ///
+  /// `Var` must be a variable that needs fix (so it must be in a group).
+  /// `HasParm` is an optional argument that will be set to true if the set of
+  /// variables, where `Var` is in, contains parameters.
+  virtual VarGrpRef getGroupOfVar(const VarDecl *Var,
+                                  bool *HasParm = nullptr) const;
+};
 
 /// 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::SmallVectorImpl<FixItHint>;
 
   /// 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. This function groups
-  /// all variables that must be fixed together (i.e their types must be changed to the
-  /// same target type to prevent type mismatches) into a single fixit.
+  /// all variables that must be fixed together (i.e their types must be changed
+  /// to the same target type to prevent type mismatches) into a single fixit.
+  ///
+  /// `D` is the declaration of the callable under analysis that owns `Variable`
+  /// and all the variables in `Group`.
+  /// `BriefMsg` is true if to NOT explain how variables are grouped together.
   virtual void handleUnsafeVariableGroup(const VarDecl *Variable,
-                                         const DefMapTy &VarGrpMap,
-                                         FixItList &&Fixes) = 0;
+                                         ArrayRef<const VarDecl *> Group,
+                                         FixItList &&Fixes, const Decl *D,
+                                         bool BriefMsg = false) = 0;
 
   /// Returns a reference to the `Preprocessor`:
   virtual bool isSafeBufferOptOut(const SourceLocation &Loc) const = 0;
 
   /// Returns the text indicating that the user needs to provide input there:
   virtual std::string
   getUserFillPlaceHolder(StringRef HintTextToUser = "placeholder") const {
     std::string s = std::string("<# ");

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def warn_unsafe_buffer_operation : Warning<
   "%select{unsafe pointer operation|unsafe pointer arithmetic|"
   "unsafe buffer access|function introduces unsafe buffer manipulation}0">,
   InGroup<UnsafeBufferUsage>, DefaultIgnore;
 def note_unsafe_buffer_operation : Note<
   "used%select{| in pointer arithmetic| in buffer access}0 here">;
 def note_unsafe_buffer_variable_fixit_group : Note<
   "change type of %0 to '%select{std::span|std::array|std::span::iterator}1' to preserve bounds information%select{|, and change %2 to '%select{std::span|std::array|std::span::iterator}1' to propagate bounds information between them}3">;
+def note_unsafe_buffer_variable_fixit_together : Note<
+  "change type of %0 to '%select{std::span|std::array|std::span::iterator}1' to preserve bounds information"
+  "%select{|, and change %2 to safe types to make function '%4' bounds-safe}3">;
 def note_safe_buffer_usage_suggestions_disabled : Note<
   "pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions">;
 def err_loongarch_builtin_requires_la32 : Error<
   "this builtin requires target: loongarch32">;
 
 def err_builtin_pass_in_regs_non_class : Error<
   "argument %0 is not an unqualified class type">;
 

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/Decl.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Lex/Preprocessor.h"
-#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/SetVector.h"
 #include <memory>
 #include <optional>
 #include <sstream>
 #include <queue>
 
 using namespace llvm;
 using namespace clang;
 using namespace ast_matchers;
if (!AssociatedWithVarDecl) {
       result.noVar.push_back(G.get());
       continue;
     }
   }
   return result;
 }
 
 struct FixableGadgetSets {
-  std::map<const VarDecl *, std::set<const FixableGadget *>> byVar;
+  std::map<const VarDecl *, std::set<const FixableGadget *>,
+           // To keep keys sorted by their locations in the map so that the
+           // order is deterministic:
+           CompareNode<VarDecl>>
+      byVar;
 };
 
 static FixableGadgetSets
 groupFixablesByVar(FixableGadgetList &&AllFixableOperations) {
   FixableGadgetSets FixablesForUnsafeVars;
   for (auto &F : AllFixableOperations) {
     DeclUseList DREs = F->getClaimedVarUseSites();
 
static std::optional<StringRef> getExprText(const Expr *E,
   return std::nullopt;
 }
 
 // Returns the literal text in `SourceRange SR`, if `SR` is a valid range.
 static std::optional<StringRef> getRangeText(SourceRange SR,
                                              const SourceManager &SM,
                                              const LangOptions &LangOpts) {
   bool Invalid = false;
-  CharSourceRange CSR = CharSourceRange::getCharRange(SR.getBegin(), SR.getEnd());
+  CharSourceRange CSR = CharSourceRange::getCharRange(SR);
   StringRef Text = Lexer::getSourceText(CSR, SM, LangOpts, &Invalid);
 
   if (!Invalid)
     return Text;
   return std::nullopt;
 }
 
+// Returns the `SourceRange` of `D`.  The reason why this function exists is
+// that `D->getSourceRange()` may return a range where the end location is the
+// starting location of the last token.  The end location of the source range
+// returned by this function is the last location of the last token.
+static SourceRange getSourceRangeToTokenEnd(const Decl *D,
+                                            const SourceManager &SM,
+                                            LangOptions LangOpts) {
+  SourceLocation Begin = D->getBeginLoc();
+  SourceLocation
+      End = // `D->getEndLoc` should always return the starting location of the
+            // last token, so we should get the end of the token
+      Lexer::getLocForEndOfToken(D->getEndLoc(), 0, SM, LangOpts);
+
+  return SourceRange(Begin, End);
+}
+
 // Returns the text of the pointee type of `T` from a `VarDecl` of a pointer
 // type. The text is obtained through from `TypeLoc`s.  Since `TypeLoc` does not
 // have source ranges of qualifiers ( The `QualifiedTypeLoc` looks hacky too me
 // :( ), `Qualifiers` of the pointee type is returned separately through the
 // output parameter `QualifiersToAppend`.
 static std::optional<std::string>
 getPointeeTypeText(const ParmVarDecl *VD, const SourceManager &SM,
                    const LangOptions &LangOpts,
static std::optional<StringRef> getFunNameText(const FunctionDecl *FD,
   // last token:
   SourceLocation EndLoc = Lexer::getLocForEndOfToken(
       FD->getNameInfo().getEndLoc(), 0, SM, LangOpts);
   SourceRange NameRange{BeginLoc, EndLoc};
 
   return getRangeText(NameRange, SM, LangOpts);
 }
 
+// Returns the text representing a `std::span` type where the element type is
+// represented by `EltTyText`.
+//
+// Note the optional parameter `Qualifiers`: one needs to pass qualifiers
+// explicitly if the element type needs to be qualified.
+static std::string
+getSpanTypeText(StringRef EltTyText,
+                std::optional<Qualifiers> Quals = std::nullopt) {
+  const char *const SpanOpen = "std::span<";
+
+  if (Quals)
+    return SpanOpen + EltTyText.str() + ' ' + Quals->getAsString() + '>';
+  return SpanOpen + EltTyText.str() + '>';
+}
+
 std::optional<FixItList>
 DerefSimplePtrArithFixableGadget::getFixits(const Strategy &s) const {
   const VarDecl *VD = dyn_cast<VarDecl>(BaseDeclRefExpr->getDecl());
 
   if (VD && s.lookup(VD) == Strategy::Kind::Span) {
     ASTContext &Ctx = VD->getASTContext();
     // std::span can't represent elements before its begin()
     if (auto ConstVal = Offset->getIntegerConstantExpr(Ctx))

 // Returns the text representation of clang::unsafe_buffer_usage attribute.
 static std::string getUnsafeBufferUsageAttributeText() {
   static const char *const RawAttr = "[[clang::unsafe_buffer_usage]]";
   std::stringstream SS;
   SS << RawAttr << getEndOfLine().str();
   return SS.str();
 }
 
-// For a `FunDecl`, one of whose `ParmVarDecl`s is being changed to have a new
-// type, this function produces fix-its to make the change self-contained.  Let
-// 'F' be the entity defined by the original `FunDecl` and "NewF" be the entity
-// defined by the `FunDecl` after the change to the parameter.  Fix-its produced
-// by this function are
+// For a `FunctionDecl`, whose `ParmVarDecl`s are being changed to have new
+// types, this function produces fix-its to make the change self-contained.  Let
+// 'F' be the entity defined by the original `FunctionDecl` and "NewF" be the
+// entity defined by the `FunctionDecl` after the change to the parameters.
+// Fix-its produced by this function are
 //   1. Add the `[[clang::unsafe_buffer_usage]]` attribute to each declaration
 //   of 'F';
 //   2. Create a declaration of "NewF" next to each declaration of `F`;
 //   3. Create a definition of "F" (as its' original definition is now belongs
 //      to "NewF") next to its original definition.  The body of the creating
 //      definition calls to "NewF".
 //
 // Example:

 // }
 //
 // The actual fix-its may contain more details, e.g., the attribute may be guard
 // by a macro
 //   #if __has_cpp_attribute(clang::unsafe_buffer_usage)
 //   [[clang::unsafe_buffer_usage]]
 //   #endif
 //
-// `NewTypeText` is the string representation of the new type, to which the
-// parameter indexed by `ParmIdx` is being changed.
+// `ParmsMask` is an array of size of `FD->getNumParams()` such
+// that `ParmsMask[i]` is true iff the `i`-th parameter will be fixed with some
+// strategy.
 static std::optional<FixItList>
-createOverloadsForFixedParams(unsigned ParmIdx, StringRef NewTyText,
-                                const FunctionDecl *FD, const ASTContext &Ctx,
-                                UnsafeBufferUsageHandler &Handler) {
+createOverloadsForFixedParams(const std::vector<bool> &ParmsMask, const Strategy &S,
   // FIXME: need to make this conflict checking better:
   if (hasConflictingOverload(FD))
     return std::nullopt;
 
   const SourceManager &SM = Ctx.getSourceManager();
   const LangOptions &LangOpts = Ctx.getLangOpts();
+  const unsigned NumParms = FD->getNumParams();
+  std::vector<std::string> NewTysTexts(NumParms);
+
+  for (unsigned i = 0; i < NumParms; i++) {
+    if (!ParmsMask[i])
+      continue;
+
+    std::optional<Qualifiers> PteTyQuals = std::nullopt;
+    std::optional<std::string> PteTyText =
+        getPointeeTypeText(FD->getParamDecl(i), SM, LangOpts, &PteTyQuals);
+
+    if (!PteTyText)
+      // something wrong in obtaining the text of the pointee type, give up
+      return std::nullopt;
+    // FIXME: whether we should create std::span type depends on the Strategy.
+    NewTysTexts[i] = getSpanTypeText(*PteTyText, PteTyQuals);
+  }
   // FIXME Respect indentation of the original code.
 
   // A lambda that creates the text representation of a function declaration
-  // with the new type signature:
+  // with the new type signatures:
   const auto NewOverloadSignatureCreator =
-      [&SM, &LangOpts](const FunctionDecl *FD, unsigned ParmIdx,
-                       StringRef NewTypeText) -> std::optional<std::string> {
+      [&SM, &LangOpts, &NewTysTexts,
+       &ParmsMask](const FunctionDecl *FD) -> std::optional<std::string> {
     std::stringstream SS;
 
     SS << ";";
     SS << getEndOfLine().str();
     // Append: ret-type func-name "("
     if (auto Prefix = getRangeText(
             SourceRange(FD->getBeginLoc(), (*FD->param_begin())->getBeginLoc()),
             SM, LangOpts))
       SS << Prefix->str();
     else
       return std::nullopt; // give up
     // Append: parameter-type-list
     const unsigned NumParms = FD->getNumParams();
 
     for (unsigned i = 0; i < NumParms; i++) {
       const ParmVarDecl *Parm = FD->getParamDecl(i);
 
       if (Parm->isImplicit())
         continue;
-      if (i == ParmIdx) {
-        SS << NewTypeText.str();
+      if (ParmsMask[i]) {
+        // This `i`-th parameter will be fixed with `NewTysTexts[i]` being its
+        // new type:
+        SS << NewTysTexts[i];
         // print parameter name if provided:
-        if (IdentifierInfo * II = Parm->getIdentifier())
-          SS << " " << II->getName().str();
-      } else if (auto ParmTypeText =
-                     getRangeText(Parm->getSourceRange(), SM, LangOpts)) {
+        if (auto II = Parm->getIdentifier())
+          SS << ' ' << II->getName().str();
+      } else if (auto ParmTypeText = getRangeText(
+                     getSourceRangeToTokenEnd(Parm, SM, LangOpts),
+                     SM, LangOpts)) {
         // print the whole `Parm` without modification:
         SS << ParmTypeText->str();
       } else
         return std::nullopt; // something wrong, give up
       if (i != NumParms - 1)
         SS << ", ";
     }
     SS << ")";
     return SS.str();
   };
 
   // A lambda that creates the text representation of a function definition with
   // the original signature:
   const auto OldOverloadDefCreator =
-      [&Handler, &SM,
-       &LangOpts](const FunctionDecl *FD, unsigned ParmIdx,
-                  StringRef NewTypeText) -> std::optional<std::string> {
+      [&Handler, &SM, &LangOpts, &NewTysTexts,
+       &ParmsMask](const FunctionDecl *FD) -> std::optional<std::string> {
     std::stringstream SS;
 
     SS << getEndOfLine().str();
     // Append: attr-name ret-type func-name "(" param-list ")" "{"
     if (auto FDPrefix = getRangeText(
             SourceRange(FD->getBeginLoc(), FD->getBody()->getBeginLoc()), SM,
             LangOpts))
       SS << getUnsafeBufferUsageAttributeText() << FDPrefix->str() << "{";
const auto OldOverloadDefCreator =
     const unsigned NumParms = FD->getNumParams();
     for (unsigned i = 0; i < NumParms; i++) {
       const ParmVarDecl *Parm = FD->getParamDecl(i);
 
       if (Parm->isImplicit())
         continue;
       assert(Parm->getIdentifier() &&
              "A parameter of a function definition has no name");
-      if (i == ParmIdx)
+      if (ParmsMask[i])
         // This is our spanified paramter!
-        SS << NewTypeText.str() << "(" << Parm->getIdentifier()->getName().str() << ", "
-           << Handler.getUserFillPlaceHolder("size") << ")";
+        SS << NewTysTexts[i] << "(" << Parm->getIdentifier()->getName().str()
+           << ", " << Handler.getUserFillPlaceHolder("size") << ")";
       else
         SS << Parm->getIdentifier()->getName().str();
       if (i != NumParms - 1)
         SS << ", ";
     }
     // finish call and the body
     SS << ");}" << getEndOfLine().str();
     // FIXME: 80-char line formatting?
     return SS.str();
   };
 
   FixItList FixIts{};
   for (FunctionDecl *FReDecl : FD->redecls()) {
     std::optional<SourceLocation> Loc = getPastLoc(FReDecl, SM, LangOpts);
 
     if (!Loc)
       return {};
     if (FReDecl->isThisDeclarationADefinition()) {
       assert(FReDecl == FD && "inconsistent function definition");
       // Inserts a definition with the old signature to the end of
       // `FReDecl`:
-      if (auto OldOverloadDef =
-              OldOverloadDefCreator(FReDecl, ParmIdx, NewTyText))
+      if (auto OldOverloadDef = OldOverloadDefCreator(FReDecl))
         FixIts.emplace_back(FixItHint::CreateInsertion(*Loc, *OldOverloadDef));
       else
         return {}; // give up
     } else {
       // Adds the unsafe-buffer attribute (if not already there) to `FReDecl`:
       if (!FReDecl->hasAttr<UnsafeBufferUsageAttr>()) {
         FixIts.emplace_back(FixItHint::CreateInsertion(
             FReDecl->getBeginLoc(), getUnsafeBufferUsageAttributeText()));
       }
       // Inserts a declaration with the new signature to the end of `FReDecl`:
-      if (auto NewOverloadDecl =
-              NewOverloadSignatureCreator(FReDecl, ParmIdx, NewTyText))
+      if (auto NewOverloadDecl = NewOverloadSignatureCreator(FReDecl))
         FixIts.emplace_back(FixItHint::CreateInsertion(*Loc, *NewOverloadDecl));
       else
         return {};
     }
   }
   return FixIts;
 }
 
-// To fix a `ParmVarDecl` to be of `std::span` type.  In addition, creating a
-// new overload of the function so that the change is self-contained (see
-// `createOverloadsForFixedParams`).
-static FixItList fixParamWithSpan(const ParmVarDecl *PVD, const ASTContext &Ctx,
-                                  UnsafeBufferUsageHandler &Handler) {
+// To fix a `ParmVarDecl` to be of `std::span` type.
+static FixItList fixParamWithSpan(const ParmVarDecl *PVD,
+                                  const ASTContext &Ctx) {
+  //  FIXME: can this code be shared with local variable fix?
+  assert(PVD->getType()->isPointerType() &&
+         "Unexpected non-pointer type parameter for fix");
   if (PVD->hasDefaultArg())
     // FIXME: generate fix-its for default values:
     return {};
-  assert(PVD->getType()->isPointerType());
-  auto *FD = dyn_cast<FunctionDecl>(PVD->getDeclContext());
-
-  if (!FD)
-    return {};
 
   std::optional<Qualifiers> PteTyQualifiers = std::nullopt;
   std::optional<std::string> PteTyText = getPointeeTypeText(
       PVD, Ctx.getSourceManager(), Ctx.getLangOpts(), &PteTyQualifiers);
 
   if (!PteTyText)
-    return {};
+    return {}; // somehow failed to obtain pointee type text
 
   std::optional<StringRef> PVDNameText = PVD->getIdentifier()->getName();
 
   if (!PVDNameText)
     return {};
 
-  std::string SpanOpen = "std::span<";
-  std::string SpanClose = ">";
-  std::string SpanTyText;
   std::stringstream SS;
 
-  SS << SpanOpen << *PteTyText;
-  // Append qualifiers to span element type:
   if (PteTyQualifiers)
-    SS << " " << PteTyQualifiers->getAsString();
-  SS << SpanClose;
-  // Save the Span type text:
-  SpanTyText = SS.str();
+    // Append qualifiers if they exist:
+    SS << getSpanTypeText(*PteTyText, PteTyQualifiers);
+  else
+    SS << getSpanTypeText(*PteTyText);
   // Append qualifiers to the type of the parameter:
   if (PVD->getType().hasQualifiers())
-    SS << " " << PVD->getType().getQualifiers().getAsString();
+    SS << ' ' << PVD->getType().getQualifiers().getAsString();
   // Append parameter's name:
-  SS << " " << PVDNameText->str();
-
-  FixItList Fixes;
-  unsigned ParmIdx = 0;
-
-  Fixes.push_back(
-      FixItHint::CreateReplacement(PVD->getSourceRange(), SS.str()));
-  for (auto *ParmIter : FD->parameters()) {
-    if (PVD == ParmIter)
-      break;
-    ParmIdx++;
-  }
-  if (ParmIdx < FD->getNumParams())
-    if (auto OverloadFix = createOverloadsForFixedParams(ParmIdx, SpanTyText,
-                                                           FD, Ctx, Handler)) {
-      Fixes.append(*OverloadFix);
-      return Fixes;
-    }
-  return {};
+  SS << ' ' << PVDNameText->str();
+  // Add replacement fix-it:
+  return {FixItHint::CreateReplacement(PVD->getSourceRange(), SS.str())};
 }
 
 static FixItList fixVariableWithSpan(const VarDecl *VD,
                                      const DeclUseTracker &Tracker,
                                      ASTContext &Ctx,
                                      UnsafeBufferUsageHandler &Handler) {
   const DeclStmt *DS = Tracker.lookupDecl(VD);
   assert(DS && "Fixing non-local variables not implemented yet!");
if (FD->isMain() || FD->isConstexpr() ||
         FD->isOverloadedOperator())
       return {}; // TODO test all these cases
   }
 
   switch (K) {
   case Strategy::Kind::Span: {
     if (VD->getType()->isPointerType()) {
       if (const auto *PVD = dyn_cast<ParmVarDecl>(VD))
-        return fixParamWithSpan(PVD, Ctx, Handler);
+        return fixParamWithSpan(PVD, Ctx);
 
       if (VD->isLocalVarDecl())
         return fixVariableWithSpan(VD, Tracker, Ctx, Handler);
     }
     return {};
   }
   case Strategy::Kind::Iterator:
   case Strategy::Kind::Array:
return llvm::any_of(FixIts, [](const FixItHint &Hint) {
     auto Range = Hint.RemoveRange;
     if (Range.getBegin().isMacroID() || Range.getEnd().isMacroID())
       // If the range (or the first token) is (part of) a macro expansion:
       return true;
     return false;
   });
 }
 
-static bool impossibleToFixForVar(const FixableGadgetSets &FixablesForAllVars,
-                                  const Strategy &S,
-                                  const VarDecl * Var) {
-  for (const auto &F : FixablesForAllVars.byVar.find(Var)->second) {
-    std::optional<FixItList> Fixits = F->getFixits(S);
-    if (!Fixits) {
-      return true;
+// Returns true iff `VD` is a parameter of the declaration `D`:
+static bool isParameterOf(const VarDecl *VD, const Decl *D) {
+  return isa<ParmVarDecl>(VD) &&
+         VD->getDeclContext() == dyn_cast<DeclContext>(D);
+}
+
+// Erasing variables in `FixItsForVariable`, if such a variable has an unfixable
+// group mate.  A variable `v` is unfixable iff `FixItsForVariable` does not
+// contain `v`.
+static void eraseVarsForUnfixableGroupMates(
+    std::map<const VarDecl *, FixItList> &FixItsForVariable,
+    const VariableGroupsManager &VarGrpMgr) {
+  // Variables will be removed from `FixItsForVariable`:
+  SmallVector<const VarDecl *, 8> ToErase;
+
+  for (auto [VD, Ignore] : FixItsForVariable) {
+    VarGrpRef Grp = VarGrpMgr.getGroupOfVar(VD);
+    if (llvm::any_of(Grp,
+                     [&FixItsForVariable](const VarDecl *GrpMember) -> bool {
+                       return FixItsForVariable.find(GrpMember) ==
+                              FixItsForVariable.end();
+                     })) {
+      // At least one group member cannot be fixed, so we have to erase the
+      // whole group:
+      for (const VarDecl *Member : Grp)
+        ToErase.push_back(Member);
+    }
+  }
+  for (auto *VarToErase : ToErase)
+    FixItsForVariable.erase(VarToErase);
+}
+
+// Returns the fix-its that create bounds-safe function overloads for the
+// function `D`, if `D`'s parameters will be changed to safe-types through
+// fix-its in `FixItsForVariable`.
+//
+// NOTE: In case `D`'s parameters will be changed but bounds-safe function
+// overloads cannot created, the whole group that contains the parameters will
+// be erased from `FixItsForVariable`.
+static FixItList createFunctionOverloadsForParms(
+    std::map<const VarDecl *, FixItList> &FixItsForVariable /* mutable */,
+    const VariableGroupsManager &VarGrpMgr, const Decl *D, const Strategy &S,
+    ASTContext &Ctx, UnsafeBufferUsageHandler &Handler) {
+  FixItList FixItsSharedByParms{};
+
+  if (auto *FD = dyn_cast<FunctionDecl>(
+          D)) { // If `D` is not a `FunctionDecl`, no parameter will be fixed.
+    std::vector<bool> ParmsNeedFixMask(FD->getNumParams(), false);
+    const VarDecl *FirstParmNeedsFix = nullptr;
+
+    for (unsigned i = 0; i < FD->getNumParams(); i++)
+      if (FixItsForVariable.find(FD->getParamDecl(i)) !=
+          FixItsForVariable.end()) {
+        ParmsNeedFixMask[i] = true;
+        FirstParmNeedsFix = FD->getParamDecl(i);
+      }
+    if (FirstParmNeedsFix) {
+      // In case at least one parameter needs to be fixed:
+      std::optional<FixItList> OverloadFixes =
+          createOverloadsForFixedParams(ParmsNeedFixMask, S, FD, Ctx, Handler);
+
+      if (OverloadFixes) {
+        FixItsSharedByParms.append(*OverloadFixes);
+      } else {
+        // Something wrong in generating `OverloadFixes`, need to remove the
+        // whole group, where parameters are in, from `FixItsForVariable` (Note
+        // that all parameters should be in the same group):
+        for (auto *Member : VarGrpMgr.getGroupOfVar(FirstParmNeedsFix))
+          FixItsForVariable.erase(Member);
       }
     }
-  return false;
+  }
+  return FixItsSharedByParms;
 }
 
+// Constructs self-contained fix-its for each variable in `FixablesForAllVars`.
 static std::map<const VarDecl *, FixItList>
 getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S,
-    ASTContext &Ctx,
+          ASTContext &Ctx,
           /* The function decl under analysis */ const Decl *D,
           const DeclUseTracker &Tracker, UnsafeBufferUsageHandler &Handler,
-    const DefMapTy &VarGrpMap) {
+          const VariableGroupsManager &VarGrpMgr) {
+  // `FixItsForVariable` will map each variable to a set of fix-its directly
+  // associated to the variable itself.  Fix-its of distict variables in
+  // `FixItsForVariable` are disjoint.
   std::map<const VarDecl *, FixItList> FixItsForVariable;
+
+  // Populate `FixItsForVariable` with fix-its directly associated with each
+  // variable.  Fix-its directly associated to a variable 'v' are the ones
+  // produced by the `FixableGadget`s whose claimed variable is 'v'.
   for (const auto &[VD, Fixables] : FixablesForAllVars.byVar) {
     FixItsForVariable[VD] =
         fixVariable(VD, S.lookup(VD), D, Tracker, Ctx, Handler);
     // 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) {
       std::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);
-      continue;
-    }
-
-    const auto VarGroupForVD = VarGrpMap.find(VD);
-    if (VarGroupForVD != VarGrpMap.end()) {
-      for (const VarDecl * V : VarGroupForVD->second) {
-        if (V == VD) {
-          continue;
-        }
-        if (impossibleToFixForVar(FixablesForAllVars, S, V)) {
-          ImpossibleToFix = true;
-          break;
-        }
-      }
-
-      if (ImpossibleToFix) {
-        FixItsForVariable.erase(VD);
-        for (const VarDecl * V : VarGroupForVD->second) {
-          FixItsForVariable.erase(V);
-        }
-        continue;
-      }
-    }
+      if (Fixits) {
         FixItsForVariable[VD].insert(FixItsForVariable[VD].end(),
-                                 FixItsForVD.begin(), FixItsForVD.end());
-
-    // Fix-it shall not overlap with macros or/and templates:
-    if (overlapWithMacro(FixItsForVariable[VD]) ||
-        clang::internal::anyConflict(FixItsForVariable[VD],
-                                     Ctx.getSourceManager())) {
-      FixItsForVariable.erase(VD);
+                                     Fixits->begin(), Fixits->end());
         continue;
       }
+      FixItsForVariable.erase(VD);
+      break;
     }
-
-  for (auto VD : FixItsForVariable) {
-    const auto VarGroupForVD = VarGrpMap.find(VD.first);
-    const Strategy::Kind ReplacementTypeForVD = S.lookup(VD.first);
-    if (VarGroupForVD != VarGrpMap.end()) {
-      for (const VarDecl * Var : VarGroupForVD->second) {
-        if (Var == VD.first) {
-          continue;
-        }
-
-        FixItList GroupFix;
-        if (FixItsForVariable.find(Var) == FixItsForVariable.end()) {
-          GroupFix = fixVariable(Var, ReplacementTypeForVD, D,
-                                 Tracker, Var->getASTContext(), Handler);
-        } else {
-          GroupFix = FixItsForVariable[Var];
-        }
 
-        for (auto Fix : GroupFix) {
-          FixItsForVariable[VD.first].push_back(Fix);
-        }
-      }
-    }
-  }
-  return FixItsForVariable;
+  // `FixItsForVariable` now contains only variables that can be
+  // fixed. A variable can be fixed if its' declaration and all Fixables
+  // associated to it can all be fixed.
+
+  // To further remove from `FixItsForVariable` variables whose group mates
+  // cannot be fixed...
+  eraseVarsForUnfixableGroupMates(FixItsForVariable, VarGrpMgr);
+  // Now `FixItsForVariable` gets further reduced: a variable is in
+  // `FixItsForVariable` iff it can be fixed and all its group mates can be
+  // fixed.
+
+  // Fix-its of bounds-safe overloads of `D` are shared by parameters of `D`.
+  // That is,  when fixing multiple parameters in one step,  these fix-its will
+  // be applied only once (instead of being applied per parameter).
+  FixItList FixItsSharedByParms = createFunctionOverloadsForParms(
+      FixItsForVariable, VarGrpMgr, D, S, Ctx, Handler);
+
+  // `FinalFixItsForVariable` will map each variable 'v' to the self-contained
+  // set of fix-its for 'v', including:
+  // 1) fix-its directly associated to every group mate of 'v' (including 'v'
+  // itself); and
+  // 2) `FixItsSharedByParms`, if parameters are in the same group as 'v'.
+  std::map<const VarDecl *, FixItList> FinalFixItsForVariable;
+
+  for (auto [VD, Ignore] : FixItsForVariable) {
+    FixItList &FinalFixItsForVD = FinalFixItsForVariable[VD];
+    bool AnyParm = false;
+    VarGrpRef Grp = VarGrpMgr.getGroupOfVar(VD, &AnyParm);
+
+    for (auto *Member : Grp) {
+      FinalFixItsForVD.append(FixItsForVariable[Member]);
+    }
+    if (AnyParm)
+      FinalFixItsForVD.append(FixItsSharedByParms);
+  }
+  // Fix-its that will be applied in one step shall NOT:
+  // 1. overlap with macros or/and templates; or
+  // 2. conflicting each other.
+  // Otherwise, the fix-its will be dropped.
+  for (auto Iter = FinalFixItsForVariable.begin();
+       Iter != FinalFixItsForVariable.end();)
+    if (overlapWithMacro(Iter->second) ||
+        clang::internal::anyConflict(Iter->second, Ctx.getSourceManager())) {
+      Iter = FinalFixItsForVariable.erase(Iter);
+    } else
+      Iter++;
+  return FinalFixItsForVariable;
 }
 
-
 static Strategy
 getNaiveStrategy(const llvm::SmallVectorImpl<const VarDecl *> &UnsafeVars) {
   Strategy S;
   for (const VarDecl *VD : UnsafeVars) {
     S.set(VD, Strategy::Kind::Span);
   }
   return S;
 }
 
+//  Manages variable groups:
+class VariableGroupsManagerImpl : public VariableGroupsManager {
+  const std::vector<VarGrpTy> Groups;
+  const std::map<const VarDecl *, unsigned> &VarGrpMap;
+  const llvm::SetVector<const VarDecl *> &GrpsUnionForParms;
+
+public:
+  VariableGroupsManagerImpl(
+      const std::vector<VarGrpTy> &Groups,
+      const std::map<const VarDecl *, unsigned> &VarGrpMap,
+      const llvm::SetVector<const VarDecl *> &GrpsUnionForParms)
+      : Groups(Groups), VarGrpMap(VarGrpMap),
+        GrpsUnionForParms(GrpsUnionForParms) {}
+
+  VarGrpRef getGroupOfVar(const VarDecl *Var, bool *HasParm) const override {
+    if (GrpsUnionForParms.contains(Var)) {
+      if (HasParm)
+        *HasParm = true;
+      return GrpsUnionForParms.getArrayRef();
+    }
+    if (HasParm)
+      *HasParm = false;
+    if (VarGrpMap.find(Var) == VarGrpMap.end())
+      return std::nullopt;
+    return Groups[VarGrpMap.at(Var)];
+  }
+};
+
 void clang::checkUnsafeBufferUsage(const Decl *D,
                                    UnsafeBufferUsageHandler &Handler,
                                    bool EmitSuggestions) {
   assert(D && D->getBody());
 
   // Do not emit fixit suggestions for functions declared in an
   // extern "C" block.
   if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
assert(FixableGadgets.size() == 0 &&
            "Fixable gadgets found but suggestions not requested!");
     return;
   }
 
   UnsafeOps = groupWarningGadgetsByVar(std::move(WarningGadgets));
   FixablesForAllVars = groupFixablesByVar(std::move(FixableGadgets));
 
   std::map<const VarDecl *, FixItList> FixItsForVariableGroup;
-  DefMapTy VariableGroupsMap{};
 
   // Filter out non-local vars and vars with unclaimed DeclRefExpr-s.
   for (auto it = FixablesForAllVars.byVar.cbegin();
        it != FixablesForAllVars.byVar.cend();) {
       // FIXME: need to deal with global variables later
       if ((!it->first->isLocalVarDecl() && !isa<ParmVarDecl>(it->first)) ||
           Tracker.hasUnclaimedUses(it->first)) {
       it = FixablesForAllVars.byVar.erase(it);
     } else {
       ++it;
     }
   }
 
   llvm::SmallVector<const VarDecl *, 16> UnsafeVars;
   for (const auto &[VD, ignore] : FixablesForAllVars.byVar)
     UnsafeVars.push_back(VD);
 
   // Fixpoint iteration for pointer assignments
-  using DepMapTy = DenseMap<const VarDecl *, std::set<const VarDecl *>>;
+  using DepMapTy = DenseMap<const VarDecl *, llvm::SetVector<const VarDecl *>>;
   DepMapTy DependenciesMap{};
   DepMapTy PtrAssignmentGraph{};
 
   for (auto it : FixablesForAllVars.byVar) {
     for (const FixableGadget *fixable : it.second) {
       std::optional<std::pair<const VarDecl *, const VarDecl *>> ImplPair =
                                   fixable->getStrategyImplications();
       if (ImplPair) {
-        std::pair<const VarDecl *, const VarDecl *> Impl = ImplPair.value();
+        std::pair<const VarDecl *, const VarDecl *> Impl = std::move(*ImplPair);
         PtrAssignmentGraph[Impl.first].insert(Impl.second);
       }
     }
   }
 
   /*
    The following code does a BFS traversal of the `PtrAssignmentGraph`
    considering all unsafe vars as starting nodes and constructs an undirected
    graph `DependenciesMap`. Constructing the `DependenciesMap` in this manner
    elimiates all variables that are unreachable from any unsafe var. In other
    words, this removes all dependencies that don't include any unsafe variable
    and consequently don't need any fixit generation.
    Note: A careful reader would observe that the code traverses
    `PtrAssignmentGraph` using `CurrentVar` but adds edges between `Var` and
    `Adj` and not between `CurrentVar` and `Adj`. Both approaches would
    achieve the same result but the one used here dramatically cuts the
    amount of hoops the second part of the algorithm needs to jump, given that
    a lot of these connections become "direct". The reader is advised not to
    imagine how the graph is transformed because of using `Var` instead of
    `CurrentVar`. The reader can continue reading as if `CurrentVar` was used,
    and think about why it's equivalent later.
    */
   std::set<const VarDecl *> VisitedVarsDirected{};
   for (const auto &[Var, ignore] : UnsafeOps.byVar) {
     if (VisitedVarsDirected.find(Var) == VisitedVarsDirected.end()) {
 
       std::queue<const VarDecl*> QueueDirected{};
       QueueDirected.push(Var);
       while(!QueueDirected.empty()) {
         const VarDecl* CurrentVar = QueueDirected.front();
         QueueDirected.pop();
         VisitedVarsDirected.insert(CurrentVar);
         auto AdjacentNodes = PtrAssignmentGraph[CurrentVar];
         for (const VarDecl *Adj : AdjacentNodes) {
           if (VisitedVarsDirected.find(Adj) == VisitedVarsDirected.end()) {
             QueueDirected.push(Adj);
           }
           DependenciesMap[Var].insert(Adj);
           DependenciesMap[Adj].insert(Var);
         }
       }
     }
   }
 
+  // `Groups` stores the set of Connected Components in the graph.
+  std::vector<VarGrpTy> Groups;
+  // `VarGrpMap` maps variables that need fix to the groups (indexes) that the
+  // variables belong to.  Group indexes refer to the elements in `Groups`.
+  // `VarGrpMap` is complete in that every variable that needs fix is in it.
+  std::map<const VarDecl *, unsigned> VarGrpMap;
+  // The union group over the ones in "Groups" that contain parameters of `D`:
+  llvm::SetVector<const VarDecl *>
+      GrpsUnionForParms; // these variables need to be fixed in one step
+
   // Group Connected Components for Unsafe Vars
   // (Dependencies based on pointer assignments)
   std::set<const VarDecl *> VisitedVars{};
   for (const auto &[Var, ignore] : UnsafeOps.byVar) {
     if (VisitedVars.find(Var) == VisitedVars.end()) {
-      std::vector<const VarDecl *> VarGroup{};
-
+      VarGrpTy &VarGroup = Groups.emplace_back();
       std::queue<const VarDecl*> Queue{};
+
       Queue.push(Var);
       while(!Queue.empty()) {
         const VarDecl* CurrentVar = Queue.front();
         Queue.pop();
         VisitedVars.insert(CurrentVar);
         VarGroup.push_back(CurrentVar);
         auto AdjacentNodes = DependenciesMap[CurrentVar];
         for (const VarDecl *Adj : AdjacentNodes) {
           if (VisitedVars.find(Adj) == VisitedVars.end()) {
             Queue.push(Adj);
           }
         }
       }
-      for (const VarDecl * V : VarGroup) {
-        if (UnsafeOps.byVar.find(V) != UnsafeOps.byVar.end()) {
-          VariableGroupsMap[V] = VarGroup;
-        }
+
+      bool HasParm = false;
+      unsigned GrpIdx = Groups.size() - 1;
+
+      for (const VarDecl *V : VarGroup) {
+        VarGrpMap[V] = GrpIdx;
+        if (!HasParm && isParameterOf(V, D))
+          HasParm = true;
       }
+      if (HasParm)
+        GrpsUnionForParms.insert(VarGroup.begin(), VarGroup.end());
     }
   }
 
   Strategy NaiveStrategy = getNaiveStrategy(UnsafeVars);
+  VariableGroupsManagerImpl VarGrpMgr(Groups, VarGrpMap, GrpsUnionForParms);
 
   FixItsForVariableGroup =
       getFixIts(FixablesForAllVars, NaiveStrategy, D->getASTContext(), D,
-                Tracker, Handler, VariableGroupsMap);
-
-  // FIXME Detect overlapping FixIts.
+                Tracker, Handler, VarGrpMgr);
 
   for (const auto &G : UnsafeOps.noVar) {
     Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/false);
   }
 
   for (const auto &[VD, WarningGadgets] : UnsafeOps.byVar) {
     auto FixItsIt = FixItsForVariableGroup.find(VD);
-    Handler.handleUnsafeVariableGroup(VD, VariableGroupsMap,
+    bool BriefMsg = false; // Set to `true` if to NOT explain how variables are
+                           // grouped together
+    // If `Group` contains parameters, make the diag message brief:
+    VarGrpRef Group = VarGrpMgr.getGroupOfVar(VD, &BriefMsg);
+
+    Handler.handleUnsafeVariableGroup(VD, Group,
                                       FixItsIt != FixItsForVariableGroup.end()
                                           ? std::move(FixItsIt->second)
-                                      : FixItList{});
+                                          : FixItList{},
+                                      D, BriefMsg);
     for (const auto &G : WarningGadgets) {
       Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/true);
     }
   }
 }

clang/lib/Sema/AnalysisBasedWarnings.cpp

 // Unsafe buffer usage analysis.
 //===----------------------------------------------------------------------===//
 
 namespace {
 class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
   Sema &S;
   bool SuggestSuggestions;  // Recommend -fsafe-buffer-usage-suggestions?
 
+  // Lists as a string the names of variables in `VarGroupForVD` except for `VD`
+  // itself:
+  std::string listVariableGroupAsString(
+      const VarDecl *VD, const ArrayRef<const VarDecl *> &VarGroupForVD) const {
+    if (VarGroupForVD.size() <= 1)
+      return "";
+
+    std::vector<StringRef> VarNames;
+    auto PutInQuotes = [](StringRef S) -> std::string {
+      return "'" + S.str() + "'";
+    };
+
+    for (auto *V : VarGroupForVD) {
+      if (V == VD)
+        continue;
+      VarNames.push_back(V->getName());
+    }
+    if (VarNames.size() == 1) {
+      return PutInQuotes(VarNames[0]);
+    }
+    if (VarNames.size() == 2) {
+      return PutInQuotes(VarNames[0]) + " and " + PutInQuotes(VarNames[1]);
+    }
+    assert(VarGroupForVD.size() > 3);
+    const unsigned N = VarNames.size() -
+                       2; // need to print the last two names as "..., X, and Y"
+    std::string AllVars = "";
+
+    for (unsigned I = 0; I < N; ++I)
+      AllVars.append(PutInQuotes(VarNames[I]) + ", ");
+    AllVars.append(PutInQuotes(VarNames[N]) + ", and " +
+                   PutInQuotes(VarNames[N + 1]));
+    return AllVars;
+  }
+
 public:
   UnsafeBufferUsageReporter(Sema &S, bool SuggestSuggestions)
     : S(S), SuggestSuggestions(SuggestSuggestions) {}
 
   void handleUnsafeOperation(const Stmt *Operation,
                              bool IsRelatedToDecl) override {
     SourceLocation Loc;
     SourceRange Range;
void handleUnsafeOperation(const Stmt *Operation,
     } else {
       S.Diag(Loc, diag::warn_unsafe_buffer_operation) << MsgParam << Range;
       if (SuggestSuggestions) {
         S.Diag(Loc, diag::note_safe_buffer_usage_suggestions_disabled);
       }
     }
   }
 
-  void handleUnsafeVariableGroup(const VarDecl *Variable,
-                                 const DefMapTy &VarGrpMap,
-                             FixItList &&Fixes) override {
+  void handleUnsafeVariableGroup(const VarDecl *Variable, const VarGrpRef Group,
+                                 FixItList &&Fixes, const Decl *D,
+                                 bool BriefMsg) override {
     assert(!SuggestSuggestions &&
            "Unsafe buffer usage fixits displayed without suggestions!");
     S.Diag(Variable->getLocation(), diag::warn_unsafe_buffer_variable)
         << Variable << (Variable->getType()->isPointerType() ? 0 : 1)
         << Variable->getSourceRange();
     if (!Fixes.empty()) {
-      const auto VarGroupForVD = VarGrpMap.find(Variable)->second;
       unsigned FixItStrategy = 0; // For now we only have 'std::span' strategy
-      const auto &FD = S.Diag(Variable->getLocation(),
-                              diag::note_unsafe_buffer_variable_fixit_group);
+      const auto &FD =
+          S.Diag(Variable->getLocation(),
+                 BriefMsg ? diag::note_unsafe_buffer_variable_fixit_together
+                          : diag::note_unsafe_buffer_variable_fixit_group);
 
       FD << Variable << FixItStrategy;
-      std::string AllVars = "";
-      if (VarGroupForVD.size() > 1) {
-        if (VarGroupForVD.size() == 2) {
-          if (VarGroupForVD[0] == Variable) {
-            AllVars.append("'" + VarGroupForVD[1]->getName().str() + "'");
-          } else {
-            AllVars.append("'" + VarGroupForVD[0]->getName().str() + "'");
-          }
-        } else {
-          bool first = false;
-          if (VarGroupForVD.size() == 3) {
-            for (const VarDecl * V : VarGroupForVD) {
-              if (V == Variable) {
-                continue;
-              }
-              if (!first) {
-                first = true;
-                AllVars.append("'" + V->getName().str() + "'" + " and ");
-              } else {
-                AllVars.append("'" + V->getName().str() + "'");
-              }
-            }
-          } else {
-            for (const VarDecl * V : VarGroupForVD) {
-              if (V == Variable) {
-                continue;
-              }
-              if (VarGroupForVD.back() != V) {
-                AllVars.append("'" + V->getName().str() + "'" + ", ");
-              } else {
-                AllVars.append("and '" + V->getName().str() + "'");
-              }
-            }
-          }
-        }
-        FD << AllVars << 1;
-      } else {
-        FD << "" << 0;
-      }
-
+      if (Group.size() > 1) {
+        FD << listVariableGroupAsString(Variable, Group)
+           << /* print the group */ 1;
+        FD << "" << /* no group mate to print */ 0;
+      if (BriefMsg) {
+        assert(isa<FunctionDecl>(D) &&
+               "Fix for non-function declarations is not supported.");
+        FD << cast<FunctionDecl>(D)->getNameAsString();
       for (const auto &F : Fixes)
         FD << F;
     }
   }
 
   bool isSafeBufferOptOut(const SourceLocation &Loc) const override {
     return S.PP.isSafeBufferOptOut(S.getSourceManager(), Loc);
   }

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

+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits -fblocks -fsafe-buffer-usage-suggestions -verify %s
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits -fblocks -fsafe-buffer-usage-suggestions \
+// RUN:            %s 2>&1 | FileCheck %s
+
+// FIXME: what about possible diagnostic message non-determinism?
+
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE+1]]:
+void parmsNoFix(int *p, int *q) {
+  int * a = new int[10];
+  int * b = new int[10]; //expected-warning{{'b' is an unsafe pointer used for buffer access}} \
+         expected-note{{change type of 'b' to 'std::span' to preserve bounds information}}
+
+  a = p;
+  a = q;
+  b[5] = 5; // expected-note{{used in buffer access here}}
+}
+
+// CHECK: fix-it:{{.*}}:{[[@LINE+2]]:21-[[@LINE+2]]:27}:"std::span<int> p"
+// CHECK: fix-it:{{.*}}:{[[@LINE+14]]:2-[[@LINE+14]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid parmsSingleton(int *p) {return parmsSingleton(std::span<int>(p, <# size #>));}\n"
+void parmsSingleton(int *p) { //expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+              expected-note{{change type of 'p' to 'std::span' to preserve bounds information}}
+  // CHECK: fix-it:{{.*}}:{[[@LINE+3]]:3-[[@LINE+3]]:12}:"std::span<int> a"
+  // CHECK: fix-it:{{.*}}:{[[@LINE+2]]:13-[[@LINE+2]]:13}:"{"
+  // CHECK: fix-it:{{.*}}:{[[@LINE+1]]:24-[[@LINE+1]]:24}:", 10}"
+  int * a = new int[10];
+  // CHECK: fix-it:{{.*}}:{[[@LINE+1]]:3-[[@LINE+1]]:10}:"std::span<int> b"
+  int * b; //expected-warning{{'b' is an unsafe pointer used for buffer access}} \
+       expected-note{{change type of 'b' to 'std::span' to preserve bounds information, and change 'a' to 'std::span' to propagate bounds information between them}}
+
+  b = a;
+  b[5] = 5; // expected-note{{used in buffer access here}}
+  p[5] = 5; // expected-note{{used in buffer access here}}
+}
+
+
+// Parameters other than `r` all will be fixed
+// CHECK: fix-it:{{.*}}:{[[@LINE+3]]:24-[[@LINE+3]]:30}:"std::span<int> p"
+// CHECK: fix-it:{{.*}}:{[[@LINE+2]]:32-[[@LINE+2]]:39}:"std::span<int *> q"
+// CHECK: fix-it:{{.*}}:{[[@LINE+1]]:41-[[@LINE+1]]:48}:"std::span<int> a"
+void * multiParmAllFix(int *p, int **q, int a[], int * r) { // expected-warning{{'p' is an unsafe pointer used for buffer access}}   expected-warning{{'q' is an unsafe pointer used for buffer access}} \
+   expected-warning{{'a' is an unsafe pointer used for buffer access}} \
+   expected-note{{change type of 'p' to 'std::span' to preserve bounds information, and change 'q' and 'a' to safe types to make function 'multiParmAllFix' bounds-safe}} \
+   expected-note{{change type of 'q' to 'std::span' to preserve bounds information, and change 'p' and 'a' to safe types to make function 'multiParmAllFix' bounds-safe}} \
+   expected-note{{change type of 'a' to 'std::span' to preserve bounds information, and change 'p' and 'q' to safe types to make function 'multiParmAllFix' bounds-safe}}
+  int tmp;
+
+  tmp = p[5]; // expected-note{{used in buffer access here}}
+  tmp = a[5]; // expected-note{{used in buffer access here}}
+  if (++q) {} // expected-note{{used in pointer arithmetic here}}
+  return r;
+}
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid * multiParmAllFix(int *p, int **q, int a[], int * r) {return multiParmAllFix(std::span<int>(p, <# size #>), std::span<int *>(q, <# size #>), std::span<int>(a, <# size #>), r);}\n"
+
+void * multiParmAllFix(int *p, int **q, int a[], int * r);
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:1-[[@LINE-1]]:1}:"{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\n"
+// CHECK: fix-it:{{.*}}:{[[@LINE-2]]:58-[[@LINE-2]]:58}:";\nvoid * multiParmAllFix(std::span<int> p, std::span<int *> q, std::span<int> a, int * r)"
+
+// Fixing local variables implicates fixing parameters
+void  multiParmLocalAllFix(int *p, int * r) {
+  // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:28-[[@LINE-1]]:34}:"std::span<int> p"
+  // CHECK: fix-it:{{.*}}:{[[@LINE-2]]:36-[[@LINE-2]]:43}:"std::span<int> r"
+  // CHECK: fix-it:{{.*}}:{[[@LINE+1]]:3-[[@LINE+1]]:10}:"std::span<int> x"
+  int * x; // expected-warning{{'x' is an unsafe pointer used for buffer access}} \
+        expected-note{{change type of 'x' to 'std::span' to preserve bounds information, and change 'p', 'z', and 'r' to safe types to make function 'multiParmLocalAllFix' bounds-safe}}
+  // CHECK: fix-it:{{.*}}:{[[@LINE+1]]:3-[[@LINE+1]]:10}:"std::span<int> z"
+  int * z; // expected-warning{{'z' is an unsafe pointer used for buffer access}} \
+              expected-note{{change type of 'z' to 'std::span' to preserve bounds information, and change 'x', 'p', and 'r' to safe types to make function 'multiParmLocalAllFix' bounds-safe}}
+  int * y;
+
+  x = p;
+  y = x;
+  // `x` needs to be fixed so does the pointer assigned to `x`, i.e.,`p`
+  x[5] = 5; // expected-note{{used in buffer access here}}
+  z = r;
+  // `z` needs to be fixed so does the pointer assigned to `z`, i.e.,`r`
+  z[5] = 5; // expected-note{{used in buffer access here}}
+  // Since `p` and `r` are parameters need to be fixed together,
+  // fixing `x` involves fixing all `p`, `r` and `z`. Similar for
+  // fixing `z`.
+}
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid  multiParmLocalAllFix(int *p, int * r) {return multiParmLocalAllFix(std::span<int>(p, <# size #>), std::span<int>(r, <# size #>));}\n"
+
+
+// Fixing parameters implicates fixing local variables
+// CHECK: fix-it:{{.*}}:{[[@LINE+2]]:29-[[@LINE+2]]:35}:"std::span<int> p"
+// CHECK: fix-it:{{.*}}:{[[@LINE+1]]:37-[[@LINE+1]]:44}:"std::span<int> r"
+void  multiParmLocalAllFix2(int *p, int * r) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+                                                  expected-note{{change type of 'p' to 'std::span' to preserve bounds information, and change 'x', 'r', and 'z' to safe types to make function 'multiParmLocalAllFix2' bounds-safe}} \
+                                                  expected-warning{{'r' is an unsafe pointer used for buffer access}} \
+                    expected-note{{change type of 'r' to 'std::span' to preserve bounds information, and change 'p', 'x', and 'z' to safe types to make function 'multiParmLocalAllFix2' bounds-safe}}
+  int * x = new int[10];
+  // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> x"
+  // CHECK: fix-it:{{.*}}:{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
+  // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}"
+  int * z = new int[10];
+  // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> z"
+  // CHECK: fix-it:{{.*}}:{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
+  // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}"
+  int * y;
+
+  p = x;
+  y = x;
+  p[5] = 5; // expected-note{{used in buffer access here}}
+  r = z;
+  r[5] = 5; // expected-note{{used in buffer access here}}
+}
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid  multiParmLocalAllFix2(int *p, int * r) {return multiParmLocalAllFix2(std::span<int>(p, <# size #>), std::span<int>(r, <# size #>));}\n"
+
+
+// No fix emitted for any of the parameter since parameter `r` cannot be fixed
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE+1]]
+void noneParmFix(int * p, int * q, int * r) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+                   expected-warning{{'q' is an unsafe pointer used for buffer access}} \
+                   expected-warning{{'r' is an unsafe pointer used for buffer access}}
+  int tmp = p[5]; // expected-note{{used in buffer access here}}
+  tmp = q[5];     // expected-note{{used in buffer access here}}
+  r++;            // expected-note{{used in pointer arithmetic here}}
+  tmp = r[5];     // expected-note{{used in buffer access here}}
+}
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]]
+
+// To show what if the `r` in `noneParmFix` can be fixed:
+void noneParmFix_control(int * p, int * q, int * r) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+                     expected-note{{change type of 'p' to 'std::span' to preserve bounds information, and change 'q' and 'r' to safe types to make function 'noneParmFix_control' bounds-safe}} \
+                           expected-warning{{'q' is an unsafe pointer used for buffer access}} \
+                     expected-note{{change type of 'q' to 'std::span' to preserve bounds information, and change 'p' and 'r' to safe types to make function 'noneParmFix_control' bounds-safe}} \
+                           expected-warning{{'r' is an unsafe pointer used for buffer access}} \
+                     expected-note{{change type of 'r' to 'std::span' to preserve bounds information, and change 'p' and 'q' to safe types to make function 'noneParmFix_control' bounds-safe}}
+  int tmp = p[5]; // expected-note{{used in buffer access here}}
+  tmp = q[5];     // expected-note{{used in buffer access here}}
+  if (++r) {}     // expected-note{{used in pointer arithmetic here}}
+  tmp = r[5];     // expected-note{{used in buffer access here}}
+}
+
+
+// No fix emitted for any of the parameter since local variable `l` cannot be fixed.
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE+1]]
+void noneParmOrLocalFix(int * p, int * q, int * r) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+                    expected-warning{{'q' is an unsafe pointer used for buffer access}} \
+                    expected-warning{{'r' is an unsafe pointer used for buffer access}}
+  int tmp = p[5];  // expected-note{{used in buffer access here}}
+  tmp = q[5];      // expected-note{{used in buffer access here}}
+  tmp = r[5];      // expected-note{{used in buffer access here}}
+  // `l` and `r` must be fixed together while all parameters must be fixed together as well:
+  int * l; l = r;     // expected-warning{{'l' is an unsafe pointer used for buffer access}}
+
+  l++;             // expected-note{{used in pointer arithmetic here}}
+}
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]]
+
+// To show what if the `l` can be fixed in `noneParmOrLocalFix`:
+void noneParmOrLocalFix_control(int * p, int * q, int * r) {// \
+  expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+  expected-note{{change type of 'p' to 'std::span' to preserve bounds information, and change 'q', 'r', and 'l' to safe types to make function 'noneParmOrLocalFix_control' bounds-safe}} \
+  expected-warning{{'q' is an unsafe pointer used for buffer access}} \
+  expected-note{{change type of 'q' to 'std::span' to preserve bounds information, and change 'p', 'r', and 'l' to safe types to make function 'noneParmOrLocalFix_control' bounds-safe}} \
+  expected-warning{{'r' is an unsafe pointer used for buffer access}} \
+  expected-note{{change type of 'r' to 'std::span' to preserve bounds information, and change 'p', 'q', and 'l' to safe types to make function 'noneParmOrLocalFix_control' bounds-safe}}
+  int tmp = p[5];  // expected-note{{used in buffer access here}}
+  tmp = q[5];      // expected-note{{used in buffer access here}}
+  tmp = r[5];      // expected-note{{used in buffer access here}}
+  int * l;         // expected-warning{{'l' is an unsafe pointer used for buffer access}} \
+          expected-note{{change type of 'l' to 'std::span' to preserve bounds information, and change 'p', 'q', and 'r' to safe types to make function 'noneParmOrLocalFix_control' bounds-safe}}
+  l = r;
+  if (++l){};         // expected-note{{used in pointer arithmetic here}}
+}
+
+
+// No fix emitted for any of the parameter since local variable `l` cannot be fixed.
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE+1]]
+void noneParmOrLocalFix2(int * p, int * q, int * r) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+                     expected-warning{{'q' is an unsafe pointer used for buffer access}} \
+                     expected-warning{{'r' is an unsafe pointer used for buffer access}}
+  int tmp = p[5]; // expected-note{{used in buffer access here}}
+  tmp = q[5];     // expected-note{{used in buffer access here}}
+  tmp = r[5];     // expected-note{{used in buffer access here}}
+
+  int * a; a = r;
+  int * b; b = a;
+  int * l; l = b;    // expected-warning{{'l' is an unsafe pointer used for buffer access}}
+
+  // `a, b, l` and parameters must be fixed together but `l` can't be fixed:
+  l++;               // expected-note{{used in pointer arithmetic here}}
+}
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]]
+
+// To show what if the `l` can be fixed in `noneParmOrLocalFix2`:
+void noneParmOrLocalFix2_control(int * p, int * q, int * r) { // \
+  expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+  expected-note{{change type of 'p' to 'std::span' to preserve bounds information, and change 'q', 'r', 'l', 'b', and 'a' to safe types to make function 'noneParmOrLocalFix2_control' bounds-safe}}                 \
+  expected-warning{{'q' is an unsafe pointer used for buffer access}} \
+  expected-note{{change type of 'q' to 'std::span' to preserve bounds information, and change 'p', 'r', 'l', 'b', and 'a' to safe types to make function 'noneParmOrLocalFix2_control' bounds-safe}}                 \
+  expected-warning{{'r' is an unsafe pointer used for buffer access}} \
+  expected-note{{change type of 'r' to 'std::span' to preserve bounds information, and change 'p', 'q', 'l', 'b', and 'a' to safe types to make function 'noneParmOrLocalFix2_control' bounds-safe}}
+  int tmp = p[5]; // expected-note{{used in buffer access here}}
+  tmp = q[5];     // expected-note{{used in buffer access here}}
+  tmp = r[5];     // expected-note{{used in buffer access here}}
+
+  int * a; a = r;
+  int * b; b = a;
+  int * l;  // expected-warning{{'l' is an unsafe pointer used for buffer access}} \
+         expected-note{{change type of 'l' to 'std::span' to preserve bounds information, and change 'p', 'q', 'r', 'b', and 'a' to safe types to make function 'noneParmOrLocalFix2_control' bounds-safe}}
+
+  l = b;
+  if(++l){} // expected-note{{used in pointer arithmetic here}}
+}
+
+// No fix emitted for any of the parameter since local variable `l` cannot be fixed
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE+1]]
+void noneParmOrLocalFix3(int * p, int * q, int * r) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+                     expected-warning{{'q' is an unsafe pointer used for buffer access}} \
+                     expected-warning{{'r' is an unsafe pointer used for buffer access}}
+  int tmp = p[5];  // expected-note{{used in buffer access here}}
+  tmp = q[5];      // expected-note{{used in buffer access here}}
+  tmp = r[5];      // expected-note{{used in buffer access here}}
+
+  int * a; a = r;
+  int * b; b = a;
+  int * l; l = b;     // expected-warning{{'l' is an unsafe pointer used for buffer access}}
+
+  l++;             // expected-note{{used in pointer arithmetic here}}
+
+  int * x; x = p; // expected-warning{{'x' is an unsafe pointer used for buffer access}}
+  tmp = x[5];  // expected-note{{used in buffer access here}}
+}
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]]
+
+void noneParmOrLocalFix3_control(int * p, int * q, int * r) { // \
+     expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+     expected-note{{change type of 'p' to 'std::span' to preserve bounds information, and change 'x', 'q', 'r', 'l', 'b', and 'a' to safe types to make function 'noneParmOrLocalFix3_control' bounds-safe}}            \
+     expected-warning{{'q' is an unsafe pointer used for buffer access}} \
+     expected-note{{change type of 'q' to 'std::span' to preserve bounds information, and change 'p', 'x', 'r', 'l', 'b', and 'a' to safe types to make function 'noneParmOrLocalFix3_control' bounds-safe}}            \
+     expected-warning{{'r' is an unsafe pointer used for buffer access}} \
+     expected-note{{change type of 'r' to 'std::span' to preserve bounds information, and change 'p', 'x', 'q', 'l', 'b', and 'a' to safe types to make function 'noneParmOrLocalFix3_control' bounds-safe}}
+  int tmp = p[5];  // expected-note{{used in buffer access here}}
+  tmp = q[5];      // expected-note{{used in buffer access here}}
+  tmp = r[5];      // expected-note{{used in buffer access here}}
+
+  int * a; a = r;
+  int * b; b = a;
+  int * l;         // expected-warning{{'l' is an unsafe pointer used for buffer access}}   \
+          expected-note{{change type of 'l' to 'std::span' to preserve bounds information, and change 'p', 'x', 'q', 'r', 'b', and 'a' to safe types to make function 'noneParmOrLocalFix3_control' bounds-safe}}
+
+  l = b;
+  if (++l){};         // expected-note{{used in pointer arithmetic here}}
+
+  int * x;            // expected-warning{{'x' is an unsafe pointer used for buffer access}} \
+             expected-note{{change type of 'x' to 'std::span' to preserve bounds information, and change 'p', 'q', 'r', 'l', 'b', and 'a' to safe types to make function 'noneParmOrLocalFix3_control' bounds-safe}}
+  x = p;
+  tmp = x[5];  // expected-note{{used in buffer access here}}
+}
+
+
+// No fix emitted for any of the parameter but some local variables will get fixed
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE+1]]
+void noneParmSomeLocalFix(int * p, int * q, int * r) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+                      expected-warning{{'q' is an unsafe pointer used for buffer access}} \
+                      expected-warning{{'r' is an unsafe pointer used for buffer access}}
+  int tmp = p[5];  // expected-note{{used in buffer access here}}
+  tmp = q[5];      // expected-note{{used in buffer access here}}
+  tmp = r[5];      // expected-note{{used in buffer access here}}
+
+  int * a; a = r;
+  int * b; b = a;
+  int * l; l = b; // expected-warning{{'l' is an unsafe pointer used for buffer access}}
+
+  l++; // expected-note{{used in pointer arithmetic here}}
+
+  //`x` and `y` can be fixed
+  int * x = new int[10];
+  // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> x"
+  // CHECK: fix-it:{{.*}}:{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
+  // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}"
+  // CHECK: fix-it:{{.*}}:{[[@LINE+1]]:3-[[@LINE+1]]:10}:"std::span<int> y"
+  int * y;   // expected-warning{{'y' is an unsafe pointer used for buffer access}} \
+          expected-note{{change type of 'y' to 'std::span' to preserve bounds information, and change 'x' to 'std::span' to propagate bounds information between them}}
+  y = x;
+  tmp = y[5];  // expected-note{{used in buffer access here}}
+}
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]]
+
+// Test that other parameters of (lambdas and blocks) do not interfere
+// the grouping of variables of the function.
+// CHECK: fix-it:{{.*}}:{[[@LINE+3]]:30-[[@LINE+3]]:37}:"std::span<int> p"
+// CHECK: fix-it:{{.*}}:{[[@LINE+2]]:39-[[@LINE+2]]:46}:"std::span<int> q"
+// CHECK: fix-it:{{.*}}:{[[@LINE+20]]:2-[[@LINE+20]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid parmsFromLambdaAndBlock(int * p, int * q) {return parmsFromLambdaAndBlock(std::span<int>(p, <# size #>), std::span<int>(q, <# size #>));}\n"
+void parmsFromLambdaAndBlock(int * p, int * q) {
+  // CHECK: fix-it:{{.*}}:{[[@LINE+1]]:3-[[@LINE+1]]:10}:"std::span<int> a"
+  int * a; // expected-warning{{'a' is an unsafe pointer used for buffer access}} \
+        expected-note{{change type of 'a' to 'std::span' to preserve bounds information, and change 'p', 'b', and 'q' to safe types to make function 'parmsFromLambdaAndBlock' bounds-safe}}
+  // CHECK: fix-it:{{.*}}:{[[@LINE+1]]:3-[[@LINE+1]]:10}:"std::span<int> b"
+  int * b; // expected-warning{{'b' is an unsafe pointer used for buffer access}} \
+              expected-note{{change type of 'b' to 'std::span' to preserve bounds information, and change 'a', 'p', and 'q' to safe types to make function 'parmsFromLambdaAndBlock' bounds-safe}}
+  auto Lamb = [](int * x) -> void { // expected-warning{{'x' is an unsafe pointer used for buffer access}}
+    x[5] = 5;                       // expected-note{{used in buffer access here}}
+  };
+
+  void (^Blk)(int*) = ^(int * y) {  // expected-warning{{'y' is an unsafe pointer used for buffer access}}
+    y[5] = 5;                       // expected-note{{used in buffer access here}}
+  };
+
+  a = p;
+  b = q;
+  a[5] = 5; // expected-note{{used in buffer access here}}
+  b[5] = 5; // expected-note{{used in buffer access here}}
+}

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

 
 
 void twoParms(int *p, int * q) {
   // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:15-[[@LINE-1]]:21}:"std::span<int> p"
   // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:23-[[@LINE-2]]:30}:"std::span<int> q"
   int tmp;
   tmp = p[5] + q[5];
 }
-// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid twoParms(int *p, int * q) {return twoParms(std::span<int>(p, <# size #>), q);}\n"
-// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:2-[[@LINE-2]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid twoParms(int *p, int * q) {return twoParms(p, std::span<int>(q, <# size #>));}\n"
+// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid twoParms(int *p, int * q) {return twoParms(std::span<int>(p, <# size #>), std::span<int>(q, <# size #>));}\n"
 
 void ptrToConst(const int * x) {
   // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:17-[[@LINE-1]]:30}:"std::span<int const> x"
   int tmp = x[5];
 }
 // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid ptrToConst(const int * x) {return ptrToConst(std::span<int const>(x, <# size #>));}\n"
 
 // The followings test cases where multiple FileIDs maybe involved
namespace {
   struct {
     // anon
   } ANON_S;
 
   struct MyStruct {
     // namned
   } NAMED_S;
 
+
   // FIXME: `decltype(ANON_S)` represents an unnamed type but it can
   // be referred as "`decltype(ANON_S)`", so the analysis should
   // fix-it.
-  void decltypeSpecifier(decltype(C) * p, decltype(ANON_S) * q, decltype(NAMED_S) * r,
-                         decltype(NAMED_S) ** rr) {
-    // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:26-[[@LINE-2]]:41}:"std::span<decltype(C)> p"
-    // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-3]]:65-[[@LINE-3]]:86}:"std::span<decltype(NAMED_S)> r"
-    // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-3]]:26-[[@LINE-3]]:49}:"std::span<decltype(NAMED_S) *> rr"
+  // As parameter `q` cannot be fixed, fixes to parameters are all being given up.
+  void decltypeSpecifierAnon(decltype(C) * p, decltype(ANON_S) * q, decltype(NAMED_S) * r,
+           decltype(NAMED_S) ** rr) {
+    // CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]]
     if (++p) {}
     if (++q) {}
     if (++r) {}
     if (++rr) {}
   }
-  // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:4-[[@LINE-1]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid decltypeSpecifier(decltype(C) * p, decltype(ANON_S) * q, decltype(NAMED_S) * r,\n{{.*}}decltype(NAMED_S) ** rr) {return decltypeSpecifier(std::span<decltype(C)>(p, <# size #>), q, r, rr);}\n
-  // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:4-[[@LINE-2]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid decltypeSpecifier(decltype(C) * p, decltype(ANON_S) * q, decltype(NAMED_S) * r,\n{{.*}}decltype(NAMED_S) ** rr) {return decltypeSpecifier(p, q, std::span<decltype(NAMED_S)>(r, <# size #>), rr);}\n"
-  // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-3]]:4-[[@LINE-3]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid decltypeSpecifier(decltype(C) * p, decltype(ANON_S) * q, decltype(NAMED_S) * r,\n{{.*}}decltype(NAMED_S) ** rr) {return decltypeSpecifier(p, q, r, std::span<decltype(NAMED_S) *>(rr, <# size #>));}\n"
+  // CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]]
+
+  void decltypeSpecifier(decltype(C) * p, decltype(NAMED_S) * r, decltype(NAMED_S) ** rr) {
+    // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:26-[[@LINE-1]]:41}:"std::span<decltype(C)> p"
+    // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:43-[[@LINE-2]]:64}:"std::span<decltype(NAMED_S)> r"
+    // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-3]]:66-[[@LINE-3]]:89}:"std::span<decltype(NAMED_S) *> rr"
+    if (++p) {}
+    if (++r) {}
+    if (++rr) {}
+  }
+  // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:4-[[@LINE-1]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid decltypeSpecifier(decltype(C) * p, decltype(NAMED_S) * r, decltype(NAMED_S) ** rr) {return decltypeSpecifier(std::span<decltype(C)>(p, <# size #>), std::span<decltype(NAMED_S)>(r, <# size #>), std::span<decltype(NAMED_S) *>(rr, <# size #>));}\n
 
 #define MACRO_TYPE(T) long T
 
   void macroType(unsigned MACRO_TYPE(int) * p, unsigned MACRO_TYPE(long) * q) {
     // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:18-[[@LINE-1]]:46}:"std::span<unsigned MACRO_TYPE(int)> p"
     // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:48-[[@LINE-2]]:77}:"std::span<unsigned MACRO_TYPE(long)> q"
     int tmp = p[5];
     tmp = q[5];
   }
-  // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:4-[[@LINE-1]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid macroType(unsigned MACRO_TYPE(int) * p, unsigned MACRO_TYPE(long) * q) {return macroType(std::span<unsigned MACRO_TYPE(int)>(p, <# size #>), q);}\n"
-  // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:4-[[@LINE-2]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid macroType(unsigned MACRO_TYPE(int) * p, unsigned MACRO_TYPE(long) * q) {return macroType(p, std::span<unsigned MACRO_TYPE(long)>(q, <# size #>));}\n"
+  // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:4-[[@LINE-1]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid macroType(unsigned MACRO_TYPE(int) * p, unsigned MACRO_TYPE(long) * q) {return macroType(std::span<unsigned MACRO_TYPE(int)>(p, <# size #>), std::span<unsigned MACRO_TYPE(long)>(q, <# size #>));}\n"
 }
 
 // The followings test various declarators:
 void decayedArray(int a[]) {
   // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:19-[[@LINE-1]]:26}:"std::span<int> a"
   int tmp;
   tmp = a[5];
 }

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

void testIncrement(char *p) { // expected-warning{{'p' is an unsafe pointer used for buffer access}}
   p++; // expected-note{{used in pointer arithmetic here}}
   --p; // expected-note{{used in pointer arithmetic here}}
   p--; // expected-note{{used in pointer arithmetic here}}
 }
 
 void * voidPtrCall(void);
 char * charPtrCall(void);
 
-void testArraySubscripts(int *p, int **pp) { // expected-note{{change type of 'pp' to 'std::span' to preserve bounds information}}
+void testArraySubscripts(int *p, int **pp) {
 // expected-warning@-1{{'p' is an unsafe pointer used for buffer access}}
 // expected-warning@-2{{'pp' is an unsafe pointer used for buffer access}}
   foo(p[1],             // expected-note{{used in buffer access here}}
       pp[1][1],         // expected-note{{used in buffer access here}}
                         // expected-warning@-1{{unsafe buffer access}}
       1[1[pp]],         // expected-note{{used in buffer access here}}
                         // expected-warning@-1{{unsafe buffer access}}
       1[pp][1]          // expected-note{{used in buffer access here}}
void testArraySubscriptsWithAuto(int *p, int **pp) {
   foo(ap4[1]);    // expected-note{{used in buffer access here}}
 }
 
 void testUnevaluatedContext(int * p) {// no-warning
   foo(sizeof(p[1]),             // no-warning
       sizeof(decltype(p[1])));  // no-warning
 }
 
-// expected-note@+1{{change type of 'a' to 'std::span' to preserve bounds information}}
 void testQualifiedParameters(const int * p, const int * const q, const int a[10], const int b[10][10]) {
   // expected-warning@-1{{'p' is an unsafe pointer used for buffer access}}
   // expected-warning@-2{{'q' is an unsafe pointer used for buffer access}}
   // expected-warning@-3{{'a' is an unsafe pointer used for buffer access}}
   // expected-warning@-4{{'b' is an unsafe pointer used for buffer access}}
 
   foo(p[1], 1[p], p[-1],    // expected-note3{{used in buffer access here}}
       q[1], 1[q], q[-1],    // expected-note3{{used in buffer access here}}