pr21217 - -fmodule-map-file should warn on non-existent file

[jtsoftware]

Summary

Add warning check for missing module map file specified with -fmodule-map-file. Hopefully I got the warning put in right. Please review.

[silvas]

Looks like this loop dates to a commit by djasper. Adding him as a reviewer.

[djasper]

This basically looks good to me, but I'd like Richard to take a look, too.

include/clang/Basic/DiagnosticLexKinds.td

617

I'd phrase it as:

"File '%0' specified via -fmodule-map-file not found"

test/Modules/pr21217.cpp

7–9

Do these serve any purpose?

[jtsoftware]

Sean, thanks for adding Daniel.

Daniel, thanks for the review. I've updated accordingly per your comments. But note that I had to have at least one include in the test, or the check is not performed. It seems like this should be okay, since any code using modules will have some include or import. Otherwise I would need to do the check earlier when options are processed.

[rsmith]

(Question for Daniel:) Why do we defer loading the explicitly-specified module map files until we perform a lookup? It seems like that will not work for Objective-C, where a module can be imported without ever looking up a file (by using @import). As John notes, this also means that there can be corner cases where we don't diagnose a missing module map file that was specified on the command line.

include/clang/Basic/DiagnosticLexKinds.td

616–618

I think this should be an error rather than a warning. I don't think there are any other cases where we ignore a missing input file that was specified on the command line. Also, diagnostic messages should start with a lowercase letter.

[jtsoftware]

Moved loading of -fmodule-map-file files to HeaderSearch initialization in Preprocessor initialization.
Changed diagnostic to an error.

[rsmith]

include/clang/Basic/DiagnosticLexKinds.td

554

Diagnostic messages should start with a lowercase letter.

lib/Lex/HeaderSearch.cpp

73–76

Please add braces and indent.

77

This seems unnecessary now you're doing it in HeaderSearch initialization.

[jtsoftware]

Sorry, I missed the hard tab, braces, and etc. However, if I remove the clearing of the ModuleMapFiles member, one of the tests, Modules/modular_maps.cpp segfaults deep in some AST code, but only on running the test on Linux, so it's kind of hard to debug, not being much of a Linux/gdb guy. Did I put this code in the right place? I was kind of uncomfortable putting it in the HeaderSearch constructor, but it is a convenient place, since HeaderSearch has convenient members. I don't see any other references to the ModuleMapFiles member, so I'm not sure what triggers the segfault only on Linux, unless it's some memory-stomping or post-free-write issue somewhere.

[rsmith]

I played with this some more, and determined that the best place to load
the module map files is from the frontend rather than from header search; I
modified your patch to do that and landed it as r223561.

include/clang/Basic/DiagnosticLexKinds.td

 def err_pp_eof_in_arc_cf_code_audited : Error<
   "'#pragma clang arc_cf_code_audited' was not ended within this file">;
 
 def warn_pp_date_time : Warning<
   "expansion of date or time macro is not reproducible">,
   ShowInSystemHeader, DefaultIgnore, InGroup<DiagGroup<"date-time">>;
 
 // Module map parsing
+def err_module_map_opt_not_found : Error<
+  "file '%0' specified via -fmodule-map-file not found">;
 def err_mmap_unknown_token : Error<"skipping stray token">;
 def err_mmap_expected_module : Error<"expected module declaration">;
 def err_mmap_expected_module_name : Error<"expected module name">;
 def err_mmap_expected_lbrace : Error<"expected '{' to start module '%0'">;
 def err_mmap_expected_rbrace : Error<"expected '}'">;
 def note_mmap_lbrace_match : Note<"to match this '{'">;
 def err_mmap_expected_rsquare : Error<"expected ']' to close attribute">;
 def note_mmap_lsquare_match : Note<"to match this ']'">;

 def err_mmap_explicit_top_level : Error<
   "'explicit' is not permitted on top-level modules">;
 def err_mmap_nested_submodule_id : Error<
   "qualified module name can only be used to define modules at the top level">;
 def err_mmap_expected_feature : Error<"expected a feature name">;
 def err_mmap_expected_attribute : Error<"expected an attribute name">;
 def warn_mmap_unknown_attribute : Warning<"unknown attribute '%0'">,
   InGroup<IgnoredAttributes>;
 
 def warn_auto_module_import : Warning<
   "treating #%select{include|import|include_next|__include_macros}0 as an "
   "import of module '%1'">, InGroup<AutoImport>, DefaultIgnore;
 def warn_uncovered_module_header : Warning<
   "umbrella header for module '%0' does not include header '%1'">, 
   InGroup<IncompleteUmbrella>;
 def err_expected_id_building_module : Error<
   "expected a module name in '__building_module' expression">;
 def error_use_of_private_header_outside_module : Error<
   "use of private header from outside its module: '%0'">;

lib/Lex/HeaderSearch.cpp

HeaderSearch::HeaderSearch(IntrusiveRefCntPtr<HeaderSearchOptions> HSOpts,
 
   ExternalLookup = nullptr;
   ExternalSource = nullptr;
   NumIncluded = 0;
   NumMultiIncludeFileOptzn = 0;
   NumFrameworkLookups = NumSubFrameworkLookups = 0;
 
   EnabledModules = LangOpts.Modules;
+
+  if (!HSOpts->ModuleMapFiles.empty()) {
+    // Preload all explicitly specified module map files. This enables modules
+    // map files lying in a directory structure separate from the header files
+    // that they describe. These cannot be loaded lazily upon encountering a
+    // header file, as there is no other known mapping from a header file to its
+    // module map file.
+    for (const auto &Filename : HSOpts->ModuleMapFiles) {
+      if (const FileEntry *File = FileMgr.getFile(Filename))
+        loadModuleMapFile(File, /*IsSystem=*/false);
+      else
+        Diags.Report(diag::err_module_map_opt_not_found) << Filename;
+    }
+    HSOpts->ModuleMapFiles.clear();
+  }
 }
 
 HeaderSearch::~HeaderSearch() {
   // Delete headermaps.
   for (unsigned i = 0, e = HeaderMaps.size(); i != e; ++i)
     delete HeaderMaps[i].second;
 }
 

 /// non-empty, indicates where the \#including file(s) are, in case a relative
 /// search is needed. Microsoft mode will pass all \#including files.
 const FileEntry *HeaderSearch::LookupFile(
     StringRef Filename, SourceLocation IncludeLoc, bool isAngled,
     const DirectoryLookup *FromDir, const DirectoryLookup *&CurDir,
     ArrayRef<std::pair<const FileEntry *, const DirectoryEntry *>> Includers,
     SmallVectorImpl<char> *SearchPath, SmallVectorImpl<char> *RelativePath,
     ModuleMap::KnownHeader *SuggestedModule, bool SkipCache) {
-  if (!HSOpts->ModuleMapFiles.empty()) {
-    // Preload all explicitly specified module map files. This enables modules
-    // map files lying in a directory structure separate from the header files
-    // that they describe. These cannot be loaded lazily upon encountering a
-    // header file, as there is no other known mapping from a header file to its
-    // module map file.
-    for (const auto &Filename : HSOpts->ModuleMapFiles)
-      if (const FileEntry *File = FileMgr.getFile(Filename))
-        loadModuleMapFile(File, /*IsSystem=*/false);
-    HSOpts->ModuleMapFiles.clear();
-  }
-
   if (SuggestedModule)
     *SuggestedModule = ModuleMap::KnownHeader();
     
   // If 'Filename' is absolute, check to see if it exists and no searching.
   if (llvm::sys::path::is_absolute(Filename)) {
     CurDir = nullptr;
 
     // If this was an #include_next "/absolute/file", fail.

test/Modules/pr21217.cpp

+// RUN: not %clang_cc1 -fmodules -fmodule-map-file=does-not-exist.modulemap -verify %s 2>&1 | \
+// RUN: FileCheck %s
+
+// CHECK: file 'does-not-exist.modulemap' specified via -fmodule-map-file not found
+
+#include "Inputs/private/common.h"