Diagnose potential size confusion with VLA params

clang/docs/ReleaseNotes.rst

@@ -222,6 +222,11 @@ Improvements to Clang's diagnostics
 
 - Improve the diagnostics for shadows template parameter to report correct location (#GH129060).
 
+- Added the ``-Wvla-potential-size-confusion`` diagnostic, which is grouped
+  under ``-Wvla`` to diagnose when a variably-modified type in a function
+  parameter list is using a variable from an outer scope as opposed to a
+  variable declared later in the parameter list.
+
 Improvements to Clang's time-trace
 ----------------------------------
 

clang/include/clang/Basic/DiagnosticGroups.td

@@ -936,7 +936,8 @@ def VexingParse : DiagGroup<"vexing-parse">;
 def VLAUseStaticAssert : DiagGroup<"vla-extension-static-assert">;
 def VLACxxExtension : DiagGroup<"vla-cxx-extension", [VLAUseStaticAssert]>;
 def VLAExtension : DiagGroup<"vla-extension", [VLACxxExtension]>;
-def VLA : DiagGroup<"vla", [VLAExtension]>;
+def VLASizeConfusion : DiagGroup<"vla-potential-size-confusion">;
+def VLA : DiagGroup<"vla", [VLAExtension, VLASizeConfusion]>;
 def VolatileRegisterVar : DiagGroup<"volatile-register-var">;
 def Visibility : DiagGroup<"visibility">;
 def ZeroLengthArray : DiagGroup<"zero-length-array">;

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -170,6 +170,13 @@ def err_vla_in_coroutine_unsupported : Error<
   "variable length arrays in a coroutine are not supported">;
 def note_vla_unsupported : Note<
   "variable length arrays are not supported for the current target">;
+def warn_vla_size_expr_shadow : Warning<
+  "variable length array size expression refers to declaration from an outer "
+  "scope">, InGroup<VLASizeConfusion>;
+def note_vla_size_expr_shadow_param : Note<
+  "does not refer to this declaration">;
+def note_vla_size_expr_shadow_actual : Note<
+  "refers to this declaration instead">;
 
 // C99 variably modified types
 def err_variably_modified_template_arg : Error<

clang/lib/Sema/SemaDecl.cpp

@@ -26,6 +26,7 @@
 #include "clang/AST/MangleNumberingContext.h"
 #include "clang/AST/NonTrivialTypeVisitor.h"
 #include "clang/AST/Randstruct.h"
+#include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/StmtCXX.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/Builtins.h"
@@ -10333,6 +10334,74 @@ Sema::ActOnFunctionDeclarator(Scope *S, Declarator &D, DeclContext *DC,
       }
     }
 
+    // Loop over the parameters to see if any of the size expressions contains
+    // a DeclRefExpr which refers to a variable from an outer scope that is
+    // also named later in the parameter list.
+    // e.g., int n; void func(int array[n], int n);
+    SmallVector<const DeclRefExpr *, 2> DRESizeExprs;
+    llvm::for_each(Params, [&](const ParmVarDecl *Param) {
+      // If we have any size expressions we need to check against, check them
+      // now.
+      for (const auto *DRE : DRESizeExprs) {
+        // Check to see if this parameter has the same name as one of the
+        // DeclRefExprs we wanted to test against. If so, then we found a
+        // situation where an earlier parameter refers to the name of a later
+        // parameter, which is (currently) only valid if there's a variable
+        // from an outer scope with the same name.
+        if (const auto *SizeExprND = dyn_cast<NamedDecl>(DRE->getDecl())) {
+          if (SizeExprND->getIdentifier() == Param->getIdentifier()) {
+            // Diagnose the DeclRefExpr from the parameter with the size
+            // expression.
+            Diag(DRE->getLocation(), diag::warn_vla_size_expr_shadow);
+            // Note the parameter that a user could be confused into thinking
+            // they're referring to.
+            Diag(Param->getLocation(), diag::note_vla_size_expr_shadow_param);
+            // Note the DeclRefExpr that's actually being used.
+            Diag(DRE->getDecl()->getLocation(),
+                 diag::note_vla_size_expr_shadow_actual);
+          }
+        }
+      }
+
+      // To check whether its size expression is a simple DeclRefExpr, we first
+      // have to walk through pointers or references, but array types always
+      // decay to a pointer, so skip if this is a DecayedType.
+      QualType QT = Param->getType();
+      while (!isa<DecayedType>(QT.getTypePtr()) &&
+             (QT->isPointerType() || QT->isReferenceType()))
+        QT = QT->getPointeeType();
+
+      // An array type is always decayed to a pointer, so we need to get the
+      // original type in that case.
+      if (const auto *DT = QT->getAs<DecayedType>())
+        QT = DT->getOriginalType();
+
+      // Now we can see if it's a VLA type with a size expression.
+      // FIXME: it would be nice to handle constant-sized arrays as well,
+      // e.g., constexpr int n = 12; void foo(int array[n], int n);
+      // however, the constant expression is replaced by its value at the time
+      // we form the type, so we've lost that information here.
+      if (!QT->hasSizedVLAType())
+        return;
+
+      const VariableArrayType *VAT = getASTContext().getAsVariableArrayType(QT);
+      if (!VAT)
+        return;
+
+      class DeclRefFinder : public RecursiveASTVisitor<DeclRefFinder> {
+        SmallVectorImpl<const DeclRefExpr *> &Found;
+
+      public:
+        DeclRefFinder(SmallVectorImpl<const DeclRefExpr *> &Found)
+            : Found(Found) {}
+        bool VisitDeclRefExpr(const DeclRefExpr *DRE) {
+          Found.push_back(DRE);
+          return true;
+        }
+      } Finder(DRESizeExprs);
+      Finder.TraverseStmt(VAT->getSizeExpr());
+    });
+
     if (!getLangOpts().CPlusPlus) {
       // In C, find all the tag declarations from the prototype and move them
       // into the function DeclContext. Remove them from the surrounding tag

clang/test/Sema/vla-potential-size-confusion.c

@@ -0,0 +1,68 @@
+// RUN: %clang_cc1 %s -std=c23 -verify=expected,c -fsyntax-only
+// RUN: %clang_cc1 %s -std=c23 -verify=good -fsyntax-only -Wno-vla
+// RUN: %clang_cc1 -x c++ %s -verify -fsyntax-only
+// RUN: %clang_cc1 -DCARET -fsyntax-only -std=c23 -fno-diagnostics-show-line-numbers -fcaret-diagnostics-max-lines=1 %s 2>&1 | FileCheck %s -strict-whitespace
+
+// good-no-diagnostics
+
+int n, m;      // #decl
+int size(int);
+
+void foo(int vla[n], int n); // expected-warning {{variable length array size expression refers to declaration from an outer scope}} \
+                                expected-note {{does not refer to this declaration}} \
+                                expected-note@#decl {{refers to this declaration instead}}
+
+void bar(int (*vla)[n], int n); // expected-warning {{variable length array size expression refers to declaration from an outer scope}} \
+                                   expected-note {{does not refer to this declaration}} \
+                                   expected-note@#decl {{refers to this declaration instead}}
+
+void baz(int n, int vla[n]); // no diagnostic expected
+
+void quux(int vla[n + 12], int n); // expected-warning {{variable length array size expression refers to declaration from an outer scope}} \
+                                      expected-note {{does not refer to this declaration}} \
+                                      expected-note@#decl {{refers to this declaration instead}}
+
+void quibble(int vla[size(n)], int n);  // expected-warning {{variable length array size expression refers to declaration from an outer scope}} \
+                                           expected-note {{does not refer to this declaration}} \
+                                           expected-note@#decl {{refers to this declaration instead}}
+
+void quobble(int vla[n + m], int n, int m);  // expected-warning 2 {{variable length array size expression refers to declaration from an outer scope}} \
+                                                expected-note 2 {{does not refer to this declaration}} \
+                                                expected-note@#decl 2 {{refers to this declaration instead}}
+
+// For const int, we still treat the function as having a variably-modified
+// type, but only in C.
+const int x = 12; // #other-decl
+void quorble(int vla[x], int x); // c-warning {{variable length array size expression refers to declaration from an outer scope}} \
+                                    c-note {{does not refer to this declaration}} \
+                                    c-note@#other-decl {{refers to this declaration instead}}
+
+// For constexpr int, the function has a constant array type. It would be nice
+// to diagnose this case as well, but the type system replaces the expression
+// with the constant value, and so the information about the name of the
+// variable used in the size expression is lost.
+constexpr int y = 12;
+void quuble(int vla[y], int y); // no diagnostic expected
+
+#ifdef __cplusplus
+struct S {
+  static int v; // #mem-var
+
+  void member_function(int vla[v], int v);  // expected-warning {{variable length array size expression refers to declaration from an outer scope}} \
+                                               expected-note {{does not refer to this declaration}} \
+                                               expected-note@#mem-var {{refers to this declaration instead}}
+};
+#endif
+
+#ifdef CARET
+// Test that caret locations make sense.
+int w;
+void quable(int vla[w], int w);
+
+// CHECK: void quable(int vla[w], int w);
+// CHECK:                     ^
+// CHECK: void quable(int vla[w], int w);
+// CHECK:                             ^
+// CHECK: int w;
+// CHECK:     ^
+#endif