[C++20] Fix a crash with modules.

[usaxena95]

Summary

Prior to this change:
We see the following crash:

assert.h assertion failed at PATH/TO/clang/lib/AST/ExprConstant.cpp:6320 in auto HandleConstructorCall(const Expr *, const LValue &, CallRef, const CXXConstructorDecl *, EvalInfo &, APValue &)::(anonymous class)::operator()(FieldDecl *, bool) const: Indirect && "fields out of order?"
*** Check failure stack trace: ***
    @     0x5555721ac3a6  __assert_fail
    @     0x555569c24a7f  HandleConstructorCall()::$_0::operator()()
    @     0x555569c239a1  HandleConstructorCall()
    @     0x555569b35ac0  HandleConstructorCall()
    @     0x555569c0bf8c  (anonymous namespace)::RecordExprEvaluator::VisitCXXConstructExpr()
    @     0x555569c16974  (anonymous namespace)::RecordExprEvaluator::VisitCXXConstructExpr()
    @     0x555569c0f60a  clang::StmtVisitorBase<>::Visit()
    @     0x555569c26fcb  (anonymous namespace)::RecordExprEvaluator::VisitCastExpr()
    @     0x555569c27a0e  clang::StmtVisitorBase<>::VisitExplicitCastExpr()
    @     0x555569c184fe  clang::StmtVisitorBase<>::VisitCXXFunctionalCastExpr()
    @     0x555569c0fd87  clang::StmtVisitorBase<>::Visit()
    @     0x555569c4c001  EvaluateRecord()
    @     0x555569b2dabf  EvaluateInPlace()
    @     0x555569b33b19  EvaluateCallArg()
    @     0x555569b80acf  EvaluateArgs()
    @     0x555569c26813  (anonymous namespace)::ExprEvaluatorBase<>::handleCallExpr()
    @     0x555569c181ef  (anonymous namespace)::RecordExprEvaluator::VisitCallExpr()
    @     0x555569c0fb96  clang::StmtVisitorBase<>::Visit()
    @     0x555569c16887  (anonymous namespace)::ExprEvaluatorBase<>::VisitCXXBindTemporaryExpr()
    @     0x555569c0f57c  clang::StmtVisitorBase<>::Visit()
    @     0x555569c1946e  (anonymous namespace)::ExprEvaluatorBase<>::VisitExprWithCleanups()
    @     0x555569c10476  clang::StmtVisitorBase<>::Visit()
    @     0x555569c4c001  EvaluateRecord()
    @     0x555569b2dabf  EvaluateInPlace()
    @     0x555569b8b754  EvaluateVarDecl()
    @     0x555569b8b843  EvaluateDecl()
    @     0x555569b872d3  EvaluateStmt()
    @     0x555569b8772c  EvaluateStmt()
    @     0x555569c2457e  HandleConstructorCall()
    @     0x555569b35ac0  HandleConstructorCall()
    @     0x555569c0bf8c  (anonymous namespace)::RecordExprEvaluator::VisitCXXConstructExpr()
    @     0x555569c16974  (anonymous namespace)::RecordExprEvaluator::VisitCXXConstructExpr()
    @     0x555569c0f60a  clang::StmtVisitorBase<>::Visit()
    @     0x555569c4c001  EvaluateRecord()
    @     0x555569b2dabf  EvaluateInPlace()
    @     0x555569b2e6d6  clang::Expr::EvaluateAsInitializer()
    @     0x555569a18b14  clang::VarDecl::evaluateValueImpl()
    @     0x555569a187da  clang::VarDecl::evaluateValue()
    @     0x555563f5a973  clang::CodeGen::ConstantEmitter::tryEmitPrivateForVarInit()
    @     0x555563f5cda3  clang::CodeGen::ConstantEmitter::tryEmitForInitializer()
    @     0x5555644eb927  clang::CodeGen::CodeGenModule::EmitGlobalVarDefinition()
    @     0x5555644d935d  clang::CodeGen::CodeGenModule::EmitGlobalDefinition()
    @     0x5555644e199e  clang::CodeGen::CodeGenModule::EmitGlobal()
    @     0x5555644d6967  clang::CodeGen::CodeGenModule::EmitTopLevelDecl()
    @     0x5555644d7dbe  clang::CodeGen::CodeGenModule::EmitTopLevelDecl()
    @     0x5555648e973e  (anonymous namespace)::CodeGeneratorImpl::HandleTopLevelDecl()
    @     0x555563950511  clang::BackendConsumer::HandleTopLevelDecl()
    @     0x555569544269  clang::ASTConsumer::HandleImplicitImportDecl()
    @     0x55556839611d  clang::Sema::BuildModuleInclude()
    @     0x555568395e33  clang::Sema::ActOnModuleInclude()
    @     0x5555669529df  clang::Parser::ParseTopLevelDecl()
    @     0x55556694af35  clang::ParseAST()
    @     0x5555663b0b66  clang::ASTFrontendAction::ExecuteAction()
    @     0x555563947e9c  clang::CodeGenAction::ExecuteAction()
    @     0x5555663b0084  clang::FrontendAction::Execute()
    @     0x555566273de5  clang::CompilerInstance::ExecuteAction()
    @     0x5555623226aa  clang::ExecuteCompilerInvocation()
    @     0x5555622ece85  cc1_main()
    @     0x5555622cb325  ExecuteCC1Tool()
    @     0x55556679c567  clang::driver::CC1Command::Execute()::$_0::operator()()

Debug values (Frame 0x555569c239a1 at Line SkipToField(FD, false);):
Context in function static bool HandleConstructorCall(const Expr *E, const LValue &This,...):

const CXXRecordDecl *RD = Definition->getParent();
...
for (const auto *I : Definition->inits()) {
if ((FD = I->getMember())) {
...
}
}
...

RD->dumpColor()
CXXRecordDecl 0x68f339c36a8 <PATH/TO/include/c++/v1/vector:428:3, line:444:3> line:428:9 imported in //third_party/stl:stl_cc_library.third_party/stl/cxx17/vector class __destroy_vector

FD->dumpColor()
FieldDecl 0x68f33694608 parent 0x68f339c3f30 <PATH/TO/include/c++/v1/vector:443:7, col:15> col:15 imported in //third_party/stl:stl_cc_library.third_party/stl/cxx17/vector referenced __vec_ 'vector<string> &'

Note that RD does not contain any FieldDecl but __destroy_vector actually contains member vector<string> &.
The crash is primarily because of this missing information of the FieldDecl from RecordDecl.

Note that FD refers to 0x68f339c3f30 as the parent which is different from RD 0x68f339c36a8.

FD->getParent()->dumpColor()
CXXRecordDecl 0x68f339c3f30 prev 0x68f339c36a8 <PATH/TO/include/c++/v1/vector:428:3, line:444:3> line:428:9 imported in //third_party/stl:stl_cc_library.third_party/stl/cxx17/vector class __destroy_vector definition
...
|-FieldDecl 0x68f33694798 first 0x68f33694608 <line:443:7, col:15> col:15 imported in //third_party/stl:stl_cc_library.third_party/stl/cxx17/vector referenced __vec_ 'vector<string> &'
...

In fact RD (0x68f339c36a8) is the previous declaration of FD->getParent().
The FieldDecl should be read from the definition data (FD->getParent() or RD->getMostRecentDecl() or RD->getDefinition())

[usaxena95]

Use getDefinition

[rsmith]

clang/lib/AST/ExprConstant.cpp

6233 ↗

(On Diff #491661)

I think it would make sense to use the definition of the class as RD here, since we're going to be iterating through RD's fields.

[usaxena95]

Add message in the assertion.

[usaxena95]

Moved the use of definition where fields are accessed.
This now resolves several other C++20/Module related crashes as well.

[ilya-biryukov]

This fix looks very sensible to me and @rsmith confirmed this pattern that we are seeing might happen (seeing members of something that was demoted from declaration to a definition).
@rsmith could you confirm the update version looks good to you?

[usaxena95]

Use definition based field access only for CXXRecordDecl.
Avoid dynamic dispatch.

[rsmith]

clang/include/clang/AST/DeclCXX.h

557–560 ↗

(On Diff #493652)

This change makes me nervous: calling field_begin / fields on a CXXRecordDecl* will now work, but calling those same functions on the same object that has been cast to RecordDecl* will still be broken. Can we change the accessors in the base class instead?

[usaxena95]

Moved to RecordDecl::field_begin. Assertion is no more valid in RecordDecl.

[ilya-biryukov]

I don't see any issues with this, so happy to LGTM. @rsmith any concerns on your side?
@usaxena95 could you give an example of the code that fails the assertion? Is it some of the tests?

[usaxena95]

In D142384#4096935, @ilya-biryukov wrote:

@usaxena95 could you give an example of the code that fails the assertion? Is it some of the tests?

assert(getDefinition()) fails about 3.6k tests.
assert(!isa<CXXRecordDecl>(this) || getDefinition()); fails a handful 23 tests in ObjC:

Failed Tests (23):
  Clang :: Analysis/CFContainers.mm
  Clang :: Analysis/blocks.m
  Clang :: Analysis/dtor-array.cpp
  Clang :: Analysis/edges-new.mm
  Clang :: Analysis/incorrect-checker-names.mm
  Clang :: Analysis/malloc.mm
  Clang :: Analysis/mig.mm
  Clang :: Analysis/retain-release.m
  Clang :: Analysis/retain-release.mm
  Clang :: Analysis/stack-capture-leak-arc.mm
  Clang :: Analysis/stack-capture-leak-no-arc.mm
  Clang :: Analysis/taint-tester.cpp
  Clang :: CodeGenObjCXX/encode.mm
  Clang-Unit :: AST/./ASTTests/0/100
  Clang-Unit :: AST/./ASTTests/1/100
  Clang-Unit :: AST/./ASTTests/14/100
  Clang-Unit :: AST/./ASTTests/16/100
  Clang-Unit :: AST/./ASTTests/76/100
  Clang-Unit :: AST/./ASTTests/77/100
  Clang-Unit :: AST/./ASTTests/78/100
  Clang-Unit :: AST/./ASTTests/79/100
  Clang-Unit :: AST/./ASTTests/98/100
  Clang-Unit :: AST/./ASTTests/99/100

bin/clang $SRC/llvm-project/clang/test/CodeGenObjCXX/encode.mm
Stacktrace: https://pastebin.pl/view/c6f505a7

assert((!isa<CXXRecordDecl>(this) || getDefinition() || !FirstDecl) && "Field without a CXX definition ?"); works. So if there is no CXX definition then the fields are always empty and current callers are fine with that.

I think this is fine, and we should just use the definition when it is available without asking the callers to request fields only when definition is available.

[ilya-biryukov]

In D142384#4098650, @usaxena95 wrote:

I think this is fine, and we should just use the definition when it is available without asking the callers to request fields only when definition is available.

Not sure about C, but ObjC stacktrace definitely looks like a potential bug to me.
We actually might introduce new failure modes (calling the function on the same RecordDecl may start retuning different results after *some* other redefinition will create the fields).

However, we also need to fix this crash in C++, so I suggest to land this and see if this will cause any fallout.

clang/lib/AST/Decl.cpp

4772–4775

Let's add a comment to make sure we avoid someone deleting this code. (Since we don't have a test).

// This is necessary for correctness for C++ with modules.
// FIXME: come up with a test case that breaks.

[usaxena95]

Addressed comments.

[ChuanqiXu]

It should be good to prevent crashes. But it looks not good that it doesn't have a test. Do you have plans to add a test case for this soon?

[ilya-biryukov]

In D142384#4101461, @ChuanqiXu wrote:

It should be good to prevent crashes. But it looks not good that it doesn't have a test. Do you have plans to add a test case for this soon?

It would be great to have a test, but we didn't manage to come up with a small repro (we spent 2 weeks on-and-off trying to come up with one).
This only breaks inside our internal builds with header modules, internal version of Bazel, multiple targets with modules and STL.

We'll keep trying to reproduce this, but can't promise any exact dates.

[hans]

It looks like this is making one of Chromium's clang plugins traverse (and flag) more code: https://bugs.chromium.org/p/chromium/issues/detail?id=1412769
No modules involved :)

I don't know if that's expected or not, but maybe that behavior change could be used as an inspiration for a test case.

[rsmith]

In D142384#4102937, @hans wrote:

I don't know if that's expected or not, but maybe that behavior change could be used as an inspiration for a test case.

At least this suggests a way to test this with a C++ unit test: get a RecordDecl* referring to a forward declaration of a struct with a definition, and iterate over its fields.

[steakhal]

@usaxena95 @ilya-biryukov, @hans, I wonder if we should backport this change to release/16.x. Otherwise, clang-16 won't have this fix. WDYT?
I'm also a bit worried that we don't have tests. And that we have "unexpected" sideeffects like what you mentioned @hans.

Given all these, I would probably vote for not backporting this, but this is the chance if we want.

[hans]

I have no opinion about merging this, but I second the worry about not having a test.

[usaxena95]

This does not seem to be a pressing issue as there are no other reported module/cpp20 related crashes which were fixed by this.
Also since it has no tests, it should be fine to postpone it to the next release.

clang/lib/AST/Decl.cpp

bool RecordDecl::isOrContainsUnion() const {
   }
 
   return false;
 }
 
 RecordDecl::field_iterator RecordDecl::field_begin() const {
   if (hasExternalLexicalStorage() && !hasLoadedFieldsFromExternalStorage())
     LoadFieldsFromExternalStorage();
-
+  // This is necessary for correctness for C++ with modules.
+  // FIXME: Come up with a test case that breaks without definition.
+  if (RecordDecl *D = getDefinition(); D && D != this)
+    return D->field_begin();
   return field_iterator(decl_iterator(FirstDecl));
 }
 
 /// completeDefinition - Notes that the definition of this type is now
 /// complete.
 void RecordDecl::completeDefinition() {
   assert(!isCompleteDefinition() && "Cannot redefine record!");
   TagDecl::completeDefinition();