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.
This way should be the same like with a.pcm for modules.
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.
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.
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.
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))
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.
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?
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.
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
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.
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)
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.
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.
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)
Oh, right, sorry, missed the ### there!