[-Wunsafe-buffer-usage] Handle lambda expressions within a method.

[t-rasmud]

Summary

Handle lambda expressions defined within a method. Currently, such lambda expressions are visited as independent units. This causes the UnsafeBufferUsage analysis to crash if the lambda expression operates on variables defined outside it. This is because the analysis expects to see the VarDecl before seeing a DRE, which doesn't hold true for lambdas.

To handle this, we are proposing to visit such expressions while visiting the outer method, instead of visiting it independently. This patch implements this proposal.

[malavikasamak]

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

166–168

Emitting this warning at this location seems confusing to me. Should this warning be emitted here? Also, in general, what is the context when this warning should be emitted.

[ziqingluo-90]

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

166–168

I think they are right. The lambda has its own p and a variables holding copies of p and a defined in outer scope.

[ziqingluo-90]

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

166–168

so maybe we could have another lambda capturing by references in order to test that warnings will be emitted at variables defined in the outer function.

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

1609

I suspect that we will also skip lambdas defined in file scopes.

[malavikasamak]

clang/lib/Analysis/UnsafeBufferUsage.cpp

1609

Yes, you are right. Added the isLocalClass check to handle this. Also will add a test case shortly.

[malavikasamak]

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

166–168

I agree that we should warn at variable declaration here. I was questioning whether this specific warning makes sense at this location, as it's supposed to appear at the unsafe operation IIUC.

I will debug further to see why this is being attached here in the first place.

[ziqingluo-90]

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

166–168

aha, I didn't look at the warnings carefully. I thought they were the warnings attached to VarDecls. Then you are right, they look strange.

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

1629

Maybe add a test showing that we will ignore implicit variables of lambda init-captures?

[NoQ]

The solution looks great! I only have minor nitpicks.

This patch is adding new matchers, so we should make sure we do the usual stuff - document them, make them discoverable, make sure clang-query knows how to pick them up, etc.

clang/include/clang/ASTMatchers/ASTMatchers.h

1973–1978 ↗

(On Diff #523217)

We'll need to add documentation, update registry, etc. - the usual bureaucracy of adding a new ASTMatcher for everybody to use.

clang/lib/Analysis/UnsafeBufferUsage.cpp

1606–1607

Formatting nitpick.

1629

Needs a line break.

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

140

Maybe add CHECK-NOT here, so that the test started failing when the underlying problem gets fixed, and so we gain more test coverage for free this way?

[t-rasmud]

Address comments: Update registry and documentation.

[NoQ]

clang/include/clang/ASTMatchers/ASTMatchers.h

1988 ↗

(On Diff #539643)

This sounds a bit as if the type int[10] is matched, we should probably elaborate. WDYT of this take?

2004 ↗

(On Diff #539643)

Added more elaboration here as well. There are a few non-compiler-engineer folks using clang-query, I think it's helpful for them to know how compiler treats these things. Technically it's probably more of a C++ Standard formalism thing, but I wouldn't require knowing that either. The fact that structured bindings are "backed" by a copy of the original "whole" object is a really obscure and unobvious piece of formalism.

[t-rasmud]

Add generated ListASTMatchersReference.html file

[t-rasmud]

Address remaining comments.

[t-rasmud]

Minor: Remove Fixme that's already completed.

[NoQ]

Alright the patch looks great! Let's remove the bits that correspond to D155134 and D155304, and then commit?

clang/lib/Analysis/UnsafeBufferUsage.cpp

bool TraverseCXXTypeidExpr(CXXTypeidExpr *Node) {
     return VisitorBase::TraverseCXXTypeidExpr(Node);
   }
 
   bool TraverseStmt(Stmt *Node, DataRecursionQueue *Queue = nullptr) {
     if (!Node)
       return true;
     if (!match(*Node))
       return false;
-    // To skip callables:
-    if (isa<LambdaExpr>(Node))
-      return true;
+
     return VisitorBase::TraverseStmt(Node);
   }
 
   bool shouldVisitTemplateInstantiations() const { return true; }
   bool shouldVisitImplicitCode() const {
     // TODO: let's ignore implicit code for now
     return false;
   }
public:
   }
 };
 } // 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>;
   using DefMapTy = DenseMap<const VarDecl *, const DeclStmt *>;
 
   // Allocate on the heap for easier move.
   std::unique_ptr<UseSetTy> Uses{std::make_unique<UseSetTy>()};
   DefMapTy Defs{};
 
getNaiveStrategy(const llvm::SmallVectorImpl<const VarDecl *> &UnsafeVars) {
   return S;
 }
 
 void clang::checkUnsafeBufferUsage(const Decl *D,
                                    UnsafeBufferUsageHandler &Handler,
                                    bool EmitFixits) {
   assert(D && D->getBody());
 
+  // We do not want to visit a Lambda expression defined inside a method independently.
+  // Instead, it should be visited along with the outer method.
+  if(const auto *fd = dyn_cast<CXXMethodDecl>(D)) {
+    if(fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
+     return;
+  }
+
   WarningGadgetSets UnsafeOps;
   FixableGadgetSets FixablesForUnsafeVars;
   DeclUseTracker Tracker;
 
   {
     auto [FixableGadgets, WarningGadgets, TrackerRes] = findGadgets(D, Handler);
     UnsafeOps = groupWarningGadgetsByVar(std::move(WarningGadgets));
     FixablesForUnsafeVars = groupFixablesByVar(std::move(FixableGadgets));
     Tracker = std::move(TrackerRes);
   }
 
   std::map<const VarDecl *, FixItList> FixItsForVariable;
 
   if (EmitFixits) {
     // Filter out non-local vars and vars with unclaimed DeclRefExpr-s.
     for (auto it = FixablesForUnsafeVars.byVar.cbegin();
          it != FixablesForUnsafeVars.byVar.cend();) {
       // FIXME: Support ParmVarDecl as well.
       if (!it->first->isLocalVarDecl() || Tracker.hasUnclaimedUses(it->first)) {
         it = FixablesForUnsafeVars.byVar.erase(it);
       } else {
         ++it;
       }
     }
 
     llvm::SmallVector<const VarDecl *, 16> UnsafeVars;
     for (const auto &[VD, ignore] : FixablesForUnsafeVars.byVar)

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

void unsafe_method_invocation_double_param() {
   // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()"
 
   m1(q, p, 9);
   // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:10}:".data()"
 
   m1(q, q, 8);
 }
 
+
+void unsafe_access_in_lamda() {
+  auto p = new int[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
+
+  auto my_lambda = [&](){
+    p[5] = 10;
+  };
+
+  foo(p);
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:8-[[@LINE-1]]:8}:".data()"
+}
+
+void fixits_in_lamda() {
+  auto p = new int[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
+
+  auto my_lambda = [&](){
+    foo(p);
+    // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:10}:".data()"
+  };
+
+  p[5] = 10;
+}
+
+// FIXME: Emit fixits for lambda captured variables
+void fixits_in_lambda_capture() {
+  auto p = new int[10];
+
+  auto my_lambda = [p](){ // No fixits emitted here.
+    foo(p);
+  };
+}

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

 int * gp = garray;  // expected-warning{{'gp' is an unsafe pointer used for buffer access}}
 int gvar = gp[1];   // FIXME: file scope unsafe buffer access is not warned
 
 // FIXME: Add test for lambda capture with initializer. E. g. auto Lam = [new_p = p]() {...
 void testLambdaCaptureAndGlobal(int * p) {
   // expected-warning@-1{{'p' is an unsafe pointer used for buffer access}}
   int a[10];              // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}}
 
-  auto Lam = [p, a]() {
-    return p[1]           // expected-note{{used in buffer access here}}
+  auto Lam = [p, a]() {   // expected-warning{{unsafe buffer access}}
+
+    return p[1]           // expected-note{{used in buffer access here}}  
       + a[1] + garray[1]  // expected-note2{{used in buffer access here}}
       + gp[1];            // expected-note{{used in buffer access here}}
-  };
+
+  };
+}
+
+void testLambdaCapture() {
+  int a[10];              // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}}
+    
+  auto Lam = [a]() {   // expected-warning{{unsafe buffer access}}
+    return a[1];           // expected-note{{used in buffer access here}}
+  };  
 }
 
 typedef T_t * T_ptr_t;
 
 void testTypedefs(T_ptr_t p) {
   // expected-warning@-1{{'p' is an unsafe pointer used for buffer access}}
   foo(p[1],       // expected-note{{used in buffer access here}}
       p[1].a[1],  // expected-note{{used in buffer access here}}