[-Wunsafe-buffer-usage] Handle unevaluated contexts that contain unsafe buffer usages

This patch handles unevaluated contexts to ensure no warnings are produced by the machinery
for buffer access made within an unevaluated contexts. However, such accesses must be
considered by a FixableGadget and produce the necessary fixits.

Reviewed by: NoQ, ziqingluo-90, jkorous

Differential revision: https://reviews.llvm.org/D144905

Author: MalavikaSamak

clang/lib/Analysis/UnsafeBufferUsage.cpp

@@ -35,9 +35,10 @@ class MatchDescendantVisitor
   MatchDescendantVisitor(const internal::DynTypedMatcher *Matcher,
                          internal::ASTMatchFinder *Finder,
                          internal::BoundNodesTreeBuilder *Builder,
-                         internal::ASTMatchFinder::BindKind Bind)
+                         internal::ASTMatchFinder::BindKind Bind,
+                         const bool ignoreUnevaluatedContext)
       : Matcher(Matcher), Finder(Finder), Builder(Builder), Bind(Bind),
-        Matches(false) {}
+        Matches(false), ignoreUnevaluatedContext(ignoreUnevaluatedContext) {}
 
   // Returns true if a match is found in a subtree of `DynNode`, which belongs
   // to the same callable of `DynNode`.
@@ -70,6 +71,48 @@ class MatchDescendantVisitor
     return VisitorBase::TraverseDecl(Node);
   }
 
+  bool TraverseGenericSelectionExpr(GenericSelectionExpr *Node) {
+    // These are unevaluated, except the result expression.
+    if(ignoreUnevaluatedContext)
+      return TraverseStmt(Node->getResultExpr());
+    return VisitorBase::TraverseGenericSelectionExpr(Node);
+  }
+
+  bool TraverseUnaryExprOrTypeTraitExpr(UnaryExprOrTypeTraitExpr *Node) {
+    // Unevaluated context.
+    if(ignoreUnevaluatedContext)
+      return true;
+    return VisitorBase::TraverseUnaryExprOrTypeTraitExpr(Node);
+  }
+
+  bool TraverseTypeOfExprTypeLoc(TypeOfExprTypeLoc Node) {
+    // Unevaluated context.
+    if(ignoreUnevaluatedContext)
+      return true;
+    return VisitorBase::TraverseTypeOfExprTypeLoc(Node);
+  }
+
+  bool TraverseDecltypeTypeLoc(DecltypeTypeLoc Node) {
+    // Unevaluated context.
+    if(ignoreUnevaluatedContext)
+      return true;
+    return VisitorBase::TraverseDecltypeTypeLoc(Node);
+  }
+
+  bool TraverseCXXNoexceptExpr(CXXNoexceptExpr *Node) {
+    // Unevaluated context.
+    if(ignoreUnevaluatedContext)
+      return true;
+    return VisitorBase::TraverseCXXNoexceptExpr(Node);
+  }
+
+  bool TraverseCXXTypeidExpr(CXXTypeidExpr *Node) {
+    // Unevaluated context.
+    if(ignoreUnevaluatedContext)
+      return true;
+    return VisitorBase::TraverseCXXTypeidExpr(Node);
+  }
+
   bool TraverseStmt(Stmt *Node, DataRecursionQueue *Queue = nullptr) {
     if (!Node)
       return true;
@@ -111,6 +154,7 @@ class MatchDescendantVisitor
   internal::BoundNodesTreeBuilder ResultBindings;
   const internal::ASTMatchFinder::BindKind Bind;
   bool Matches;
+  bool ignoreUnevaluatedContext;
 };
 
 // Because we're dealing with raw pointers, let's define what we mean by that.
@@ -121,11 +165,18 @@ static auto hasPointerType() {
 static auto hasArrayType() {
     return hasType(hasCanonicalType(arrayType()));
 }
-  
-AST_MATCHER_P(Stmt, forEveryDescendant, internal::Matcher<Stmt>, innerMatcher) {
+
+AST_MATCHER_P(Stmt, forEachDescendantEvaluatedStmt, internal::Matcher<Stmt>, innerMatcher) {
   const DynTypedMatcher &DTM = static_cast<DynTypedMatcher>(innerMatcher);
 
-  MatchDescendantVisitor Visitor(&DTM, Finder, Builder, ASTMatchFinder::BK_All);
+  MatchDescendantVisitor Visitor(&DTM, Finder, Builder, ASTMatchFinder::BK_All, true);
+  return Visitor.findMatch(DynTypedNode::create(Node));
+}
+
+AST_MATCHER_P(Stmt, forEachDescendantStmt, internal::Matcher<Stmt>, innerMatcher) {
+  const DynTypedMatcher &DTM = static_cast<DynTypedMatcher>(innerMatcher);
+
+  MatchDescendantVisitor Visitor(&DTM, Finder, Builder, ASTMatchFinder::BK_All, false);
   return Visitor.findMatch(DynTypedNode::create(Node));
 }
 
@@ -870,32 +921,31 @@ findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler) {
 
   // clang-format off
   M.addMatcher(
-    stmt(forEveryDescendant(
-      eachOf(
+    stmt(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(eachOf(
+      forEachDescendantStmt(stmt(eachOf(
 #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")
-      )),
-      stmt(anyOf(
+        // In parallel, match all DeclRefExprs so that to find out
+        // whether there are any uncovered by gadgets.
+        declRefExpr(anyOf(hasPointerType(), hasArrayType()), to(varDecl())).bind("any_dre")
+      ))),
+      forEachDescendantEvaluatedStmt(stmt(anyOf(
         // Add Gadget::matcher() for every gadget in the registry.
 #define WARNING_GADGET(x)                                                              \
         allOf(x ## Gadget::matcher().bind(#x), notInSafeBufferOptOut(&Handler)),
 #include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
-        // In parallel, match all DeclRefExprs so that to find out
-        // whether there are any uncovered by gadgets.
-        declRefExpr(anyOf(hasPointerType(), hasArrayType()), to(varDecl())).bind("any_dre")
-      )))
-    )),
+        // 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")
+      ))
+    ))),
     &CB
   );
   // clang-format on

clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-unevaluated-context.cpp

@@ -0,0 +1,79 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits -fsyntax-only %s 2>&1 | FileCheck %s
+
+namespace std {
+  class type_info;
+  class bad_cast;
+  class bad_typeid;
+}
+using size_t = __typeof(sizeof(int));
+void *malloc(size_t);
+
+void foo(...);
+int bar(int *ptr);
+
+void uneval_context_fix_pointer_dereference() {
+  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}"
+
+  int tmp = p[5];
+  typeid(foo(*p));
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:14-[[@LINE-1]]:15}:""
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:"[0]"
+  _Generic(*p, int: 2, float: 3);
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:12-[[@LINE-1]]:13}:""
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:14-[[@LINE-2]]:14}:"[0]" 
+}
+
+void uneval_context_fix_pointer_array_access() {
+  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}"
+
+  int tmp = p[5];
+  typeid(foo(p[5]));
+  _Generic(p[2], int: 2, float: 3);
+}
+
+void uneval_context_fix_pointer_reference() {
+  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}"
+
+  int tmp = p[5];
+  typeid(bar(p));
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:15-[[@LINE-1]]:15}:".data()"
+}
+
+// The FixableGagdtes are not working in the following scenarios:
+// 1. sizeof(DRE)
+// 2. typeid(DRE)
+// 3. __typeof(DRE)
+// 4. _Generic(expr, type_1: DRE, type_2:)
+// 5. decltype(DRE) var = y;
+// 6. noexcept(DRE);
+// This is becauste the UPC and ULC context matchers do not handle these contexts
+// and almost all FixableGagdets currently depend on these matchers.
+
+// FIXME: Emit fixits for each of the below use.
+void uneval_context_fix_pointer_dereference_not_handled() {
+  auto p = new int[10];
+  int tmp = p[5];
+
+  foo(sizeof(*p), sizeof(decltype(*p)));
+  __typeof(*p) x;
+  int *q = (int *)malloc(sizeof(*p));
+  int y = sizeof(*p);
+  __is_pod(__typeof(*p));
+  __is_trivially_constructible(__typeof(*p), decltype(*p));
+  _Generic(*p, int: 2, float: 3);
+  _Generic(1, int: *p, float: 3);
+  _Generic(1, int: 2, float: *p);
+  decltype(*p) var = y;
+  noexcept(*p);
+  typeid(*p);
+}
+

clang/test/SemaCXX/warn-unsafe-buffer-usage-warning-unevaluated-context.cpp

@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage -fblocks -include %s -verify %s
+
+// RUN: %clang -x c++ -fsyntax-only -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
+// RUN: %clang_cc1 -std=c++11 -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
+// RUN: %clang_cc1 -std=c++20 -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
+// CHECK-NOT: [-Wunsafe-buffer-usage]
+
+#ifndef INCLUDED
+#define INCLUDED
+#pragma clang system_header
+
+// no spanification warnings for system headers
+void foo(...);  // let arguments of `foo` to hold testing expressions
+#else
+
+namespace std {
+  class type_info;
+  class bad_cast;
+  class bad_typeid;
+}
+using size_t = __typeof(sizeof(int));
+void *malloc(size_t);
+
+void foo(int v) {
+}
+
+void foo(int *p){}
+
+void uneval_context_fix() {
+  auto p = new int[10]; // expected-warning{{'p' is an unsafe pointer used for buffer access}}
+
+  // Warn on the following DREs
+  _Generic(1, int: p[2], float: 3); // expected-note{{used in buffer access here}}
+
+  // Do not warn for following DREs
+  auto q = new int[10];
+  foo(sizeof(q[1]), // no-note
+      sizeof(decltype(q[1]))); // no-note
+  __typeof(q[5]) x; // no-note
+  int *r = (int *)malloc(sizeof(q[5])); // no-note
+  int y = sizeof(q[5]); // no-note
+  __is_pod(__typeof(q[5])); // no-note
+  __is_trivially_constructible(__typeof(q[5]), decltype(q[5])); // no-note
+  _Generic(q[1], int: 2, float: 3); // no-note
+  _Generic(1, int: 2, float: q[3]); // no-note
+  decltype(q[2]) var = y; // no-note
+  noexcept(q[2]); // no-note
+  typeid(q[3]); // no-note
+}
+#endif

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

@@ -102,12 +102,6 @@ void testArraySubscriptsWithAuto(int *p, int **pp) {
   foo(ap4[1]);    // expected-note{{used in buffer access here}}
 }
 
-//TODO: do not warn for unevaluated context
-void testUnevaluatedContext(int * p) {// expected-warning{{'p' is an unsafe pointer used for buffer access}}
-  foo(sizeof(p[1]),             // expected-note{{used in buffer access here}}
-      sizeof(decltype(p[1])));  // expected-note{{used in buffer access here}}
-}
-
 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}}