RecordDecl::LoadFieldsFromExternalStorage() expels existing decls from the DeclContext linked list

[llvmbot]

Bugzilla Link	24420
Version	trunk
OS	All
Reporter	LLVM Bugzilla Contributor
CC	@zygoloid

Extended Description

Unlike DeclContext::LoadLexicalDeclsFromExternalStorage() which preserve existing decls with:

// Splice the newly-read declarations into the beginning of the list // of declarations. Decl ExternalFirst, ExternalLast; std::tie(ExternalFirst, ExternalLast) = BuildDeclChain(Decls, FieldsAlreadyLoaded); ExternalLast->NextInContextAndBits.setPointer(FirstDecl); FirstDecl = ExternalFirst; if (!LastDecl) LastDecl = ExternalLast;

RecordDecl::LoadFieldsFromExternalStorage() resets the linked list:

std::tie(FirstDecl, LastDecl) = BuildDeclChain(Decls, /FieldsAlreadyLoaded=/false);

Assuming it to be empty may be correct most of the time but in my particular usage of Clang this becomes a serious hassle because implicit constructors/operators/etc. may be declared before RecordDecl::LoadFieldsFromExternalStorage() gets called, there's no way to control this and the only workaround I've found is to traverse the entire PCH's AST before doing anything to ensure the fields get deserialized before implicit methods get declared by Sema, which defeats the benefits of lazy deserialization.

(The implicit methods are still returned by Sema::LookupXXXX after being expelled, and they still believe their lexical decl context to be the record, despite missing from a dump())

[ec04fc15-fa35-46f2-80e1-5d271f2ef708]

Yes, this is wrong. How did you manage to get an implicit special member function declared without loading the fields? (Is there some way to write a unit test for this against an unmodified clang binary?)