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