[PATCH] D52939: ExprConstant: Propagate correct return values from constant evaluation.
rsmith added inline comments.
Comment at: clang/lib/AST/ExprConstant.cpp:711
+
+ /// Evaluate as a constant expression, as per C++11-and-later constexpr
+ /// rules. Stop if we find that the expression is not a constant
jyknight wrote:
> rsmith wrote:
> > This is not the intent. The evaluator uses the rules of the current
> > language mode. However, it doesn't enforce the C and C++98 syntactic
> > restrictions on what can appear in a constant expression, because it turns
> > out to be cleaner to check those separately rather than to interleave them
> > with evaluation.
> >
> > We should probably document this as evaluating following the evaluation
> > (but not syntactic) constant expression rules of the current language mode.
> I'm not really sure what you mean to distinguish between C/C++98 "evaluation
> constant expression rules" and the definition of a C/C++98 ICE?
>
> Perhaps you mean that if evaluation succeeds, it will have produced a value
> consistent with the rules of the current language, but that it may not
> produce the diagnostics in all cases it ought to?
>
> AFAICT, the evaluator is designed to emit diagnostics only for the C++11 (and
> later) constant expression rules, as well as for inability to evaluate. The
> diagnostic notes are usually dropped on the floor in C++98 and C modes, as
> only "fold" is typically used there -- along with the separate CheckICE to
> produce warnings.
>
> I believe the only place the diagnostics were being actually emitted in
> pre-c++11 modes is for the diagnose_if and enable_if attributes' "potential
> constant expression" check. And there it really seems to me to be using the
> c++11 evaluation semantics.
>
> The existence of isCXX11ConstantExpr suggests an intent for the function to
> provide the C++11 rules, as well, no?
>
>
> Perhaps you mean that if evaluation succeeds, it will have produced a value
> consistent with the rules of the current language, but that it may not
> produce the diagnostics in all cases it ought to?
The concern I have with this comment is that it describes a happenstance of
implementation rather than the intent. The intent is certainly *not* that we
will use the C++11 rules when calling this in C++98 mode, but it just so
happens that there are no interesting differences there right now.
Looking at this another way, the fact that we only guarantee to produce "not a
constant expression because" notes in C++11 onwards is unrelated to the
evaluation mode. Rather, the reason for that is instead because all the
validity / ICE checking in other language modes is based on the syntactic form
of the expression (and is checked separately as a result).
As a concrete suggestion, how about something like:
"Evaluate as a constant expression. Stop if we find that the expression is not
a constant expression. Note that this doesn't enforce the syntactic ICE
requirements of C and C++98."
Comment at: clang/lib/AST/ExprConstant.cpp:952
+/// with diagnostics.
+bool keepEvaluatingAfterNote() {
switch (EvalMode) {
jyknight wrote:
> rsmith wrote:
> > "Note" is beside the point here. We should be talking about non-constant
> > expressions; that's what matters.
> Renamed to keepEvaluatingAfterNonConstexpr, updated comment.
Capitalize as "...NonConstExpr" instead, since this is just an abbreviation of
the term "constant expression", not C++11's notion of "constexpr". (I'd also
lengthen this to "...NonConstantExpr" or "...NonConstantExpression" to further
clarify.)
Comment at: clang/lib/AST/ExprConstant.cpp:1440-1444
+ if (checkSubobject(Info, E, isa(D) ? CSK_Field : CSK_Base)) {
Designator.addDeclUnchecked(D, Virtual);
+return true;
+ }
+ return Info.keepEvaluatingAfterNote();
jyknight wrote:
> rsmith wrote:
> > `checkSubobject` should return `false` if it produced a diagnostic that
> > should cause evaluation to halt; this call to `keepEvaluatingAfterNote`
> > should not be necessary. (And likewise below.)
> Are you sure?
>
> Today, checkSubobject returns false if it sees something "invalid" -- and the
> callers seem to depend on that -- but invalid subobject designators are still
> allowed during evaluation in fold mode.
>
> E.g., this is valid in C:
> `struct Foo { int x[]; }; struct Foo f; int *g = f.x;`
>
> But, in C++, `constexpr int *g = f.x;` is an error, diagnosing
> note_constexpr_unsupported_unsized_array.
>
> All the LValue code, with its mutable "Result" state, and carrying of a
> couple different "Invalid" bits off on the side, is really rather confusing!
What I mean is that you should change the code to make what I said be true, if
necessary. It's the callee's responsibility to say whether we hit something we
can't evaluate past. The caller should not be guessing that the only reason
that `checkSubobject` can fail is due to a `C
[PATCH] D52939: ExprConstant: Propagate correct return values from constant evaluation.
jyknight updated this revision to Diff 169641.
jyknight marked 11 inline comments as done.
jyknight added a comment.
Address most comments.
https://reviews.llvm.org/D52939
Files:
clang/include/clang/AST/Expr.h
clang/lib/AST/ExprConstant.cpp
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -412,52 +412,10 @@
MostDerivedArraySize = 2;
MostDerivedPathLength = Entries.size();
}
-void diagnoseUnsizedArrayPointerArithmetic(EvalInfo &Info, const Expr *E);
-void diagnosePointerArithmetic(EvalInfo &Info, const Expr *E,
+bool diagnosePointerArithmetic(EvalInfo &Info, const Expr *E,
const APSInt &N);
/// Add N to the address of this subobject.
-void adjustIndex(EvalInfo &Info, const Expr *E, APSInt N) {
- if (Invalid || !N) return;
- uint64_t TruncatedN = N.extOrTrunc(64).getZExtValue();
- if (isMostDerivedAnUnsizedArray()) {
-diagnoseUnsizedArrayPointerArithmetic(Info, E);
-// Can't verify -- trust that the user is doing the right thing (or if
-// not, trust that the caller will catch the bad behavior).
-// FIXME: Should we reject if this overflows, at least?
-Entries.back().ArrayIndex += TruncatedN;
-return;
- }
-
- // [expr.add]p4: For the purposes of these operators, a pointer to a
- // nonarray object behaves the same as a pointer to the first element of
- // an array of length one with the type of the object as its element type.
- bool IsArray = MostDerivedPathLength == Entries.size() &&
- MostDerivedIsArrayElement;
- uint64_t ArrayIndex =
- IsArray ? Entries.back().ArrayIndex : (uint64_t)IsOnePastTheEnd;
- uint64_t ArraySize =
- IsArray ? getMostDerivedArraySize() : (uint64_t)1;
-
- if (N < -(int64_t)ArrayIndex || N > ArraySize - ArrayIndex) {
-// Calculate the actual index in a wide enough type, so we can include
-// it in the note.
-N = N.extend(std::max(N.getBitWidth() + 1, 65));
-(llvm::APInt&)N += ArrayIndex;
-assert(N.ugt(ArraySize) && "bounds check failed for in-bounds index");
-diagnosePointerArithmetic(Info, E, N);
-setInvalid();
-return;
- }
-
- ArrayIndex += TruncatedN;
- assert(ArrayIndex <= ArraySize &&
- "bounds check succeeded for out-of-bounds index");
-
- if (IsArray)
-Entries.back().ArrayIndex = ArrayIndex;
- else
-IsOnePastTheEnd = (ArrayIndex != 0);
-}
+bool adjustIndex(EvalInfo &Info, const Expr *E, APSInt N);
};
/// A stack frame in the constexpr call stack.
@@ -721,43 +679,75 @@
bool IsSpeculativelyEvaluating;
enum EvaluationMode {
- /// Evaluate as a constant expression. Stop if we find that the expression
- /// is not a constant expression.
+ // The various EvaluationMode kinds vary in terms of what sorts of
+ // expressions they accept.
+ //
+ // Key for the following table:
+ // * N = Expressions which are evaluable, but are not a constant
+ // expression (according to the language rules) are OK
+ // (keepEvaluatingAfterNonConstexpr)
+ // * S = Failure to evaluate which only occurs in expressions used for
+ // side-effect are okay. (keepEvaluatingAfterSideEffect)
+ // * F = Failure to evaluate is okay. (keepEvaluatingAfterFailure)
+ // * E = Eagerly evaluate certain builtins to a value which would normally
+ // be deferred until after optimizations.
+ //
+ // +-+---+---+---+---+
+ // | | N | S | F | E |
+ // +-+---+---+---+---+
+ // |EM_ConstantExpression| . | . | . | . |
+ // |EM_ConstantFold | Y | . | . | . |
+ // |EM_ConstantFoldUnevaluated | Y | . | . | Y |
+ // |EM_IgnoreSideEffects | Y | Y | . | . |
+ // |EM_PotentialConstantExpression | . | Y | Y | . |
+ // |EM_PotentialConstantExpressionUnevaluated| . | Y | Y | Y |
+ // |EM_EvaluateForOverflow | Y | Y | Y | . |
+ // +-+---+---+---+---+
+ //
+ // TODO: Fix EM_ConstantExpression and EM_PotentialConstantExpression to
+ // also eagerly evaluate. (and then delete
+ // EM_PotentialConstantExpressionUnevaluated as a duplicate). This will
+ // be somewhat tricky to change, because LLVM currently verifies that an
+ // expression is a constant expression (possibly strictly with
+ // EM_ConstantExpression) in Sema/*, while it evaluates the value
+ // separately in CodeGen/* with EM_ConstantF
[PATCH] D52939: ExprConstant: Propagate correct return values from constant evaluation.
jyknight added inline comments.
Comment at: clang/lib/AST/ExprConstant.cpp:12-32
// Constant expression evaluation produces four main results:
//
// * A success/failure flag indicating whether constant folding was
successful.
//This is the 'bool' return value used by most of the code in this file. A
//'false' return value indicates that constant folding has failed, and any
//appropriate diagnostic has already been produced.
//
rsmith wrote:
> Is this comment still correct?
Yes -- the "potential" constant expression mode still has this semantic.
Comment at: clang/lib/AST/ExprConstant.cpp:682
enum EvaluationMode {
- /// Evaluate as a constant expression. Stop if we find that the
expression
- /// is not a constant expression.
+ /* The various EvaluationMode kinds vary in terms of what sorts of
+ * expressions they accept.
rsmith wrote:
> Nit: LLVM style avoids `/*...*/` comments even for long comment blocks like
> this (https://llvm.org/docs/CodingStandards.html#comment-formatting).
Done.
Comment at: clang/lib/AST/ExprConstant.cpp:686-687
+ Key for the following table:
+ * D = Diagnostic notes (about non-constexpr, but still evaluable
+ constructs) are OK (keepEvaluatingAfterNote)
+ * S = Failure to evaluate which only occurs in expressions used for
rsmith wrote:
> I think talking about diagnostic notes here distracts from the purpose, which
> is that this column indicates that we accept constructs that are not
> permitted by the language mode's constant expression rules.
Reworded.
Comment at: clang/lib/AST/ExprConstant.cpp:688-690
+ * S = Failure to evaluate which only occurs in expressions used for
+ side-effect are okay. (keepEvaluatingAfterSideEffect)
+ * F = Failure to evaluate is okay. (keepEvaluatingAfterFailure)
rsmith wrote:
> "is okay" here is misleading, I think. The point of
> `keepEvaluatingAfter[...]` is that it's safe to stop evaluating if they
> return false, because later evaluations can't affect the overall result.
The overall return value of the evaluation indicates whether the expression was
evaluable under a specified set of conditions. The different modes cause
failure under different conditions -- so I think "is okay" really _is_ what is
meant here. In this particular case, "failure to evaluate" can still return
success!
Not sure if some rewording would communicate that better?
Comment at: clang/lib/AST/ExprConstant.cpp:692
+ * E = Eagerly evaluate certain builtins to a value which would
normally
+ defer until after optimizations.
+
rsmith wrote:
> defer -> be deferred?
Done.
Comment at: clang/lib/AST/ExprConstant.cpp:706-707
+
+ TODO: Fix EM_ConstantExpression and EM_PotentialConstantExpression to
+ also eagerly evaluate. (and then delete
+ EM_PotentialConstantExpressionUnevaluated as a duplicate)
rsmith wrote:
> Allowing `EM_ConstantExpression` to evaluate expressions that
> `EM_ConstantFold` cannot evaluate would violate our invariants. We rely on
> being able to check up front that an expression is a constant expression and
> then later just ask for its value, and for the later query to always succeed
> and return the same value as the earlier one. (This is one of the reasons why
> I suggested in D52854 that we add an explicit AST representation for constant
> expressions.)
I agree, it would right now. But I do think it should be fixed not to in the
future. Let me reword the TODO, noting that it's not trivial.
Comment at: clang/lib/AST/ExprConstant.cpp:711
+
+ /// Evaluate as a constant expression, as per C++11-and-later constexpr
+ /// rules. Stop if we find that the expression is not a constant
rsmith wrote:
> This is not the intent. The evaluator uses the rules of the current language
> mode. However, it doesn't enforce the C and C++98 syntactic restrictions on
> what can appear in a constant expression, because it turns out to be cleaner
> to check those separately rather than to interleave them with evaluation.
>
> We should probably document this as evaluating following the evaluation (but
> not syntactic) constant expression rules of the current language mode.
I'm not really sure what you mean to distinguish between C/C++98 "evaluation
constant expression rules" and the definition of a C/C++98 ICE?
Perhaps you mean that if evaluation succeeds, it will have produced a value
consistent with the rules of the current language, but that it may not produce
the diagnostics in all cases it ought to?
AFAICT, the evaluator is designed to emit diagnostics only for the C++11 (and
later) constant expression rules, as well as for inability to e
[PATCH] D52939: ExprConstant: Propagate correct return values from constant evaluation.
rsmith added inline comments.
Comment at: clang/lib/AST/ExprConstant.cpp:12-32
// Constant expression evaluation produces four main results:
//
// * A success/failure flag indicating whether constant folding was
successful.
//This is the 'bool' return value used by most of the code in this file. A
//'false' return value indicates that constant folding has failed, and any
//appropriate diagnostic has already been produced.
//
Is this comment still correct?
Comment at: clang/lib/AST/ExprConstant.cpp:682
enum EvaluationMode {
- /// Evaluate as a constant expression. Stop if we find that the
expression
- /// is not a constant expression.
+ /* The various EvaluationMode kinds vary in terms of what sorts of
+ * expressions they accept.
Nit: LLVM style avoids `/*...*/` comments even for long comment blocks like
this (https://llvm.org/docs/CodingStandards.html#comment-formatting).
Comment at: clang/lib/AST/ExprConstant.cpp:686-687
+ Key for the following table:
+ * D = Diagnostic notes (about non-constexpr, but still evaluable
+ constructs) are OK (keepEvaluatingAfterNote)
+ * S = Failure to evaluate which only occurs in expressions used for
I think talking about diagnostic notes here distracts from the purpose, which
is that this column indicates that we accept constructs that are not permitted
by the language mode's constant expression rules.
Comment at: clang/lib/AST/ExprConstant.cpp:688-690
+ * S = Failure to evaluate which only occurs in expressions used for
+ side-effect are okay. (keepEvaluatingAfterSideEffect)
+ * F = Failure to evaluate is okay. (keepEvaluatingAfterFailure)
"is okay" here is misleading, I think. The point of `keepEvaluatingAfter[...]`
is that it's safe to stop evaluating if they return false, because later
evaluations can't affect the overall result.
Comment at: clang/lib/AST/ExprConstant.cpp:692
+ * E = Eagerly evaluate certain builtins to a value which would
normally
+ defer until after optimizations.
+
defer -> be deferred?
Comment at: clang/lib/AST/ExprConstant.cpp:706-707
+
+ TODO: Fix EM_ConstantExpression and EM_PotentialConstantExpression to
+ also eagerly evaluate. (and then delete
+ EM_PotentialConstantExpressionUnevaluated as a duplicate)
Allowing `EM_ConstantExpression` to evaluate expressions that `EM_ConstantFold`
cannot evaluate would violate our invariants. We rely on being able to check up
front that an expression is a constant expression and then later just ask for
its value, and for the later query to always succeed and return the same value
as the earlier one. (This is one of the reasons why I suggested in D52854 that
we add an explicit AST representation for constant expressions.)
Comment at: clang/lib/AST/ExprConstant.cpp:711
+
+ /// Evaluate as a constant expression, as per C++11-and-later constexpr
+ /// rules. Stop if we find that the expression is not a constant
This is not the intent. The evaluator uses the rules of the current language
mode. However, it doesn't enforce the C and C++98 syntactic restrictions on
what can appear in a constant expression, because it turns out to be cleaner to
check those separately rather than to interleave them with evaluation.
We should probably document this as evaluating following the evaluation (but
not syntactic) constant expression rules of the current language mode.
Comment at: clang/lib/AST/ExprConstant.cpp:720-722
+ /// Like EM_ConstantFold, but eagerly attempt to evaluate some builtins
+ /// that'd otherwise fail constant evaluation to defer until after
+ /// optimization. This affects __builtin_object_size (and in the future
This sentence is a little hard to read.
Comment at: clang/lib/AST/ExprConstant.cpp:891
/// expression.
///
OptionalDiagnostic CCEDiag(SourceLocation Loc, diag::kind DiagId
Nit: delete line
Comment at: clang/lib/AST/ExprConstant.cpp:952
+/// with diagnostics.
+bool keepEvaluatingAfterNote() {
switch (EvalMode) {
"Note" is beside the point here. We should be talking about non-constant
expressions; that's what matters.
Comment at: clang/lib/AST/ExprConstant.cpp:1227-1230
+diagnosePointerArithmetic(Info, E, N);
+setInvalid();
+if (!Info.keepEvaluatingAfterNote())
+ return false;
I would strongly prefer to keep the emission of the diagnostic and the
corresponding `return false` adjacent whenever possible, so I'd prefer that we
add a `bool` return value to `diagnoseP
[PATCH] D52939: ExprConstant: Propagate correct return values from constant evaluation.
jyknight created this revision.
jyknight added a reviewer: rsmith.
The constant evaluation now returns false whenever indicating failure
would be appropriate for the requested mode, instead of returning
"true" for success, and depending on the caller examining the various
status variables after the fact, in some circumstances.
In EM_ConstantExpression mode, evaluation is aborted as soon as a
diagnostic note is generated, and therefore, a "true" return value now
actually indicates that the expression was a constexpr. (You no longer
have to additionally check for a lack of diagnostic messages.)
In EM_ConstantFold, evaluation is now aborted upon running into a
side-effect -- so a "true" return value now actually indicates that
there were no side-effects
Also -- update and clarify documentation for the evaluation modes.
This is a cleanup, not intending to change the functionality, as
callers had been checking those properties manually before.
https://reviews.llvm.org/D52939
Files:
clang/include/clang/AST/Expr.h
clang/lib/AST/ExprConstant.cpp
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -412,52 +412,10 @@
MostDerivedArraySize = 2;
MostDerivedPathLength = Entries.size();
}
-void diagnoseUnsizedArrayPointerArithmetic(EvalInfo &Info, const Expr *E);
void diagnosePointerArithmetic(EvalInfo &Info, const Expr *E,
const APSInt &N);
/// Add N to the address of this subobject.
-void adjustIndex(EvalInfo &Info, const Expr *E, APSInt N) {
- if (Invalid || !N) return;
- uint64_t TruncatedN = N.extOrTrunc(64).getZExtValue();
- if (isMostDerivedAnUnsizedArray()) {
-diagnoseUnsizedArrayPointerArithmetic(Info, E);
-// Can't verify -- trust that the user is doing the right thing (or if
-// not, trust that the caller will catch the bad behavior).
-// FIXME: Should we reject if this overflows, at least?
-Entries.back().ArrayIndex += TruncatedN;
-return;
- }
-
- // [expr.add]p4: For the purposes of these operators, a pointer to a
- // nonarray object behaves the same as a pointer to the first element of
- // an array of length one with the type of the object as its element type.
- bool IsArray = MostDerivedPathLength == Entries.size() &&
- MostDerivedIsArrayElement;
- uint64_t ArrayIndex =
- IsArray ? Entries.back().ArrayIndex : (uint64_t)IsOnePastTheEnd;
- uint64_t ArraySize =
- IsArray ? getMostDerivedArraySize() : (uint64_t)1;
-
- if (N < -(int64_t)ArrayIndex || N > ArraySize - ArrayIndex) {
-// Calculate the actual index in a wide enough type, so we can include
-// it in the note.
-N = N.extend(std::max(N.getBitWidth() + 1, 65));
-(llvm::APInt&)N += ArrayIndex;
-assert(N.ugt(ArraySize) && "bounds check failed for in-bounds index");
-diagnosePointerArithmetic(Info, E, N);
-setInvalid();
-return;
- }
-
- ArrayIndex += TruncatedN;
- assert(ArrayIndex <= ArraySize &&
- "bounds check succeeded for out-of-bounds index");
-
- if (IsArray)
-Entries.back().ArrayIndex = ArrayIndex;
- else
-IsOnePastTheEnd = (ArrayIndex != 0);
-}
+bool adjustIndex(EvalInfo &Info, const Expr *E, APSInt N);
};
/// A stack frame in the constexpr call stack.
@@ -721,43 +679,68 @@
bool IsSpeculativelyEvaluating;
enum EvaluationMode {
- /// Evaluate as a constant expression. Stop if we find that the expression
- /// is not a constant expression.
+ /* The various EvaluationMode kinds vary in terms of what sorts of
+ * expressions they accept.
+
+ Key for the following table:
+ * D = Diagnostic notes (about non-constexpr, but still evaluable
+ constructs) are OK (keepEvaluatingAfterNote)
+ * S = Failure to evaluate which only occurs in expressions used for
+ side-effect are okay. (keepEvaluatingAfterSideEffect)
+ * F = Failure to evaluate is okay. (keepEvaluatingAfterFailure)
+ * E = Eagerly evaluate certain builtins to a value which would normally
+ defer until after optimizations.
+
+ +-+---+---+---+---+
+ | | D | S | F | E |
+ +-+---+---+---+---+
+ |EM_ConstantExpression| . | . | . | . |
+ |EM_ConstantFold | Y | . | . | . |
+ |EM_ConstantFoldUnevaluated | Y | . | . | Y |
+ |EM_IgnoreSideEffects | Y | Y | . | . |
+ |EM_PotentialConstantExpression | . | Y | Y | . |
+ |EM_PotentialConstan
