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

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

In https://reviews.llvm.org/D29753#688834, @bruno wrote:

> > 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.


It doesn't emit anything itself. But like most AST interactions, it *can* trigger more declarations being imported lazily from an external AST source, which can in turn result in them being passed to the AST consumer. And that's why the serialization code generally has to be careful to not call unbounded operations on the AST, because it doesn't want to reenter itself. But in this case we're violating that principle.

> 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..

With this patch, we're calling `DeclMustBeEmitted` in the *non-local* module case (where there can be non-deserialized items).

>> 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?

It only fixes the cases where `getImportedOwningModule` returns 0, which is the normal case for a PCH, but is rare for a declaration from a module.

> I couldn't come up with a testcase that triggered it for modules though.

Something like this triggers it for me:

  $ cat test/PCH/empty-def-fwd-struct.modulemap
  module M { header "empty-def-fwd-struct.h" }
  $ clang -cc1 -x c++ -std=c++14 -fmodules -emit-module -fmodule-name=M test/PCH/empty-def-fwd-struct.modulemap -o tmp.pcm
  $ cat use.cpp
  #include "test/PCH/empty-def-fwd-struct.h"
  $ clang -cc1 -x c++ -std=c++14 -fmodules -emit-llvm -fmodule-file=tmp.pcm use.cpp -o -
  clang: [...]/src/tools/clang/lib/AST/RecordLayoutBuilder.cpp:2933: const clang::ASTRecordLayout &clang::ASTContext::getASTRecordLayout(const clang::RecordDecl *) const: Assertion `D && "Cannot get layout of forward declarations!"' failed.



================
Comment at: test/PCH/empty-def-fwd-struct.h:2
+// RUN: %clang_cc1 -emit-pch -x c++-header %s -std=c++14 -o %t.pch
+// RUN: %clang_cc1 -emit-llvm -x c++ /dev/null -std=c++14 -include-pch %t.pch -o %t.o
+struct FVector;
----------------
Since you don't care about what's in the produced LLVM IR, you can use `-emit-llvm-only` instead here.


https://reviews.llvm.org/D29753