[-Wunsafe-buffer-usage] Fix-Its transforming `&DRE[any]` to `&DRE.data()[any]`

[ziqingluo-90]

Summary

For an expression of the form &DRE[any-expr] under an Unspecified Pointer Context (UPC), we generate a fix-it for it with respect to a strategy. In case the strategy is std::span (it is the only supported one for now), the fix-it replaces the expression with &DRE.data()[any-expr].

A UPC includes at least the contexts where

• the expression is being casted to an integer; and
• the expression is an argument of a call to a function that is not marked unsafe.

[jkorous]

clang/lib/Analysis/UnsafeBufferUsage.cpp

155

I am just wondering how does the callee matcher work in situation with multiple re-declarations 🤔

Something like this:

void foo(int* ptr);
[[clang::unsafe_buffer_usage]] void foo(int* ptr);
void foo(int* ptr);

void bar(int* ptr) {
  foo(ptr);
}

501

I am wondering what will happen in the weird corner-case of &5[ptr] - I feel the Fix-It we produce would be incorrect.

Here's a suggestion - we could use hasLHS instead of hasBase here and add a FIXME that when we find the time we should also properly support the corner-case. That would be a pretty low-priority though - we definitely have more important patterns to support first.

WDYT?

834

Since we use std::nullopt in getFixits to signal errors - we should either use the same strategy in fixUPCAddressofArraySubscriptWithSpan or translate the empty return value from it to nullopt here.
(FWIWI I am leaning towards the former.)
Forwarding the empty Fix-It would be incorrect.

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

501

I'm not sure if I understand your concern. For &5[ptr], we will generate a fix-it ptr.data() + 5 in cases ptr is assigned a span strategy. It is same as the case of &ptr[5].

834

Oh, that's a bug I made! Thank you for finding it for me.

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

155

I think we are fine. According to the doc of FunctionDecl:

/// Represents a function declaration or definition.
///
/// Since a given function can be declared several times in a program,
/// there may be several FunctionDecls that correspond to that
/// function. Only one of those FunctionDecls will be found when
/// traversing the list of declarations in the context of the
/// FunctionDecl (e.g., the translation unit); this FunctionDecl
/// contains all of the information known about the function. Other,
/// previous declarations of the function are available via the
/// getPreviousDecl() chain.

[NoQ]

Why do we prefer DRE.data() + any to &DRE.data()[any]? It could be much less intrusive this way, and the safety guarantees are the same.

[ziqingluo-90]

In D143128#4108375, @NoQ wrote:

Why do we prefer DRE.data() + any to &DRE.data()[any]? It could be much less intrusive this way, and the safety guarantees are the same.

It is actually (DRE.data() + any) versus &DRE.data()[any]. Are they quite the same in terms of being intrusive?

[ziqingluo-90]

Let fixUPCAddressofArraySubscriptWithSpan return std::nullopt instead of an empty list when we should give up on the fix-it.

Add a few test cases for some corner cases.

[jkorous]

clang/lib/Analysis/UnsafeBufferUsage.cpp

155

I see! Sound like we should be fine indeed and the test seems to confirm.
Thank you!

501

Oh, my bad! I assumed (AKA didn't check) that we're just replacing the parts of the code around the DRE and index.
You're right. Please ignore me :)

[ziqingluo-90]

Rebased.

[ziqingluo-90]

Now we have different FixableGadgets that may match the same Stmt (representing a context).
So in order to discover all "Fixable"s, we can no longer match anyOf FixableGadgets' matchers. Instead, we match eachOf them.

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

678

Change from anyOf to eachOf

[NoQ]

In D143128#4108502, @ziqingluo-90 wrote:

In D143128#4108375, @NoQ wrote:

Why do we prefer DRE.data() + any to &DRE.data()[any]? It could be much less intrusive this way, and the safety guarantees are the same.

It is actually (DRE.data() + any) versus &DRE.data()[any]. Are they quite the same in terms of being intrusive?

They may be equally verbose but I think the latter looks much more similar to the original code in terms of shape. This could be valuable because there's probably a reason why the programmer preferred to write it that way. Or even if there wasn't any reason, the users still may find it surprising that we change the shape for no apparent reason.

[jkorous]

This is an interesting topic. In the abstract I see the question as: "Should the Fix-Its prioritize how the code will fit the desired end state (presumably modern idiomatic C++) or carefully respect the state of the code as is now?"

The only thing I feel pretty strongly about is that no matter what philosophy we decide to use here we should apply it consistently to all our Fix-Its (which might or might not already be the case).

And FWIW I can also imagine at some point in the future we might either have two dialects of the Fix-Its or that a separate modernizer tool (completely independent of Safe Buffers) could suggest transformations like:
"Would you like to change &DRE.data()[any] to (DRE.data() + any)?"

[ymandel]

In D143128#4167626, @jkorous wrote:

This is an interesting topic. In the abstract I see the question as: "Should the Fix-Its prioritize how the code will fit the desired end state (presumably modern idiomatic C++) or carefully respect the state of the code as is now?"

The only thing I feel pretty strongly about is that no matter what philosophy we decide to use here we should apply it consistently to all our Fix-Its (which might or might not already be the case).

And FWIW I can also imagine at some point in the future we might either have two dialects of the Fix-Its or that a separate modernizer tool (completely independent of Safe Buffers) could suggest transformations like:
"Would you like to change &DRE.data()[any] to (DRE.data() + any)?"

Fantastic topic! :) In our experience at Google, we've generally followed the philosophy of leaving the code at least as good as it was before we touched it. So, if the idiom is archaic, but we're just adjusting it, that's fine (as in this example), but our tools shouldn't generate non-idiomatic (or anti-idiomic) code. We've also often taken the path of "leave cleanups to a separate pass", especially when we already have say, a clang tidy check, that does that kind of clean up running regularly. But, this one is more a judgment call. I'd probably lean towards "make the code better once you're at it", but certainly see the conservative argument.

[t-rasmud]

clang/lib/Analysis/UnsafeBufferUsage.cpp

885

Nit: "costly"

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

75

Can we have a test case for &p[0]?
IIUC, this would generate a fixit p.data() + 0 which is correct but we might want to optimize it to p.data() sometime in the future.

[NoQ]

In D143128#4167626, @jkorous wrote:

This is an interesting topic. In the abstract I see the question as: "Should the Fix-Its prioritize how the code will fit the desired end state (presumably modern idiomatic C++) or carefully respect the state of the code as is now?"

The only thing I feel pretty strongly about is that no matter what philosophy we decide to use here we should apply it consistently to all our Fix-Its (which might or might not already be the case).

And FWIW I can also imagine at some point in the future we might either have two dialects of the Fix-Its or that a separate modernizer tool (completely independent of Safe Buffers) could suggest transformations like:
"Would you like to change &DRE.data()[any] to (DRE.data() + any)?"

In D143128#4167655, @ymandel wrote:

Fantastic topic! :) In our experience at Google, we've generally followed the philosophy of leaving the code at least as good as it was before we touched it. So, if the idiom is archaic, but we're just adjusting it, that's fine (as in this example), but our tools shouldn't generate non-idiomatic (or anti-idiomic) code. We've also often taken the path of "leave cleanups to a separate pass", especially when we already have say, a clang tidy check, that does that kind of clean up running regularly. But, this one is more a judgment call. I'd probably lean towards "make the code better once you're at it", but certainly see the conservative argument.

That's a fantastic topic in general, but in *this* case I'm really not sure which one's ultimately "better", I'd totally write code both ways and at different times prefer one over the other.

I honestly actually prefer code like &DRE[any]/&DRE.data()[any] to the one with pointer arithmetic, specifically because it avoids the subject of pointer arithmetic, but instead talks about "Let's take this object, for which we already have a name, and take its address". The readers don't have to typecheck the expression in their heads in order to figure out whether correct offset multipliers are applied during pointer arithmetic. Another advantage of &DRE.data()[any] is that it's really easy to transform it to idiomatic &DRE[any] once the user confirms that the bounds checks don't actually get in the way in their case. This is much harder to achieve with DRE.data() + any given that DRE + any isn't valid code anymore.

[ziqingluo-90]

Thanks for the valuable discussion about the philosophy on the ideal forms of fix-its. In this case, I think &DRE.data()[any] and (DRE.data() + any) are both straightforward enough for the user to tell what has been changed and why we change that. And I believe both forms are idiomatic so that the user are likely happy with the form. I will keep this discussion in mind as we have other cases whose fix-its may be less idiomatic, such as D144304.

Since we have to choose one form for this case, I buy @NoQ 's argument to use &DRE.data()[any].

In addition, thanks to @t-rasmud for reminding me of that &DRE[0] can be converted to DRE.data() optimally. In such a specific case, I think we do want it to be the most concise form without confusing the user.

[malavikasamak]

clang/lib/Analysis/UnsafeBufferUsage.cpp

901

The title and summary of this patch indicates this gadget emits fixit of the form &DRE.data() + any when it encounters &DRE[any], but the code seems to emits &DRE.data()[any] instead? If yes, can you please update the summary?

[NoQ]

Looks great! LGTM except there's some dead code.

clang/lib/Analysis/UnsafeBufferUsage.cpp

509–512

These dyn_casts are already checked by the matcher. They can be turned into casts and this function can return {DRE} unconditionally.

830

Similarly, this check is redundant, it's already guaranteed by the matcher.

882–884

Same here!

[emgullufsen]

Just a heads up it seems like a lot of premerge checks builds are now showing this test as failing on Windows x64:

********************
Failed Tests (1):
  Clang :: SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp

Here are some links from buildkite for a few of the builds that failed on this test:
https://buildkite.com/llvm-project/premerge-checks/builds/145054
https://buildkite.com/llvm-project/premerge-checks/builds/145048
https://buildkite.com/llvm-project/premerge-checks/builds/145044

[DavidSpickett]

The test is also failing on our Windows on Arm bot: https://lab.llvm.org/buildbot/#/builders/65/builds/8950

[DavidSpickett]

I've reverted this, please try to fix the test then reland.

The full test output can be downloaded from the buildbot page, if you need any more information let me know.

[ziqingluo-90]

In D143128#4245112, @DavidSpickett wrote:

I've reverted this, please try to fix the test then reland.

The full test output can be downloaded from the buildbot page, if you need any more information let me know.

Thank you @DavidSpickett and @emgullufsen for catching this issue and reverting the patch for me. I have added a specific target triple for the test and re-laned it.

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

virtual void handleUnsafeOperation(const Stmt *Operation,
                                      bool IsRelatedToDecl) = 0;
 
   /// Invoked when a fix is suggested against a variable.
   virtual void handleFixableVariable(const VarDecl *Variable,
                                      FixItList &&List) = 0;
 
   /// Returns the text indicating that the user needs to provide input there:
   virtual std::string
-  getUserFillPlaceHolder(StringRef HintTextToUser = "placeholder") {
+  getUserFillPlaceHolder(StringRef HintTextToUser = "placeholder") const {
     std::string s = std::string("<# ");
     s += HintTextToUser;
     s += " #>";
     return s;
   }
 };
 
 // This function invokes the analysis and allows the caller to react to it

clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def

 #define FIXABLE_GADGET(name) GADGET(name)
 #endif
 
 WARNING_GADGET(Increment)
 WARNING_GADGET(Decrement)
 WARNING_GADGET(ArraySubscript)
 WARNING_GADGET(PointerArithmetic)
 WARNING_GADGET(UnsafeBufferUsageAttr)
-FIXABLE_GADGET(ULCArraySubscript)
+FIXABLE_GADGET(ULCArraySubscript)          // `DRE[any]` in an Unspecified Lvalue Context
+FIXABLE_GADGET(UPCAddressofArraySubscript) // '&DRE[any]' in an Unspecified Pointer Context
 
 #undef FIXABLE_GADGET
 #undef WARNING_GADGET
 #undef GADGET

clang/lib/Analysis/UnsafeBufferUsage.cpp

private:
   const internal::DynTypedMatcher *const Matcher;
   internal::ASTMatchFinder *const Finder;
   internal::BoundNodesTreeBuilder *const Builder;
   internal::BoundNodesTreeBuilder ResultBindings;
   const internal::ASTMatchFinder::BindKind Bind;
   bool Matches;
 };
 
+// Because we're dealing with raw pointers, let's define what we mean by that.
+static auto hasPointerType() {
+    return hasType(hasCanonicalType(pointerType()));
+}
+
+static auto hasArrayType() {
+    return hasType(hasCanonicalType(arrayType()));
+}
+
 AST_MATCHER_P(Stmt, forEveryDescendant, internal::Matcher<Stmt>, innerMatcher) {
   const DynTypedMatcher &DTM = static_cast<DynTypedMatcher>(innerMatcher);
   
   MatchDescendantVisitor Visitor(&DTM, Finder, Builder, ASTMatchFinder::BK_All);  
   return Visitor.findMatch(DynTypedNode::create(Node));
 }
 
 AST_MATCHER_P(CastExpr, castSubExpr, internal::Matcher<Expr>, innerMatcher) {
   return innerMatcher.matches(*Node.getSubExpr(), Finder, Builder);
 }
 
 // Returns a matcher that matches any expression 'e' such that `innerMatcher`
 // matches 'e' and 'e' is in an Unspecified Lvalue Context.
 static internal::Matcher<Stmt>
 isInUnspecifiedLvalueContext(internal::Matcher<Expr> innerMatcher) {
   return implicitCastExpr(hasCastKind(CastKind::CK_LValueToRValue),
                           castSubExpr(innerMatcher));
   // FIXME: add assignmentTo context...
 }
+
+// Returns a matcher that matches any expression `e` such that `InnerMatcher`
+// matches `e` and `e` is in an Unspecified Pointer Context (UPC).
+static internal::Matcher<Stmt>
+isInUnspecifiedPointerContext(internal::Matcher<Stmt> InnerMatcher) {
+  // A UPC can be
+  // 1. an argument of a function call (except the callee has [[unsafe_...]]
+  // attribute), or
+  // 2. the operand of a cast operation; or
+  // ...
+  auto CallArgMatcher =
+      callExpr(hasAnyArgument(allOf(
+                   hasPointerType() /* array also decays to pointer type*/,
+                   InnerMatcher)),
+               unless(callee(functionDecl(hasAttr(attr::UnsafeBufferUsage)))));
+  auto CastOperandMatcher =
+      explicitCastExpr(hasCastKind(CastKind::CK_PointerToIntegral),
+                       castSubExpr(allOf(hasPointerType(), InnerMatcher)));
+
+ return stmt(anyOf(CallArgMatcher, CastOperandMatcher));
+  // FIXME: any more cases? (UPC excludes the RHS of an assignment.  For now we
+  // don't have to check that.)
+}
 } // namespace clang::ast_matchers
 
 namespace {
 // Because the analysis revolves around variables and their types, we'll need to
 // track uses of variables (aka DeclRefExprs).
 using DeclUseList = SmallVector<const DeclRefExpr *, 1>;
 
 // Convenience typedef.
 using FixItList = SmallVector<FixItHint, 4>;
 
 // Defined below.
 class Strategy;
 } // namespace
 
-// Because we're dealing with raw pointers, let's define what we mean by that.
-static auto hasPointerType() {
-    return hasType(hasCanonicalType(pointerType()));
-}
-
-static auto hasArrayType() {
-    return hasType(hasCanonicalType(arrayType()));
-}
-
 namespace {
 /// Gadget is an individual operation in the code that may be of interest to
 /// this analysis. Each (non-abstract) subclass corresponds to a specific
 /// rigid AST structure that constitutes an operation on a pointer-type object.
 /// Discovery of a gadget in the code corresponds to claiming that we understand
 /// what this part of code is doing well enough to potentially improve it.
 /// Gadgets can be warning (immediately deserving a warning) or fixable (not
 /// always deserving a warning per se, but requires our attention to identify
public:
 
   virtual DeclUseList getClaimedVarUseSites() const override {
     if (const auto *DRE = dyn_cast<DeclRefExpr>(Node->getBase()->IgnoreImpCasts())) {
       return {DRE};
     }
     return {};
   }
 };
+
+// Represents expressions of the form `&DRE[any]` in the Unspecified Pointer
+// Context (see `isInUnspecifiedPointerContext`).
+// Note here `[]` is the built-in subscript operator.
+class UPCAddressofArraySubscriptGadget : public FixableGadget {
+private:
+  static constexpr const char *const UPCAddressofArraySubscriptTag =
+      "AddressofArraySubscriptUnderUPC";
+  const UnaryOperator *Node; // the `&DRE[any]` node
+
+public:
+  UPCAddressofArraySubscriptGadget(const MatchFinder::MatchResult &Result)
+      : FixableGadget(Kind::ULCArraySubscript),
+        Node(Result.Nodes.getNodeAs<UnaryOperator>(
+            UPCAddressofArraySubscriptTag)) {
+    assert(Node != nullptr && "Expecting a non-null matching result");
+  }
+
+  static bool classof(const Gadget *G) {
+    return G->getKind() == Kind::UPCAddressofArraySubscript;
+  }
+
+  static Matcher matcher() {
+    return expr(isInUnspecifiedPointerContext(expr(ignoringImpCasts(
+        unaryOperator(hasOperatorName("&"),
+                      hasUnaryOperand(arraySubscriptExpr(
+                          hasBase(ignoringParenImpCasts(declRefExpr())))))
+            .bind(UPCAddressofArraySubscriptTag)))));
+  }
+
+  virtual std::optional<FixItList> getFixits(const Strategy &) const override;
+
+  virtual const Stmt *getBaseStmt() const override { return Node; }
+
+  virtual DeclUseList getClaimedVarUseSites() const override {
+    if (const auto *ArraySubst =
+            dyn_cast<ArraySubscriptExpr>(Node->getSubExpr()))
+      if (const auto *DRE =
+              dyn_cast<DeclRefExpr>(ArraySubst->getBase()->IgnoreImpCasts())) {
+        return {DRE};
+      }
+    return {};
+  }
+};
 } // namespace
 
 namespace {
 // An auxiliary tracking facility for the fixit analysis. It helps connect
 // declarations to its and make sure we've covered all uses with our analysis
 // before we try to fix the declaration.
 class DeclUseTracker {
   using UseSetTy = SmallSet<const DeclRefExpr *, 16>;
#include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
   M.addMatcher(
     stmt(forEveryDescendant(
       eachOf(
       // A `FixableGadget` matcher and a `WarningGadget` matcher should not disable
       // each other (they could if they were put in the same `anyOf` group).
       // We also should make sure no two `FixableGadget` (resp. `WarningGadget`) matchers
       // match for the same node, so that we can group them
       // in one `anyOf` group (for better performance via short-circuiting).
       stmt(anyOf(
 #define FIXABLE_GADGET(x)                                                              \
         x ## Gadget::matcher().bind(#x),
 #include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
         // Also match DeclStmts because we'll need them when fixing
         // their underlying VarDecls that otherwise don't have
         // any backreferences to DeclStmts.
         declStmt().bind("any_ds")
       )),
if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) {
       case Strategy::Kind::Array:
       case Strategy::Kind::Vector:
         llvm_unreachable("unsupported strategies for FixableGadgets");
       }
     }
   return std::nullopt;
 }
 
+static std::optional<FixItList> // forward declaration
+fixUPCAddressofArraySubscriptWithSpan(const UnaryOperator *Node);
+
+std::optional<FixItList>
+UPCAddressofArraySubscriptGadget::getFixits(const Strategy &S) const {
+  auto DREs = getClaimedVarUseSites();
+
+  if (DREs.size() == 1)
+    if (const auto *VD = dyn_cast<VarDecl>(DREs.front()->getDecl())) {
+      switch (S.lookup(VD)) {
+      case Strategy::Kind::Span:
+        return fixUPCAddressofArraySubscriptWithSpan(Node);
+      case Strategy::Kind::Wontfix:
+      case Strategy::Kind::Iterator:
+      case Strategy::Kind::Array:
+      case Strategy::Kind::Vector:
+        llvm_unreachable("unsupported strategies for FixableGadgets");
+      }
+    }
+  return std::nullopt; // something went wrong, no fix-it
+}
+
 // Return the text representation of the given `APInt Val`:
 static std::string getAPIntText(APInt Val) {
   SmallVector<char> Txt;
   Val.toString(Txt, 10, true);
   // APInt::toString does not add '\0' to the end of the string for us:
   Txt.push_back('\0');
   return Txt.data();
 }
static StringRef getExprText(const Expr *E, const SourceManager &SM,
                                const LangOptions &LangOpts) {
   SourceLocation LastCharLoc = getPastLoc(E, SM, LangOpts);
 
   return Lexer::getSourceText(
       CharSourceRange::getCharRange(E->getBeginLoc(), LastCharLoc), SM,
       LangOpts);
 }
 
+// Generates fix-its replacing an expression of the form `&DRE[e]` with
+// `(DRE.data() + e)`:
+static std::optional<FixItList>
+fixUPCAddressofArraySubscriptWithSpan(const UnaryOperator *Node) {
+  if (const auto *ArraySub = dyn_cast<ArraySubscriptExpr>(Node->getSubExpr()))
+    if (const auto *DRE =
+            dyn_cast<DeclRefExpr>(ArraySub->getBase()->IgnoreImpCasts())) {
+      // FIXME: this `getASTContext` call is costy, we should pass the
+      // ASTContext in:
+      const ASTContext &Ctx = DRE->getDecl()->getASTContext();
+      const Expr *Idx = ArraySub->getIdx();
+      const SourceManager &SM = Ctx.getSourceManager();
+      const LangOptions &LangOpts = Ctx.getLangOpts();
+      SmallString<32> StrBuffer;
+
+      StrBuffer.append("(");
+      StrBuffer.append(getExprText(DRE, SM, LangOpts));
+      StrBuffer.append(".data() + ");
+      StrBuffer.append(getExprText(Idx, SM, LangOpts));
+      StrBuffer.append(")");
+      return FixItList{FixItHint::CreateReplacement(Node->getSourceRange(),
+                                           StrBuffer.str())};
+    }
+  return std::nullopt; // something went wrong. no fix-it
+}
+
 // For a non-null initializer `Init` of `T *` type, this function returns
 // `FixItHint`s producing a list initializer `{Init,  S}` as a part of a fix-it
 // to output stream.
 // In many cases, this function cannot figure out the actual extent `S`.  It
 // then will use a place holder to replace `S` to ask users to fill `S` in.  The
 // initializer shall be used to initialize a variable of type `std::span<T>`.
 //
 // FIXME: Support multi-level pointers

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

+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+int f(unsigned long, void *);
+
+[[clang::unsafe_buffer_usage]]
+int unsafe_f(unsigned long, void *);
+
+void address_to_integer(int x) {
+  int * p = new int[10];
+  unsigned long n = (unsigned long) &p[5];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:37-[[@LINE-1]]:42}:"(p.data() + 5)"
+  unsigned long m = (unsigned long) &p[x];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:37-[[@LINE-1]]:42}:"(p.data() + x)"
+}
+
+void call_argument(int x) {
+  int * p = new int[10];
+
+  f((unsigned long) &p[5], &p[x]);
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:21-[[@LINE-1]]:26}:"(p.data() + 5)"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:28-[[@LINE-2]]:33}:"(p.data() + x)"
+}
+
+void ignore_unsafe_calls(int x) {
+  // Cannot fix `&p[x]` for now as it is an argument of an unsafe
+  // call. So no fix for variable `p`.
+  int * p = new int[10];
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+  unsafe_f((unsigned long) &p[5],
+     // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+     &p[x]);
+
+  int * q = new int[10];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> q"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}"
+  unsafe_f((unsigned long) &q[5],
+     // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:28-[[@LINE-1]]:33}:"(q.data() + 5)"
+     (void*)0);
+}
+
+void odd_subscript_form() {
+  int * p = new int[10];
+  unsigned long n = (unsigned long) &5[p];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:37-[[@LINE-1]]:42}:"(p.data() + 5)"
+}
+
+// To test multiple function declarations, each of which carries
+// different incomplete informations:
+[[clang::unsafe_buffer_usage]]
+void unsafe_g(void*);
+
+void unsafe_g(void*);
+
+void multiple_unsafe_fundecls() {
+  int * p = new int[10];
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+  unsafe_g(&p[5]);
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+}
+
+void unsafe_h(void*);
+
+[[clang::unsafe_buffer_usage]]
+void unsafe_h(void*);
+
+void unsafe_h(void* p) { ((char*)p)[10]; }
+
+void multiple_unsafe_fundecls2() {
+  int * p = new int[10];
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+  unsafe_h(&p[5]);
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+}