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.
Details
Diff Detail
- Repository
- rC Clang
Event Timeline
clang/docs/ReleaseNotes.rst | ||
---|---|---|
131 | 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)? |
clang/docs/ReleaseNotes.rst | ||
---|---|---|
131 | 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. |
clang/docs/ReleaseNotes.rst | ||
---|---|---|
131 | 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? |
clang/docs/ReleaseNotes.rst | ||
---|---|---|
131 | 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. |
clang/docs/ReleaseNotes.rst | ||
---|---|---|
131 | 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 |
clang/docs/ReleaseNotes.rst | ||
---|---|---|
131 |
It seems I can't: So either I get one of these work in time for 11.0, or it's the flag.
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? |
clang/docs/ReleaseNotes.rst | ||
---|---|---|
131 |
Yeah, sorry about that - might've been rebuilding the wrong compiler or something. Do see it now - thanks!
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).
This'll still need a separate Driver test to test the changes in lib/Driver
clang/docs/ReleaseNotes.rst | ||
---|---|---|
123–125 | 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 | ||
5630–5635 | 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) |
clang/docs/ReleaseNotes.rst | ||
---|---|---|
123–125 | 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 | ||
5630–5635 | I don't know. Keeping it the same seems cleaner to me and it's just the tiny change in CompilerInvocation.cpp. |
clang/docs/ReleaseNotes.rst | ||
---|---|---|
123–125 | 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 | ||
5630–5635 | 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. |
Only add -fpch-* as driver options that map to -fmodules-*.
Change caveats in the documentation to extra notes at the end.
Looks good - thanks!
clang/include/clang/Driver/Options.td | ||
---|---|---|
1443–1458 | Refactor this to use OptInFFlag if possible |
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.