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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
