[PATCH] D62009: [clang] perform semantic checking in constant context

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Interesting. I think all of the new warnings in the test cases here are undesirable (they duplicate errors produced by the constant evaluator), but the removed warnings all look like improvements.

Generally, I think our goals should be:

For code patterns that might lead to undefined behavior when executed with certain values:

- If the code appears in a constant context (`isConstantEvaluated()` or a subexpression of a `ConstantExpr`), we should diagnose the problem from the constant evaluator if it actually happens and otherwise not diagnose it.
- Otherwise, we should warn on the problem if the code is reachable and otherwise not diagnose it (that is, use `DiagRuntimeBehavior`).

For code patterns that are suspicious but defined (eg, we think they might do something other than what the programmer intended), we should typically diagnose using `Diag`, regardless of whether they appear in a constant context.

`DiagRuntimeBehavior` already deals with much of this for us: if called in a `ConstantEvaluated` context, it suppresses diagnostics.

One way we could proceed would be to extend what you did for `CheckCompletedExpr` to the cases where `SemaChecking.cpp` (and friends) recurse into a `ConstantExpr`: temporarily create a `ConstantEvaluated` expression evaluation context for the purpose of the checks, so that we can track that we're within such a construct. But an `ExpressionEvaluationContextRecord` is not a trivial thing to create and pass around, and carries additional semantic implications on top of the context kind, so perhaps just adding a flag to it to indicate that we're performing semantic analysis inside a `ConstantExpr` within the context would be better.



================
Comment at: clang/lib/Sema/SemaChecking.cpp:4066-4073
 static void CheckNonNullArgument(Sema &S,
                                  const Expr *ArgExpr,
                                  SourceLocation CallSiteLoc) {
   if (CheckNonNullExpr(S, ArgExpr))
-    S.DiagRuntimeBehavior(CallSiteLoc, ArgExpr,
-           S.PDiag(diag::warn_null_arg) << ArgExpr->getSourceRange());
+    DiagRuntimeOrConstant(S, CallSiteLoc, ArgExpr,
+                          S.PDiag(diag::warn_null_arg)
+                              << ArgExpr->getSourceRange());
----------------
For `__attribute__((nonnull))`, I think the right thing to do is to change the constant expression evaluator to consider passing a null pointer argument for a nonnull-attributed parameter to be non-constant. (It's undefined behavior, so we should not permit it in constant evaluations.) For `_Nonnull`, I think the situation is less clear -- that case is explicitly not undefined behavior, so it's not clear that treating it as non-constant is the right choice. But as with the other cases here, `isConstantEvaluated()` does not imply the code is necessarily reachable, so bypassing `DiagRuntimeBehavior`'s check for reachability doesn't seem like the appropriate behavior.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:6089
   if (Result.getSExtValue() < Low || Result.getSExtValue() > High) {
-    if (RangeIsError)
+    if (RangeIsError || isConstantEvaluated())
       return Diag(TheCall->getBeginLoc(), diag::err_argument_invalid_range)
----------------
`isConstantEvaluated()` doesn't imply that the expression is necessarily reachable, so always giving an error here doesn't seem right. Instead, we should (already) have the relevant range checks in the constant evaluator for such builtins that are usable in constant expressions, and should never need to check here (if the code is reached during a constant evaluation, then the evaluation will be non-constant and hence ill-formed).

I think we should just return early from this function if `isConstantEvaluated()` like you do in `checkFortifiedBuiltinMemoryFunction`.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:6612-6613
     bool Cond;
-    if (C->getCond()->EvaluateAsBooleanCondition(Cond, S.getASTContext())) {
+    if (C->getCond()->EvaluateAsBooleanCondition(Cond, S.getASTContext(),
+                                                 S.isConstantEvaluated())) {
       if (Cond)
----------------
If we're in a constant-evaluated context, I think we can and should simply skip all format string checking. There are three cases here:

1) The call with the format attribute is unreached: no diagnostic should be issued.
2) The call with the format attribute is reached but not constant-evaluatable: an error will be produced that we couldn't constant-evaluate.
3) The call with the format attribute is reached and is constant-evaluatable: the constant evaluator needs to check that the format string matches the arguments anyway (because in general we permit non-constant format strings), so it will need to properly handle this case.

Case 3 is currently theoretical (we don't constant-evaluate any format-string functions). But in any case, I think the conclusion remains: we don't need to do these checks in a constant-evaluated context.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62009/new/

https://reviews.llvm.org/D62009