llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Aryan Kadole (ak1932) <details> <summary>Changes</summary> This PR continues the work of @<!-- -->AaronBallman PR #<!-- -->129772 in adding diagnostic to a potential confusion when using a later function parameter as the size bound for a variable length array. The confusion would arise when using a later parameter of the same name as outer scoped variable as the bound, in which case the compiler should recognize the outer scoped variable as the bound and warn the user of the confusion. I have resolved the comments received on that PR with the following additions: 1. refactor the code to a seperate function and only invoke it when bounds-safety is enabled 2. move to ConstDynamicRecursiveASTVisitor instead of RecursiveASTVisitor 3. add a constexpr test for member function --- Full diff: https://github.com/llvm/llvm-project/pull/181550.diff 6 Files Affected: - (modified) clang/docs/ReleaseNotes.rst (+4) - (modified) clang/include/clang/Basic/DiagnosticGroups.td (+2-1) - (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+8) - (modified) clang/include/clang/Sema/Sema.h (+4) - (modified) clang/lib/Sema/SemaDecl.cpp (+75) - (added) clang/test/Sema/vla-potential-size-confusion.c (+72) ``````````diff diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index c648e8b0ec6fa..9ac78d610e04d 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -244,6 +244,10 @@ Improvements to Clang's diagnostics - The ``-Wloop-analysis`` warning has been extended to catch more cases of variable modification inside lambda expressions (#GH132038). +- 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 ---------------------------------- diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td index 0372cf062ec67..0572327d60ae3 100644 --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -1114,7 +1114,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">; diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 85a023435ba23..5ec88da7c878e 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -229,6 +229,14 @@ 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< diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index 9424b80d5cdb6..fa3bbd473f1aa 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -4239,6 +4239,10 @@ class Sema final : public SemaBase { /// ParmVarDecl pointers. void DiagnoseUnusedParameters(ArrayRef<ParmVarDecl *> Parameters); + /// Diagnose vla size declaration confusion between file scope variables + /// and function parameters + void DiagnoseVlaSizeParameter(const SmallVector<ParmVarDecl *, 16> &Params); + /// Diagnose whether the size of parameters or return value of a /// function or obj-c method definition is pass-by-value and larger than a /// specified threshold. diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 4dfde4bf8cedf..ed2e2606a91af 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -20,6 +20,7 @@ #include "clang/AST/DeclCXX.h" #include "clang/AST/DeclObjC.h" #include "clang/AST/DeclTemplate.h" +#include "clang/AST/DynamicRecursiveASTVisitor.h" #include "clang/AST/EvaluatedExprVisitor.h" #include "clang/AST/Expr.h" #include "clang/AST/ExprCXX.h" @@ -10057,6 +10058,76 @@ static bool isStdBuiltin(ASTContext &Ctx, FunctionDecl *FD, } } +void Sema::DiagnoseVlaSizeParameter( + const SmallVector<ParmVarDecl *, 16> &Params) { + // 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()); + SizeExprND && 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 ConstDynamicRecursiveASTVisitor { + SmallVectorImpl<const DeclRefExpr *> &Found; + + public: + DeclRefFinder(SmallVectorImpl<const DeclRefExpr *> &Found) + : Found(Found) {} + bool VisitDeclRefExpr(const DeclRefExpr *DRE) override { + Found.push_back(DRE); + return true; + } + } Finder(DRESizeExprs); + Finder.TraverseStmt(VAT->getSizeExpr()); + }); +} + NamedDecl* Sema::ActOnFunctionDeclarator(Scope *S, Declarator &D, DeclContext *DC, TypeSourceInfo *TInfo, LookupResult &Previous, @@ -10566,6 +10637,10 @@ Sema::ActOnFunctionDeclarator(Scope *S, Declarator &D, DeclContext *DC, } } + if (getLangOpts().BoundsSafety) { + DiagnoseVlaSizeParameter(Params); + } + 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 diff --git a/clang/test/Sema/vla-potential-size-confusion.c b/clang/test/Sema/vla-potential-size-confusion.c new file mode 100644 index 0000000000000..85f9e2ae09540 --- /dev/null +++ b/clang/test/Sema/vla-potential-size-confusion.c @@ -0,0 +1,72 @@ +// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 6 +// RUN: %clang_cc1 -fexperimental-bounds-safety %s -std=c23 -verify=expected,c -fsyntax-only +// RUN: %clang_cc1 -fexperimental-bounds-safety %s -std=c23 -verify=good -fsyntax-only -Wno-vla +// RUN: %clang_cc1 -fexperimental-bounds-safety -x c++ %s -verify -fsyntax-only +// RUN: %clang_cc1 -fexperimental-bounds-safety -DCARET -fsyntax-only -std=c23 -fno-diagnostics-show-line-numbers -fcaret-diagnostics-max-lines=1 %s + +// 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 + constexpr static int y = 12; + + 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}} + + void member_function_with_const_arr(int cla[y], int y); // no diagnostic expected +}; +#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 `````````` </details> https://github.com/llvm/llvm-project/pull/181550 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
