Page MenuHomePhabricator

[Frontend/Modules] Show diagnostics on prebuilt module configuration mismatch too
ClosedPublic

Authored by whisperity on Oct 16 2018, 11:05 AM.

Details

Summary

The current version only emits the below error for a module (attempted to be loaded) from the prebuilt-module-path:

error: module file blabla.pcm cannot be loaded due to a configuration mismatch with the current compilation [-Wmodule-file-config-mismatch]

With this change, if the prebuilt module is used, we allow the proper diagnostic behind the configuration mismatch to be shown.

error: POSIX thread support was disabled in PCH file but is currently enabled
error: module file blabla.pcm cannot be loaded due to a configuration mismatch with the current compilation [-Wmodule-file-config-mismatch]

(A few lines later an error is emitted anyways, so there is no reason not to complain for configuration mismatches if a config mismatch is found and kills the build.)

Diff Detail

Repository
rL LLVM

Event Timeline

whisperity created this revision.Oct 16 2018, 11:05 AM

Don't know enough about the code to have an opinion on the fix - but in any case this would need a test case, if possible

@dblaikie I have created a test, but unfortunately %clang_cpp in LIT invokes clang --driver-mode=cpp which is not the same as if clang++ is called. I'm trying to construct the following command-line

clang++ -fmodules-ts -fprebuilt-module-path=%t/mods --precompile -c file.cppm -o file.pcm

However, using %clang_cc1 I can't get it to accept --precompile as a valid argument, and using %clang_cpp I get an "unused argument" warning for --precompile and the output file is just a preprocessed output (like -E), which will, of course, cause errors, but not the errors I am wanting to test about.

whisperity retitled this revision from [Frontend] Show diagnostics on prebuilt module configuration mismatch too to [Frontend/Modules] Show diagnostics on prebuilt module configuration mismatch too.

Updating the diff just in case so that I don't lose the test code.

@dblaikie I have created a test, but unfortunately %clang_cpp in LIT invokes clang --driver-mode=cpp which is not the same as if clang++ is called. I'm trying to construct the following command-line

clang++ -fmodules-ts -fprebuilt-module-path=%t/mods --precompile -c file.cppm -o file.pcm

However, using %clang_cc1 I can't get it to accept --precompile as a valid argument, and using %clang_cpp I get an "unused argument" warning for --precompile and the output file is just a preprocessed output (like -E), which will, of course, cause errors, but not the errors I am wanting to test about.

Hey, sorry for the delay - you can use "clang -# <other arguments>" (or "clang++ -# <other arguments>" to get clang to print out the underlying -cc1 command line that is used.

If you're changing the driver then we'd write a driver test (that tests that, given "clang -foo -###" it produces some "clang -cc1 -bar" command line to run the frontend.

But since you're changing the driver, it's not interesting to (re) test how --precompile is lowered from the driver to cc1. Instead we test the frontend (cc1) directly.

whisperity planned changes to this revision.Nov 6 2018, 3:22 AM

@dblaikie I have created a test, but unfortunately %clang_cpp in LIT invokes clang --driver-mode=cpp which is not the same as if clang++ is called. I'm trying to construct the following command-line

clang++ -fmodules-ts -fprebuilt-module-path=%t/mods --precompile -c file.cppm -o file.pcm

However, using %clang_cc1 I can't get it to accept --precompile as a valid argument, and using %clang_cpp I get an "unused argument" warning for --precompile and the output file is just a preprocessed output (like -E), which will, of course, cause errors, but not the errors I am wanting to test about.

Hey, sorry for the delay - you can use "clang -# <other arguments>" (or "clang++ -# <other arguments>" to get clang to print out the underlying -cc1 command line that is used.

If you're changing the driver then we'd write a driver test (that tests that, given "clang -foo -###" it produces some "clang -cc1 -bar" command line to run the frontend.

But since you're changing the driver, it's not interesting to (re) test how --precompile is lowered from the driver to cc1. Instead we test the frontend (cc1) directly.

Understood, thank you for the help, I'll try looking into this. It was just at first a strange behaviour to see that conventionally an external call command messes up and behaves different.
Until then I'll re-mark this as a WIP because the tests are half-baked anyways.

whisperity updated this revision to Diff 172908.Nov 7 2018, 2:02 AM

Test was added.

dblaikie accepted this revision.Nov 7 2018, 1:49 PM

While I'm not 100% sure about the actual fix - I'm confident enough that @rsmith can provide any further clarification in post-commit.

the test case can probably be simplified before commit - I'd suggest removing the "positive" case (the case without any diagnostics) as that's likely already covered in other tests that added this functionality to begin with, so I'd only have the case which produces the diagnostics - that should simplify/reduce the RUN lines, remove some of the #ifdefs, etc. (& potentially, you could roll the use and definition of the module into the same file as the RUN lines - so it's easier to eyeball all the relevant stuff in one place) & remove the function from the module - as this seems to produce a diagnostic even if the module is empty? Ultimately something like:

RUN: compile -DMOD_DEF to %t/module_mismatch.pcm
RUN: compile -DMOD_USE with prebuilt-module-path=%t

#ifdef MOD_DEF
export module module_mismatch
#endif
#ifdef MOD_USE
import module_mismatch
#endif

(also removing the extra "prebuilt_modules" directory name to reduce the length of command line arguments, etc)

This revision is now accepted and ready to land.Nov 7 2018, 1:49 PM

Yes, down the line I realised the function is not needed. (It emits a diagnostic because the diagnostic comes from comparing the AST file's config blocks to the current (at the time of import) compilation.)

I have simplified the tests.
If @rsmith has no objections (or sadly time to look at this), please commit on my behalf, I have no SVN access.

This revision was automatically updated to reflect the committed changes.