[clang] fc031d2 - Switch the default of VerifyIntegerConstantExpression from constant
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 15 16:58:59 PDT 2020
Author: Richard Smith
Date: 2020-10-15T16:58:47-07:00
New Revision: fc031d29bea856f2b91a250fd81c5f9fb79dbe07
URL: https://github.com/llvm/llvm-project/commit/fc031d29bea856f2b91a250fd81c5f9fb79dbe07
DIFF: https://github.com/llvm/llvm-project/commit/fc031d29bea856f2b91a250fd81c5f9fb79dbe07.diff
LOG: Switch the default of VerifyIntegerConstantExpression from constant
folding to not constant folding.
Constant folding of ICEs is done as a GCC compatibility measure, but new
code was picking it up, presumably by accident, due to the bad default.
While here, also switch the flag from a bool to an enum to make it more
obvious what it means at call sites. This highlighted a couple of places
where our behavior is different between C++11 and C++14 due to switching
from checking for an ICE to checking for a converted constant
expression (where there is no 'fold' codepath).
Added:
Modified:
clang/include/clang/Sema/Sema.h
clang/lib/Sema/SemaDecl.cpp
clang/lib/Sema/SemaDeclAttr.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/lib/Sema/SemaExceptionSpec.cpp
clang/lib/Sema/SemaExpr.cpp
clang/lib/Sema/SemaExprCXX.cpp
clang/lib/Sema/SemaInit.cpp
clang/lib/Sema/SemaOpenMP.cpp
clang/lib/Sema/SemaStmt.cpp
clang/lib/Sema/SemaTemplate.cpp
clang/lib/Sema/SemaType.cpp
clang/test/SemaCXX/enum.cpp
clang/test/SemaCXX/new-delete.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index b5f0c08300bf..52658715ee8c 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -11608,17 +11608,27 @@ class Sema final {
virtual ~VerifyICEDiagnoser() {}
};
+ enum AllowFoldKind {
+ NoFold,
+ AllowFold,
+ };
+
/// VerifyIntegerConstantExpression - Verifies that an expression is an ICE,
/// and reports the appropriate diagnostics. Returns false on success.
/// Can optionally return the value of the expression.
ExprResult VerifyIntegerConstantExpression(Expr *E, llvm::APSInt *Result,
VerifyICEDiagnoser &Diagnoser,
- bool AllowFold = true);
+ AllowFoldKind CanFold = NoFold);
ExprResult VerifyIntegerConstantExpression(Expr *E, llvm::APSInt *Result,
unsigned DiagID,
- bool AllowFold = true);
+ AllowFoldKind CanFold = NoFold);
ExprResult VerifyIntegerConstantExpression(Expr *E,
- llvm::APSInt *Result = nullptr);
+ llvm::APSInt *Result = nullptr,
+ AllowFoldKind CanFold = NoFold);
+ ExprResult VerifyIntegerConstantExpression(Expr *E,
+ AllowFoldKind CanFold = NoFold) {
+ return VerifyIntegerConstantExpression(E, nullptr, CanFold);
+ }
/// VerifyBitField - verifies that a bit field expression is an ICE and has
/// the correct width, and that the field type is valid.
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 9a6682e837dd..8542c20ec069 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -16418,7 +16418,7 @@ ExprResult Sema::VerifyBitField(SourceLocation FieldLoc,
return BitWidth;
llvm::APSInt Value;
- ExprResult ICE = VerifyIntegerConstantExpression(BitWidth, &Value);
+ ExprResult ICE = VerifyIntegerConstantExpression(BitWidth, &Value, AllowFold);
if (ICE.isInvalid())
return ICE;
BitWidth = ICE.get();
@@ -17536,6 +17536,8 @@ EnumConstantDecl *Sema::CheckEnumConstant(EnumDecl *Enum,
if (Enum->isDependentType() || Val->isTypeDependent())
EltTy = Context.DependentTy;
else {
+ // FIXME: We don't allow folding in C++11 mode for an enum with a fixed
+ // underlying type, but do allow it in all other contexts.
if (getLangOpts().CPlusPlus11 && Enum->isFixed()) {
// C++11 [dcl.enum]p5: If the underlying type is fixed, [...] the
// constant-expression in the enumerator-definition shall be a converted
@@ -17549,8 +17551,9 @@ EnumConstantDecl *Sema::CheckEnumConstant(EnumDecl *Enum,
else
Val = Converted.get();
} else if (!Val->isValueDependent() &&
- !(Val = VerifyIntegerConstantExpression(Val,
- &EnumVal).get())) {
+ !(Val =
+ VerifyIntegerConstantExpression(Val, &EnumVal, AllowFold)
+ .get())) {
// C99 6.7.2.2p2: Make sure we have an integer constant expression.
} else {
if (Enum->isComplete()) {
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 0ccfb1504636..91d1e7edb489 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -3723,10 +3723,8 @@ void Sema::AddAlignValueAttr(Decl *D, const AttributeCommonInfo &CI, Expr *E) {
if (!E->isValueDependent()) {
llvm::APSInt Alignment;
- ExprResult ICE
- = VerifyIntegerConstantExpression(E, &Alignment,
- diag::err_align_value_attribute_argument_not_int,
- /*AllowFold*/ false);
+ ExprResult ICE = VerifyIntegerConstantExpression(
+ E, &Alignment, diag::err_align_value_attribute_argument_not_int);
if (ICE.isInvalid())
return;
@@ -3832,10 +3830,8 @@ void Sema::AddAlignedAttr(Decl *D, const AttributeCommonInfo &CI, Expr *E,
// FIXME: Cache the number on the AL object?
llvm::APSInt Alignment;
- ExprResult ICE
- = VerifyIntegerConstantExpression(E, &Alignment,
- diag::err_aligned_attribute_argument_not_int,
- /*AllowFold*/ false);
+ ExprResult ICE = VerifyIntegerConstantExpression(
+ E, &Alignment, diag::err_aligned_attribute_argument_not_int);
if (ICE.isInvalid())
return;
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 138faa161c4e..cbcaf3cc4360 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -1078,7 +1078,7 @@ static IsTupleLike isTupleLike(Sema &S, SourceLocation Loc, QualType T,
if (E.isInvalid())
return IsTupleLike::Error;
- E = S.VerifyIntegerConstantExpression(E.get(), &Size, Diagnoser, false);
+ E = S.VerifyIntegerConstantExpression(E.get(), &Size, Diagnoser);
if (E.isInvalid())
return IsTupleLike::Error;
@@ -15996,9 +15996,10 @@ Decl *Sema::BuildStaticAssertDeclaration(SourceLocation StaticAssertLoc,
AssertExpr = FullAssertExpr.get();
llvm::APSInt Cond;
- if (!Failed && VerifyIntegerConstantExpression(AssertExpr, &Cond,
- diag::err_static_assert_expression_is_not_constant,
- /*AllowFold=*/false).isInvalid())
+ if (!Failed && VerifyIntegerConstantExpression(
+ AssertExpr, &Cond,
+ diag::err_static_assert_expression_is_not_constant)
+ .isInvalid())
Failed = true;
if (!Failed && !Cond) {
diff --git a/clang/lib/Sema/SemaExceptionSpec.cpp b/clang/lib/Sema/SemaExceptionSpec.cpp
index d7695f9d7d7a..851e28741e49 100644
--- a/clang/lib/Sema/SemaExceptionSpec.cpp
+++ b/clang/lib/Sema/SemaExceptionSpec.cpp
@@ -99,9 +99,7 @@ ExprResult Sema::ActOnNoexceptSpec(SourceLocation NoexceptLoc,
llvm::APSInt Result;
Converted = VerifyIntegerConstantExpression(
- Converted.get(), &Result,
- diag::err_noexcept_needs_constant_expression,
- /*AllowFold*/ false);
+ Converted.get(), &Result, diag::err_noexcept_needs_constant_expression);
if (!Converted.isInvalid())
EST = !Result ? EST_NoexceptFalse : EST_NoexceptTrue;
return Converted;
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 0407d5bb7f6c..7b4e9edbfbf3 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -14989,9 +14989,8 @@ ExprResult Sema::ActOnChooseExpr(SourceLocation BuiltinLoc,
} else {
// The conditional expression is required to be a constant expression.
llvm::APSInt condEval(32);
- ExprResult CondICE
- = VerifyIntegerConstantExpression(CondExpr, &condEval,
- diag::err_typecheck_choose_expr_requires_constant, false);
+ ExprResult CondICE = VerifyIntegerConstantExpression(
+ CondExpr, &condEval, diag::err_typecheck_choose_expr_requires_constant);
if (CondICE.isInvalid())
return ExprError();
CondExpr = CondICE.get();
@@ -15853,7 +15852,8 @@ bool Sema::DiagnoseAssignmentResult(AssignConvertType ConvTy,
}
ExprResult Sema::VerifyIntegerConstantExpression(Expr *E,
- llvm::APSInt *Result) {
+ llvm::APSInt *Result,
+ AllowFoldKind CanFold) {
class SimpleICEDiagnoser : public VerifyICEDiagnoser {
public:
SemaDiagnosticBuilder diagnoseNotICEType(Sema &S, SourceLocation Loc,
@@ -15866,13 +15866,13 @@ ExprResult Sema::VerifyIntegerConstantExpression(Expr *E,
}
} Diagnoser;
- return VerifyIntegerConstantExpression(E, Result, Diagnoser);
+ return VerifyIntegerConstantExpression(E, Result, Diagnoser, CanFold);
}
ExprResult Sema::VerifyIntegerConstantExpression(Expr *E,
llvm::APSInt *Result,
unsigned DiagID,
- bool AllowFold) {
+ AllowFoldKind CanFold) {
class IDDiagnoser : public VerifyICEDiagnoser {
unsigned DiagID;
@@ -15885,7 +15885,7 @@ ExprResult Sema::VerifyIntegerConstantExpression(Expr *E,
}
} Diagnoser(DiagID);
- return VerifyIntegerConstantExpression(E, Result, Diagnoser, AllowFold);
+ return VerifyIntegerConstantExpression(E, Result, Diagnoser, CanFold);
}
Sema::SemaDiagnosticBuilder
@@ -15902,7 +15902,7 @@ Sema::VerifyICEDiagnoser::diagnoseFold(Sema &S, SourceLocation Loc) {
ExprResult
Sema::VerifyIntegerConstantExpression(Expr *E, llvm::APSInt *Result,
VerifyICEDiagnoser &Diagnoser,
- bool AllowFold) {
+ AllowFoldKind CanFold) {
SourceLocation DiagLoc = E->getBeginLoc();
if (getLangOpts().CPlusPlus11) {
@@ -16020,7 +16020,7 @@ Sema::VerifyIntegerConstantExpression(Expr *E, llvm::APSInt *Result,
Notes.clear();
}
- if (!Folded || !AllowFold) {
+ if (!Folded || !CanFold) {
if (!Diagnoser.Suppress) {
Diagnoser.diagnoseNotICE(*this, DiagLoc) << E->getSourceRange();
for (const PartialDiagnosticAt &Note : Notes)
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index b96649597274..80b1e90faa0e 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -1767,6 +1767,8 @@ Sema::ActOnCXXNew(SourceLocation StartLoc, bool UseGlobal,
DeclaratorChunk::ArrayTypeInfo &Array = D.getTypeObject(I).Arr;
if (Expr *NumElts = (Expr *)Array.NumElts) {
if (!NumElts->isTypeDependent() && !NumElts->isValueDependent()) {
+ // FIXME: GCC permits constant folding here. We should either do so consistently
+ // or not do so at all, rather than changing behavior in C++14 onwards.
if (getLangOpts().CPlusPlus14) {
// C++1y [expr.new]p6: Every constant-expression in a noptr-new-declarator
// shall be a converted constant expression (5.19) of type std::size_t
@@ -1777,10 +1779,10 @@ Sema::ActOnCXXNew(SourceLocation StartLoc, bool UseGlobal,
CCEK_ArrayBound)
.get();
} else {
- Array.NumElts
- = VerifyIntegerConstantExpression(NumElts, nullptr,
- diag::err_new_array_nonconst)
- .get();
+ Array.NumElts =
+ VerifyIntegerConstantExpression(
+ NumElts, nullptr, diag::err_new_array_nonconst, AllowFold)
+ .get();
}
if (!Array.NumElts)
return ExprError();
@@ -5486,9 +5488,9 @@ static uint64_t EvaluateArrayTypeTrait(Sema &Self, ArrayTypeTrait ATT,
case ATT_ArrayExtent: {
llvm::APSInt Value;
uint64_t Dim;
- if (Self.VerifyIntegerConstantExpression(DimExpr, &Value,
- diag::err_dimension_expr_not_constant_integer,
- false).isInvalid())
+ if (Self.VerifyIntegerConstantExpression(
+ DimExpr, &Value, diag::err_dimension_expr_not_constant_integer)
+ .isInvalid())
return 0;
if (Value.isSigned() && Value.isNegative()) {
Self.Diag(KeyLoc, diag::err_dimension_expr_not_constant_integer)
diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index 751b785ce531..fc3ff1412219 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -3130,7 +3130,8 @@ CheckArrayDesignatorExpr(Sema &S, Expr *Index, llvm::APSInt &Value) {
SourceLocation Loc = Index->getBeginLoc();
// Make sure this is an integer constant expression.
- ExprResult Result = S.VerifyIntegerConstantExpression(Index, &Value);
+ ExprResult Result =
+ S.VerifyIntegerConstantExpression(Index, &Value, Sema::AllowFold);
if (Result.isInvalid())
return Result;
diff --git a/clang/lib/Sema/SemaOpenMP.cpp b/clang/lib/Sema/SemaOpenMP.cpp
index d0b5a0d03e2d..961a004e95e1 100644
--- a/clang/lib/Sema/SemaOpenMP.cpp
+++ b/clang/lib/Sema/SemaOpenMP.cpp
@@ -5836,7 +5836,8 @@ Sema::DeclGroupPtrTy Sema::ActOnOpenMPDeclareSimdDirective(
NewStep = PerformOpenMPImplicitIntegerConversion(Step->getExprLoc(), Step)
.get();
if (NewStep)
- NewStep = VerifyIntegerConstantExpression(NewStep).get();
+ NewStep =
+ VerifyIntegerConstantExpression(NewStep, /*FIXME*/ AllowFold).get();
}
NewSteps.push_back(NewStep);
}
@@ -12812,7 +12813,8 @@ ExprResult Sema::VerifyPositiveIntegerConstantInClause(Expr *E,
E->isInstantiationDependent() || E->containsUnexpandedParameterPack())
return E;
llvm::APSInt Result;
- ExprResult ICE = VerifyIntegerConstantExpression(E, &Result);
+ ExprResult ICE =
+ VerifyIntegerConstantExpression(E, &Result, /*FIXME*/ AllowFold);
if (ICE.isInvalid())
return ExprError();
if ((StrictlyPositive && !Result.isStrictlyPositive()) ||
diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index 5b4aaa678974..431d592e24ba 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -467,7 +467,7 @@ Sema::ActOnCaseExpr(SourceLocation CaseLoc, ExprResult Val) {
ExprResult ER = E;
if (!E->isValueDependent())
- ER = VerifyIntegerConstantExpression(E);
+ ER = VerifyIntegerConstantExpression(E, AllowFold);
if (!ER.isInvalid())
ER = DefaultLvalueConversion(ER.get());
if (!ER.isInvalid())
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 4ecae8faad66..660964c8f92b 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -7105,8 +7105,7 @@ ExprResult Sema::CheckTemplateArgument(NonTypeTemplateParmDecl *Param,
}
} Diagnoser(ArgType);
- Arg = VerifyIntegerConstantExpression(Arg, &Value, Diagnoser,
- false).get();
+ Arg = VerifyIntegerConstantExpression(Arg, &Value, Diagnoser).get();
if (!Arg)
return ExprError();
}
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index b823c962f21d..5eb05bfcaee3 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -2196,7 +2196,8 @@ QualType Sema::BuildExtIntType(bool IsUnsigned, Expr *BitWidth,
return Context.getDependentExtIntType(IsUnsigned, BitWidth);
llvm::APSInt Bits(32);
- ExprResult ICE = VerifyIntegerConstantExpression(BitWidth, &Bits);
+ ExprResult ICE =
+ VerifyIntegerConstantExpression(BitWidth, &Bits, /*FIXME*/ AllowFold);
if (ICE.isInvalid())
return QualType();
@@ -2272,9 +2273,13 @@ static ExprResult checkArraySize(Sema &S, Expr *&ArraySize,
}
} Diagnoser(VLADiag, VLAIsError);
+ // FIXME: GCC does *not* allow folding here in general; see PR44406.
+ // For GCC compatibility, we should remove this folding and leave it to
+ // TryFixVariablyModifiedType to convert VLAs to constant array types.
ExprResult R = S.VerifyIntegerConstantExpression(
ArraySize, &SizeVal, Diagnoser,
- (S.LangOpts.GNUMode || S.LangOpts.OpenCL));
+ (S.LangOpts.GNUMode || S.LangOpts.OpenCL) ? Sema::AllowFold
+ : Sema::NoFold);
if (Diagnoser.IsVLA)
return ExprResult();
return R;
diff --git a/clang/test/SemaCXX/enum.cpp b/clang/test/SemaCXX/enum.cpp
index b193a17ebf9e..01bc3a40fc03 100644
--- a/clang/test/SemaCXX/enum.cpp
+++ b/clang/test/SemaCXX/enum.cpp
@@ -107,6 +107,16 @@ enum { overflow = 123456 * 234567 };
// expected-warning at -5 {{overflow in expression; result is -1106067520 with type 'int'}}
#endif
+// FIXME: This is not consistent with the above case.
+enum NoFold : int { overflow2 = 123456 * 234567 };
+#if __cplusplus >= 201103L
+// expected-error at -2 {{enumerator value is not a constant expression}}
+// expected-note at -3 {{value 28958703552 is outside the range of representable values}}
+#else
+// expected-warning at -5 {{overflow in expression; result is -1106067520 with type 'int'}}
+// expected-warning at -6 {{extension}}
+#endif
+
// PR28903
struct PR28903 {
enum {
diff --git a/clang/test/SemaCXX/new-delete.cpp b/clang/test/SemaCXX/new-delete.cpp
index 64d32f235b59..2fc917ce7488 100644
--- a/clang/test/SemaCXX/new-delete.cpp
+++ b/clang/test/SemaCXX/new-delete.cpp
@@ -1,6 +1,7 @@
// RUN: %clang_cc1 -fsyntax-only -verify %s -triple=i686-pc-linux-gnu -Wno-new-returns-null
// RUN: %clang_cc1 -fsyntax-only -verify %s -triple=i686-pc-linux-gnu -Wno-new-returns-null -std=c++98
// RUN: %clang_cc1 -fsyntax-only -verify %s -triple=i686-pc-linux-gnu -Wno-new-returns-null -std=c++11
+// RUN: %clang_cc1 -fsyntax-only -verify %s -triple=i686-pc-linux-gnu -Wno-new-returns-null -std=c++14
#include <stddef.h>
@@ -621,3 +622,13 @@ int *p0 = dependent_array_size();
int *p3 = dependent_array_size(1, 2, 3);
int *fail = dependent_array_size("hello"); // expected-note {{instantiation of}}
#endif
+
+// FIXME: Our behavior here is incredibly inconsistent. GCC allows
+// constant-folding in array bounds in new-expressions.
+int (*const_fold)[12] = new int[3][&const_fold + 12 - &const_fold];
+#if __cplusplus >= 201402L
+// expected-error at -2 {{array size is not a constant expression}}
+// expected-note at -3 {{cannot refer to element 12 of non-array}}
+#elif __cplusplus < 201103L
+// expected-error at -5 {{cannot allocate object of variably modified type}}
+#endif
More information about the cfe-commits
mailing list