Commit 7339c0f7 authored by Aaron Ballman's avatar Aaron Ballman
Browse files

Diagnose use of VLAs in C++ by default 

Clang supports VLAs in C++ as an extension, but we currently only warn
on their use when you pass -Wvla, -Wvla-extension, or -pedantic.
However, VLAs as they're expressed in C have been considered by WG21
and rejected, are easy to use accidentally to the surprise of users
(e.g., https://ddanilov.me/default-non-standard-features/), and they
have potential security implications beyond constant-size arrays
(https://wiki.sei.cmu.edu/confluence/display/c/ARR32-C.+Ensure+size+arguments+for+variable+length+arrays+are+in+a+valid+range).
C++ users should strongly consider using other functionality such as
std::vector instead.

This seems like sufficiently compelling evidence to warn users about
VLA use by default in C++ modes. This patch enables the -Wvla-extension
diagnostic group in C++ language modes by default, and adds the warning
group to -Wall in GNU++ language modes. The warning is still opt-in in
C language modes, where support for VLAs is somewhat less surprising to
users.

RFC: https://discourse.llvm.org/t/rfc-diagnosing-use-of-vlas-in-c/73109
Fixes https://github.com/llvm/llvm-project/issues/62836
Differential Revision: https://reviews.llvm.org/D156565
parent 2ec7bba7

clang/docs/ReleaseNotes.rst

+5 −0
Original line number Diff line number Diff line
@@ -346,6 +346,11 @@ Improvements to Clang's diagnostics 
      |               ~~~~~~~~~^~~~~~~~

- Clang now always diagnoses when using non-standard layout types in ``offsetof`` .

  (`#64619: <https://github.com/llvm/llvm-project/issues/64619>`_)

- Clang now diagnoses use of variable-length arrays in C++ by default (and

  under ``-Wall`` in GNU++ mode). This is an extension supported by Clang and

  GCC, but is very easy to accidentally use without realizing it's a

  nonportable construct that has different semantics from a constant-sized

  array. (`#62836 <https://github.com/llvm/llvm-project/issues/62836>`_)



Bug Fixes in This Version

-------------------------

clang/include/clang/Basic/DiagnosticGroups.td

+3 −2
Original line number Diff line number Diff line
@@ -848,7 +848,8 @@ def OverridingMethodMismatch : DiagGroup<"overriding-method-mismatch">; 
def VariadicMacros : DiagGroup<"variadic-macros">;

def VectorConversion : DiagGroup<"vector-conversion">;      // clang specific

def VexingParse : DiagGroup<"vexing-parse">;

def VLAExtension : DiagGroup<"vla-extension">;

def VLAUseStaticAssert : DiagGroup<"vla-extension-static-assert">;

def VLAExtension : DiagGroup<"vla-extension", [VLAUseStaticAssert]>;

def VLA : DiagGroup<"vla", [VLAExtension]>;

def VolatileRegisterVar : DiagGroup<"volatile-register-var">;

def Visibility : DiagGroup<"visibility">;
@@ -1085,7 +1086,7 @@ def Consumed : DiagGroup<"consumed">; 
// warning should be active _only_ when -Wall is passed in, mark it as

// DefaultIgnore in addition to putting it here.

def All : DiagGroup<"all", [Most, Parentheses, Switch, SwitchBool,

                            MisleadingIndentation, PackedNonPod]>;

                            MisleadingIndentation, PackedNonPod, VLAExtension]>;



// Warnings that should be in clang-cl /w4.

def : DiagGroup<"CL4", [All, Extra]>;

clang/include/clang/Basic/DiagnosticSemaKinds.td

+12 −0
Original line number Diff line number Diff line
@@ -137,6 +137,18 @@ def err_half_const_requires_fp16 : Error< 
// C99 variable-length arrays

def ext_vla : Extension<"variable length arrays are a C99 feature">,

  InGroup<VLAExtension>;

// In C++ language modes, we warn by default as an extension, while in GNU++

// language modes, we warn as an extension but add the warning group to -Wall.

def ext_vla_cxx : ExtWarn<

  "variable length arrays in C++ are a Clang extension">,

  InGroup<VLAExtension>;

def ext_vla_cxx_in_gnu_mode : Extension<ext_vla_cxx.Summary>,

  InGroup<VLAExtension>;

def ext_vla_cxx_static_assert : ExtWarn<

  "variable length arrays in C++ are a Clang extension; did you mean to use "

  "'static_assert'?">, InGroup<VLAUseStaticAssert>;

def ext_vla_cxx_in_gnu_mode_static_assert : Extension<

  ext_vla_cxx_static_assert.Summary>, InGroup<VLAUseStaticAssert>;

def warn_vla_used : Warning<"variable length array used">,

  InGroup<VLA>, DefaultIgnore;

def err_vla_in_sfinae : Error<

clang/lib/Sema/SemaType.cpp

+27 −0
Original line number Diff line number Diff line
@@ -2582,6 +2582,24 @@ QualType Sema::BuildArrayType(QualType T, ArrayType::ArraySizeModifier ASM, 
    return QualType();

  }



  auto IsStaticAssertLike = [](const Expr *ArraySize, ASTContext &Context) {

    // If the array size expression is a conditional expression whose branches

    // are both integer constant expressions, one negative and one positive,

    // then it's assumed to be like an old-style static assertion. e.g.,

    //   int old_style_assert[expr ? 1 : -1];

    // We will accept any integer constant expressions instead of assuming the

    // values 1 and -1 are always used.

    if (const auto *CondExpr = dyn_cast_if_present<ConditionalOperator>(

            ArraySize->IgnoreParenImpCasts())) {

      std::optional<llvm::APSInt> LHS =

          CondExpr->getLHS()->getIntegerConstantExpr(Context);

      std::optional<llvm::APSInt> RHS =

          CondExpr->getRHS()->getIntegerConstantExpr(Context);

      return LHS && RHS && LHS->isNegative() != RHS->isNegative();

    }

    return false;

  };



  // VLAs always produce at least a -Wvla diagnostic, sometimes an error.

  unsigned VLADiag;

  bool VLAIsError;
@@ -2598,6 +2616,15 @@ QualType Sema::BuildArrayType(QualType T, ArrayType::ArraySizeModifier ASM, 
  } else if (getLangOpts().OpenMP && isInOpenMPTaskUntiedContext()) {

    VLADiag = diag::err_openmp_vla_in_task_untied;

    VLAIsError = true;

  } else if (getLangOpts().CPlusPlus) {

    if (getLangOpts().CPlusPlus11 && IsStaticAssertLike(ArraySize, Context))

      VLADiag = getLangOpts().GNUMode

                    ? diag::ext_vla_cxx_in_gnu_mode_static_assert

                    : diag::ext_vla_cxx_static_assert;

    else

      VLADiag = getLangOpts().GNUMode ? diag::ext_vla_cxx_in_gnu_mode

                                      : diag::ext_vla_cxx;

    VLAIsError = false;

  } else {

    VLADiag = diag::ext_vla;

    VLAIsError = false;

clang/test/AST/Interp/literals.cpp

+4 −4
Original line number Diff line number Diff line 
// RUN: %clang_cc1 -fexperimental-new-constant-interpreter -fms-extensions -std=c++11 -verify %s

// RUN: %clang_cc1 -fexperimental-new-constant-interpreter -fms-extensions -std=c++20 -verify %s

// RUN: %clang_cc1 -std=c++11 -fms-extensions -verify=ref %s

// RUN: %clang_cc1 -std=c++20 -fms-extensions -verify=ref %s

// RUN: %clang_cc1 -fexperimental-new-constant-interpreter -Wno-vla -fms-extensions -std=c++11 -verify %s

// RUN: %clang_cc1 -fexperimental-new-constant-interpreter -Wno-vla -fms-extensions -std=c++20 -verify %s

// RUN: %clang_cc1 -std=c++11 -fms-extensions -Wno-vla -verify=ref %s

// RUN: %clang_cc1 -std=c++20 -fms-extensions -Wno-vla -verify=ref %s



#define INT_MIN (~__INT_MAX__)

#define INT_MAX __INT_MAX__