add -fpch-codegen/debuginfo options mapping to -fmodules-codegen/debuginfo

[llunak]

Summary

Using -fmodules-* options for PCHs from D69778 is a bit confusing, so add -fpch-* variants. Having extra options also makes it simple to do a configure check for the feature.
Also document the options in the release notes.

[dblaikie]

clang/docs/ReleaseNotes.rst

79

In general/at least the original design of -fmodules-codegen/debuginfo was that these flags only be passed to the step that creates the module (or pch). Are these flags needed on this line (where the pch is used)?

[llunak]

clang/docs/ReleaseNotes.rst

79

Strictly speaking no, but the flags with this patch make driver add -building-pch-with-obj, which is needed for the feature, and I found it clearer to repeat the flags there rather than spell out "-Xclang -building-pch-with-obj" for both invocations. Would you prefer to remove this and have both the invocations do that? Or maybe even better with -fbuilding-pch-with-obj in the driver, now that the flag no longer would be just sort of internal?

With clang-cl it should be as simple as adding the flags to the PCH creation invocation, but with the GCC-like driver it's non-trivial to do it correctly, so I'd like to keep it as simple as possible. I'm not even entirely sure this section is sufficient as documentation of how to do it, that's why I decided to simply list the commands.

[dblaikie]

clang/docs/ReleaseNotes.rst

79

This might be more of my confusion/desire for these flags to be more consolidated between pch/modules.

Why is -building-pch-with-obj needed for the consuming side? Could that be changed to work the same way modules do, using the flags in the AST module to know a pch object is being used?

[llunak]

clang/docs/ReleaseNotes.rst

79

The flag is needed to know which of the compilations using the PCH is the one generating the shared code (i.e. otherwise they'd all skip generating it). There's no behind-the-scenes step like with clang-cl /Yc (or modules, I presume?). And I actually consider this to be a feature, given how long compiling the shared object can take with -fpch-codegen, this way parallel compilation of users of the PCH is not blocked.

[dblaikie]

clang/docs/ReleaseNotes.rst

79

oh, with modules you pass the .pcm directly to the compiler as the input file - that generates the .o file, without extra flags. (so still parallelizable)

I see here you instead include the pch along with an empty .cpp.

Can you pass the pch directly to the compiler, rather than via an implicit inclusion in an empty file?

(I can't seem to reproduce the pch object file behavior, though - am I missing something?

$ cat a.h
#ifndef A_H
#define A_H
struct t1 { };
inline int f1(int i, int j) { return i + j; }
#endif
$ cat source.cpp
#include "a.h"
int main() {
  f1(1, 2);
  t1 v1;
}
$ clang++-tot -x c++-header a.h -o header.pch -Xclang -fmodules-codegen -Xclang -fmodules-debuginfo -Xclang -building-pch-with-obj -g
$ clang++-tot -c empty.cpp -o shared.o -include-pch header.pch -Xclang -fmodules-codegen -Xclang -fmodules-debuginfo -fdata-sections -ffunction-sections -Xclang -building-pch-with-obj -g
$ nm shared.o
$ clang++-tot -c source.cpp -o source.o -include-pch header.pch -g
$ nm source.o
0000000000000000 T main
0000000000000000 W _Z2f1ii

[llunak]

clang/docs/ReleaseNotes.rst

79

Can you pass the pch directly to the compiler, rather than via an implicit inclusion in an empty file?

It seems I can't:
$ test-clang++ -c -include-pch header.pch
clang-11: error: no input files
$ test-clang++ -c header.pch
g++: warning: header.pch: linker input file unused because linking not done
$ test-clang++ -x precompiled-header -c header.pch
clang-11: error: language not recognized: 'precompiled-header'
clang-11: warning: header.pch: 'linker' input unused [-Wunused-command-line-argument]

So either I get one of these work in time for 11.0, or it's the flag.

I can't seem to reproduce the pch object file behavior, though - am I missing something?

I don't know, the same steps work fine for me, shared.o contains the symbol and source.o a reference to it. I pushed 31b05692cd33b6dcc39402169b36d895e1aa87f4 only on Thursday, maybe your build does not include it?
$ test-clang --version
clang version 11.0.0 (https://github.com/llvm/llvm-project.git 09a95f51fb1fb86442418d891f67a43e2a3ca698)

[dblaikie]

clang/docs/ReleaseNotes.rst

79

I don't know, the same steps work fine for me, shared.o contains the symbol and source.o a reference to it. I pushed 31b05692cd33b6dcc39402169b36d895e1aa87f4 only on Thursday, maybe your build does not include it?

Yeah, sorry about that - might've been rebuilding the wrong compiler or something. Do see it now - thanks!

$ test-clang++ -c header.pch

This is the one that works with .pcm files. But after looking into the code for a while, I see that -building-pch-with-obj was implemented because it allows compatibility with /Yc, where the pch object isn't a separate object, but an object from some designated source file that also gets to be the pch home.

This helps me understand better the design choices of the patch that introduced the original functionality - and yours that added on top of it. (I think I can workaround/improve some of the original patch's quirks, using your work that implements the alternative strategy: use MODULAR_CODEGEN_DECLS if the module is the main file, or if the module was built with -building-pch-with-obj (should be rephrased as "if the module has MODULAR_CODEGEN_DECLS"), and that flag has been passed again.)

And now I think I'm in a better place for the design discussion:

Yes, I think if we're going to add non-MSVC flags to support this feature (the feature does already work with a combination of MSVC (/Yc) and non-MSVC (-fmodules-codegen/-fmodules-debuginfo) I assume?) I think I'd like it to lean more towards the way modules are built: intentionally supporting building the pch like a pcm is built, as a standalone thing.

--- clang/lib/Driver/Types.cpp
+++ clang/lib/Driver/Types.cpp
@@ -284,7 +284,7 @@ types::ID types::lookupTypeForExtension(llvm::StringRef Ext) {
            .Case("mii", TY_PP_ObjCXX)
            .Case("obj", TY_Object)
            .Case("ifs", TY_IFS)
-           .Case("pch", TY_PCH)
+           .Case("pch", TY_ModuleFile)
            .Case("pcm", TY_ModuleFile)
            .Case("c++m", TY_CXXModule)
            .Case("cppm", TY_CXXModule)

Gets the basic functionality working, but I'm not sure if that's the right direction in general (@rsmith - any ideas if that's valid/a good direction to go in? I guess probably not because it might cause other things that want to detect PCH not be able to? Though no tests fail, FWIW - which surprises me a bit) - might be necessary, instead, to teach other things that are special casing TY_ModuleFile or TY_PCH to treat them more similarly.

[llunak]

I see. I had no idea PCM's worked like that.
At D83716 I've posted a patch that allows the same for PCH's.
I'll update this patch to reflect that change (since "Loc.F->Kind == ModuleKind::MK_MainFile" is true with this approach, -building-pch-with-obj is not needed).

[llunak]

Updated to include change from D83716 (including testing it).

[dblaikie]

This'll still need a separate Driver test to test the changes in lib/Driver

clang/docs/ReleaseNotes.rst

71–73

Presumably this only applies if you've not linked to all the libraries used by the headers you're precompiling? Is that a supported scenario. (seems likely to be brittle - introducing calls to things declared in the headers could cause the link to fail?)

If possible I think it'd be best to omit that comment (& the use of function/data-sections and gc-sections in the example) as an orthogonal concern.

clang/lib/Driver/ToolChains/Clang.cpp

5645–5650

Perhaps these should map to -fmodules-codegen to simplify the frontend implementation?

(maybe the -fpch flags could be implemented as aliases for -fmodules*? Well, I guess -fmodules-codegen/debuginfo aren't driver options at the moment anyway - so if/when they do get promoted that can be handled then)

[llunak]

Update also the -cc1 test to reflect D83716.

[llunak]

clang/docs/ReleaseNotes.rst

71–73

You can have that problem even if you link to all the needed libraries. It's enough if something pulls in a header that has an inline function that references a non-exported symbol from another library. Even if it is not used by your code, -fpch-codegen will still emit it anyway into the shared object file. I've run into this several times, so I find it worth mentioning (not _that_ many times given how large the LO codebase is, but still, some people bound to hit it too). But I can alter the description to show the "clean" invocation and then mention that those extra flags may be needed in some cases.

clang/lib/Driver/ToolChains/Clang.cpp

5645–5650

I don't know. Keeping it the same seems cleaner to me and it's just the tiny change in CompilerInvocation.cpp.

[dblaikie]

clang/docs/ReleaseNotes.rst

71–73

OK, so you have two shared libraries A and B and a PCH for A that has an inline function that calls a non-exported function in B?

That seems pretty fragile - but if that's the reality of how code's written in the wild, yeah, makes sense to include a caveat about it - calling it out separately/after the simple case sounds good to me, thanks!

clang/lib/Driver/ToolChains/Clang.cpp

5645–5650

Yeah, I could see it going either way - but we usually try to canonicalize in in the driver so there's less variation by the time it comes to the frontend. Please change this patch to only add a pch-debuginfo/pch-codegen driver option, not a cc1 option & leave the frontend support only using the modules-* naming.

[llunak]

Only add -fpch-* as driver options that map to -fmodules-*.
Change caveats in the documentation to extra notes at the end.

[dblaikie]

Looks good - thanks!

clang/include/clang/Driver/Options.td

1443–1458

Refactor this to use OptInFFlag if possible

[llunak]

Used OptInFFlag and adapted to changes in D83716.

clang/docs/ReleaseNotes.rst

 
 - ...
 
 New Compiler Flags
 ------------------
 
 - ...
 
+- -fpch-codegen and -fpch-debuginfo generate shared code and/or debuginfo
+  for contents of a precompiled header in a separate object file. This object
+  file needs to be linked in, but its contents do not need to be generated
+  for other objects using the precompiled header. This should usually save
+  compile time. If not using clang-cl, the separate object file needs to
+  be created explicitly from the precompiled header.
+  Example of use:
+
+  .. code-block:: console
+
+    $ clang++ -x c++-header header.h -o header.pch -fpch-codegen -fpch-debuginfo
+    $ clang++ -c header.pch -o shared.o
+    $ clang++ -c source.cpp -o source.o -include-pch header.pch
+    $ clang++ -o binary source.o shared.o
+
+  - Using -fpch-instantiate-templates when generating the precompiled header
+    usually increases the amount of code/debuginfo that can be shared.
+  - In some cases, especially when building with optimizations enabled, using
+    -fpch-codegen may generate so much code in the shared object that compiling
+    it may be a net loss in build time.
+  - Since headers may bring in private symbols of other libraries, it may be
+    sometimes necessary to discard unused symbols (such as by adding
+    -Wl,--gc-sections on ELF platforms to the linking command, and possibly
+    adding -fdata-sections -ffunction-sections to the command generating
+    the shared object).
+
 Deprecated Compiler Flags
 -------------------------
 
 The following options are deprecated and ignored. They will be removed in
 future versions of Clang.
 
 - ...
 

clang/include/clang/Driver/Options.td

def fno_pch_validate_input_files_content:
   Group<f_Group>, Flags<[DriverOption]>;
 def fpch_instantiate_templates:
   Flag <["-"], "fpch-instantiate-templates">,
   Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Instantiate templates already while building a PCH">;
 def fno_pch_instantiate_templates:
   Flag <["-"], "fno-pch-instantiate-templates">,
   Group<f_Group>, Flags<[CC1Option]>;
+defm pch_codegen: OptInFFlag<"pch-codegen", "Generate ", "Do not generate ",
+  "code for uses of this PCH that assumes an explicit object file will be built for the PCH">;
+defm pch_debuginfo: OptInFFlag<"pch-debuginfo", "Generate ", "Do not generate ",
+  "debug info for types in an object file built from this PCH and do not generate them elsewhere">;
 
 def fmodules : Flag <["-"], "fmodules">, Group<f_Group>,
   Flags<[DriverOption, CC1Option]>,
   HelpText<"Enable the 'modules' language feature">;
 def fimplicit_module_maps : Flag <["-"], "fimplicit-module-maps">, Group<f_Group>,
   Flags<[DriverOption, CC1Option]>,
   HelpText<"Implicitly search the file system for module map files.">;
 def fmodules_ts : Flag <["-"], "fmodules-ts">, Group<f_Group>,
   Flags<[CC1Option]>, HelpText<"Enable support for the C++ Modules TS">;
 def fmodule_maps : Flag <["-"], "fmodule-maps">, Alias<fimplicit_module_maps>;
 def fmodule_name_EQ : Joined<["-"], "fmodule-name=">, Group<f_Group>,
   Flags<[DriverOption,CC1Option]>, MetaVarName<"<name>">,
   HelpText<"Specify the name of the module to build">;
 def fmodule_name : Separate<["-"], "fmodule-name">, Alias<fmodule_name_EQ>;
 def fmodule_implementation_of : Separate<["-"], "fmodule-implementation-of">,
   Flags<[CC1Option]>, Alias<fmodule_name_EQ>;
 def fsystem_module : Flag<["-"], "fsystem-module">, Flags<[CC1Option]>,
   HelpText<"Build this module as a system module. Only used with -emit-module">;
 def fmodule_map_file : Joined<["-"], "fmodule-map-file=">,
   Group<f_Group>, Flags<[DriverOption,CC1Option]>, MetaVarName<"<file>">,

clang/lib/Driver/ToolChains/Clang.cpp

   RenderModulesOptions(C, D, Args, Input, Output, CmdArgs, HaveModules);
 
   if (Args.hasFlag(options::OPT_fpch_validate_input_files_content,
                    options::OPT_fno_pch_validate_input_files_content, false))
     CmdArgs.push_back("-fvalidate-ast-input-files-content");
   if (Args.hasFlag(options::OPT_fpch_instantiate_templates,
                    options::OPT_fno_pch_instantiate_templates, false))
     CmdArgs.push_back("-fpch-instantiate-templates");
+  if (Args.hasFlag(options::OPT_fpch_codegen, options::OPT_fno_pch_codegen,
+                   false))
+    CmdArgs.push_back("-fmodules-codegen");
+  if (Args.hasFlag(options::OPT_fpch_debuginfo, options::OPT_fno_pch_debuginfo,
+                   false))
+    CmdArgs.push_back("-fmodules-debuginfo");
 
   Args.AddLastArg(CmdArgs, options::OPT_fexperimental_new_pass_manager,
                   options::OPT_fno_experimental_new_pass_manager);
 
   ObjCRuntime Runtime = AddObjCRuntimeArgs(Args, Inputs, CmdArgs, rewriteKind);
   RenderObjCOptions(TC, D, RawTriple, Args, Runtime, rewriteKind != RK_None,
                     Input, CmdArgs);
 

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
+// Create PCH with -fpch-codegen.
+// RUN: %clang -x c++-header -fpch-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
+// Create PCH with -fpch-debuginfo.
+// RUN: %clang -x c++-header -fpch-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.
+// Create PCH's object file for -fpch-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.
+// Create PCH's object file for -fpch-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"