Page MenuHomePhabricator

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

Authored by llunak on Jul 11 2020, 6:44 AM.

Details

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.

Diff Detail

Event Timeline

llunak created this revision.Jul 11 2020, 6:44 AM
dblaikie added inline comments.Jul 11 2020, 9:31 AM
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 added inline comments.Jul 11 2020, 10:17 AM
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 added inline comments.Jul 11 2020, 11:16 AM
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 marked an inline comment as done.Jul 11 2020, 11:32 AM
llunak added inline comments.
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 added inline comments.Jul 11 2020, 1:12 PM
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 marked an inline comment as done and an inline comment as not done.Jul 11 2020, 2:03 PM
llunak added inline comments.
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 added inline comments.Jul 12 2020, 12:06 PM
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.

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 this revision to Diff 277559.Jul 13 2020, 1:51 PM
llunak edited the summary of this revision. (Show Details)

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

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 updated this revision to Diff 277563.Jul 13 2020, 2:06 PM

Update also the -cc1 test to reflect D83716.

llunak marked an inline comment as done.Jul 13 2020, 2:20 PM
llunak added inline comments.
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 added inline comments.Jul 13 2020, 4:43 PM
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 updated this revision to Diff 277671.Jul 13 2020, 10:38 PM

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

dblaikie accepted this revision.Jul 17 2020, 1:51 PM

Looks good - thanks!

clang/include/clang/Driver/Options.td
1443–1458

Refactor this to use OptInFFlag if possible

This revision is now accepted and ready to land.Jul 17 2020, 1:51 PM
llunak updated this revision to Diff 278998.Jul 18 2020, 7:00 AM

Used OptInFFlag and adapted to changes in D83716.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 22 2020, 1:23 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript