Fix "modular" import when we find the header in the current directory

[gribozavr]

Summary

When searching for a header, Clang will first try to look in the current directory. But if we find the header in this case, we don't set 'SuggestedModule', so the header is included in non-modular way, even if module.map has an entry for it. Fixing this issue fixes module tests for -fmodules-decluse, and test/Modules/submodules.cpp.

test/Modules/submodules.cpp already had a FIXME that this patch fixes.

-fmodules-decluse tests had dormant errors that this patch uncovers and fixes. Specifically, XC and XD #included "b.h", but did not specify the use dependency in module.map.

Please review.

[gribozavr]

@chapuni: This patch will probably also have the same effect as r196859, which was reverted. I don't have a Windows machine and I am not sure how to fix the failing test (Misc/remap-file.c) -- http://bb.pgr.jp/builders/cmake-clang-i686-mingw32/builds/6254/steps/test-clang/logs/Clang%20%3A%3A%20Misc__remap-file.c

[rsmith]

Makes sense to me, thanks. Please go ahead once the Windows issue is
resolved.

[gribozavr]

@chapuni: As a different option, are you OK with XFAIL'ing this test on Windows for now? This test is for file remapping functionality, which is used in libclang. But in libclang it is used with memory buffers, not with real files, so I don't think the actual use case will break on Windows.

[djasper]

These aren't dormant errors in the module.map of the declare-use tests, it is intended design. We only want to display these warnings if the corresponding modules are actively built. This is quite important. As a user, you might otherwise get a lot of warnings that you can't fix (as they are in somebody else's module). However, you still want to be enable this check for your own module. We could also check indirect modules guarded on an additional -fmodules-indirect-check flag, but I personally think that this is unnecessary complexity.

Interestingly, I can't from the patch tell why it would now find these extra errors. verifyModuleInclude still tests that the RequestingModule is the current SourceModule. I'll take a close look tomorrow.

[djasper]

Ok. The "problem" is that the -fmodules-decluse creates an error on the transitive build steps when actually building XC and XD, which the modules under test depend on.

I have temporarily circumvented this in r197021 by letting the declare-use tests only checks modules without actively building them (and their transitive dependencies). So you should now be able to commit this without changing the test/Modules/Inputs/declare-use/module.map.

I think that we'll probably want to disable layering checks for the build steps of the transitively used modules. Any thoughts on this?

[djasper]

What's the state of this?

include/clang/Lex/HeaderSearch.h

struct HeaderFileInfo {
   ModuleMap::ModuleHeaderRole getHeaderRole() const {
     return static_cast<ModuleMap::ModuleHeaderRole>(HeaderRole);
   }
 
   /// \brief Set the HeaderRole properly typed.
   void setHeaderRole(ModuleMap::ModuleHeaderRole Role) {
     HeaderRole = Role;
   }
+
+  SrcMgr::CharacteristicKind getDirInfo() const {
+    return static_cast<SrcMgr::CharacteristicKind>(DirInfo);
+  }
 };
 
 /// \brief An external source of header file information, which may supply
 /// information about header files already included.
 class ExternalHeaderFileInfoSource {
 public:
   virtual ~ExternalHeaderFileInfoSource();
   

include/clang/Lex/Preprocessor.h

public:
   /// The caller is expected to provide a buffer that is large enough to hold
   /// the spelling of the filename, but is also expected to handle the case
   /// when this method decides to use a different buffer.
   ///
   /// \returns true if the input filename was in <>'s or false if it was
   /// in ""'s.
   bool GetIncludeFilenameSpelling(SourceLocation Loc,StringRef &Filename);
 
+private:
+  const FileEntry *lookupFileImpl(SourceLocation FilenameLoc,
+                                  StringRef Filename,
+                                  bool IsAngled,
+                                  const DirectoryLookup *FromDir,
+                                  const DirectoryLookup *&CurDir,
+                                  SmallVectorImpl<char> *SearchPath,
+                                  SmallVectorImpl<char> *RelativePath,
+                                  ModuleMap::KnownHeader *SuggestedModule,
+                                  bool SkipCache);
+
+public:
   /// \brief Given a "foo" or \<foo> reference, look up the indicated file.
   ///
   /// Returns null on failure.  \p isAngled indicates whether the file
   /// reference is for system \#include's or not (i.e. using <> instead of "").
   const FileEntry *LookupFile(SourceLocation FilenameLoc, StringRef Filename,
-                              bool isAngled, const DirectoryLookup *FromDir,
+                              bool IsAngled, const DirectoryLookup *FromDir,
                               const DirectoryLookup *&CurDir,
                               SmallVectorImpl<char> *SearchPath,
                               SmallVectorImpl<char> *RelativePath,
                               ModuleMap::KnownHeader *SuggestedModule,
                               bool SkipCache = false);
 
+  /// \brief Lookup a file using normal rules, like \#include, but don't count
+  /// this as a module use.  This should be used for diagnostics purposes when
+  /// the file is not actually included in the translation unit.
+  const FileEntry *lookupFileForDiagnostics(SourceLocation FilenameLoc,
+                                            StringRef Filename,
+                                            bool IsAngled);
+
   /// \brief Get the DirectoryLookup structure used to find the current
   /// FileEntry, if CurLexer is non-null and if applicable. 
   ///
   /// This allows us to implement \#include_next and find directory-specific
   /// properties.
   const DirectoryLookup *GetCurDirLookup() { return CurDirLookup; }
 
   /// \brief Return true if we're in the top-level file, not in a \#include.

lib/Frontend/VerifyDiagnosticConsumer.cpp

if (!PH.Next("@")) {
         if (Line > 0)
           ExpectedLoc = SM.translateLineCol(SM.getFileID(Pos), Line, 1);
       } else if (PP && PH.Search(":")) {
         // Specific source file.
         StringRef Filename(PH.C, PH.P-PH.C);
         PH.Advance();
 
         // Lookup file via Preprocessor, like a #include.
-        const DirectoryLookup *CurDir;
-        const FileEntry *FE = PP->LookupFile(Pos, Filename, false, NULL, CurDir,
-                                             NULL, NULL, 0);
+        const FileEntry *FE =
+            PP->lookupFileForDiagnostics(Pos, Filename, /*IsAngled=*/false);
         if (!FE) {
           Diags.Report(Pos.getLocWithOffset(PH.C-PH.Begin),
                        diag::err_verify_missing_file) << Filename << KindStr;
           continue;
         }
 
         if (SM.translateFile(FE).isInvalid())
           SM.createFileID(FE, Pos, SrcMgr::C_User);

lib/Lex/HeaderSearch.cpp

const FileEntry *HeaderSearch::LookupFile(
   }
 
   // Unless disabled, check to see if the file is in the #includer's
   // directory.  This has to be based on CurFileEnt, not CurDir, because
   // CurFileEnt could be a #include of a subdirectory (#include "foo/bar.h") and
   // a subsequent include of "baz.h" should resolve to "whatever/foo/baz.h".
   // This search is not done for <> headers.
   if (CurFileEnt && !isAngled && !NoCurDirSearch) {
-    SmallString<1024> TmpDir;
-    // Concatenate the requested file onto the directory.
-    // FIXME: Portability.  Filename concatenation should be in sys::Path.
-    TmpDir += CurFileEnt->getDir()->getName();
-    TmpDir.push_back('/');
-    TmpDir.append(Filename.begin(), Filename.end());
-    if (const FileEntry *FE = FileMgr.getFile(TmpDir.str(),/*openFile=*/true)) {
+    HeaderFileInfo &IncludingHFI = getFileInfo(CurFileEnt);
+    bool InUserSpecifiedSystemFramework = false;
+    DirectoryLookup DL(CurFileEnt->getDir(), IncludingHFI.getDirInfo(),
+                       !IncludingHFI.Framework.empty());
+    if (const FileEntry *FE =
+            DL.LookupFile(Filename, *this, SearchPath, RelativePath,
+                          SuggestedModule, InUserSpecifiedSystemFramework)) {
       // Leave CurDir unset.
       // This file is a system header or C++ unfriendly if the old file is.
       //
       // Note that we only use one of FromHFI/ToHFI at once, due to potential
       // reallocation of the underlying vector potentially making the first
       // reference binding dangling.
       HeaderFileInfo &FromHFI = getFileInfo(CurFileEnt);
-      unsigned DirInfo = FromHFI.DirInfo;
+      SrcMgr::CharacteristicKind DirInfo = FromHFI.getDirInfo();
       bool IndexHeaderMapHeader = FromHFI.IndexHeaderMapHeader;
       StringRef Framework = FromHFI.Framework;
 
       HeaderFileInfo &ToHFI = getFileInfo(FE);
       ToHFI.DirInfo = DirInfo;
       ToHFI.IndexHeaderMapHeader = IndexHeaderMapHeader;
       ToHFI.Framework = Framework;
 
-      if (SearchPath != NULL) {
-        StringRef SearchPathRef(CurFileEnt->getDir()->getName());
-        SearchPath->clear();
-        SearchPath->append(SearchPathRef.begin(), SearchPathRef.end());
-      }
-      if (RelativePath != NULL) {
-        RelativePath->clear();
-        RelativePath->append(Filename.begin(), Filename.end());
-      }
+      // If the directory characteristic is User but this framework was
+      // user-specified to be treated as a system framework, promote the
+      // characteristic.
+      if (ToHFI.DirInfo == SrcMgr::C_User && InUserSpecifiedSystemFramework)
+        ToHFI.DirInfo = SrcMgr::C_System;
+
       return FE;
     }
   }
 
   CurDir = 0;
 
   // If this is a system #include, ignore the user #include locs.
   unsigned i = isAngled ? AngledDirIdx : 0;

lib/Lex/PPDirectives.cpp

void Preprocessor::verifyModuleInclude(SourceLocation FilenameLoc,
   // FIXME: Add support for FixIts in module map files and offer adding the
   // required use declaration.
   if (RequestingModule && getLangOpts().ModulesDeclUse &&
       violatesUseDeclarations(RequestingModule, RequestedModule.getModule()))
     Diag(FilenameLoc, diag::error_undeclared_use_of_module)
         << RequestingModule->getFullModuleName() << Filename;
 }
 
-const FileEntry *Preprocessor::LookupFile(
+const FileEntry *Preprocessor::lookupFileImpl(
     SourceLocation FilenameLoc,
     StringRef Filename,
-    bool isAngled,
+    bool IsAngled,
     const DirectoryLookup *FromDir,
     const DirectoryLookup *&CurDir,
     SmallVectorImpl<char> *SearchPath,
     SmallVectorImpl<char> *RelativePath,
     ModuleMap::KnownHeader *SuggestedModule,
     bool SkipCache) {
   // If the header lookup mechanism may be relative to the current file, pass in
   // info about where the current file is.
if (CurFileEnt == 0) {
       FID = SourceMgr.getMainFileID();
       CurFileEnt = SourceMgr.getFileEntryForID(FID);
     }
   }
 
   // Do a standard file entry lookup.
   CurDir = CurDirLookup;
   const FileEntry *FE = HeaderInfo.LookupFile(
-      Filename, isAngled, FromDir, CurDir, CurFileEnt,
+      Filename, IsAngled, FromDir, CurDir, CurFileEnt,
       SearchPath, RelativePath, SuggestedModule, SkipCache);
-  if (FE) {
-    if (SuggestedModule)
-      verifyModuleInclude(FilenameLoc, Filename, FE);
+  if (FE)
     return FE;
-  }
 
   // Otherwise, see if this is a subframework header.  If so, this is relative
   // to one of the headers on the #include stack.  Walk the list of the current
   // headers on the #include stack and pass them to HeaderInfo.
   if (IsFileLexer()) {
     if ((CurFileEnt = SourceMgr.getFileEntryForID(CurPPLexer->getFileID())))
       if ((FE = HeaderInfo.LookupSubframeworkHeader(Filename, CurFileEnt,
                                                     SearchPath, RelativePath,
if (IsFileLexer(ISEntry)) {
           return FE;
     }
   }
 
   // Otherwise, we really couldn't find the file.
   return 0;
 }
 
+const FileEntry *Preprocessor::LookupFile(
+    SourceLocation FilenameLoc,
+    StringRef Filename,
+    bool IsAngled,
+    const DirectoryLookup *FromDir,
+    const DirectoryLookup *&CurDir,
+    SmallVectorImpl<char> *SearchPath,
+    SmallVectorImpl<char> *RelativePath,
+    ModuleMap::KnownHeader *SuggestedModule,
+    bool SkipCache) {
+  const FileEntry *FE =
+      lookupFileImpl(FilenameLoc, Filename, IsAngled, FromDir, CurDir,
+                     SearchPath, RelativePath, SuggestedModule, SkipCache);
+  if (FE && SuggestedModule)
+    verifyModuleInclude(FilenameLoc, Filename, FE);
+  return FE;
+}
+
+const FileEntry *Preprocessor::lookupFileForDiagnostics(
+      SourceLocation FilenameLoc, StringRef Filename, bool IsAngled) {
+  const DirectoryLookup *CurDir;
+  return lookupFileImpl(FilenameLoc, Filename, IsAngled, NULL, CurDir,
+                        NULL, NULL, NULL, false);
+}
+
 
 //===----------------------------------------------------------------------===//
 // Preprocessor Directive Handling.
 //===----------------------------------------------------------------------===//
 
 class Preprocessor::ResetMacroExpansionHelper {
 public:
   ResetMacroExpansionHelper(Preprocessor *pp)

test/Modules/Inputs/declare-use/h.h

 #ifndef H_H
 #define H_H
 #include "c.h"
-#include "d.h" // expected-error {{does not depend on a module exporting}}
+#include "d.h" // expected-error {{module XH does not depend on a module exporting 'd.h'}}
 #include "h1.h"
 const int h1 = aux_h*c*7*d;
 #endif

test/Modules/Inputs/declare-use/module.map

 module XA {
   header "a.h"
 }
 
 module XB {
   header "b.h"
 }
 
 module XC {
   header "c.h"
   use XA
+  use XB
 }
 
 module XD {
   header "d.h"
   use XA
+  use XB
 }
 
 module XE {
   header "e.h"
   use XA
   use XB
 }
 

test/Modules/declare-use2.cpp

 // RUN: rm -rf %t
 // RUN: %clang_cc1 -x objective-c++ -fmodules-cache-path=%t -fmodules -fmodules-decluse -fmodule-name=XH -I %S/Inputs/declare-use %s -verify
 
 #include "h.h"
 #include "e.h"
-#include "f.h" // expected-error {{does not depend on a module exporting}}
+#include "f.h" // expected-error {{module XH does not depend on a module exporting 'f.h'}}
 const int h2 = h1+e+f;

test/Modules/submodules.cpp

 @import std.hash_map;
 
 hash_map<int, float> ints_to_floats2;
 
 @import import_self.b;
 extern MyTypeA import_self_test_a; // expected-error {{must be imported from module 'import_self.a'}}
 // expected-note@import-self-a.h:1 {{here}}
 extern MyTypeC import_self_test_c;
-// FIXME: This should be valid; import_self.b re-exports import_self.d.
-extern MyTypeD import_self_test_d; // expected-error {{must be imported from module 'import_self.d'}}
-// expected-note@import-self-d.h:1 {{here}}
+
+// import_self.b re-exports import_self.d.
+extern MyTypeD import_self_test_d;