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

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL296656: [PCH] Avoid VarDecl emission attempt if no owning 
module avaiable (authored by bruno).

Changed prior to commit:
  https://reviews.llvm.org/D29753?vs=87774=90212#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D29753

Files:
  cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
  cfe/trunk/test/PCH/empty-def-fwd-struct.h


Index: cfe/trunk/test/PCH/empty-def-fwd-struct.h
===
--- cfe/trunk/test/PCH/empty-def-fwd-struct.h
+++ cfe/trunk/test/PCH/empty-def-fwd-struct.h
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -emit-pch -x c++-header %s -std=c++14 -o %t.pch
+// RUN: %clang_cc1 -emit-llvm-only -x c++ /dev/null -std=c++14 -include-pch 
%t.pch -o %t.o
+struct FVector;
+struct FVector {};
+struct FBox {
+  FVector Min;
+  FBox(int);
+};
+namespace {
+FBox InvalidBoundingBox(0);
+}
+
Index: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
===
--- cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
+++ cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
@@ -2530,8 +2530,8 @@
 
   // An ImportDecl or VarDecl imported from a module will get emitted when
   // we import the relevant module.
-  if ((isa(D) || isa(D)) && Ctx.DeclMustBeEmitted(D) &&
-  D->getImportedOwningModule())
+  if ((isa(D) || isa(D)) && D->getImportedOwningModule() 
&&
+  Ctx.DeclMustBeEmitted(D))
 return false;
 
   if (isa(D) || 


Index: cfe/trunk/test/PCH/empty-def-fwd-struct.h
===
--- cfe/trunk/test/PCH/empty-def-fwd-struct.h
+++ cfe/trunk/test/PCH/empty-def-fwd-struct.h
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -emit-pch -x c++-header %s -std=c++14 -o %t.pch
+// RUN: %clang_cc1 -emit-llvm-only -x c++ /dev/null -std=c++14 -include-pch %t.pch -o %t.o
+struct FVector;
+struct FVector {};
+struct FBox {
+  FVector Min;
+  FBox(int);
+};
+namespace {
+FBox InvalidBoundingBox(0);
+}
+
Index: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
===
--- cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
+++ cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
@@ -2530,8 +2530,8 @@
 
   // An ImportDecl or VarDecl imported from a module will get emitted when
   // we import the relevant module.
-  if ((isa(D) || isa(D)) && Ctx.DeclMustBeEmitted(D) &&
-  D->getImportedOwningModule())
+  if ((isa(D) || isa(D)) && D->getImportedOwningModule() &&
+  Ctx.DeclMustBeEmitted(D))
 return false;
 
   if (isa(D) || 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 ::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

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

[Richard Smith via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.

Yes, I'm OK with this as a stopgap fix for 4.0. I would have preferred a more 
full fix for 4.0 but we've run out of time for that, and the PCH case does seem 
more pressing.


https://reviews.llvm.org/D29753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Hans Wennborg via Phabricator via cfe-commits]

hans accepted this revision.
hans added a comment.
This revision is now accepted and ready to land.

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

> > What do folks think?
>
> IMO we should do it.


Go ahead and commit this, but keep the bug open so we can work on fixing it 
properly eventually.


https://reviews.llvm.org/D29753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.



> What do folks think?

IMO we should do it.


https://reviews.llvm.org/D29753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

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

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

[Hans Wennborg via Phabricator via cfe-commits]

hans added a subscriber: mehdi_amini.
hans added a comment.

To unblock 4.0, I'm leaning towards merging Bruno's patch as a stop-gap fix.

I realize it probably only fixes the problem for PCH, and not modules. But PCH 
is used more widely than modules, so maybe it's good enough for now?

We've run out of time for 4.0, and it seems fixing this properly might be 
involved.

What do folks think?


https://reviews.llvm.org/D29753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

This seems to be stuck. Bruno, Richard, do you think there's a chance this can 
be fixed for 4.0?


https://reviews.llvm.org/D29753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

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

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.


https://reviews.llvm.org/D29753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Hans Wennborg via cfe-commits]

Richard, can you take a look when you have a moment? The PR is marked
as a release blocker.

On Thu, Feb 9, 2017 at 1:54 PM, Duncan P. N. Exon Smith via
Phabricator via cfe-commits  wrote:
> dexonsmith added a comment.
>
> I'm not comfortable signing off on this, but it seems like this should be set 
> up as a blocker for LLVM 4.0 if it isn't already.
>
>
>
> 
> Comment at: lib/Serialization/ASTReaderDecl.cpp:2518-2523
>// An ImportDecl or VarDecl imported from a module will get emitted when
>// we import the relevant module.
> -  if ((isa(D) || isa(D)) && Ctx.DeclMustBeEmitted(D) &&
> -  D->getImportedOwningModule())
> +  if ((isa(D) || isa(D)) && 
> D->getImportedOwningModule() &&
> +  Ctx.DeclMustBeEmitted(D))
>  return false;
>
> 
> It's not locally obvious why the order matters.  Can you add a comment 
> explaining why you need to check getImportedOwningModule first?  It might be 
> worth splitting Ctx.DeclMustBeEmitted into its own; e.g.,
> ```
> // An ImportDecl or VarDecl imported from a module will get emitted when
> // we import the relevant module.
> if ((isa(D) || isa(D)) && D->getImportedOwningModule())
>   // Only check DeclMustBeEmitted if D has been fully imported, since it may
>   // emit D as a side effect.
>   if (Ctx.DeclMustBeEmitted(D))
> return false;
> ```
> but anything that prevents someone from swapping the conditions when they 
> refactor this would be good enough I think.
>
>
> https://reviews.llvm.org/D29753
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

I'm not comfortable signing off on this, but it seems like this should be set 
up as a blocker for LLVM 4.0 if it isn't already.




Comment at: lib/Serialization/ASTReaderDecl.cpp:2518-2523
   // An ImportDecl or VarDecl imported from a module will get emitted when
   // we import the relevant module.
-  if ((isa(D) || isa(D)) && Ctx.DeclMustBeEmitted(D) &&
-  D->getImportedOwningModule())
+  if ((isa(D) || isa(D)) && D->getImportedOwningModule() 
&&
+  Ctx.DeclMustBeEmitted(D))
 return false;
 

It's not locally obvious why the order matters.  Can you add a comment 
explaining why you need to check getImportedOwningModule first?  It might be 
worth splitting Ctx.DeclMustBeEmitted into its own; e.g.,
```
// An ImportDecl or VarDecl imported from a module will get emitted when
// we import the relevant module.
if ((isa(D) || isa(D)) && D->getImportedOwningModule())
  // Only check DeclMustBeEmitted if D has been fully imported, since it may
  // emit D as a side effect.
  if (Ctx.DeclMustBeEmitted(D))
return false;
```
but anything that prevents someone from swapping the conditions when they 
refactor this would be good enough I think.


https://reviews.llvm.org/D29753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno created this revision.

This fixes PR31863, a regression introduced in r276159.

Consider this snippet:

struct FVector;
struct FVector {};
struct FBox {

  FVector Min;
  FBox(int);

};
namespace {
FBox InvalidBoundingBox(0);
}

While parsing the DECL_VAR for 'struct FBox', clang recursively read all the
dep decls until it finds the DECL_CXX_RECORD forward declaration for 'struct
FVector'. Then, it resumes all the way up back to DECL_VAR handling in
`ReadDeclRecord`, where it checks if `isConsumerInterestedIn` for the decl.

One of the condition for `isConsumerInterestedIn` to return false is if the
VarDecl is imported from a module `D->getImportedOwningModule()`, because it
will get emitted when we import the relevant module. However, before checking
if it comes from a module, clang checks if `Ctx.DeclMustBeEmitted(D)`, which
triggers the emission of 'struct FBox'. Since one of its fields is still
incomplete, it crashes.

Instead, check if `D->getImportedOwningModule()` is true before calling
`Ctx.DeclMustBeEmitted(D)`.


https://reviews.llvm.org/D29753

Files:
  lib/Serialization/ASTReaderDecl.cpp
  test/PCH/empty-def-fwd-struct.h


Index: test/PCH/empty-def-fwd-struct.h
===
--- /dev/null
+++ test/PCH/empty-def-fwd-struct.h
@@ -0,0 +1,12 @@
+// 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;
+struct FVector {};
+struct FBox {
+  FVector Min;
+  FBox(int);
+};
+namespace {
+FBox InvalidBoundingBox(0);
+}
+
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -2517,8 +2517,8 @@
 
   // An ImportDecl or VarDecl imported from a module will get emitted when
   // we import the relevant module.
-  if ((isa(D) || isa(D)) && Ctx.DeclMustBeEmitted(D) &&
-  D->getImportedOwningModule())
+  if ((isa(D) || isa(D)) && D->getImportedOwningModule() 
&&
+  Ctx.DeclMustBeEmitted(D))
 return false;
 
   if (isa(D) || 


Index: test/PCH/empty-def-fwd-struct.h
===
--- /dev/null
+++ test/PCH/empty-def-fwd-struct.h
@@ -0,0 +1,12 @@
+// 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;
+struct FVector {};
+struct FBox {
+  FVector Min;
+  FBox(int);
+};
+namespace {
+FBox InvalidBoundingBox(0);
+}
+
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -2517,8 +2517,8 @@
 
   // An ImportDecl or VarDecl imported from a module will get emitted when
   // we import the relevant module.
-  if ((isa(D) || isa(D)) && Ctx.DeclMustBeEmitted(D) &&
-  D->getImportedOwningModule())
+  if ((isa(D) || isa(D)) && D->getImportedOwningModule() &&
+  Ctx.DeclMustBeEmitted(D))
 return false;
 
   if (isa(D) || 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits