Changeset View
Standalone View
clang/lib/Serialization/ASTReaderDecl.cpp
| Show First 20 Lines • Show All 579 Lines • ▼ Show 20 Lines | if (!LexicalDC) | ||||
| LexicalDC = SemaDC; | LexicalDC = SemaDC; | ||||
| DeclContext *MergedSemaDC = Reader.MergedDeclContexts.lookup(SemaDC); | DeclContext *MergedSemaDC = Reader.MergedDeclContexts.lookup(SemaDC); | ||||
| // Avoid calling setLexicalDeclContext() directly because it uses | // Avoid calling setLexicalDeclContext() directly because it uses | ||||
| // Decl::getASTContext() internally which is unsafe during derialization. | // Decl::getASTContext() internally which is unsafe during derialization. | ||||
| D->setDeclContextsImpl(MergedSemaDC ? MergedSemaDC : SemaDC, LexicalDC, | D->setDeclContextsImpl(MergedSemaDC ? MergedSemaDC : SemaDC, LexicalDC, | ||||
| Reader.getContext()); | Reader.getContext()); | ||||
| } | } | ||||
| D->setLocation(ThisDeclLoc); | D->setLocation(ThisDeclLoc); | ||||
| D->setInvalidDecl(Record.readInt()); | D->InvalidDecl = Record.readInt(); | ||||
rsmith: The bug is here: we should not be calling `Decl::setInvalidDecl` here because it has invariants… | |||||
Didn't notice that we're a friend and can access private members—but it makes sense. That change fixes the bug as well. aaronpuchert: Didn't notice that we're a friend and can access private members—but it makes sense. That… | |||||
| if (Record.readInt()) { // hasAttrs | if (Record.readInt()) { // hasAttrs | ||||
| AttrVec Attrs; | AttrVec Attrs; | ||||
| Record.readAttributes(Attrs); | Record.readAttributes(Attrs); | ||||
| // Avoid calling setAttrs() directly because it uses Decl::getASTContext() | // Avoid calling setAttrs() directly because it uses Decl::getASTContext() | ||||
| // internally which is unsafe during derialization. | // internally which is unsafe during derialization. | ||||
| D->setAttrsImpl(Attrs, Reader.getContext()); | D->setAttrsImpl(Attrs, Reader.getContext()); | ||||
| } | } | ||||
| D->setImplicit(Record.readInt()); | D->setImplicit(Record.readInt()); | ||||
| ▲ Show 20 Lines • Show All 899 Lines • ▼ Show 20 Lines | void ASTDeclReader::VisitParmVarDecl(ParmVarDecl *PD) { | ||||
| // FIXME: If this is a redeclaration of a function from another module, handle | // FIXME: If this is a redeclaration of a function from another module, handle | ||||
| // inheritance of default arguments. | // inheritance of default arguments. | ||||
| } | } | ||||
| void ASTDeclReader::VisitDecompositionDecl(DecompositionDecl *DD) { | void ASTDeclReader::VisitDecompositionDecl(DecompositionDecl *DD) { | ||||
| VisitVarDecl(DD); | VisitVarDecl(DD); | ||||
| auto **BDs = DD->getTrailingObjects<BindingDecl *>(); | auto **BDs = DD->getTrailingObjects<BindingDecl *>(); | ||||
| for (unsigned I = 0; I != DD->NumBindings; ++I) { | for (unsigned I = 0; I != DD->NumBindings; ++I) { | ||||
| BDs[I] = readDeclAs<BindingDecl>(); | BDs[I] = readDeclAs<BindingDecl>(); | ||||
Not Done ReplyInline ActionsBindingDecl might not be fully initialized here: if we enter deserialization to deserialize a BindingDecl, and then recurse into reading its binding expression, and then deserialize the DecompositionDecl, we can land here before we finish with the BindingDecl. If we called something that looked at the Binding expression on the BindingDecl, that'd be a problem. But the general idea is that deserialization should never invoke AST functions with invariants (and generally should set state directly rather than using AST member functions in order to avoid accidentally calling a function with an invariant). So it shouldn't matter whether we deserialize the DecompositionDecl or the BindingDecls first. rsmith: `BindingDecl` might not be fully initialized here: if we enter deserialization to deserialize a… | |||||
Not Done ReplyInline ActionsRight, we could end up reading a binding that is already being read higher up in the stack. It would have been visited already, but we wouldn't have the expression yet. aaronpuchert: Right, we could end up reading a binding that is already being read higher up in the stack. It… | |||||
| BDs[I]->setDecomposedDecl(DD); | BDs[I]->setDecomposedDecl(DD); | ||||
| } | } | ||||
| } | } | ||||
| void ASTDeclReader::VisitBindingDecl(BindingDecl *BD) { | void ASTDeclReader::VisitBindingDecl(BindingDecl *BD) { | ||||
| VisitValueDecl(BD); | VisitValueDecl(BD); | ||||
| BD->Binding = Record.readExpr(); | BD->Binding = Record.readExpr(); | ||||
| } | } | ||||
Not Done ReplyInline ActionsHm, presumably BindingDecl::Decomp gets set here because readExpr always reads an expression that involves the DecompositionDecl, which calls setDecomposedDecl? That seems ... very subtle. If that's the intended way for this to work, we should at least add a comment for that (or better, an assert that Decomp got set), and BindingDecl::Decomp should be a regular pointer not a LazyDeclPtr. (But even then, is this chain of reasoning guaranteed to hold even for invalid declarations? Maybe we should be serializing the DecompositionDecl* and setting Decomp here?) rsmith: Hm, presumably `BindingDecl::Decomp` gets set here because `readExpr` always reads an… | |||||
Not Done ReplyInline ActionsOr the BindingDecl is read via the DecompositionDecl in the first place, but otherwise I think that's right. An assertion works neither before nor after this change: if the DecompositionDecl is read first, or contains more than one binding, it's created before some BindingDecl. Then readExpr will presumably just build a DeclRefExpr to the still unfinished decomposition that we're building higher in the stack, so we can't assume the back reference to be set. For invalid declarations, I think we should still have a correspondence between decomposition and bindings, but the expression is obviously nullptr in some cases. So maybe if we only load a BindingDecl (would have to be a global variable then, right?) and not the decomposition, Decomp might stay uninitialized, but I'm not sure where to check that. If we want to deserialize the DecompositionDecl here, we'd probably need to store declarations both ways? I think we'd still want to find all bindings for a decomposition that we're reading. Regarding the LazyDeclPtr I think you're right—I don't see us initializing this with an offset anywhere. So using a plain pointer should be trivially NFC. aaronpuchert: Or the `BindingDecl` is read via the `DecompositionDecl` in the first place, but otherwise I… | |||||
| void ASTDeclReader::VisitFileScopeAsmDecl(FileScopeAsmDecl *AD) { | void ASTDeclReader::VisitFileScopeAsmDecl(FileScopeAsmDecl *AD) { | ||||
| VisitDecl(AD); | VisitDecl(AD); | ||||
| AD->setAsmString(cast<StringLiteral>(Record.readExpr())); | AD->setAsmString(cast<StringLiteral>(Record.readExpr())); | ||||
| AD->setRParenLoc(readSourceLocation()); | AD->setRParenLoc(readSourceLocation()); | ||||
| } | } | ||||
| void ASTDeclReader::VisitBlockDecl(BlockDecl *BD) { | void ASTDeclReader::VisitBlockDecl(BlockDecl *BD) { | ||||
| ▲ Show 20 Lines • Show All 3,166 Lines • Show Last 20 Lines | |||||
The bug is here: we should not be calling Decl::setInvalidDecl here because it has invariants, and the Decl is incomplete at this point.