[Clang] Handle consteval expression in array bounds expressions #66222

cor3ntin · 2023-09-13T15:53:06Z

The bounds of a c++ array is a constant-expression. And in C++ it is also a constant expression.

But we also support VLAs, ie arrays with non-constant bounds.

We need to take care to handle the case of a consteval function (which are specified to be only immediately called in non-constant contexts) that appear in arrays bounds.

This introduces Sema::isAlwayConstantEvaluatedContext, and a flag in ExpressionEvaluationContextRecord, such that immediate functions in array bounds are always immediately invoked.

Sema had both isConstantEvaluatedContext and
isConstantEvaluated, so I took the opportunity to cleanup that.

The change in TimeProfilerTest.cpp is an unfortunate manifestation of the problem that #66203 seeks to address.

Fixes #65520

llvmbot · 2023-09-13T15:54:13Z

@llvm/pr-subscribers-clang

Changes

The bounds of a c++ array is a _constant-expression_. And in C++ it is also a constant expression.

But we also support VLAs, ie arrays with non-constant bounds.

We need to take care to handle the case of a consteval function (which are specified to be only immediately called in non-constant contexts) that appear in arrays bounds.

This introduces Sema::isAlwayConstantEvaluatedContext, and a flag in ExpressionEvaluationContextRecord, such that immediate functions in array bounds are always immediately invoked.

Sema had both isConstantEvaluatedContext and
isConstantEvaluated, so I took the opportunity to cleanup that.

The change in TimeProfilerTest.cpp is an unfortunate manifestation of the problem that #66203 seeks to address.

Fixes #65520

Patch is 20.71 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/66222.diff

11 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+4)
(modified) clang/include/clang/Parse/Parser.h (+1)
(modified) clang/include/clang/Sema/Sema.h (+32-28)
(modified) clang/lib/Parse/ParseDecl.cpp (+1-1)
(modified) clang/lib/Parse/ParseExpr.cpp (+9)
(modified) clang/lib/Sema/SemaCUDA.cpp (+1-1)
(modified) clang/lib/Sema/SemaChecking.cpp (+30-25)
(modified) clang/lib/Sema/SemaExpr.cpp (+2-2)
(modified) clang/lib/Sema/TreeTransform.h (+3)
(modified) clang/test/SemaCXX/cxx2a-consteval.cpp (+15)
(modified) clang/unittests/Support/TimeProfilerTest.cpp (+1)

<pre>
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 3cdad2f7b9f0e5a..1d2c0830b2910d5 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -281,6 +281,10 @@ Bug Fixes to C++ Support
a non-template inner-class between the function and the class template.
(#65810 &lt;https://github.com/llvm/llvm-project/issues/65810&gt;_)

+- Fix a crash when calling a consteval function in an expression used as

the size of an array.
(#65520 &lt;https://github.com/llvm/llvm-project/issues/65520&gt;_)

Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^

Fixed an import failure of recursive friend class template.
diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h
index f599b8b98d031fb..7303db939de7fe0 100644
--- a/clang/include/clang/Parse/Parser.h
+++ b/clang/include/clang/Parse/Parser.h
@@ -1766,6 +1766,7 @@ class Parser : public CodeCompletionHandler {
ExprResult ParseConstantExpressionInExprEvalContext(
TypeCastState isTypeCast = NotTypeCast);
ExprResult ParseConstantExpression();

ExprResult ParseArrayBoundExpression();
ExprResult ParseCaseExpression(SourceLocation CaseLoc);
ExprResult ParseConstraintExpression();
ExprResult
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 304108df9f8d029..77a8cb67be16c91 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -1062,20 +1062,6 @@ class Sema final {
}
};

/// Whether the AST is currently being rebuilt to correct immediate
/// invocations. Immediate invocation candidates and references to consteval
/// functions aren't tracked when this is set.
bool RebuildingImmediateInvocation = false;
/// Used to change context to isConstantEvaluated without pushing a heavy
/// ExpressionEvaluationContextRecord object.
bool isConstantEvaluatedOverride;
bool isConstantEvaluated() const {
return ExprEvalContexts.back().isConstantEvaluated() ||
```
      isConstantEvaluatedOverride;
```
}
/// RAII object to handle the state changes required to synthesize
/// a function body.
class SynthesizedFunctionScope {
@@ -1361,6 +1347,10 @@ class Sema final {

bool IsCurrentlyCheckingDefaultArgumentOrInitializer = false;

// We are in a constant context, but we also allow
// non constant expressions, for example for array bounds (which may be VLAs).
bool InConditionallyConstantEvaluateContext = false;
// When evaluating immediate functions in the initializer of a default
// argument or default member initializer, this is the declaration whose
// default initializer is being evaluated and the location of the call
@@ -9844,30 +9834,44 @@ class Sema final {
/// diagnostics that will be suppressed.
std::optional<sema::TemplateDeductionInfo *> isSFINAEContext() const;

/// Determines whether we are currently in a context that
/// is not evaluated as per C++ [expr] p5.
bool isUnevaluatedContext() const {

/// Whether the AST is currently being rebuilt to correct immediate
/// invocations. Immediate invocation candidates and references to consteval
/// functions aren't tracked when this is set.
bool RebuildingImmediateInvocation = false;
/// Used to change context to isConstantEvaluated without pushing a heavy
/// ExpressionEvaluationContextRecord object.
bool isConstantEvaluatedOverride;
const ExpressionEvaluationContextRecord &currentEvaluationContext() const {
assert(!ExprEvalContexts.empty() &&
"Must be in an expression evaluation context");

return ExprEvalContexts.back().isUnevaluated();
}

return ExprEvalContexts.back();
};

bool isConstantEvaluatedContext() const {

assert(!ExprEvalContexts.empty() &&

      &amp;quot;Must be in an expression evaluation context&amp;quot;);

return ExprEvalContexts.back().isConstantEvaluated();

return currentEvaluationContext().isConstantEvaluated() ||
```
      isConstantEvaluatedOverride;
```
}
bool isAlwaysConstantEvaluatedContext() const {
const ExpressionEvaluationContextRecord &Ctx = currentEvaluationContext();
return (Ctx.isConstantEvaluated() || isConstantEvaluatedOverride) &&

      !Ctx.InConditionallyConstantEvaluateContext;

}
/// Determines whether we are currently in a context that
/// is not evaluated as per C++ [expr] p5.
bool isUnevaluatedContext() const {
return currentEvaluationContext().isUnevaluated();
}

bool isImmediateFunctionContext() const {

assert(!ExprEvalContexts.empty() &&

      &amp;quot;Must be in an expression evaluation context&amp;quot;);

return ExprEvalContexts.back().isImmediateFunctionContext();

return currentEvaluationContext().isImmediateFunctionContext();
}

bool isCheckingDefaultArgumentOrInitializer() const {

assert(!ExprEvalContexts.empty() &&

      &amp;quot;Must be in an expression evaluation context&amp;quot;);

const ExpressionEvaluationContextRecord &Ctx = ExprEvalContexts.back();

const ExpressionEvaluationContextRecord &Ctx = currentEvaluationContext();
return (Ctx.Context ==
ExpressionEvaluationContext::PotentiallyEvaluatedIfUsed) ||
Ctx.IsCurrentlyCheckingDefaultArgumentOrInitializer;
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 748b9d53c9f5b33..5114e181582d91a 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -7687,7 +7687,7 @@ void Parser::ParseBracketDeclarator(Declarator &D) {
// Parse the constant-expression or assignment-expression now (depending
// on dialect).
if (getLangOpts().CPlusPlus) {

 NumElements = ParseConstantExpression();

```
 NumElements = ParseArrayBoundExpression();
```
} else {
EnterExpressionEvaluationContext Unevaluated(
Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated);
diff --git a/clang/lib/Parse/ParseExpr.cpp b/clang/lib/Parse/ParseExpr.cpp
index f8bf785da2896a3..e6de742a01f3f6d 100644
--- a/clang/lib/Parse/ParseExpr.cpp
+++ b/clang/lib/Parse/ParseExpr.cpp
@@ -221,6 +221,15 @@ ExprResult Parser::ParseConstantExpression() {
return ParseConstantExpressionInExprEvalContext(NotTypeCast);
}

+ExprResult Parser::ParseArrayBoundExpression() {

EnterExpressionEvaluationContext ConstantEvaluated(

 Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated);

// If we parse the bound of a VLA... we parse a non-constant
// constant-expression!
Actions.ExprEvalContexts.back().InConditionallyConstantEvaluateContext = true;
return ParseConstantExpressionInExprEvalContext(NotTypeCast);
+}

ExprResult Parser::ParseCaseExpression(SourceLocation CaseLoc) {
EnterExpressionEvaluationContext ConstantEvaluated(
Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated);
diff --git a/clang/lib/Sema/SemaCUDA.cpp b/clang/lib/Sema/SemaCUDA.cpp
index 88f5484575db17a..bc676be371b8cf3 100644
--- a/clang/lib/Sema/SemaCUDA.cpp
+++ b/clang/lib/Sema/SemaCUDA.cpp
@@ -803,7 +803,7 @@ bool Sema::CheckCUDACall(SourceLocation Loc, FunctionDecl *Callee) {
assert(getLangOpts().CUDA && "Should only be called during CUDA compilation");
assert(Callee && "Callee may not be null.");

auto &ExprEvalCtx = ExprEvalContexts.back();

const auto &ExprEvalCtx = currentEvaluationContext();
if (ExprEvalCtx.isUnevaluated() || ExprEvalCtx.isConstantEvaluated())
return true;

diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index fad70223362eddd..5208dbd9d3c8f58 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -1068,7 +1068,7 @@ static bool ProcessFormatStringLiteral(const Expr *FormatExpr,
void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD,
CallExpr *TheCall) {
if (TheCall->isValueDependent() || TheCall->isTypeDependent() ||

```
 isConstantEvaluated())
```

```
 isConstantEvaluatedContext())
```
return;

bool UseDABAttr = false;
@@ -3175,7 +3175,7 @@ bool Sema::CheckCDEBuiltinFunctionCall(const TargetInfo &TI, unsigned BuiltinID,

bool Sema::CheckARMCoprocessorImmediate(const TargetInfo &TI,
const Expr *CoprocArg, bool WantCDE) {

if (isConstantEvaluated())

if (isConstantEvaluatedContext())
return false;

// We can't check the value of a dependent argument.
@@ -6595,7 +6595,7 @@ static void CheckNonNullArguments(Sema &S,
assert((FDecl || Proto) && "Need a function declaration or prototype");

// Already checked by constant evaluator.

if (S.isConstantEvaluated())

if (S.isConstantEvaluatedContext())
return;
// Check the attributes attached to the method/function itself.
llvm::SmallBitVector NonNullArgs;
@@ -8952,7 +8952,7 @@ bool Sema::SemaBuiltinConstantArg(CallExpr *TheCall, int ArgNum,
/// TheCall is a constant expression in the range [Low, High].
bool Sema::SemaBuiltinConstantArgRange(CallExpr *TheCall, int ArgNum,
int Low, int High, bool RangeIsError) {

if (isConstantEvaluated())

if (isConstantEvaluatedContext())
return false;
llvm::APSInt Result;

@@ -9666,7 +9666,7 @@ checkFormatStringExpr(Sema &S, const Expr *E, ArrayRef<const Expr *> Args,
llvm::SmallBitVector &CheckedVarArgs,
UncoveredArgHandler &UncoveredArg, llvm::APSInt Offset,
bool IgnoreStringsWithoutSpecifiers = false) {

if (S.isConstantEvaluated())

if (S.isConstantEvaluatedContext())
return SLCT_NotALiteral;
tryAgain:
assert(Offset.isSigned() && "invalid offset");
@@ -9706,8 +9706,8 @@ checkFormatStringExpr(Sema &S, const Expr *E, ArrayRef<const Expr *> Args,
bool CheckLeft = true, CheckRight = true;

bool Cond;

if (C->getCond()->EvaluateAsBooleanCondition(Cond, S.getASTContext(),

                                            S.isConstantEvaluated())) {

if (C->getCond()->EvaluateAsBooleanCondition(

       Cond, S.getASTContext(), S.isConstantEvaluatedContext())) {
 if (Cond)
   CheckRight = false;
 else

@@ -9971,9 +9971,11 @@ checkFormatStringExpr(Sema &S, const Expr *E, ArrayRef<const Expr *> Args,
Expr::EvalResult LResult, RResult;

   bool LIsInt = BinOp-&amp;gt;getLHS()-&amp;gt;EvaluateAsInt(

     LResult, S.Context, Expr::SE_NoSideEffects, S.isConstantEvaluated());

     LResult, S.Context, Expr::SE_NoSideEffects,

     S.isConstantEvaluatedContext());
 bool RIsInt = BinOp-&amp;gt;getRHS()-&amp;gt;EvaluateAsInt(

     RResult, S.Context, Expr::SE_NoSideEffects, S.isConstantEvaluated());

     RResult, S.Context, Expr::SE_NoSideEffects,

     S.isConstantEvaluatedContext());

 if (LIsInt != RIsInt) {
   BinaryOperatorKind BinOpKind = BinOp-&amp;gt;getOpcode();

@@ -10001,7 +10003,7 @@ checkFormatStringExpr(Sema &S, const Expr *E, ArrayRef<const Expr *> Args,
Expr::EvalResult IndexResult;
if (ASE->getRHS()->EvaluateAsInt(IndexResult, S.Context,
Expr::SE_NoSideEffects,

                                  S.isConstantEvaluated())) {

                                  S.isConstantEvaluatedContext())) {
   sumOffsets(Offset, IndexResult.Val.getInt(), BO_Add,
              /*RHS is int*/ true);
   E = ASE-&amp;gt;getBase();

@@ -13928,7 +13930,7 @@ static bool CheckTautologicalComparison(Sema &S, BinaryOperator *E,
return false;

IntRange OtherValueRange = GetExprRange(

 S.Context, Other, S.isConstantEvaluated(), /*Approximate*/ false);

```
 S.Context, Other, S.isConstantEvaluatedContext(), /*Approximate*/ false);
```
QualType OtherT = Other->getType();
if (const auto *AT = OtherT->getAs<AtomicType>())
@@ -14143,8 +14145,9 @@ static void AnalyzeComparison(Sema &S, BinaryOperator *E) {
}

// Otherwise, calculate the effective range of the signed operand.

IntRange signedRange = GetExprRange(

 S.Context, signedOperand, S.isConstantEvaluated(), /*Approximate*/ true);

IntRange signedRange =

 GetExprRange(S.Context, signedOperand, S.isConstantEvaluatedContext(),

```
              /*Approximate*/ true);
```
// Go ahead and analyze implicit conversions in the operands. Note
// that we skip the implicit conversions on both sides.
@@ -14162,7 +14165,7 @@ static void AnalyzeComparison(Sema &S, BinaryOperator *E) {
if (E->isEqualityOp()) {
unsigned comparisonWidth = S.Context.getIntWidth(T);
IntRange unsignedRange =

   GetExprRange(S.Context, unsignedOperand, S.isConstantEvaluated(),

```
   GetExprRange(S.Context, unsignedOperand, S.isConstantEvaluatedContext(),
                /*Approximate*/ true);
```
// We should never be unable to prove that the unsigned operand is
@@ -15026,7 +15029,7 @@ static void CheckImplicitConversion(Sema &S, Expr *E, QualType T,
if (Target->isUnsaturatedFixedPointType()) {
Expr::EvalResult Result;
if (E->EvaluateAsFixedPoint(Result, S.Context, Expr::SE_AllowSideEffects,

                             S.isConstantEvaluated())) {

                             S.isConstantEvaluatedContext())) {
   llvm::APFixedPoint Value = Result.Val.getFixedPoint();
   llvm::APFixedPoint MaxVal = S.Context.getFixedPointMax(T);
   llvm::APFixedPoint MinVal = S.Context.getFixedPointMin(T);

@@ -15041,7 +15044,7 @@ static void CheckImplicitConversion(Sema &S, Expr *E, QualType T,
}
} else if (Target->isIntegerType()) {
Expr::EvalResult Result;

 if (!S.isConstantEvaluated() &amp;amp;&amp;amp;

 if (!S.isConstantEvaluatedContext() &amp;amp;&amp;amp;
     E-&amp;gt;EvaluateAsFixedPoint(Result, S.Context,
                             Expr::SE_AllowSideEffects)) {
   llvm::APFixedPoint FXResult = Result.Val.getFixedPoint();

@@ -15064,7 +15067,7 @@ static void CheckImplicitConversion(Sema &S, Expr *E, QualType T,
} else if (Target->isUnsaturatedFixedPointType()) {
if (Source->isIntegerType()) {
Expr::EvalResult Result;

 if (!S.isConstantEvaluated() &amp;amp;&amp;amp;

 if (!S.isConstantEvaluatedContext() &amp;amp;&amp;amp;
     E-&amp;gt;EvaluateAsInt(Result, S.Context, Expr::SE_AllowSideEffects)) {
   llvm::APSInt Value = Result.Val.getInt();

@@ -15090,8 +15093,9 @@ static void CheckImplicitConversion(Sema &S, Expr *E, QualType T,
if (SourceBT && TargetBT && SourceBT->isIntegerType() &&
TargetBT->isFloatingType() && !IsListInit) {
// Determine the number of precision bits in the source integer type.

IntRange SourceRange = GetExprRange(S.Context, E, S.isConstantEvaluated(),

                                   /*Approximate*/ true);

IntRange SourceRange =

   GetExprRange(S.Context, E, S.isConstantEvaluatedContext(),

```
                /*Approximate*/ true);
```
unsigned int SourcePrecision = SourceRange.Width;

// Determine the number of precision bits in the
@@ -15159,8 +15163,8 @@ static void CheckImplicitConversion(Sema &S, Expr *E, QualType T,

IntRange SourceTypeRange =
IntRange::forTargetOfCanonicalType(S.Context, Source);

IntRange LikelySourceRange =

 GetExprRange(S.Context, E, S.isConstantEvaluated(), /*Approximate*/ true);

IntRange LikelySourceRange = GetExprRange(
```
 S.Context, E, S.isConstantEvaluatedContext(), /*Approximate*/ true);
```
IntRange TargetRange = IntRange::forTargetOfCanonicalType(S.Context, Target);

if (LikelySourceRange.Width > TargetRange.Width) {
@@ -15168,7 +15172,7 @@ static void CheckImplicitConversion(Sema &S, Expr *E, QualType T,
// TODO: this should happen for bitfield stores, too.
Expr::EvalResult Result;
if (E->EvaluateAsInt(Result, S.Context, Expr::SE_AllowSideEffects,

                    S.isConstantEvaluated())) {

                    S.isConstantEvaluatedContext())) {
 llvm::APSInt Value(32);
 Value = Result.Val.getInt();

@@ -16032,7 +16036,8 @@ class SequenceChecker : public ConstEvaluatedExprVisitor<SequenceChecker> {
if (!EvalOK || E->isValueDependent())
return false;
EvalOK = E->EvaluateAsBooleanCondition(

     Result, Self.SemaRef.Context, Self.SemaRef.isConstantEvaluated());

```
     Result, Self.SemaRef.Context,
```

     Self.SemaRef.isConstantEvaluatedContext());
 return EvalOK;

}

@@ -17159,7 +17164,7 @@ void Sema::CheckArrayAccess(const Expr *BaseExpr, const Expr *IndexExpr,
const ArraySubscriptExpr *ASE,
bool AllowOnePastEnd, bool IndexNegated) {
// Already diagnosed by the constant evaluator.

if (isConstantEvaluated())

if (isConstantEvaluatedContext())
return;

IndexExpr = IndexExpr->IgnoreParenImpCasts();
@@ -18548,7 +18553,7 @@ void Sema::CheckArgumentWithTypeTag(const ArgumentWithTypeTagAttr *Attr,
TypeTagData TypeInfo;
if (!GetMatchingCType(ArgumentKind, TypeTagExpr, Context,
TypeTagForDatatypeMagicValues.get(), FoundWrongKind,

                   TypeInfo, isConstantEvaluated())) {

```
                   TypeInfo, isConstantEvaluatedContext())) {
```
if (FoundWrongKind)
Diag(TypeTagExpr->getExprLoc(),
diag::warn_type_tag_for_datatype_wrong_kind)
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 92496b03ecabe54..667444917a9b31e 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -18282,7 +18282,7 @@ void Sema::MarkExpressionAsImmediateEscalating(Expr *E) {

ExprResult Sema::CheckForImmediateInvocation(ExprResult E, FunctionDecl *Decl) {
if (isUnevaluatedContext() || !E.isUsable() || !Decl ||

 !Decl-&amp;gt;isImmediateFunction() || isConstantEvaluated() ||

 !Decl-&amp;gt;isImmediateFunction() || isAlwaysConstantEvaluatedContext() ||
 isCheckingDefaultArgumentOrInitializer() ||
 RebuildingImmediateInvocation || isImmediateFunctionContext())

return E;
@@ -20668,7 +20668,7 @@ void Sema::MarkDeclRefReferenced(DeclRefExpr *E, const Expr *Base) {
OdrUse = false;

if (auto *FD = dyn_cast<FunctionDecl>(E->getDecl())) {

if (!isUnevaluatedContext() && !isConstantEvaluated() &&

if (!isUnevaluatedContext() && !isConstantEvaluatedContext() &&
!isImmediateFunctionContext() &&
!isCheckingDefaultArgumentOrInitializer() &&
FD->isImmediateFunction() && !RebuildingImmediateInvocation &&
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index 603a23275889f21..39ac6e9cfbcdcce 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -5486,6 +5486,9 @@ TreeTransform<Derived>::TransformDependentSizedArrayType(TypeLocBuilder &TLB,
EnterExpressionEvaluationContext Unevaluated(
SemaRef, Sema::ExpressionEvaluationContext::ConstantEvaluated);
// VLA bounds are not truly constant.
SemaRef.ExprEvalContexts.back().InConditionallyConstantEvaluateContext = true;
// Prefer the expression from the TypeLoc; the other may have been uniqued.
Expr *origSize = TL.getSizeExpr();
if (!origSize) origSize = T->getSizeExpr();
diff --git a/clang/test/SemaCX...

tbaederr · 2023-09-13T16:11:37Z

@hazohelet Does this collide or intersect with the work wrt. (un)evaluated contexts in https://reviews.llvm.org/D155064 ?

clang/lib/Sema/SemaChecking.cpp

shafik · 2023-09-14T02:58:41Z

clang/lib/Sema/SemaChecking.cpp

@@ -13928,7 +13930,7 @@ static bool CheckTautologicalComparison(Sema &S, BinaryOperator *E,
    return false;

  IntRange OtherValueRange = GetExprRange(
-      S.Context, Other, S.isConstantEvaluated(), /*Approximate*/ false);
+      S.Context, Other, S.isConstantEvaluatedContext(), /*Approximate*/ false);


Suggested change

S.Context, Other, S.isConstantEvaluatedContext(), /*Approximate*/ false);

S.Context, Other, S.isConstantEvaluatedContext(), /*Approximate=*/ false);

shafik · 2023-09-14T03:00:29Z

clang/include/clang/Sema/Sema.h

+
+  /// Used to change context to isConstantEvaluated without pushing a heavy
+  /// ExpressionEvaluationContextRecord object.
+  bool isConstantEvaluatedOverride;


Is there no good default value?

This is set in sema constructors, but i can move the initialization here (this is just code that i moved around). We are a bit inconsistent in general at using or not default member init.

shafik

If the function is constexpr do we treat it as a VLA?

cor3ntin · 2023-09-14T07:12:14Z

If the function is constexpr do we treat it as a VLA?

If it can be constant evaluated, yes. (So anything with UB in it is promoted to a VLA, it's fabulous)

hazohelet · 2023-09-14T11:34:30Z

@hazohelet Does this collide or intersect with the work wrt. (un)evaluated contexts in https://reviews.llvm.org/D155064 ?

I wasn't aware of VLA cases in that patch.
My patch needs modification so that use of is_constant_evaluated and consteval-if NOT considered tautologically-true when they happen InConditionallyConstantEvaluateContext, which is a few lines of change.

@cor3ntin
Is it alright to set InConditionallyConstantEvaluateContext true only when the outer evaluation context isAlwaysConstantEvaluatedContext() so that we can say the array bound expr is always constant-evaluated if it happens in always-constant-evaluated context?
Something like this:

 ExprResult Parser::ParseArrayBoundExpression() {
+  bool IsAlwaysConstantEvaluated = Actions.isAlwaysConstantEvaluatedContext();
   EnterExpressionEvaluationContext ConstantEvaluated(
       Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated);
   // If we parse the bound of a VLA... we parse a non-constant
   // constant-expression!
-  Actions.ExprEvalContexts.back().InConditionallyConstantEvaluateContext = true;
+  Actions.ExprEvalContexts.back().InConditionallyConstantEvaluateContext = !IsAlwaysConstantEvaluated;
   return ParseConstantExpressionInExprEvalContext(NotTypeCast);
 }

Also, does this change suggest that we let some diagnostic mechanisms consider VLA cases? (e.g. DiagRuntimeBehavior should not early-return if the context is InConditionallyConstantEvaluateContext to diagnose div-by-zero in https://godbolt.org/z/556rKnMTG)

cor3ntin · 2023-09-14T12:09:21Z

Is it alright to set InConditionallyConstantEvaluateContext true only when the outer evaluation context isAlwaysConstantEvaluatedContext() so that we can say the array bound expr is always constant-evaluated if it happens in always-constant-evaluated context?

Where do you think it matters? I can't this of cases where ConstantEvaluated context are nested in one another without some intertwined potentially evaluated context

Also, does this change suggest that we let some diagnostic mechanisms consider VLA cases? (e.g. DiagRuntimeBehavior should not early-return if the context is InConditionallyConstantEvaluateContext to diagnose div-by-zero in https://godbolt.org/z/556rKnMTG)

Yes. We probably should fix that (i think it's a separate patch)

hazohelet · 2023-09-15T05:35:55Z

Where do you think it matters? I can't this of cases where ConstantEvaluated context are nested in one another without some intertwined potentially evaluated context

I was mostly thinking about cases where the array-bound expression appears inside immediate function context.
The current checks for "Is this always constant-evaluated?" only see the innermost evaluation context (e.g. Sema::isConstantEvaluated, and Sema::DiagRuntimeBehavior), so I hope the newly pushed evaluation contexts on ExprEvalContexts be strict about whether it's always-constant-evaluated or not.
Although this PR is not relevant, for the same reason, I don't think it's ideal that potentially-evaluated-contexts appear inside constant-evaluated context.

cor3ntin · 2023-09-15T06:24:12Z

My patch needs modification so that use of is_constant_evaluated and consteval-if NOT considered tautologically-true when they happen InConditionallyConstantEvaluateContext, which is a few lines of change.

Hum, maybe to do that we need to set ahead of time whether we support VLAs or not because if we don't then it's not tautological... But if we do that then whether we get tautological warnings depend on the value of Wvla , which is interesting. At the same time, if user disable vla support they ought to have well behaving arrays so maybe that's our best option

I was mostly thinking about cases where the array-bound expression appears inside immediate function context.

I'll add a test. immediate contexts are recursive, constant evaluated contexts are not, i don't think there is a case where you would get an additional evaluation context that is not for a full expression, and if you have a full expression you do expect the immediate invocation to be called anyway - eg, lambda case.

http://eel.is/c++draft/expr#const-19

hazohelet · 2023-09-15T11:50:38Z

I'll add a test. immediate contexts are recursive, constant evaluated contexts are not, i don't think there is a case where you would get an additional evaluation context that is not for a full expression, and if you have a full expression you do expect the immediate invocation to be called anyway - eg, lambda case.

It turns out I was misunderstanding the standard about the recursiveness of constant-evaluated-context. I found a flaw in my patch that comes from it (pushing constant-evaluated context upon lambda body if it happens in constant-evaluated context). Thanks!

github-actions · 2023-09-26T19:08:32Z

✅ With the latest revision this PR passed the C/C++ code formatter.

shafik · 2023-09-27T18:28:56Z

clang/lib/Sema/TreeTransform.h

@@ -5490,6 +5490,9 @@ TreeTransform<Derived>::TransformDependentSizedArrayType(TypeLocBuilder &TLB,
  EnterExpressionEvaluationContext Unevaluated(
      SemaRef, Sema::ExpressionEvaluationContext::ConstantEvaluated);

+  // VLA bounds are not truly constant.


This is not always a VLA right? So maybe saying If we have a VLA then it won't be a constant

clang/lib/Sema/SemaChecking.cpp

erichkeane

I think this generally makes a lot of sense, I have 1 nit (plus a bunch of the 'bool' comments without the = sign that @shafik suggested, but were closed without comment), otherwise LGTM.

erichkeane · 2023-09-27T18:37:16Z

clang/lib/Parse/ParseExpr.cpp

+      Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated);
+  // If we parse the bound of a VLA... we parse a non-constant
+  // constant-expression!
+  Actions.ExprEvalContexts.back().InConditionallyConstantEvaluateContext = true;


I think I'd rather this be an argument to the above constructor here.

@erichkeane this constructor already has a bunch of defaulted boolean / pointer parameters, it would be a mess. We'd need to add a tag which seems overkill for 1 use

In reality, I'd probably prefer something like;

auto Expr = ExpressionEvaluationContext::CreateForVLA

kind of thing (or perhaps a builder-pattern like below), but that is probably a sizable refactor for everything. I think I can hold my nose here and be OK for now.

ExprEvalContext Eva(ThingsEverythingNeeds).ForVLA()

erichkeane

Still OK with this, but Shafik should do the final approval.

cor3ntin · 2023-10-03T16:16:14Z

@shafik ping!

shafik

LGTM after fixing nits.

clang/lib/Sema/SemaChecking.cpp

The bounds of a c++ array is a _constant-expression_. And in C++ it is also a constant expression. But we also support VLAs, ie arrays with non-constant bounds. We need to take care to handle the case of a consteval function (which are specified to be only immediately called in non-constant contexts) that appear in arrays bounds. This introduces `Sema::isAlwayConstantEvaluatedContext`, and a flag in ExpressionEvaluationContextRecord, such that immediate functions in array bounds are always immediately invoked. Sema had both `isConstantEvaluatedContext` and `isConstantEvaluated`, so I took the opportunity to cleanup that. The change in `TimeProfilerTest.cpp` is an unfortunate manifestation of the problem that llvm#66203 seeks to address. Fixes llvm#65520

cor3ntin added clang:frontend Language frontend issues, e.g. anything involving "Sema" consteval C++20 consteval labels Sep 13, 2023

cor3ntin requested a review from a team as a code owner September 13, 2023 15:53

llvmbot added the clang Clang issues not falling into any other category label Sep 13, 2023

cor3ntin force-pushed the vla_consteval branch from a95e4a8 to d7f281e Compare September 13, 2023 16:05