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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
