[PATCH] D29753: [PCH] Avoid early VarDecl emission attempt if no owning module avaiable

Bruno Cardoso Lopes via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Feb 28 11:37:07 PST 2017


bruno added a comment.



> It seems to me that the problem here is that `DeclMustBeEmitted` is not safe to call in the middle of deserialization if anything partially-deserialized might be reachable from the declaration we're querying, and yet we're currently calling it in that case.

`DeclMustBeEmitted` name seems misleading since we does more than just checking, it actually tries to emit stuff. If it's a local module, shouldn't be everything already available? Do we have non-deserialized items for a local module? Maybe I get the wrong idea of what local means in this context..

> I don't see how this patch actually addresses that problem, though: if you build this testcase as a module instead of a PCH, won't it still fail in the same way that it does now?

`D->getImportedOwningModule` calls `isFromASTFile`, which refers to PCH and modules, shouldn't it work for both then? I couldn't come up with a testcase that triggered it for modules though.

> I think we probably need to track all the declarations we deserialize in a top-level deserialization,  and only check whether they're interesting when we finish recursive deserialization (somewhere around `ASTReader::FinishedDeserializing`). For the update record case, this will need some care in order to retain the property that we only pass a declaration to the consumer once, but I think we can make that work by observing that it should generally be safe to call `DeclMustBeEmitted` on a declaration that we didn't create in this deserialization step, and when we're loading update records we know whether that's the case.

Right. This is a more involved fix and I don't fully understand all r276159 consequences yet. OTOH, the current patch improves the situation and unblock some big projects to compile with clang ToT, for instance, Unreal Engine 4.


https://reviews.llvm.org/D29753





More information about the cfe-commits mailing list