[-Wunsafe-buffer-usage] Replace assert that declarations are always found

[ziqingluo-90]

How about having some tests?

[t-rasmud]

In D157018#4559135, @ziqingluo-90 wrote:

How about having some tests?

My understanding is that we are not sure of when this assert might be hit. This was probably hit when lambda captures were not handled but not anymore. We want to replace the assert with a debug note to catch cases when we can't find a VarDecl for a declaration statement. @NoQ Could you please confirm if my understanding is correct?

[ziqingluo-90]

My understanding is that we are not sure of when this assert might be hit.

aha, I cannot think of an example either so I was curious. What you said makes sense to me.

[NoQ]

I don't have any test cases in mind immediately. We could try our usual suspects such as globals and DecompositionDecls (they're subclass of VarDecl, unlike BindingDecl). But it could totally be that it dodges them because of other checks we already have.

But I agree that the assertion should be removed: we just no longer believe that it needs to hold!

clang/lib/Analysis/UnsafeBufferUsage.cpp

2058

Other notes start with small letter 😅

[NoQ]

Oo I think I broke it! https://godbolt.org/z/7efM6TvxM

We needed to use the tuple mode of decomposition because it eagerly unpacks the tuple into, in our case, a raw pointer that we can try to analyze.

This, of course, makes it much harder to write the test because you'll need to mock <tuple>. Looks like we have some existing mocks in test/Analysis/uninit-structured-binding-tuple.cpp, they might help.

I suspect it'll be much easier to break this assertion once we start supporting std::array. So it's great that we're removing it.

[NoQ]

Yeah nvm that's a different assertion and an unrelated crash. Which we'll also need to fix but not in this patch. LGTM then!

clang/lib/Analysis/UnsafeBufferUsage.cpp

for (const Decl *D : DS->decls()) {
         // assert(Defs.count(VD) == 0 && "Definition already discovered!");
         Defs[VD] = DS;
       }
     }
   }
 
   const DeclStmt *lookupDecl(const VarDecl *VD) const {
     auto It = Defs.find(VD);
-    assert(It != Defs.end() && "Definition never discovered!");
+    if (It == Defs.end())
+      return nullptr;
     return It->second;
   }
 };
 } // namespace
 
 namespace {
 // Strategy is a map from variables to the way we plan to emit fixes for
 // these variables. It is figured out gradually by trying different fixes
static FixItList fixParamWithSpan(const ParmVarDecl *PVD, const ASTContext &Ctx,
   return {};
 }
 
 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 (!DS) {
+    DEBUG_NOTE_DECL_FAIL(VD, " : variables declared this way not implemented yet");
+    return {};
+  }
   if (!DS->isSingleDecl()) {
     // FIXME: to support handling multiple `VarDecl`s in a single `DeclStmt`
     DEBUG_NOTE_DECL_FAIL(VD, " : multiple VarDecls");
     return {};
   }
   // Currently DS is an unused variable but we'll need it when
   // non-single decls are implemented, where the pointee type name
   // and the '*' are spread around the place.

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

 
 void implied_unclaimed_var(int *b) {  // expected-warning{{'b' is an unsafe pointer used for buffer access}}
   int *a = new int[3];  // expected-warning{{'a' is an unsafe pointer used for buffer access}}
   a[4] = 7;  // expected-note{{used in buffer access here}}
   a = b;  // debug-note{{safe buffers debug: gadget 'PointerAssignment' refused to produce a fix}}
   b++;  // expected-note{{used in pointer arithmetic here}} \
         // debug-note{{safe buffers debug: failed to produce fixit for 'b' : has an unclaimed use}}
 }
+
+int *a = new int[3];  // expected-warning{{'a' is an unsafe pointer used for buffer access}} \
+// debug-note{{safe buffers debug: failed to produce fixit for 'a' : neither local nor a parameter}}
+void test_globals() {
+  a[7] = 4;  // expected-note{{used in buffer access here}}
+}
+
+void test_decomp_decl() {
+  int a[2] = {1, 2};
+  auto [x, y] = a;
+  x = 9;
+}