[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[Luboš Luňák via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG31b05692cd33: make -fmodules-codegen and -fmodules-debuginfo 
work also with PCHs (authored by llunak).

Changed prior to commit:
  https://reviews.llvm.org/D69778?vs=263839=276726#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778

Files:
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/Modules/Inputs/codegen-flags/foo.h
  clang/test/PCH/codegen.cpp

Index: clang/test/PCH/codegen.cpp
===
--- /dev/null
+++ clang/test/PCH/codegen.cpp
@@ -0,0 +1,42 @@
+// This test is the PCH version of Modules/codegen-flags.test . It uses its inputs.
+// The purpose of this test is just verify that the codegen options work with PCH as well.
+// All the codegen functionality should be tested in Modules/.
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fmodules-codegen -x c++-header -building-pch-with-obj -emit-pch %S/../Modules/Inputs/codegen-flags/foo.h -o %t/foo-cg.pch
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fmodules-debuginfo -x c++-header -building-pch-with-obj -emit-pch %S/../Modules/Inputs/codegen-flags/foo.h -o %t/foo-di.pch
+
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - %s -include-pch %t/foo-cg.pch -building-pch-with-obj -fmodules-codegen | FileCheck --check-prefix=CG %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - %s -include-pch %t/foo-di.pch -building-pch-with-obj -fmodules-debuginfo | FileCheck --check-prefix=DI %s
+
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - -include-pch %t/foo-cg.pch %S/../Modules/Inputs/codegen-flags/use.cpp | FileCheck --check-prefix=CG-USE %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - -include-pch %t/foo-di.pch %S/../Modules/Inputs/codegen-flags/use.cpp | FileCheck --check-prefix=DI-USE %s
+
+// Test with template instantiation in the pch.
+
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fmodules-codegen -x c++-header -building-pch-with-obj -emit-pch -fpch-instantiate-templates %S/../Modules/Inputs/codegen-flags/foo.h -o %t/foo-cg.pch
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fmodules-debuginfo -x c++-header -building-pch-with-obj -emit-pch -fpch-instantiate-templates %S/../Modules/Inputs/codegen-flags/foo.h -o %t/foo-di.pch
+
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - %s -include-pch %t/foo-cg.pch -building-pch-with-obj -fmodules-codegen | FileCheck --check-prefix=CG %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - %s -include-pch %t/foo-di.pch -building-pch-with-obj -fmodules-debuginfo | FileCheck --check-prefix=DI %s
+
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - -include-pch %t/foo-cg.pch %S/../Modules/Inputs/codegen-flags/use.cpp | FileCheck --check-prefix=CG-USE %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - -include-pch %t/foo-di.pch %S/../Modules/Inputs/codegen-flags/use.cpp | FileCheck --check-prefix=DI-USE %s
+
+
+// CG: define weak_odr void @_Z2f1v
+// CG: DICompileUnit
+// CG-NOT: DICompositeType
+
+// CG-USE: declare void @_Z2f1v
+// CG-USE: DICompileUnit
+// CG-USE: distinct !DICompositeType(tag: DW_TAG_structure_type, name: "foo"
+
+// DI-NOT: define
+// DI: distinct !DICompositeType(tag: DW_TAG_structure_type, name: "foo"
+
+// DI-USE: define linkonce_odr void @_Z2f1v
+// DI-USE: = !DICompositeType(tag: DW_TAG_structure_type, name: "foo", {{.*}}, flags: DIFlagFwdDecl
Index: clang/test/Modules/Inputs/codegen-flags/foo.h
===
--- clang/test/Modules/Inputs/codegen-flags/foo.h
+++ clang/test/Modules/Inputs/codegen-flags/foo.h
@@ -1,4 +1,7 @@
+#ifndef FOO_H
+#define FOO_H
 struct foo {
 };
 inline void f1() {
 }
+#endif
Index: clang/lib/Serialization/ASTWriterDecl.cpp
===
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -2458,9 +2458,10 @@
 
   assert(FD->doesThisDeclarationHaveABody());
   bool ModulesCodegen = false;
-  if (Writer->WritingModule && !FD->isDependentContext()) {
+  if (!FD->isDependentContext()) {
 Optional Linkage;
-if (Writer->WritingModule->Kind == Module::ModuleInterfaceUnit) {
+if (Writer->WritingModule &&
+Writer->WritingModule->Kind == Module::ModuleInterfaceUnit) {
   // When building a C++ Modules TS module interface unit, a strong
   // definition in the module 

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[David Blaikie via Phabricator via cfe-commits]

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

Sounds good


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778



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

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[Luboš Luňák via Phabricator via cfe-commits]

llunak added a comment.

Ping ...


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778



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

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[Luboš Luňák via Phabricator via cfe-commits]

llunak added a comment.

Ping.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778



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

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[Luboš Luňák via Phabricator via cfe-commits]

llunak added a comment.

So what more needs to be done here to get this change accepted? It's already 
missed 10.0 and 11.1 is getting branched off in about 3 weeks.

- The change was already accepted once and the problem leading to its revert 
has already been fixed.
- It just enables for PCH something that's been enabled for modules for a long 
time (so whatever may be wrong with it will probably be wrong with modules as 
well).
- I've been using the feature for 8 months.
- There's no publicly reported problem with it (I know aganea reported them 
above, but they are private, so nobody can do anything about them).
- The feature is opt-in and disabled by default.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778



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

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

In D69778#2035805 , @llunak wrote:

> @aganea Have you tried how this version of the patch affects PR44953? If not, 
> could you please try?


I've applied the patch, I don't see the issue raised in PR44953 anymore.

However as discussed offline with @llunak I'm just raising a concern: when 
building with this patch + D69585  
(`-fpch-instantiate-templates`) + `-fmodules-debuginfo -fmodules-codegen` as 
instructed, the PCH compilation in one of games crashes with the callstack 
below. On another game, the compilation succeeds, but I can see link errors for 
a few symbols referenced from the .PCH.OBJ.

This patch may be fine on its own, because `-fmodules-debuginfo 
-fmodules-codegen` are not enabled by default with `/Yc`, but more work is 
needed to make this feature the default. This could be done in a subsequent 
patch.

  Unknown code
  UNREACHABLE executed at 
D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\TargetLowering.cpp:5726!
  Stack dump:
  0.Program arguments: -cc1 -triple x86_64-pc-windows-msvc19.20.0 -emit-obj 
(edited) -building-pch-with-obj -fpch-instantiate-templates -fmodules-codegen 
-fmodules-debuginfo -include-pch D:\(edited)\MyProject.pch 
-pch-through-header=(edited)\precomp.h -include (edited)\precomp.h (edited) 
-fms-compatibility-version=19.20 -std=c++17 -fdelayed-template-parsing 
-finline-functions -fobjc-runtime=gcc -fdiagnostics-show-option 
-fdiagnostics-absolute-paths -vectorize-loops -vectorize-slp -O3 -faddrsig -o 
D:\(edited)\MyProject.pch.obj -x c++ D:\(edited)\precomp.cpp
  1. parser at end of file
  2.Code generation
  3.Running pass 'Function Pass Manager' on module '(edited)\precomp.cpp'.
  4.Running pass 'X86 DAG->DAG Instruction Selection' on function 
'@"?GetDeterminant@Matrix44f@MyNamespace@@QEBAMXZ"'
  #0 0x7ff6bada1c06 HandleAbort 
D:\llvm-project\llvm\lib\Support\Windows\Signals.inc:408:0
  #1 0x7ff6beb4b0e1 raise 
D:\llvm-project\build64_stage0\minkernel\crts\ucrt\src\appcrt\misc\signal.cpp:547:0
  #2 0x7ff6beb40e5c abort 
D:\llvm-project\build64_stage0\minkernel\crts\ucrt\src\appcrt\startup\abort.cpp:71:0
  #3 0x7ff6bad58207 llvm::llvm_unreachable_internal(char const *, char 
const *, unsigned int) D:\llvm-project\llvm\lib\Support\ErrorHandling.cpp:210:0
  #4 0x7ff6bbb14d2e llvm::TargetLowering::getNegatedExpression(class 
llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const 
D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\TargetLowering.cpp:5726:0
  #5 0x7ff6bba1dd26 llvm::X86TargetLowering::getNegatedExpression(class 
llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const 
D:\llvm-project\llvm\lib\Target\X86\X86ISelLowering.cpp:42707:0
  #6 0x7ff6bbb140b3 llvm::TargetLowering::getNegatedExpression(class 
llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const 
D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\TargetLowering.cpp:5671:0
  #7 0x7ff6bba1dd26 llvm::X86TargetLowering::getNegatedExpression(class 
llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const 
D:\llvm-project\llvm\lib\Target\X86\X86ISelLowering.cpp:42707:0
  #8 0x7ff6bbb1496c llvm::TargetLowering::getNegatedExpression(class 
llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const 
D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\TargetLowering.cpp:5643:0
  #9 0x7ff6bba1dd26 llvm::X86TargetLowering::getNegatedExpression(class 
llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const 
D:\llvm-project\llvm\lib\Target\X86\X86ISelLowering.cpp:42707:0
  #10 0x7ff6bbb1496c llvm::TargetLowering::getNegatedExpression(class 
llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const 
D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\TargetLowering.cpp:5643:0
  #11 0x7ff6bba1dd26 llvm::X86TargetLowering::getNegatedExpression(class 
llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const 
D:\llvm-project\llvm\lib\Target\X86\X86ISelLowering.cpp:42707:0
  #12 0x7ff6bbb14538 llvm::TargetLowering::getNegatedExpression(class 
llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const 
D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\TargetLowering.cpp:5708:0
  #13 0x7ff6bba1dd26 llvm::X86TargetLowering::getNegatedExpression(class 
llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const 
D:\llvm-project\llvm\lib\Target\X86\X86ISelLowering.cpp:42707:0
  #14 0x7ff6bca4c6fb `anonymous namespace'::DAGCombiner::visit 
D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\DAGCombiner.cpp:1573:0
  #15 0x7ff6bca30e4b `anonymous namespace'::DAGCombiner::combine 
D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\DAGCombiner.cpp:1634:0
  #16 0x7ff6bca3052f llvm::SelectionDAG::Combine(enum llvm::CombineLevel, 
class llvm::AAResults *, enum llvm::CodeGenOpt::Level) 

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[Luboš Luňák via Phabricator via cfe-commits]

llunak added a subscriber: aganea.
llunak added a comment.

In D69778#2035318 , @dblaikie wrote:

> Do you have a sense of the larger testing that PR44953 was reduced from? Have 
> you tried compiling a non-trivial codebase (I assume you might've tested it 
> with Open Office, for instance) - wonder why PR44953 didn't show up there? 
> (perhaps just the construct needed to tickle the issue isn't common/isn't 
> common in Open Office code, etc)


I've been building LibreOffice and some of its 3rd-party dependencies such as 
Skia for months with this patch, without problems. PR44953 seems to be a corner 
case, we do not use D3DX12 headers in LO, and (I presume) __declspec(selectany) 
is a niche feature. And while I do not understand why PR44953 was happening, I 
do understand why my original patch was triggering it, and it will not happen 
with this patch (or, if it will happen, then it'll happen with modules as 
well). And I've tested with the testcase in PR44953.

(It's LibreOffice BTW, OpenOffice is anything but dead.)

> and/or maybe see if the person who filed PR44953 might be able to test this 
> version of the patch on the original codebase that bug was reduced from?

@aganea Have you tried how this version of the patch affects PR44953? If not, 
could you please try?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778



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

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D69778#2035006 , @llunak wrote:

> In D69778#2032363 , @dblaikie wrote:
>
> > So the original commit ( cbc9d22e49b4 
> >  ) was 
> > reverted at some point, and now you're proposing recommitting it with a 
> > slight change?
>
>
>
>
>   Yes.
>   
>
> > Could you summarize (in the summary (which should hopefully be included in 
> > the commit message eventually)) the commit (include the hash), the revert 
> > (including the hash), reasons for revert and what's changed in this patch 
> > compared to the original commit that addresses the reasons for reverting?
>
> Done and see D74846  for details and the 
> exact change.
>
>   (& ideally what extra testing was done on the newer version of the patch to 
> ensure the original issue or another like it would now be caught before 
> committing)
>   
>
> There's no testing besides checking that PR44953 no longer crashes. As said 
> in D74846 , I don't see how to do a test for 
> this, and if there's a problem with this patch then the same problem should 
> also exist with modules.


Do you have a sense of the larger testing that PR44953 was reduced from? Have 
you tried compiling a non-trivial codebase (I assume you might've tested it 
with Open Office, for instance) - wonder why PR44953 didn't show up there? 
(perhaps just the construct needed to tickle the issue isn't common/isn't 
common in Open Office code, etc) and/or maybe see if the person who filed 
PR44953 might be able to test this version of the patch on the original 
codebase that bug was reduced from?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778



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

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[Luboš Luňák via Phabricator via cfe-commits]

llunak updated this revision to Diff 263839.
llunak edited the summary of this revision.

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778

Files:
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/Modules/Inputs/codegen-flags/foo.h
  clang/test/PCH/codegen.cpp

Index: clang/test/PCH/codegen.cpp
===
--- /dev/null
+++ clang/test/PCH/codegen.cpp
@@ -0,0 +1,30 @@
+// This test is the PCH version of Modules/codegen-flags.test . It uses its inputs.
+// The purpose of this test is just verify that the codegen options work with PCH as well.
+// All the codegen functionality should be tested in Modules/.
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fmodules-codegen -x c++-header -building-pch-with-obj -emit-pch %S/../Modules/Inputs/codegen-flags/foo.h -o %t/foo-cg.pch
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fmodules-debuginfo -x c++-header -building-pch-with-obj -emit-pch %S/../Modules/Inputs/codegen-flags/foo.h -o %t/foo-di.pch
+
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - %s -include-pch %t/foo-cg.pch -building-pch-with-obj -fmodules-codegen | FileCheck --check-prefix=CG %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - %s -include-pch %t/foo-di.pch -building-pch-with-obj -fmodules-debuginfo | FileCheck --check-prefix=DI %s
+
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - -include-pch %t/foo-cg.pch %S/../Modules/Inputs/codegen-flags/use.cpp | FileCheck --check-prefix=CG-USE %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - -include-pch %t/foo-di.pch %S/../Modules/Inputs/codegen-flags/use.cpp | FileCheck --check-prefix=DI-USE %s
+
+// CG: define weak_odr void @_Z2f1v
+// CG: DICompileUnit
+// CG-NOT: DICompositeType
+
+// CG-USE: declare void @_Z2f1v
+// CG-USE: DICompileUnit
+// CG-USE: distinct !DICompositeType(tag: DW_TAG_structure_type, name: "foo"
+
+// DI-NOT: define
+// DI: distinct !DICompositeType(tag: DW_TAG_structure_type, name: "foo"
+
+// DI-USE: define linkonce_odr void @_Z2f1v
+// DI-USE: = !DICompositeType(tag: DW_TAG_structure_type, name: "foo", {{.*}}, flags: DIFlagFwdDecl
Index: clang/test/Modules/Inputs/codegen-flags/foo.h
===
--- clang/test/Modules/Inputs/codegen-flags/foo.h
+++ clang/test/Modules/Inputs/codegen-flags/foo.h
@@ -1,4 +1,7 @@
+#ifndef FOO_H
+#define FOO_H
 struct foo {
 };
 inline void f1() {
 }
+#endif
Index: clang/lib/Serialization/ASTWriterDecl.cpp
===
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -2460,9 +2460,10 @@
 
   assert(FD->doesThisDeclarationHaveABody());
   bool ModulesCodegen = false;
-  if (Writer->WritingModule && !FD->isDependentContext()) {
+  if (!FD->isDependentContext()) {
 Optional Linkage;
-if (Writer->WritingModule->Kind == Module::ModuleInterfaceUnit) {
+if (Writer->WritingModule &&
+Writer->WritingModule->Kind == Module::ModuleInterfaceUnit) {
   // When building a C++ Modules TS module interface unit, a strong
   // definition in the module interface is provided by the compilation of
   // that module interface unit, not by its users. (Inline functions are
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -5631,8 +5631,8 @@
 
   // getODRHash will compute the ODRHash if it has not been previously computed.
   Record->push_back(D->getODRHash());
-  bool ModulesDebugInfo = Writer->Context->getLangOpts().ModulesDebugInfo &&
-  Writer->WritingModule && !D->isDependentType();
+  bool ModulesDebugInfo =
+  Writer->Context->getLangOpts().ModulesDebugInfo && !D->isDependentType();
   Record->push_back(ModulesDebugInfo);
   if (ModulesDebugInfo)
 Writer->ModularCodegenDecls.push_back(Writer->GetDeclRef(D));
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -503,8 +503,12 @@
 }
 
 void ASTDeclReader::ReadFunctionDefinition(FunctionDecl *FD) {
-  if (Record.readInt())
+  if (Record.readInt()) {
 Reader.DefinitionSource[FD] = Loc.F->Kind == ModuleKind::MK_MainFile;
+if (Reader.getContext().getLangOpts().BuildingPCHWithObjectFile &&
+Reader.DeclIsFromPCHWithObjectFile(FD))
+  

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[Luboš Luňák via Phabricator via cfe-commits]

llunak added a comment.

In D69778#2032363 , @dblaikie wrote:

> So the original commit ( cbc9d22e49b4 
>  ) was 
> reverted at some point, and now you're proposing recommitting it with a 
> slight change?


Yes.

> Could you summarize (in the summary (which should hopefully be included in 
> the commit message eventually)) the commit (include the hash), the revert 
> (including the hash), reasons for revert and what's changed in this patch 
> compared to the original commit that addresses the reasons for reverting?

Done and see D74846  for details and the exact 
change.

(& ideally what extra testing was done on the newer version of the patch to 
ensure the original issue or another like it would now be caught before 
committing)

There's no testing besides checking that PR44953 no longer crashes. As said in 
D74846 , I don't see how to do a test for 
this, and if there's a problem with this patch then the same problem should 
also exist with modules.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778



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

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[Luboš Luňák via Phabricator via cfe-commits]

llunak updated this revision to Diff 263838.
llunak edited the summary of this revision.
llunak added a comment.

Updated commit message.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778

Files:
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/Modules/Inputs/codegen-flags/foo.h
  clang/test/PCH/codegen.cpp

Index: clang/test/PCH/codegen.cpp
===
--- /dev/null
+++ clang/test/PCH/codegen.cpp
@@ -0,0 +1,30 @@
+// This test is the PCH version of Modules/codegen-flags.test . It uses its inputs.
+// The purpose of this test is just verify that the codegen options work with PCH as well.
+// All the codegen functionality should be tested in Modules/.
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fmodules-codegen -x c++-header -building-pch-with-obj -emit-pch %S/../Modules/Inputs/codegen-flags/foo.h -o %t/foo-cg.pch
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fmodules-debuginfo -x c++-header -building-pch-with-obj -emit-pch %S/../Modules/Inputs/codegen-flags/foo.h -o %t/foo-di.pch
+
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - %s -include-pch %t/foo-cg.pch -building-pch-with-obj -fmodules-codegen | FileCheck --check-prefix=CG %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - %s -include-pch %t/foo-di.pch -building-pch-with-obj -fmodules-debuginfo | FileCheck --check-prefix=DI %s
+
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - -include-pch %t/foo-cg.pch %S/../Modules/Inputs/codegen-flags/use.cpp | FileCheck --check-prefix=CG-USE %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - -include-pch %t/foo-di.pch %S/../Modules/Inputs/codegen-flags/use.cpp | FileCheck --check-prefix=DI-USE %s
+
+// CG: define weak_odr void @_Z2f1v
+// CG: DICompileUnit
+// CG-NOT: DICompositeType
+
+// CG-USE: declare void @_Z2f1v
+// CG-USE: DICompileUnit
+// CG-USE: distinct !DICompositeType(tag: DW_TAG_structure_type, name: "foo"
+
+// DI-NOT: define
+// DI: distinct !DICompositeType(tag: DW_TAG_structure_type, name: "foo"
+
+// DI-USE: define linkonce_odr void @_Z2f1v
+// DI-USE: = !DICompositeType(tag: DW_TAG_structure_type, name: "foo", {{.*}}, flags: DIFlagFwdDecl
Index: clang/test/Modules/Inputs/codegen-flags/foo.h
===
--- clang/test/Modules/Inputs/codegen-flags/foo.h
+++ clang/test/Modules/Inputs/codegen-flags/foo.h
@@ -1,4 +1,7 @@
+#ifndef FOO_H
+#define FOO_H
 struct foo {
 };
 inline void f1() {
 }
+#endif
Index: clang/lib/Serialization/ASTWriterDecl.cpp
===
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -2460,9 +2460,10 @@
 
   assert(FD->doesThisDeclarationHaveABody());
   bool ModulesCodegen = false;
-  if (Writer->WritingModule && !FD->isDependentContext()) {
+  if (!FD->isDependentContext()) {
 Optional Linkage;
-if (Writer->WritingModule->Kind == Module::ModuleInterfaceUnit) {
+if (Writer->WritingModule &&
+Writer->WritingModule->Kind == Module::ModuleInterfaceUnit) {
   // When building a C++ Modules TS module interface unit, a strong
   // definition in the module interface is provided by the compilation of
   // that module interface unit, not by its users. (Inline functions are
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -5631,8 +5631,8 @@
 
   // getODRHash will compute the ODRHash if it has not been previously computed.
   Record->push_back(D->getODRHash());
-  bool ModulesDebugInfo = Writer->Context->getLangOpts().ModulesDebugInfo &&
-  Writer->WritingModule && !D->isDependentType();
+  bool ModulesDebugInfo =
+  Writer->Context->getLangOpts().ModulesDebugInfo && !D->isDependentType();
   Record->push_back(ModulesDebugInfo);
   if (ModulesDebugInfo)
 Writer->ModularCodegenDecls.push_back(Writer->GetDeclRef(D));
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -503,8 +503,12 @@
 }
 
 void ASTDeclReader::ReadFunctionDefinition(FunctionDecl *FD) {
-  if (Record.readInt())
+  if (Record.readInt()) {
 Reader.DefinitionSource[FD] = Loc.F->Kind == ModuleKind::MK_MainFile;
+if (Reader.getContext().getLangOpts().BuildingPCHWithObjectFile &&
+

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[David Blaikie via Phabricator via cfe-commits]

dblaikie requested changes to this revision.
dblaikie added a comment.
This revision now requires changes to proceed.

So the original commit ( cbc9d22e49b4 
 ) was 
reverted at some point, and now you're proposing recommitting it with a slight 
change?

Could you summarize (in the summary (which should hopefully be included in the 
commit message eventually)) the commit (include the hash), the revert 
(including the hash), reasons for revert and what's changed in this patch 
compared to the original commit that addresses the reasons for reverting? (& 
ideally what extra testing was done on the newer version of the patch to ensure 
the original issue or another like it would now be caught before committing)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778



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

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[Luboš Luňák via Phabricator via cfe-commits]

llunak added a comment.

Ping? I've reopened this one as suggested in D74846 
, but apparently it's kept the accepted state, 
so I'm not sure if this needs another approval or what to do here.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778



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

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[Luboš Luňák via Phabricator via cfe-commits]

llunak updated this revision to Diff 261722.
llunak added a comment.

Reopening because of https://reviews.llvm.org/D74846, updated version includes 
the partial revert from there.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778

Files:
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/Modules/Inputs/codegen-flags/foo.h
  clang/test/PCH/codegen.cpp

Index: clang/test/PCH/codegen.cpp
===
--- /dev/null
+++ clang/test/PCH/codegen.cpp
@@ -0,0 +1,30 @@
+// This test is the PCH version of Modules/codegen-flags.test . It uses its inputs.
+// The purpose of this test is just verify that the codegen options work with PCH as well.
+// All the codegen functionality should be tested in Modules/.
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fmodules-codegen -x c++-header -building-pch-with-obj -emit-pch %S/../Modules/Inputs/codegen-flags/foo.h -o %t/foo-cg.pch
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fmodules-debuginfo -x c++-header -building-pch-with-obj -emit-pch %S/../Modules/Inputs/codegen-flags/foo.h -o %t/foo-di.pch
+
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - %s -include-pch %t/foo-cg.pch -building-pch-with-obj -fmodules-codegen | FileCheck --check-prefix=CG %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - %s -include-pch %t/foo-di.pch -building-pch-with-obj -fmodules-debuginfo | FileCheck --check-prefix=DI %s
+
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - -include-pch %t/foo-cg.pch %S/../Modules/Inputs/codegen-flags/use.cpp | FileCheck --check-prefix=CG-USE %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - -include-pch %t/foo-di.pch %S/../Modules/Inputs/codegen-flags/use.cpp | FileCheck --check-prefix=DI-USE %s
+
+// CG: define weak_odr void @_Z2f1v
+// CG: DICompileUnit
+// CG-NOT: DICompositeType
+
+// CG-USE: declare void @_Z2f1v
+// CG-USE: DICompileUnit
+// CG-USE: distinct !DICompositeType(tag: DW_TAG_structure_type, name: "foo"
+
+// DI-NOT: define
+// DI: distinct !DICompositeType(tag: DW_TAG_structure_type, name: "foo"
+
+// DI-USE: define linkonce_odr void @_Z2f1v
+// DI-USE: = !DICompositeType(tag: DW_TAG_structure_type, name: "foo", {{.*}}, flags: DIFlagFwdDecl
Index: clang/test/Modules/Inputs/codegen-flags/foo.h
===
--- clang/test/Modules/Inputs/codegen-flags/foo.h
+++ clang/test/Modules/Inputs/codegen-flags/foo.h
@@ -1,4 +1,7 @@
+#ifndef FOO_H
+#define FOO_H
 struct foo {
 };
 inline void f1() {
 }
+#endif
Index: clang/lib/Serialization/ASTWriterDecl.cpp
===
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -2460,9 +2460,10 @@
 
   assert(FD->doesThisDeclarationHaveABody());
   bool ModulesCodegen = false;
-  if (Writer->WritingModule && !FD->isDependentContext()) {
+  if (!FD->isDependentContext()) {
 Optional Linkage;
-if (Writer->WritingModule->Kind == Module::ModuleInterfaceUnit) {
+if (Writer->WritingModule &&
+Writer->WritingModule->Kind == Module::ModuleInterfaceUnit) {
   // When building a C++ Modules TS module interface unit, a strong
   // definition in the module interface is provided by the compilation of
   // that module interface unit, not by its users. (Inline functions are
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -5631,8 +5631,8 @@
 
   // getODRHash will compute the ODRHash if it has not been previously computed.
   Record->push_back(D->getODRHash());
-  bool ModulesDebugInfo = Writer->Context->getLangOpts().ModulesDebugInfo &&
-  Writer->WritingModule && !D->isDependentType();
+  bool ModulesDebugInfo =
+  Writer->Context->getLangOpts().ModulesDebugInfo && !D->isDependentType();
   Record->push_back(ModulesDebugInfo);
   if (ModulesDebugInfo)
 Writer->ModularCodegenDecls.push_back(Writer->GetDeclRef(D));
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -503,8 +503,12 @@
 }
 
 void ASTDeclReader::ReadFunctionDefinition(FunctionDecl *FD) {
-  if (Record.readInt())
+  if (Record.readInt()) {
 Reader.DefinitionSource[FD] = Loc.F->Kind == ModuleKind::MK_MainFile;
+if 

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[Luboš Luňák via Phabricator via cfe-commits]

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

Reopening because of https://reviews.llvm.org/D74846, updated version includes 
the partial revert from there.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778



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

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D69778#1929817 , @llunak wrote:

> In D69778#1929737 , @dblaikie wrote:
>
> > I'm not sure how that could be possible - there's no data transferred 
> > between the compilation of the TU's object files and the PCH's object file, 
> > right?
>
>
> Yes, there is, in a way - the PCH itself. I didn't say everything from the 
> PCH was shared, in fact it's exactly the point that it's not everything from 
> the PCH, unlike -fmodules-codegen.
>
> > Looks to me like the original patch claims the PCH object file contains all 
> > the dllexported inline functions from the header. Which, yes, is different 
> > from -fmodules-codegen, but doesn't sound like it's based on usage & I'm 
> > not sure how it could be based on usage.
>
> It's based on what's to be emitted. See ASTContext::DeclMustBeEmitted(), I 
> think the comment there says it all.


Ah, I understand what you're getting at now - thanks for walking me through it!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778



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

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[Luboš Luňák via Phabricator via cfe-commits]

llunak added a comment.

In D69778#1929737 , @dblaikie wrote:

> I'm not sure how that could be possible - there's no data transferred between 
> the compilation of the TU's object files and the PCH's object file, right?


Yes, there is, in a way - the PCH itself. I didn't say everything from the PCH 
was shared, in fact it's exactly the point that it's not everything from the 
PCH, unlike -fmodules-codegen.

> Looks to me like the original patch claims the PCH object file contains all 
> the dllexported inline functions from the header. Which, yes, is different 
> from -fmodules-codegen, but doesn't sound like it's based on usage & I'm not 
> sure how it could be based on usage.

It's based on what's to be emitted. See ASTContext::DeclMustBeEmitted(), I 
think the comment there says it all.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778



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

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D69778#1928417 , @llunak wrote:

> In D69778#1928111 , @dblaikie wrote:
>
> > In D69778#1927761 , @llunak wrote:
> >
> > > It comes from 08c5a7b8fda1b97df9d027d1ac096f93edeb6c2f . AFAICT 
> > > -fmodules-codegen is a superset of -building-pch-with-obj, the important 
> > > difference is that -building-pch-with-obj decides which code will be 
> > > shared while building TUs, while -fmodules-codegen decides while building 
> > > the PCH.
> >
> >
> > I'm not sure I follow - how would that be possible? building the object 
> > from the PCH is done independently of any usage, so it can't depend on the 
> > way the PCH is used by any TU that includes it, I think?
>
>
> If a TU uses a PCH, it'll result in some code emitted in every TU that uses 
> the same PCH (e.g. template instantiations from the PCH). And 
> -building-pch-with-obj detects these and discards them in all TUs except the 
> PCH's object file. That means that the code shared in the PCH's object file 
> is only code that would be duplicated in all TUs using the PCH.


I'm not sure how that could be possible - there's no data transferred between 
the compilation of the TU's object files and the PCH's object file, right?

Looks to me like the original patch claims the PCH object file contains all the 
dllexported inline functions from the header. Which, yes, is different from 
-fmodules-codegen, but doesn't sound like it's based on usage & I'm not sure 
how it could be based on usage.

> This is fundamentally different from -fmodules-codegen, which decides what 
> will be emitted and shared already when creating the PCH, which has the 
> consequence that it selects also a lot of code that will be eventually 
> unused, which causes problems like the -Wl,--gc-sections part or not being 
> necessarily worth it (IIRC even your -fmodules-codegen commit says so). So 
> while -fmodules-codegen should be generally superior, -building-pch-with-obj 
> is trivial to use. That's my understanding of both the approaches (and I 
> spent quite a lot of time looking at both when working on D69778 
>  and D69585 
> ).




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778



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

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[Luboš Luňák via Phabricator via cfe-commits]

llunak added a comment.

In D69778#1928111 , @dblaikie wrote:

> In D69778#1927761 , @llunak wrote:
>
> > It comes from 08c5a7b8fda1b97df9d027d1ac096f93edeb6c2f . AFAICT 
> > -fmodules-codegen is a superset of -building-pch-with-obj, the important 
> > difference is that -building-pch-with-obj decides which code will be shared 
> > while building TUs, while -fmodules-codegen decides while building the PCH.
>
>
> I'm not sure I follow - how would that be possible? building the object from 
> the PCH is done independently of any usage, so it can't depend on the way the 
> PCH is used by any TU that includes it, I think?


If a TU uses a PCH, it'll result in some code emitted in every TU that uses the 
same PCH (e.g. template instantiations from the PCH). And 
-building-pch-with-obj detects these and discards them in all TUs except the 
PCH's object file. That means that the code shared in the PCH's object file is 
only code that would be duplicated in all TUs using the PCH. This is 
fundamentally different from -fmodules-codegen, which decides what will be 
emitted and shared already when creating the PCH, which has the consequence 
that it selects also a lot of code that will be eventually unused, which causes 
problems like the -Wl,--gc-sections part or not being necessarily worth it 
(IIRC even your -fmodules-codegen commit says so). So while -fmodules-codegen 
should be generally superior, -building-pch-with-obj is trivial to use. That's 
my understanding of both the approaches (and I spent quite a lot of time 
looking at both when working on D69778  and 
D69585 ).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778



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

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D69778#1927761 , @llunak wrote:

> In D69778#1927526 , @dblaikie wrote:
>
> > @rnk  - know anything about the history of -building-pch-with-obj and 
> > whether it could be replaced/merged with -fmodules-codegen? 
> > (-fmodules-codegen + -fmodules-debuginfo, perhaps?)
>
>
> It comes from 08c5a7b8fda1b97df9d027d1ac096f93edeb6c2f . AFAICT 
> -fmodules-codegen is a superset of -building-pch-with-obj, the important 
> difference is that -building-pch-with-obj decides which code will be shared 
> while building TUs, while -fmodules-codegen decides while building the PCH.


I'm not sure I follow - how would that be possible? building the object from 
the PCH is done independently of any usage, so it can't depend on the way the 
PCH is used by any TU that includes it, I think?

Hmm, yeah, seems it's not context-sensitive (actually Hans says on the original 
review it is context sensitive - but in the reverse: it ensures that 
/unreferenced/ dllexported functions are exported), but it is dllexport 
sensitive. So that's probably enough to not make this generalizable - seems I 
did ask these questions back when the feature was reviewed: 
https://reviews.llvm.org/D48426 & I'd forgotten. Sorry about that. Seems 
there's not much/nothing to do here - the two features are similar, but 
sufficiently different. ( @hans in case he's got any thoughts here)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778



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

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[Luboš Luňák via Phabricator via cfe-commits]

llunak added a comment.

In D69778#1927526 , @dblaikie wrote:

> @rnk  - know anything about the history of -building-pch-with-obj and whether 
> it could be replaced/merged with -fmodules-codegen? (-fmodules-codegen + 
> -fmodules-debuginfo, perhaps?)


It comes from 08c5a7b8fda1b97df9d027d1ac096f93edeb6c2f . AFAICT 
-fmodules-codegen is a superset of -building-pch-with-obj, the important 
difference is that -building-pch-with-obj decides which code will be shared 
while building TUs, while -fmodules-codegen decides while building the PCH. 
This in practice means that -building-pch-with-obj shares only code that would 
be actually used, which makes the result easier to use - using PCH with 
-fmodules-codegen pretty much requires something like -Wl,--gc-sections, 
otherwise there are many undefined references to internal symbols from other 
libraries caused by emitted code that isn't actually used. Since 
-building-pch-with-obj is internally used by clang-cl /Yc , -fmodules-codegen 
cannot be a drop-in replacement for it, unless the Microsoft linker 
automatically discards unused symbols (I don't know, no idea about Windows 
stuff).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778



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

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a subscriber: rnk.
dblaikie added a comment.

In D69778#1779042 , @llunak wrote:

> In D69778#1776472 , @dblaikie wrote:
>
> > I guess one aspect is that -building-pch-with-obj seems like it duplicates 
> > the fmodules-codegen concept (both basically are a flag passed during 
> > pcm/pch build time that says "I promise to build an object file from this 
> > pcm/pch, so rely on that assumption when building other things that depend 
> > on the pcm/pch) - if I'd noticed the -building-pch-with-obj going in I 
> > would've made the point then.
>
>
> Is that described somewhere? There's no mention of modules codegen anywhere 
> under clang/docs. I was under the impression that modules created the codegen 
> part directly in the .pcm in ELF (or whatever) format.


Nah, not as yet - I think I provided some examples in this/other reviews you & 
I have been having, and in the clang tests. Ah, yeah, this one: 
https://reviews.llvm.org/D69779#1801771 (

>> But yeah, that's out of scope for this patch, but might be something that'd 
>> be good to look into to try to merge these features/codepaths more.
> 
> Note that AFAICT the -building-pch-with-obj flag is pretty much internal, 
> used only by the MSVC-like PCH creation, so presumably this could be still 
> unified more without any real harm. And I think I could do the work if it'd 
> be agreed that this would be a good thing.

@rnk  - know anything about the history of -building-pch-with-obj and whether 
it could be replaced/merged with -fmodules-codegen? (-fmodules-codegen + 
-fmodules-debuginfo, perhaps?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778



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

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[Luboš Luňák via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGcbc9d22e49b4: make -fmodules-codegen and -fmodules-debuginfo 
work also with PCHs (authored by llunak).

Changed prior to commit:
  https://reviews.llvm.org/D69778?vs=227634=238115#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778

Files:
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/Modules/Inputs/codegen-flags/foo.h
  clang/test/PCH/codegen.cpp

Index: clang/test/PCH/codegen.cpp
===
--- /dev/null
+++ clang/test/PCH/codegen.cpp
@@ -0,0 +1,30 @@
+// This test is the PCH version of Modules/codegen-flags.test . It uses its inputs.
+// The purpose of this test is just verify that the codegen options work with PCH as well.
+// All the codegen functionality should be tested in Modules/.
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fmodules-codegen -x c++-header -building-pch-with-obj -emit-pch %S/../Modules/Inputs/codegen-flags/foo.h -o %t/foo-cg.pch
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fmodules-debuginfo -x c++-header -building-pch-with-obj -emit-pch %S/../Modules/Inputs/codegen-flags/foo.h -o %t/foo-di.pch
+
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - %s -include-pch %t/foo-cg.pch -building-pch-with-obj -fmodules-codegen | FileCheck --check-prefix=CG %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - %s -include-pch %t/foo-di.pch -building-pch-with-obj -fmodules-debuginfo | FileCheck --check-prefix=DI %s
+
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - -include-pch %t/foo-cg.pch %S/../Modules/Inputs/codegen-flags/use.cpp | FileCheck --check-prefix=CG-USE %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - -include-pch %t/foo-di.pch %S/../Modules/Inputs/codegen-flags/use.cpp | FileCheck --check-prefix=DI-USE %s
+
+// CG: define weak_odr void @_Z2f1v
+// CG: DICompileUnit
+// CG-NOT: DICompositeType
+
+// CG-USE: declare void @_Z2f1v
+// CG-USE: DICompileUnit
+// CG-USE: distinct !DICompositeType(tag: DW_TAG_structure_type, name: "foo"
+
+// DI-NOT: define
+// DI: distinct !DICompositeType(tag: DW_TAG_structure_type, name: "foo"
+
+// DI-USE: define linkonce_odr void @_Z2f1v
+// DI-USE: = !DICompositeType(tag: DW_TAG_structure_type, name: "foo", {{.*}}, flags: DIFlagFwdDecl
Index: clang/test/Modules/Inputs/codegen-flags/foo.h
===
--- clang/test/Modules/Inputs/codegen-flags/foo.h
+++ clang/test/Modules/Inputs/codegen-flags/foo.h
@@ -1,4 +1,7 @@
+#ifndef FOO_H
+#define FOO_H
 struct foo {
 };
 inline void f1() {
 }
+#endif
Index: clang/lib/Serialization/ASTWriterDecl.cpp
===
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1010,15 +1010,16 @@
 
   if (D->getStorageDuration() == SD_Static) {
 bool ModulesCodegen = false;
-if (Writer.WritingModule &&
-!D->getDescribedVarTemplate() && !D->getMemberSpecializationInfo() &&
+if (!D->getDescribedVarTemplate() && !D->getMemberSpecializationInfo() &&
 !isa(D)) {
   // When building a C++ Modules TS module interface unit, a strong
   // definition in the module interface is provided by the compilation of
   // that module interface unit, not by its users. (Inline variables are
   // still emitted in module users.)
   ModulesCodegen =
-  (Writer.WritingModule->Kind == Module::ModuleInterfaceUnit &&
+  (((Writer.WritingModule &&
+ Writer.WritingModule->Kind == Module::ModuleInterfaceUnit) ||
+Writer.Context->getLangOpts().BuildingPCHWithObjectFile) &&
Writer.Context->GetGVALinkageForVariable(D) == GVA_StrongExternal);
 }
 Record.push_back(ModulesCodegen);
@@ -2425,9 +2426,11 @@
 
   assert(FD->doesThisDeclarationHaveABody());
   bool ModulesCodegen = false;
-  if (Writer->WritingModule && !FD->isDependentContext()) {
+  if (!FD->isDependentContext()) {
 Optional Linkage;
-if (Writer->WritingModule->Kind == Module::ModuleInterfaceUnit) {
+if ((Writer->WritingModule &&
+ Writer->WritingModule->Kind == Module::ModuleInterfaceUnit) ||
+Writer->Context->getLangOpts().BuildingPCHWithObjectFile) {
   // When building a C++ Modules TS module interface unit, a strong
   // definition in the module interface is provided by the compilation of
   // that module interface unit, not by its users. (Inline functions are
Index: 

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[Luboš Luňák via Phabricator via cfe-commits]

llunak added a comment.

In D69778#1776472 , @dblaikie wrote:

> I guess one aspect is that -building-pch-with-obj seems like it duplicates 
> the fmodules-codegen concept (both basically are a flag passed during pcm/pch 
> build time that says "I promise to build an object file from this pcm/pch, so 
> rely on that assumption when building other things that depend on the 
> pcm/pch) - if I'd noticed the -building-pch-with-obj going in I would've made 
> the point then.


Is that described somewhere? There's no mention of modules codegen anywhere 
under clang/docs. I was under the impression that modules created the codegen 
part directly in the .pcm in ELF (or whatever) format.

> But yeah, that's out of scope for this patch, but might be something that'd 
> be good to look into to try to merge these features/codepaths more.

Note that AFAICT the -building-pch-with-obj flag is pretty much internal, used 
only by the MSVC-like PCH creation, so presumably this could be still unified 
more without any real harm. And I think I could do the work if it'd be agreed 
that this would be a good thing.

As for this patch itself, could you please also review 
https://reviews.llvm.org/D69585 and https://reviews.llvm.org/D69779 ? I have 
not really tried this patch without the first one, so I'm a bit hesitant to 
merge it alone, and I quickly ran into the problem fixed by the second patch, 
so I'd prefer to merge them all three together if possible.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778



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

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

> In D69778#1772125 , @dblaikie wrote:
> 
>> I was/am still wondering whether there's a way to coalesce these codepaths 
>> between PCH and the existing modular code generation support?
> 
> 
> In what way could this be united more? The way I understand it, the code 
> paths are pretty much the same, they just need to account for being invoked 
> in different contexts. This patch is tiny compared to what it was originally 
> before I made it reuse all the codegen code.

I guess one aspect is that -building-pch-with-obj seems like it duplicates the 
fmodules-codegen concept (both basically are a flag passed during pcm/pch build 
time that says "I promise to build an object file from this pcm/pch, so rely on 
that assumption when building other things that depend on the pcm/pch) - if I'd 
noticed the -building-pch-with-obj going in I would've made the point then.

But yeah, that's out of scope for this patch, but might be something that'd be 
good to look into to try to merge these features/codepaths more.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778



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

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[Luboš Luňák via Phabricator via cfe-commits]

llunak added a comment.

In D69778#1771799 , @rsmith wrote:

> It's a bit weird for this to be controlled by a `-fmodules` flag, but it's 
> only a `-cc1` flag, so I'm OK with that; we can rename it if/when we expose 
> it from the driver.


It's a bit weird, but I didn't want to add -fpch-codegen and then duplicate the 
checks everywhere. And the -building-pch-with-obj part requires a non-trivial 
build setup anyway. But I can create another patch that maps it all to some 
common flag, if wanted.

In D69778#1772125 , @dblaikie wrote:

> I was/am still wondering whether there's a way to coalesce these codepaths 
> between PCH and the existing modular code generation support?


In what way could this be united more? The way I understand it, the code paths 
are pretty much the same, they just need to account for being invoked in 
different contexts. This patch is tiny compared to what it was originally 
before I made it reuse all the codegen code.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778



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

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

@rsmith - thanks for looking at this. I'd looked at it a bit and I was/am still 
wondering whether there's a way to coalesce these codepaths between PCH and the 
existing modular code generation support? But if you're good with it, that's 
certainly good enough for me.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778



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

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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

It's a bit weird for this to be controlled by a `-fmodules` flag, but it's only 
a `-cc1` flag, so I'm OK with that; we can rename it if/when we expose it from 
the driver.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778



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

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[Luboš Luňák via Phabricator via cfe-commits]

llunak added a comment.

Ping.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69778/new/

https://reviews.llvm.org/D69778



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

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[Luboš Luňák via Phabricator via cfe-commits]

llunak created this revision.
llunak added a reviewer: dblaikie.
llunak added a project: clang.
Herald added a subscriber: cfe-commits.

This patch allows to build PCH's (with -building-pch-with-obj and the extra .o 
file) with -fmodules-codegen -fmodules-debuginfo to allow emitting shared code 
into the extra .o file (presumably similarly how modules emit that to wherever 
they emit it). This saves up to 20% of build time here. I can build pretty much 
a complete debug build of LibreOffice with this, with just two minor problems 
(to be sorted out later).

The patch is fairly simple, it basically just duplicates -fmodules checks to 
also alternatively check -building-pch-with-obj.


Repository:
  rC Clang

https://reviews.llvm.org/D69778

Files:
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/Modules/Inputs/codegen-flags/foo.h
  clang/test/PCH/codegen.cpp

Index: clang/test/PCH/codegen.cpp
===
--- /dev/null
+++ clang/test/PCH/codegen.cpp
@@ -0,0 +1,30 @@
+// This test is the PCH version of Modules/codegen-flags.test . It uses its inputs.
+// The purpose of this test is just verify that the codegen options work with PCH as well.
+// All the codegen functionality should be tested in Modules/.
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fmodules-codegen -x c++-header -building-pch-with-obj -emit-pch %S/../Modules/Inputs/codegen-flags/foo.h -o %t/foo-cg.pch
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fmodules-debuginfo -x c++-header -building-pch-with-obj -emit-pch %S/../Modules/Inputs/codegen-flags/foo.h -o %t/foo-di.pch
+
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - %s -include-pch %t/foo-cg.pch -building-pch-with-obj -fmodules-codegen | FileCheck --check-prefix=CG %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - %s -include-pch %t/foo-di.pch -building-pch-with-obj -fmodules-debuginfo | FileCheck --check-prefix=DI %s
+
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - -include-pch %t/foo-cg.pch %S/../Modules/Inputs/codegen-flags/use.cpp | FileCheck --check-prefix=CG-USE %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - -include-pch %t/foo-di.pch %S/../Modules/Inputs/codegen-flags/use.cpp | FileCheck --check-prefix=DI-USE %s
+
+// CG: define weak_odr void @_Z2f1v
+// CG: DICompileUnit
+// CG-NOT: DICompositeType
+
+// CG-USE: declare void @_Z2f1v
+// CG-USE: DICompileUnit
+// CG-USE: distinct !DICompositeType(tag: DW_TAG_structure_type, name: "foo"
+
+// DI-NOT: define
+// DI: distinct !DICompositeType(tag: DW_TAG_structure_type, name: "foo"
+
+// DI-USE: define linkonce_odr void @_Z2f1v
+// DI-USE: = !DICompositeType(tag: DW_TAG_structure_type, name: "foo", {{.*}}, flags: DIFlagFwdDecl
Index: clang/test/Modules/Inputs/codegen-flags/foo.h
===
--- clang/test/Modules/Inputs/codegen-flags/foo.h
+++ clang/test/Modules/Inputs/codegen-flags/foo.h
@@ -1,3 +1,4 @@
+#pragma once
 struct foo {
 };
 inline void f1() {
Index: clang/lib/Serialization/ASTWriterDecl.cpp
===
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -990,15 +990,16 @@
 
   if (D->getStorageDuration() == SD_Static) {
 bool ModulesCodegen = false;
-if (Writer.WritingModule &&
-!D->getDescribedVarTemplate() && !D->getMemberSpecializationInfo() &&
+if (!D->getDescribedVarTemplate() && !D->getMemberSpecializationInfo() &&
 !isa(D)) {
   // When building a C++ Modules TS module interface unit, a strong
   // definition in the module interface is provided by the compilation of
   // that module interface unit, not by its users. (Inline variables are
   // still emitted in module users.)
   ModulesCodegen =
-  (Writer.WritingModule->Kind == Module::ModuleInterfaceUnit &&
+  (((Writer.WritingModule &&
+ Writer.WritingModule->Kind == Module::ModuleInterfaceUnit) ||
+Writer.Context->getLangOpts().BuildingPCHWithObjectFile) &&
Writer.Context->GetGVALinkageForVariable(D) == GVA_StrongExternal);
 }
 Record.push_back(ModulesCodegen);
@@ -2397,9 +2398,11 @@
 
   assert(FD->doesThisDeclarationHaveABody());
   bool ModulesCodegen = false;
-  if (Writer->WritingModule && !FD->isDependentContext()) {
+  if (!FD->isDependentContext()) {
 Optional Linkage;
-if (Writer->WritingModule->Kind == Module::ModuleInterfaceUnit) {
+if ((Writer->WritingModule &&
+ Writer->WritingModule->Kind == Module::ModuleInterfaceUnit) ||
+