[PATCH] D86207: (De-)serialize BindingDecls before DecompositionDecl

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:585
   D->setLocation(ThisDeclLoc);
   D->setInvalidDecl(Record.readInt());
   if (Record.readInt()) { // hasAttrs
----------------
The bug is here: we should not be calling `Decl::setInvalidDecl` here because it has invariants, and the `Decl` is incomplete at this point.


================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:1500
   for (unsigned I = 0; I != DD->NumBindings; ++I) {
     BDs[I] = readDeclAs<BindingDecl>();
     BDs[I]->setDecomposedDecl(DD);
----------------
`BindingDecl` 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 `BindingDecl`s first.


================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:1509
   BD->Binding = Record.readExpr();
 }
 
----------------
Hm, 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?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86207/new/

https://reviews.llvm.org/D86207