Accept 'clang++ -c a.pch -o a.o' to create PCH's object file

[llunak]

Summary

This way should be the same like with a.pcm for modules.
An alternative way is 'clang++ -c empty.cpp -include-pch a.pch -o a.o -Xclang -building-pch-with-obj', which is what clang-cl's /Yc does internally.

[dblaikie]

Could you add a test case?

[dblaikie]

(/maybe/ two test cases - one for the driver, one for the frontend)

[llunak]

This is tested in D83623 (I'd prefer not to split the changes to the test to reflect both the state before and after D83623).

[dblaikie]

In D83716#2148433, @llunak wrote:

This is tested in D83623 (I'd prefer not to split the changes to the test to reflect both the state before and after D83623).

Code should be tested in the commit that adds the code & commit should be separate into individual pieces of functionality (so potentially splitting even this patch in two - one for the frontend change & test for it (which I guess is the test change from D83623) and another for the driver change (which isn't currently tested in either review, I think)).

I don't think this would require much churn in the test cases - seems like it's additive? The other review would introduce -fpch-codegen/-fpch-debuginfo which would be purely driver options & get an added driver test there for them & the documentation along with them.

[llunak]

Adjusted test/PCH/codegen.cpp to use/test the change (not much point in testing it without codegen, as in that case the shared object will be empty).

[dblaikie]

clang/test/PCH/codegen.cpp

39–50

where's the change to dso_local/not-dso_local come from? Is that expected/good/correct?

[llunak]

clang/test/PCH/codegen.cpp

39–50

The driver adds "-mrelocation-model static" (by default, in the absence of -fPIC etc.), while the frontend defaults to "-mrelocation pic". I don't know about correct, but here it's harmless/unrelated.

[dblaikie]

clang/test/PCH/codegen.cpp

39–50

Ah, sorry, didn't see there was a driver test here.

The driver shouldn't be tested here - it should be tested in test/Driver and should only test the driver functionality (using -### to test that the commands the driver plans to execute have the desired arguments) - one of the reasons the driver functionality should probably be in a separate commit - it's separate/isolated functionality.

[llunak]

clang/test/PCH/codegen.cpp

39–50

I don't understand what you mean. Are you asking for a test that simply checks that 'clang++ -c a.pch' results in 'clang -cc1 a.pch' (or whatever is is)?

[dblaikie]

clang/test/PCH/codegen.cpp

39–50

Yep! that's how the driver's tested. for example: https://github.com/llvm/llvm-project/commit/9914c3a2bae84bb92e5269165c4d098140549b46

Though, honestly, if you're not planning to use this functionality (only using /Yc instead) I'd suggest maybe it's not time to implement a driver flag for this, if it's going to go unused (or without a clear user/use case/validation that this is needed/ongoing exercise of the functionality, etc). & maybe this patch (or patches, if it gets split up) isn't suitable right now. (basically the reason I haven't added -fmodules-codegen driver flags either, since it hasn't been shipped/picked up directly by anyone (though the underlying functionality's exercised by C++20 standard modules which requires codegen - and somewhat by /Yc, moreso if we can merge the functionality a bit more as proposed))

[llunak]

clang/test/PCH/codegen.cpp

39–50

I'm not going to use /Yc. I'm going to use -Xclang -building-pch-with-obj. That's why I initially went with -fbuilding-pch-with-obj, until you said that the proper approach is this patch.

And what you're saying looks like Catch-22: No driver flag until somebody uses it, but who's going to use if there's no driver flag? Although if the situation ends up being that Clang11 has the functionality but only "secret" flags to enable it, then we'll get to find out.

[dblaikie]

clang/test/PCH/codegen.cpp

39–50

Not entirely - just that we don't often add speculative features to the compiler - we add stuff that someone (usually the person contributing it) has a use for. So they add the feature and start using it, avoiding the Catch 22. (if it were a retail product, where the people developing it are distinct from the people using it - yeah, in that case there's more speculative implementation and/or user studies, etc)

But, yes, having -Xclang flags that make it clear this isn't a shipping feature (though we've done that even more agressively in some cases - see the rather ominously named "-enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang") still lets folks experiment with it (as I've done with modular codegen) & see how it plays out, what the most desirable interface to the feature would be, etc. Which is already the state - users could use -Xclang -fmodules-codegen.

Oh, I guess by secret you mean the flags you're proposing, but instead not documenting them so as not to overly publicize them?

[llunak]

By "secret" flags I mean prefixing them with -Xclang. But I guess that's how this situation will turn out. If this one and D83623 don't make it into Clang11, then people will need to use the -Xclang ones, and then these new ones don't make much sense, because who'll support one set of flags now and implement an optional new set of flags half a year later merely to have a slightly nicer spelling of the flags.

As for my use and not documenting the flags, I've been using this for the last 9 months and with the review process dragging so much I would have given up a long time ago if I cared only about myself. Saving 20% of build time sounds like something that could be easily used by many others. I'm rather sure I want to publicize this.

Anyway, so what now? Should I scratch this and D83623, or should I change D83623 to only document the flags as the -Xclang variants, or what to do?

[dblaikie]

In D83716#2154546, @llunak wrote:

By "secret" flags I mean prefixing them with -Xclang. But I guess that's how this situation will turn out. If this one and D83623 don't make it into Clang11, then people will need to use the -Xclang ones, and then these new ones don't make much sense, because who'll support one set of flags now and implement an optional new set of flags half a year later merely to have a slightly nicer spelling of the flags.

It's certainly happened before - internal flags for limited prototyping/testing/experimentation while finalizing the functionality, etc.

But sure, let's go with public driver flags for this.

Please split this patch (or at least move the driver testing out of test/PCH into test/Driver) for the driver portion separately from the frontend support.

So then it'd be 3 patches:

• frontend/cc1 support for compiling pch files directly (rather than only indirectly via -building-pch-with-obj)
• driver support for compiling .pch files the same as module files
• driver flags -fpch-codegen/-fpch-debuginfo and documentation for that use case

[llunak]

Moved driver test under test/Driver/.

[dblaikie]

clang/test/Driver/pch-codegen.cpp

17–18

FWIW, this is generally not/should not be tested.

Please only test the Driver (clang -###) in test/Driver and only test the Frontend (clang _cc1) in the other directories.

29–41

This is already covnered in the PCH/codegen.cpp test, right? (except for the driver interaction - and that's already tested above with the driver tests)

[llunak]

clang/test/Driver/pch-codegen.cpp

17–18

The test below errors out if the PCH does not exist, already in the driver code. I can change the comment to "Actually create the PCH, needed by the tests below." if that helps.

29–41

No. The tests above test creating the PCH. This part tests creating the PCH's object file. When testing PCH codegen, it makes sense to test both of these.

[dblaikie]

clang/test/Driver/pch-codegen.cpp

17–18

Ah, it doesn't read the file though, right? so could you change this to touch or otherwise create an empty file to test the driver with? (*checks* Yeah, there's a bunch of other tests in clang/test/Driver that use touch to create input files for Driver testing)

29–41

Oh, right, sorry, missed the ### there!

[llunak]

Updated driver test to just create a dummy file for PCH.

[dblaikie]

Looks good - thanks!

clang/lib/Driver/Types.cpp

bool types::isAcceptedByClang(ID Id) {
   case TY_CXX: case TY_PP_CXX:
   case TY_ObjCXX: case TY_PP_ObjCXX: case TY_PP_ObjCXX_Alias:
   case TY_CHeader: case TY_PP_CHeader:
   case TY_CLHeader:
   case TY_ObjCHeader: case TY_PP_ObjCHeader:
   case TY_CXXHeader: case TY_PP_CXXHeader:
   case TY_ObjCXXHeader: case TY_PP_ObjCXXHeader:
   case TY_CXXModule: case TY_PP_CXXModule:
-  case TY_AST: case TY_ModuleFile:
+  case TY_AST: case TY_ModuleFile: case TY_PCH:
   case TY_LLVM_IR: case TY_LLVM_BC:
     return true;
   }
 }
 
 bool types::isObjC(ID Id) {
   switch (Id) {
   default:

clang/lib/Frontend/CompilerInvocation.cpp

static InputKind ParseFrontendArgs(FrontendOptions &Opts, ArgList &Args,
   InputKind DashX(Language::Unknown);
   if (const Arg *A = Args.getLastArg(OPT_x)) {
     StringRef XValue = A->getValue();
 
     // Parse suffixes: '<lang>(-header|[-module-map][-cpp-output])'.
     // FIXME: Supporting '<lang>-header-cpp-output' would be useful.
     bool Preprocessed = XValue.consume_back("-cpp-output");
     bool ModuleMap = XValue.consume_back("-module-map");
-    IsHeaderFile =
-        !Preprocessed && !ModuleMap && XValue.consume_back("-header");
+    IsHeaderFile = !Preprocessed && !ModuleMap &&
+                   XValue != "precompiled-header" &&
+                   XValue.consume_back("-header");
 
     // Principal languages.
     DashX = llvm::StringSwitch<InputKind>(XValue)
                 .Case("c", Language::C)
                 .Case("cl", Language::OpenCL)
                 .Case("cuda", Language::CUDA)
                 .Case("hip", Language::HIP)
                 .Case("c++", Language::CXX)
if (DashX.isUnknown() && Preprocessed && !IsHeaderFile && !ModuleMap)
                   .Case("objc++", Language::ObjCXX)
                   .Default(Language::Unknown);
 
     // Some special cases cannot be combined with suffixes.
     if (DashX.isUnknown() && !Preprocessed && !ModuleMap && !IsHeaderFile)
       DashX = llvm::StringSwitch<InputKind>(XValue)
                   .Case("cpp-output", InputKind(Language::C).getPreprocessed())
                   .Case("assembler-with-cpp", Language::Asm)
-                  .Cases("ast", "pcm",
+                  .Cases("ast", "pcm", "precompiled-header",
                          InputKind(Language::Unknown, InputKind::Precompiled))
                   .Case("ir", Language::LLVM_IR)
                   .Default(Language::Unknown);
 
     if (DashX.isUnknown())
       Diags.Report(diag::err_drv_invalid_value)
         << A->getAsString(Args) << A->getValue();
 

clang/test/Driver/pch-codegen.cpp

+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+
+// Create PCH without codegen.
+// RUN: %clang -x c++-header %S/../Modules/Inputs/codegen-flags/foo.h -o %t/foo-cg.pch -### 2>&1 | FileCheck %s -check-prefix=CHECK-PCH-CREATE
+// CHECK-PCH-CREATE: -emit-pch
+// CHECK-PCH-CREATE-NOT: -fmodules-codegen
+// CHECK-PCH-CREATE-NOT: -fmodules-debuginfo
+
+// Create PCH with -fmodules-codegen.
+// RUN: %clang -x c++-header -Xclang -fmodules-codegen %S/../Modules/Inputs/codegen-flags/foo.h -o %t/foo-cg.pch -### 2>&1 | FileCheck %s -check-prefix=CHECK-PCH-CODEGEN-CREATE
+// CHECK-PCH-CODEGEN-CREATE: -emit-pch
+// CHECK-PCH-CODEGEN-CREATE: -fmodules-codegen
+// CHECK-PCH-CODEGEN-CREATE: "-x" "c++-header"
+// CHECK-PCH-CODEGEN-CREATE-NOT: -fmodules-debuginfo
+
+// Create PCH with -fmodules-debuginfo.
+// RUN: %clang -x c++-header -Xclang -fmodules-debuginfo %S/../Modules/Inputs/codegen-flags/foo.h -g -o %t/foo-di.pch -### 2>&1 | FileCheck %s -check-prefix=CHECK-PCH-DEBUGINFO-CREATE
+// CHECK-PCH-DEBUGINFO-CREATE: -emit-pch
+// CHECK-PCH-DEBUGINFO-CREATE: -fmodules-debuginfo
+// CHECK-PCH-DEBUGINFO-CREATE: "-x" "c++-header"
+// CHECK-PCH-DEBUGINFO-CREATE-NOT: -fmodules-codegen
+
+// Create PCH's object file for -fmodules-codegen.
+// RUN: touch %t/foo-cg.pch
+// RUN: %clang -c %t/foo-cg.pch -o %t/foo-cg.o -### 2>&1 | FileCheck %s -check-prefix=CHECK-PCH-CODEGEN-OBJ
+// CHECK-PCH-CODEGEN-OBJ: -emit-obj
+// CHECK-PCH-CODEGEN-OBJ: "-main-file-name" "foo-cg.pch"
+// CHECK-PCH-CODEGEN-OBJ: "-o" "{{.*}}foo-cg.o"
+// CHECK-PCH-CODEGEN-OBJ: "-x" "precompiled-header"
+
+// Create PCH's object file for -fmodules-debuginfo.
+// RUN: touch %t/foo-di.pch
+// RUN: %clang -c %t/foo-di.pch -g -o %t/foo-di.o -### 2>&1 | FileCheck %s -check-prefix=CHECK-PCH-DEBUGINFO-OBJ
+// CHECK-PCH-DEBUGINFO-OBJ: -emit-obj
+// CHECK-PCH-DEBUGINFO-OBJ: "-main-file-name" "foo-di.pch"
+// CHECK-PCH-DEBUGINFO-OBJ: "-o" "{{.*}}foo-di.o"
+// CHECK-PCH-DEBUGINFO-OBJ: "-x" "precompiled-header"

clang/test/PCH/codegen.cpp

 // 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 - -x precompiled-header %t/foo-cg.pch | FileCheck --check-prefix=CG %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - -x precompiled-header %t/foo-di.pch | 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 - -x precompiled-header %t/foo-cg.pch | FileCheck --check-prefix=CG %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - -x precompiled-header %t/foo-di.pch | 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