[PATCH] D52939: ExprConstant: Propagate correct return values from constant evaluation.
James Y Knight via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sun Oct 14 19:54:53 PDT 2018
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 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?
================
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
----------------
rsmith wrote:
> This sentence is a little hard to read.
Reworded.
================
Comment at: clang/lib/AST/ExprConstant.cpp:891
/// expression.
///
OptionalDiagnostic CCEDiag(SourceLocation Loc, diag::kind DiagId
----------------
rsmith wrote:
> Nit: delete line
Done.
================
Comment at: clang/lib/AST/ExprConstant.cpp:952
+ /// with diagnostics.
+ bool keepEvaluatingAfterNote() {
switch (EvalMode) {
----------------
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.
================
Comment at: clang/lib/AST/ExprConstant.cpp:1227-1230
+ diagnosePointerArithmetic(Info, E, N);
+ setInvalid();
+ if (!Info.keepEvaluatingAfterNote())
+ return false;
----------------
rsmith wrote:
> 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 `diagnosePointerArithmetic`.
>
> (Eventually, we should remove these `bool` return values entirely and replace them with an enum, where the failure value is only ever created by the `...Diag` functions and from places where we can prove that a diagnostic has already been emitted, so that we can easily ensure that every failure path produces a diagnostic first.)
Done, for diagnosePointerArithmetic.
================
Comment at: clang/lib/AST/ExprConstant.cpp:1440-1444
+ if (checkSubobject(Info, E, isa<FieldDecl>(D) ? CSK_Field : CSK_Base)) {
Designator.addDeclUnchecked(D, Virtual);
+ return true;
+ }
+ return Info.keepEvaluatingAfterNote();
----------------
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!
================
Comment at: clang/lib/AST/ExprConstant.cpp:11487
+ // mode, which we use here, but not in, e.g. EM_ConstantFold or others.)
+ assert(Diags.empty() == IsConstExpr);
+
----------------
rsmith wrote:
> How confident are you that all `return false;`s produce an accompanying note? We have had a long tail of situations where such notes are missing from some cases where evaluation fails. (Those were previously QoI bugs but will now result in an assertion failure.)
I attempted to ensure they were all covered. But I cannot be completely 100% sure.
I care more about the inverse really, though: that everywhere evaluation _should_ fail actually returns false, instead of only emitting a note.
================
Comment at: clang/lib/AST/ExprConstant.cpp:11599-11600
Evaluate(ResultScratch, Info, E);
+ // Same as above -- the success return value from Evaluate is not
+ // useful, we just care about diagnoses.
return Diags.empty();
----------------
rsmith wrote:
> I fear that a "Same as above" comment will become stale during refactorings.
Good point; copied the comment instead.
https://reviews.llvm.org/D52939
More information about the cfe-commits
mailing list