[PATCH] D145737: PR60985: Fix merging of lambda closure types across modules.

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

rsmith added inline comments.


================
Comment at: clang/lib/Serialization/ASTReader.cpp:7364
   ReadingKindTracker ReadingKind(Read_Decl, *this);
+  Deserializing D(this);
 
----------------
shafik wrote:
> Curious, why do we need this here and below.
I'm actually not sure how we got away with not having this before. This was probably always broken but just rarely caused problems?

Without this, the following can happen:
1) Deserializing a list of `CXXCtorInitializer`s recursively triggers some other deserialization step, that does create a `Deserializing` object (for example, we deserialize a new type).
2) That other deserialization step finishes, and the `~Deserializing` destructor notices it's the last one, so performs pending actions.
3) Those pending actions perform an ODR checking step (or something else, it doesn't matter what -- the point is that during the post-deserialization cleanup we do things that assume we're not deserializing any more). Maybe there's a static data member for which we loaded multiple definitions from different modules.
4) That step triggers loading a variable's initializer, which calls `GetExternalDeclStmt`, which moves around the `DeclsCursor` and doesn't put it back when it's done. (That should be fine, because we're supposed to be done with recursive deserialization at this point -- and `GetExternalDeclStmt` asserts that. But the assert doesn't fire because there's no `Deserializing` object here.)
5) `~Deserializing` finishes and we continue loading `CXXCtorInitializers`, but we crash because the `DeclsCursor` has been moved to some random other place.

Broadly, we should always have a `Deserializing` object whenever we're doing any kind of recursive deserialization, in order to delay any pending cleanup actions (that assume we're not in the middle of deserialization) until we're done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145737