[clang] Fix a crash in constant evaluation

[kadircet]

Summary

This was showing up in our internal crash collector. I have no idea how
to test it out though, open for suggestions if there are easy paths but
otherwise I'd move forward with the patch.

[usaxena95]

Thanks. This looks reasonable.
As discussed offline, this verifiably fixes the crash but it is hard to come up with a reduced reproducer.

[erichkeane]

clang/lib/AST/ExprConstant.cpp

4797

It seems to me that we shouldn't GET to this function with an incomplete type. I suspect whoever calls this is doing so incorrectly.

[erichkeane]

Also, without a reduced example lit-test, we shouldn't be making changes. Use creduce or something, but PLEASE come back with a lit test.

[shafik]

clang/lib/AST/ExprConstant.cpp

4797

Also note we only check in ExprConstant.cpp for hasDefinition() in one other place in findCompleteObject and that is around extern see: https://github.com/llvm/llvm-project/commit/c0d04a2567c22631595bed8092bc042bb91ea4ee#diff-255a21a02a8966766225831836d482547787baf9a770fbf67178ebb7d7347e27

[kadircet]

sorry, i missed the other comments around having a lit test. reverting the change.

clang/lib/AST/ExprConstant.cpp

4797

It seems to me that we shouldn't GET to this function with an incomplete type. I suspect whoever calls this is doing so incorrectly.

Agreed, that's also my assumption. but we've been unable to get a minimal crasher. i am not a fan of landing these changes without reproducers but this was clearly fixing the issue we had (moreover, it's happening on invalid code).

moreover we're checking for recorddecl being invalid up above, so it felt quite likely to hit this code path with incomplete types as well (or there were some changes up the callstack that forgot to update the implementation here).

[aaron.ballman]

clang/lib/AST/ExprConstant.cpp

4797

Agreed, that's also my assumption. but we've been unable to get a minimal crasher. i am not a fan of landing these changes without reproducers but this was clearly fixing the issue we had (moreover, it's happening on invalid code).

It's hard to say that it actually is fixing the issue instead of papering over the root cause elsewhere in the project. Having test coverage helps us to determine whether the fix is correct or incorrect.

moreover we're checking for recorddecl being invalid up above, so it felt quite likely to hit this code path with incomplete types as well (or there were some changes up the callstack that forgot to update the implementation here).

If the type is incomplete, why is the record not invalid? This smells like we're possibly missing a call to RequireCompleteType() somewhere else.

[aaron.ballman]

Marking this as requesting changes so we don't have another accidental commit.

[kadircet]

Add reproducer.

I think the issue is about keeping constexpr functions valid even when their
bodies contain invalid decls under certain instantiations, which I believe is
the right behaviour. As the function body might be invalid for a certain
instantiation at constexpr time, but might be valid for others (or even for the
same instantiation later on in the TU).

[ilya-biryukov]

I was just passing by, but wanted to add more context from our investigation with @kadircet.
If variables with incomplete types appear inside non-template constexpr function this gets detected by a call to CheckConstexprFunctionDefinition inside ActOnFinishFunctionBody:

if (!IsInstantiation && FD && FD->isConstexpr() && !FD->isInvalidDecl() &&
    !CheckConstexprFunctionDefinition(FD, CheckConstexprKind::Diagnose))
  FD->setInvalidDecl();

So the resulting constexpr function is marked as invalid and Clang does not attempt to evaluate its body.
However, CheckConstexprFunctionDefinition does not run for template function instantiation, so the VarDecl marked invalid ends up inside a valid constexpr function and Clang does run the evaluation.

[shafik]

In D132918#3766909, @ilya-biryukov wrote:

I was just passing by, but wanted to add more context from our investigation with @kadircet.
If variables with incomplete types appear inside non-template constexpr function this gets detected by a call to CheckConstexprFunctionDefinition inside ActOnFinishFunctionBody:

if (!IsInstantiation && FD && FD->isConstexpr() && !FD->isInvalidDecl() &&
    !CheckConstexprFunctionDefinition(FD, CheckConstexprKind::Diagnose))
  FD->setInvalidDecl();

So the resulting constexpr function is marked as invalid and Clang does not attempt to evaluate its body.
However, CheckConstexprFunctionDefinition does not run for template function instantiation, so the VarDecl marked invalid ends up inside a valid constexpr function and Clang does run the evaluation.

This is helpful information but I am not sure this convinces me that EvaluateVarDecl is the correct place to check. Why not in EvaluateDecl or EvaluateStmt? Both locations we could also check.

[kadircet]

This is helpful information but I am not sure this convinces me that EvaluateVarDecl is the correct place to check. Why not in EvaluateDecl or EvaluateStmt? Both locations we could also check.

I have done the check inside EvaluateVarDecl because the invalid evaluation happens when we try to analyze type of an invalid var decl.
I don't know if it would be safe to say we shouldn't evaluate any invalid decl. e.g. EvaluateDecl returns true for any non-vardecl today, without checking for anything at all. I don't know if changing the contract there, let alone at the statement level, would be correct. but doing it inside the vardecl evaluation is at least correct, but might be redundant.

[ilya-biryukov]

In D132918#3773237, @shafik wrote:

This is helpful information but I am not sure this convinces me that EvaluateVarDecl is the correct place to check. Why not in EvaluateDecl or EvaluateStmt? Both locations we could also check.

So the options are:

1. assert(Decl->isValid()) in EvaluateVarDecl and checks at call sites in EvaluateDecl and EvaluateStmt.
2. a single check in EvaluateVarDecl.

The actual observable behavior of the compiler seems equivalent (EvaluateDecl can only fail inside EvaluateVarDecl anyway).
I think either is fine, neither of the approaches seems to be a big win over the other.

I will stamp this as I'm already in the reviewers list, think this change is ok and believe that Aaron's comments were addressed.
However, I still suggest to give time for @shafik to respond before landing this.

[aaron.ballman]

LGTM!

[shafik]

LGTM, I spent some time debugging this and I don't see an obvious alternative.

[kadircet]

Landed as abd2b1a9d0421f99d3d132dc99af55ae52f3ac3e

clang/lib/AST/ExprConstant.cpp

if (auto *RD = T->getAsCXXRecordDecl()) {
     if (RD->isInvalidDecl()) {
       Result = APValue();
       return false;
     }
     if (RD->isUnion()) {
       Result = APValue((const FieldDecl *)nullptr);
       return true;
     }
+    // Can't access properties of an incomplete type.
+    if (!RD->hasDefinition()) {
+      Result = APValue();
+      return false;
+    }
     Result = APValue(APValue::UninitStruct(), RD->getNumBases(),
                      std::distance(RD->field_begin(), RD->field_end()));
 
     unsigned Index = 0;
     for (CXXRecordDecl::base_class_const_iterator I = RD->bases_begin(),
                                                   End = RD->bases_end();
          I != End; ++I, ++Index)
       Success &= getDefaultInitValue(I->getType(), Result.getStructBase(Index));