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

[whisperity]

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.)

[dblaikie]

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

[whisperity]

@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]

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

[dblaikie]

In D53334#1273877, @whisperity wrote:

@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]

In D53334#1288057, @dblaikie wrote:

In D53334#1273877, @whisperity wrote:

@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]

Test was added.

[dblaikie]

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)

[whisperity]

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.

lib/Frontend/CompilerInstance.cpp

if (FrontendTimerGroup)
       Timer.init("loading." + ModuleFileName, "Loading " + ModuleFileName,
                  *FrontendTimerGroup);
     llvm::TimeRegion TimeLoading(FrontendTimerGroup ? &Timer : nullptr);
 
     // Try to load the module file. If we are not trying to load from the
     // module cache, we don't know how to rebuild modules.
     unsigned ARRFlags = Source == ModuleCache ?
                         ASTReader::ARR_OutOfDate | ASTReader::ARR_Missing :
+                        Source == PrebuiltModulePath ?
+                            0 :
                             ASTReader::ARR_ConfigurationMismatch;
     switch (ModuleManager->ReadAST(ModuleFileName,
                                    Source == PrebuiltModulePath
                                        ? serialization::MK_PrebuiltModule
                                        : Source == ModuleBuildPragma
                                              ? serialization::MK_ExplicitModule
                                              : serialization::MK_ImplicitModule,
                                    ImportLoc, ARRFlags)) {
     case ASTReader::Success: {

test/Modules/Inputs/module-mismatch.cppm

+#ifdef HAS_PTHREAD
+export module module_mismatch;
+#else
+export module module_no_mismatch;
+#endif
+
+export bool hasPthreads() {
+#ifdef HAS_PTHREAD
+    return true;
+#else
+    return false;
+#endif
+}

test/Modules/mismatch-diagnostics.cpp

+// RUN: rm -rf %t
+// RUN: mkdir -p %t/prebuilt_modules
+//
+// RUN: %clang_cc1 -triple %itanium_abi_triple                          \
+// RUN:     -fmodules-ts -fprebuilt-module-path=%t/prebuilt-modules     \
+// RUN:     -emit-module-interface -pthread -DHAS_PTHREAD=1             \
+// RUN:     %S/Inputs/module-mismatch.cppm                              \
+// RUN:     -o %t/prebuilt_modules/module_mismatch.pcm
+//
+// RUN: %clang_cc1 -triple %itanium_abi_triple                          \
+// RUN:     -fmodules-ts -fprebuilt-module-path=%t/prebuilt-modules     \
+// RUN:     -emit-module-interface                                      \
+// RUN:     %S/Inputs/module-mismatch.cppm                              \
+// RUN:     -o %t/prebuilt_modules/module_no_mismatch.pcm
+//
+// RUN: not %clang_cc1 -triple %itanium_abi_triple -fmodules-ts         \
+// RUN:     -fprebuilt-module-path=%t/prebuilt_modules -DMISMATCH_CHECK \
+// RUN:     %s 2>&1 | FileCheck %s 
+//
+// RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules-ts             \
+// RUN:     -fprebuilt-module-path=%t/prebuilt_modules %s
+
+#ifdef MISMATCH_CHECK
+import module_mismatch;
+// CHECK: error: POSIX thread support was enabled in PCH file but is currently disabled
+// CHECK-NEXT: module file {{.*}}/module_mismatch.pcm cannot be loaded due to a configuration mismatch with the current compilation
+#else
+import module_no_mismatch; // expect-no-diagnostics
+#endif
+