[Modules] Don't parse/load explicit module maps if modules are disabled

[andrewjcg]

Summary

In some build environments/systems, flags for explicit module map files
may be propagated up to dependents which may not choose to enable use of
modules (e.g. if a particular compilation doesn't currently work in
modular builds).

In this case, the explicit module files are still passed, even though
they're not used (and can trigger additional I/O at header search path,
like realpath'ing include roots if an explicit module map contains a
umbrella dir).

This diff avoids parsing/loading these module map files if modules
haven't been enabled.

[bruno]

Hi Andrew, thanks for improving this. I think this makes sense: dependents can choose to not use modules without having to trigger the build system to rebuild all dependencies. Can you add a simple testcase to prove the point of the change?

clang/lib/Frontend/FrontendAction.cpp

814

Is this clang-formatted?

[andrewjcg]

Can you add a simple testcase to prove the point of the change?

Yup, will do!

clang/lib/Frontend/FrontendAction.cpp

814

Ahh, yeah. Should I avoid this sort of thing (and e.g. instead do this in explicit codemod/refactoring diffs)?

[tschuett]

How about a malformed module map is not loaded and gives no errors?

[andrewjcg]

How about a malformed module map is not loaded and gives no errors?

Heh yeah, was thinking the same :)

[bruno]

clang/lib/Frontend/FrontendAction.cpp

814

It's fine in this case cause it's relevant as part of your changes (indentation for the block will change anyways), just double checking on the final formatting.

[andrewjcg]

add test

[bruno]

clang/test/Modules/explicit-module-maps.cpp

5

Minor suggestion here given the content of the file is pretty simple, how about something along these lines:

// RUN: rm -rf %t
// RUN: mkdir %t
// RUN: echo 'DOES NOT PARSE!' > %t/invalid.modulemap
// RUN: %clang_cc1 -fmodule-map-file=%t/invalid.modulemap -verify %s

[andrewjcg]

simplify test

[rsmith]

Sorry for not noticing this review earlier.

We can't make this change. Module map files are still useful and still used even when modules are disabled -- they power the undeclared inclusion and private header diagnostics, and with -fmodules-local-submodule-visibility you still get modular semantics for modular headers even when -fmodules is disabled.

[andrewjcg]

Ahh, I see, make sense.

The motivating issue was due to an apparent bug where realpaths in umbrella dir support for module map files get leaked into dep files for includes starting with .. (e.g. #include "../foo.h") in non-modular builds.

I'll try get a repo together for a bug report for that issue instead.

clang/lib/Frontend/FrontendAction.cpp

if (Input.getKind().getFormat() == InputKind::ModuleMap) {
     }
   }
 
   // Initialize the action.
   if (!BeginSourceFileAction(CI))
     goto failure;
 
   // If we were asked to load any module map files, do so now.
+  if (CI.getLangOpts().Modules) {
     for (const auto &Filename : CI.getFrontendOpts().ModuleMapFiles) {
       if (auto File = CI.getFileManager().getFile(Filename))
         CI.getPreprocessor().getHeaderSearchInfo().loadModuleMapFile(
-          *File, /*IsSystem*/false);
+            *File, /*IsSystem*/ false);
       else
         CI.getDiagnostics().Report(diag::err_module_map_not_found) << Filename;
     }
+  }
 
   // Add a module declaration scope so that modules from -fmodule-map-file
   // arguments may shadow modules found implicitly in search paths.
   CI.getPreprocessor()
       .getHeaderSearchInfo()
       .getModuleMap()
       .finishModuleDeclarationScope();
 

clang/test/Modules/explicit-module-maps.cpp

+// Test that an explicit module map (which fails to parse/load in this case)
+// isn't used if modules aren't enabled.
+
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: echo 'DOES NOT PARSE!' > %t/invalid.modulemap
+// RUN: %clang_cc1 -fmodule-map-file=%t/invalid.modulemap -verify %s
+// expected-no-diagnostics