[Frontend] Include module map header declaration in dependency file output

[erik.pilkington]

Summary

This patch makes clang include headers found in modulemap files in the .d file. This is necessary to capture symlinked headers, which previously were ignored, since only the canonical version of the file makes it into the pcm. This is a corollary to the -module-dependency-dir version from r264971.

rdar://44604913

Thanks for taking a look!
Erik

[bruno]

Hi Erik, thanks for improving this. Comments below.

clang/include/clang/Lex/ModuleMap.h

653

While here, can you add a \param entry for Imported?

clang/lib/Frontend/DependencyFile.cpp

94

Can you add a testecase for when HeaderPath isn't absolute?

clang/lib/Lex/ModuleMap.cpp

1196

Remove this empty line

[vsapsai]

Have a few comments but didn't try to come up with edge cases yet.

clang/include/clang/Lex/ModuleMap.h

652–653

How OrigHeaderName is different from Module::Header.NameAsWritten? Based on the names they should be the same, curious if and when it's not the case.

clang/lib/Lex/ModuleMap.cpp

1200–1201

Would it be better to build FullPath outside of the loop?

clang/test/Modules/dependency-file-symlinks.c

21–22

That's neat. I might steal this approach for my own changes.

[vsapsai]

clang/test/Modules/dependency-file-symlinks.c

4

Oh, and can you please change the order of flags, so it is rm -rf %t? The real reason is my personal preference but -rf is actually more consistent with existing tests, it is used more than 10x more often than -fr.

[erik.pilkington]

Address review comments. Thanks!

clang/include/clang/Lex/ModuleMap.h

652–653

Oh, good point. I think NameAsWritten is what we're looking for here. Thanks!

clang/lib/Frontend/DependencyFile.cpp

94

Sure, new patch adds a test. This was meant to avoid including dependencies found in -fmodule-files, but the new patch does this more directly by adding a bool Imported parameter to this function. I sprinkled some comments around explaining.

[erik.pilkington]

Ping!

[vsapsai]

clang/lib/Frontend/DependencyFile.cpp

100

This /*FromModule=*/false looks confusing and suspicious but seems like we don't use this argument, so it doesn't really matter and worth cleaning up (out of scope for this change).

105–109

What is the purpose of this method and the one in DFGMMCallback? It looks correct-ish but after removing this callback, the tests are still passing.

clang/lib/Lex/ModuleMap.cpp

1198–1203

It is strange but after removing this part the tests are still passing. I suspect the code is correct but the test allows some roundabout way to add symlink to dependencies. In my experiments only DFGMMCallback::moduleMapAddHeader affects the tests. Is it expected?

[vsapsai]

clang/lib/Lex/ModuleMap.cpp

1198–1203

Looks like you have 3 cases:

1. Add all files in module map to dependencies, even if a file isn't #included anywhere (this turned out to be link.h).
2. For -fmodule-file replace header files in dependencies with .pcm file (that's what Imported achieves).
3. Some scenario with symlinks. Here I haven't found a representative test case.

[erik.pilkington]

clang/lib/Lex/ModuleMap.cpp

1198–1203

3 and 1 should be the same; the problem is that a FileEntry's name mutates over the course of the compile to refer to the most recent reference to it. (see FileManager::getFile() and the FIXME from 2007). This puts us in a pretty awkward position here: we're trying to recover the set of symlinks that clang used to refer to this file, but I think that that information is lost in the general case. The Right Thing To Do is probably to actually track that somewhere. I think we could also probably work around this by attaching the DependencyFileGenerator callback to the module builder thread, in which case we'd be able to ensure we use the most-recent version of a FileEntry.

I'll keep trying to get a reproducer for this, but FYI you can check it out in action for the file ncurses.h in the SDK.

[vsapsai]

clang/lib/Lex/ModuleMap.cpp

1198–1203

I think your test case reproduces ncurses.h situation properly. And now I see the problem is with symlinks. It is fixed by DFGMMCallback::moduleMapAddHeader but moduleMapAddHeader(FullPathAsWritten doesn't affect that. I suspect it can be useful when in module map you have entries both for a real file and its symlink but that's a different case.

I was playing a little bit more with umbrella headers and symlinks and think there is a case not covered. I'll have to double check that's not a mistake in my testing and will post it as an example. It is fairly convoluted but maybe it can be reduced to something more likely to happen in a real codebase.

[vsapsai]

The test case I've promised is

touch a.h
touch b.h
touch c.h
ln -s b.h d.h

echo '#include "a.h"' > umbrella.h
echo '#include "b.h"' >> umbrella.h

echo '#include "b.h"' > test_case.c

cat <<EOF > module.modulemap
module my_module {
  umbrella header "umbrella.h" export *
  header "c.h" export *
  header "d.h" export *
}
EOF

clang -cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=./cache ./test_case.c -MT test_case.o -dependency-file -

Expected b.h to be present in test_case.o dependencies but it's not.

Keep in mind this test case can be unrelated to your change.

clang/include/clang/Lex/ModuleMap.h

public:
   /// \param File The file itself.
   /// \param IsSystem Whether this is a module map from a system include path.
   virtual void moduleMapFileRead(SourceLocation FileStart,
                                  const FileEntry &File, bool IsSystem) {}
 
   /// Called when a header is added during module map parsing.
   ///
   /// \param Filename The header file itself.
-  virtual void moduleMapAddHeader(StringRef Filename) {}
+  /// \param IsSystem Whether this header is from a system include path.
+  /// \param Imported if this header was imported from an AST file.
+  virtual void moduleMapAddHeader(StringRef Filename, bool IsSystem,
+                                  bool Imported = false) {}
 
   /// Called when an umbrella header is added during module map parsing.
   ///
   /// \param FileMgr FileManager instance
   /// \param Header The umbrella header to collect.
+  /// \param IsSystem Whether this header is from a system include path.
+  /// \param Imported if this header was imported from an AST file.
   virtual void moduleMapAddUmbrellaHeader(FileManager *FileMgr,
-                                          const FileEntry *Header) {}
+                                          const FileEntry *Header,
+                                          bool IsSystem,
+                                          bool Imported = false) {}
 };
 
 class ModuleMap {
   SourceManager &SourceMgr;
   DiagnosticsEngine &Diags;
   const LangOptions &LangOpts;
   const TargetInfo *Target;
   HeaderSearch &HeaderInfo;
public:
 
   /// Sets the umbrella directory of the given module to the given
   /// directory.
   void setUmbrellaDir(Module *Mod, const DirectoryEntry *UmbrellaDir,
                       Twine NameAsWritten);
 
   /// Adds this header to the given module.
   /// \param Role The role of the header wrt the module.
+  /// \param Imported If this header was found in an AST file.
   void addHeader(Module *Mod, Module::Header Header,
                  ModuleHeaderRole Role, bool Imported = false);
 
   /// Marks this header as being excluded from the given module.
   void excludeHeader(Module *Mod, Module::Header Header);
 
   /// Parse the given module map file, and record any modules we
   /// encounter.
   ///
   /// \param File The file to be parsed.

clang/lib/Frontend/DependencyFile.cpp

struct DepCollectorMMCallbacks : public ModuleMapCallbacks {
   void moduleMapFileRead(SourceLocation Loc, const FileEntry &Entry,
                          bool IsSystem) override {
     StringRef Filename = Entry.getName();
     DepCollector.maybeAddDependency(Filename, /*FromModule*/false,
                                     /*IsSystem*/IsSystem,
                                     /*IsModuleFile*/false,
                                     /*IsMissing*/false);
   }
+
+  void moduleMapAddHeader(StringRef HeaderPath, bool IsSystem,
+                          bool Imported) override {
+    // If this file name was found embedded in an AST file, then don't track it
+    // here. This is to avoid including -fmodule-file= dependencies.
+    if (Imported)
+      return;
+
+    DepCollector.maybeAddDependency(HeaderPath, /*FromModule=*/false, IsSystem,
+                                    /*IsModuleFile*/ false,
+                                    /*IsMissing=*/false);
+  }
+
+  void moduleMapAddUmbrellaHeader(FileManager *FileMgr, const FileEntry *Header,
+                                  bool IsSystem, bool Imported) override {
+    DepCollectorMMCallbacks::moduleMapAddHeader(Header->getName(), IsSystem,
+                                                Imported);
+  }
 };
 
 struct DepCollectorASTListener : public ASTReaderListener {
   DependencyCollector &DepCollector;
   DepCollectorASTListener(DependencyCollector &L) : DepCollector(L) { }
   bool needsInputFileVisitation() override { return true; }
   bool needsSystemInputFileVisitation() override {
     return DepCollector.needSystemDependencies();
public:
 
   bool AddFilename(StringRef Filename);
   bool includeSystemHeaders() const { return IncludeSystemHeaders; }
   bool includeModuleFiles() const { return IncludeModuleFiles; }
 };
 
 class DFGMMCallback : public ModuleMapCallbacks {
   DFGImpl &Parent;
+
+  void addFile(StringRef Path, bool IsSystem) {
+    if (!IsSystem || Parent.includeSystemHeaders())
+      Parent.AddFilename(Path);
+  }
+
 public:
   DFGMMCallback(DFGImpl &Parent) : Parent(Parent) {}
+
   void moduleMapFileRead(SourceLocation Loc, const FileEntry &Entry,
                          bool IsSystem) override {
-    if (!IsSystem || Parent.includeSystemHeaders())
-      Parent.AddFilename(Entry.getName());
+    addFile(Entry.getName(), IsSystem);
+  }
+
+  void moduleMapAddHeader(StringRef HeaderPath, bool IsSystem,
+                          bool Imported) override {
+    // If this file name was found embedded in an AST file, then don't track it
+    // here. This is to avoid including -fmodule-file= dependencies.
+    if (Imported)
+      return;
+    addFile(HeaderPath, IsSystem);
+  }
+
+  void moduleMapAddUmbrellaHeader(FileManager *FileMgr, const FileEntry *Header,
+                                  bool IsSystem, bool Imported) override {
+    DFGMMCallback::moduleMapAddHeader(Header->getName(), IsSystem, Imported);
   }
 };
 
 class DFGASTReaderListener : public ASTReaderListener {
   DFGImpl &Parent;
 public:
   DFGASTReaderListener(DFGImpl &Parent)
   : Parent(Parent) { }

clang/lib/Frontend/ModuleDependencyCollector.cpp

struct ModuleDependencyPPCallbacks : public PPCallbacks {
   }
 };
 
 struct ModuleDependencyMMCallbacks : public ModuleMapCallbacks {
   ModuleDependencyCollector &Collector;
   ModuleDependencyMMCallbacks(ModuleDependencyCollector &Collector)
       : Collector(Collector) {}
 
-  void moduleMapAddHeader(StringRef HeaderPath) override {
+  void moduleMapAddHeader(StringRef HeaderPath, bool IsSystem,
+                          bool Imported) override {
     if (llvm::sys::path::is_absolute(HeaderPath))
       Collector.addFile(HeaderPath);
   }
   void moduleMapAddUmbrellaHeader(FileManager *FileMgr,
-                                  const FileEntry *Header) override {
+                                  const FileEntry *Header,
+                                  bool IsSystem,
+                                  bool Imported) override {
     StringRef HeaderFilename = Header->getName();
-    moduleMapAddHeader(HeaderFilename);
+    moduleMapAddHeader(HeaderFilename, IsSystem, Imported);
     // The FileManager can find and cache the symbolic link for a framework
     // header before its real path, this means a module can have some of its
     // headers to use other paths. Although this is usually not a problem, it's
     // inconsistent, and not collecting the original path header leads to
     // umbrella clashes while rebuilding modules in the crash reproducer. For
     // example:
     //    ApplicationServices.framework/Frameworks/ImageIO.framework/ImageIO.h
     // instead of:
     //    ImageIO.framework/ImageIO.h
     //
     // FIXME: this shouldn't be necessary once we have FileName instances
     // around instead of FileEntry ones. For now, make sure we collect all
     // that we need for the reproducer to work correctly.
     StringRef UmbreallDirFromHeader =
         llvm::sys::path::parent_path(HeaderFilename);
     StringRef UmbrellaDir = Header->getDir()->getName();
     if (!UmbrellaDir.equals(UmbreallDirFromHeader)) {
       SmallString<128> AltHeaderFilename;
       llvm::sys::path::append(AltHeaderFilename, UmbrellaDir,
                               llvm::sys::path::filename(HeaderFilename));
       if (FileMgr->getFile(AltHeaderFilename))
-        moduleMapAddHeader(AltHeaderFilename);
+        moduleMapAddHeader(AltHeaderFilename, IsSystem, Imported);
     }
   }
 };
 
 }
 
 // TODO: move this to Support/Path.h and check for HAVE_REALPATH?
 static bool real_path(StringRef SrcPath, SmallVectorImpl<char> &RealPath) {

clang/lib/Lex/ModuleMap.cpp

                                   Twine NameAsWritten) {
   Headers[UmbrellaHeader].push_back(KnownHeader(Mod, NormalHeader));
   Mod->Umbrella = UmbrellaHeader;
   Mod->UmbrellaAsWritten = NameAsWritten.str();
   UmbrellaDirs[UmbrellaHeader->getDir()] = Mod;
 
   // Notify callbacks that we just added a new header.
   for (const auto &Cb : Callbacks)
-    Cb->moduleMapAddUmbrellaHeader(&SourceMgr.getFileManager(), UmbrellaHeader);
+    Cb->moduleMapAddUmbrellaHeader(&SourceMgr.getFileManager(), UmbrellaHeader,
+                                   Mod->IsSystem);
 }
 
 void ModuleMap::setUmbrellaDir(Module *Mod, const DirectoryEntry *UmbrellaDir,
                                Twine NameAsWritten) {
   Mod->Umbrella = UmbrellaDir;
   Mod->UmbrellaAsWritten = NameAsWritten.str();
   UmbrellaDirs[UmbrellaDir] = Mod;
 }

       LangOpts.isCompilingModule() && Mod->getTopLevelModule() == SourceModule;
   if (!Imported || isCompilingModuleHeader) {
     // When we import HeaderFileInfo, the external source is expected to
     // set the isModuleHeader flag itself.
     HeaderInfo.MarkFileModuleHeader(Header.Entry, Role,
                                     isCompilingModuleHeader);
   }
 
-  // Notify callbacks that we just added a new header.
-  for (const auto &Cb : Callbacks)
-    Cb->moduleMapAddHeader(Header.Entry->getName());
+  SmallString<128> FullPathAsWritten;
+  if (Mod->Directory) {
+    FullPathAsWritten = Mod->Directory->getName();
+    llvm::sys::path::append(FullPathAsWritten, Header.NameAsWritten);
+  }
+
+  for (const auto &Cb : Callbacks) {
+    Cb->moduleMapAddHeader(Header.Entry->getName(), Mod->IsSystem, Imported);
+
+    // If the header in the module map refers to a symlink, Header.Entry
+    // refers to the actual file. The callback should be notified of both.
+    if (!FullPathAsWritten.empty() &&
+        !FullPathAsWritten.equals(Header.Entry->getName())) {
+      Cb->moduleMapAddHeader(FullPathAsWritten, Mod->IsSystem, Imported);
+    }
+  }
 }
 
 void ModuleMap::excludeHeader(Module *Mod, Module::Header Header) {
   // Add this as a known header so we won't implicitly add it to any
   // umbrella directory module.
   // FIXME: Should we only exclude it from umbrella modules within the
   // specified module?
   (void) Headers[Header.Entry];

clang/test/Modules/Inputs/dfg-symlinks.modulemap

+module my_module {
+  header "link.h" export *
+  header "misc.h" export *
+}

clang/test/Modules/dependency-file-symlinks.c

+// REQUIRES: shell
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/cache
+
+// Set up %t as follows:
+//  * misc.h #includes target.h
+//  * target.h is empty
+//  * link.h is a symlink to target.h
+
+// RUN: cp %S/Inputs/dfg-symlinks.modulemap %t/module.modulemap
+// RUN: echo "int foo();" > %t/target.h
+// RUN: ln -s %t/target.h %t/link.h
+// RUN: echo '#include "target.h"' >> %t/misc.h
+// RUN: echo 'int foo();'          >> %t/misc.h
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache %s \
+// RUN:   -I %t -MT dependency-file-symlinks.o -dependency-file - | FileCheck --check-prefix=CHECK_IMP %s
+
+// Run clang again, to make sure that we get identical output with the cached
+// modules.
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache %s \
+// RUN:   -I %t -MT dependency-file-symlinks.o -dependency-file - | FileCheck --check-prefix=CHECK_IMP %s
+
+// Verify that the an explicit use of a module via -fmodule-file doesn't include
+// any headers from that files's modulemap.
+
+// RUN: %clang_cc1 -fmodules -fmodule-name="my_module" -emit-module \
+// RUN:   -o %t/my_module.pcm %t/module.modulemap
+
+// RUN: %clang_cc1 -fmodules -fmodule-file=%t/my_module.pcm %s -MT  \
+// RUN:   dependency-file-symlinks.o -I %t -dependency-file - | \
+// RUN:   FileCheck --check-prefix CHECK_EXP %s
+
+#include "misc.h"
+
+int main() {
+  void *p = foo;
+}
+
+// CHECK_IMP:      dependency-file-symlinks.o:
+// CHECK_IMP-NEXT:   {{.*}}dependency-file-symlinks.c
+// CHECK_IMP-NEXT:   {{.*}}link.h
+// CHECK_IMP-NEXT:   {{.*}}misc.h
+// CHECK_IMP-NEXT:   {{.*}}module.modulemap
+// CHECK_IMP-NEXT:   {{.*}}target.h
+
+// CHECK_EXP:       dependency-file-symlinks.o:
+// CHECK_EXP-NEXT:    {{.*}}my_module.pcm
+// CHECK_EXP-NEXT:    {{.*}}dependency-file-symlinks.c
+
+// CHECK_EXP-NOT:     misc.h
+// CHECK_EXP-NOT:     link.h
+// CHECK_EXP-NOT:     target.h

clang/test/Modules/dependency-gen-pch.m

 // RUN: rm -rf %t-mcp
 // RUN: mkdir -p %t-mcp
 // REQUIRES: x86-registered-target
 
 // RUN: %clang_cc1 -isysroot %S/Inputs/System -triple x86_64-apple-darwin10 -module-file-deps -dependency-file %t.d -MT %s.o -I %S/Inputs -fmodules -fimplicit-module-maps -fdisable-module-hash -fmodules-cache-path=%t-mcp -emit-pch -o %t.pch %s
 // RUN: FileCheck %s < %t.d
 // CHECK: dependency-gen-pch.m.o
-// CHECK-NEXT: dependency-gen-pch.m
-// CHECK-NEXT: Inputs{{.}}module.map
-// CHECK-NEXT: diamond_top.pcm
-// CHECK-NEXT: Inputs{{.}}diamond_top.h
+// CHECK: dependency-gen-pch.m
+// CHECK: Inputs{{.}}diamond_top.h
+// CHECK: Inputs{{.}}module.map
+// CHECK: diamond_top.pcm
 
 #import "diamond_top.h"

clang/test/Modules/dependency-gen.m

 // RUN: rm -rf %t-mcp
 // RUN: mkdir -p %t-mcp
 
 // RUN: %clang_cc1 -x objective-c -isystem %S/Inputs/System/usr/include -dependency-file %t.d.1 -MT %s.o -I %S/Inputs -fsyntax-only -fmodules -fimplicit-module-maps -fmodules-cache-path=%t-mcp %s
 // RUN: FileCheck %s < %t.d.1
 // CHECK: dependency-gen.m
-// CHECK: Inputs{{.}}module.map
 // CHECK: Inputs{{.}}diamond_top.h
+// CHECK: Inputs{{.}}module.map
 // CHECK-NOT: usr{{.}}include{{.}}module.map
 // CHECK-NOT: stdint.h
 
 
 // RUN: %clang_cc1 -x objective-c -isystem %S/Inputs/System/usr/include -dependency-file %t.d.2 -MT %s.o -I %S/Inputs -sys-header-deps -fsyntax-only -fmodules -fimplicit-module-maps -fmodules-cache-path=%t-mcp %s
 // RUN: FileCheck %s -check-prefix=CHECK-SYS < %t.d.2
 // CHECK-SYS: dependency-gen.m
-// CHECK-SYS: Inputs{{.}}module.map
 // CHECK-SYS: Inputs{{.}}diamond_top.h
-// CHECK-SYS: usr{{.}}include{{.}}module.map
+// CHECK-SYS: Inputs{{.}}module.map
 // CHECK-SYS: stdint.h
+// CHECK-SYS: usr{{.}}include{{.}}module.map
 
 #import "diamond_top.h"
 #import "stdint.h" // inside sysroot

clang/test/Modules/relative-dep-gen.cpp

 // RUN: cp %s %t
 // RUN: cd %t
 // RUN: %clang_cc1 -fmodules -fno-implicit-modules -fmodule-file=%t/mod-cwd.pcm -dependency-file %t/use-explicit-no-map-cwd.d -MT use.o relative-dep-gen.cpp -fsyntax-only -fmodule-map-file-home-is-cwd
 // RUN: cat %t/use-explicit-no-map-cwd.d
 // RUN: FileCheck --check-prefix=CHECK-USE %s < %t/use-explicit-no-map-cwd.d
 
 #include "Inputs/relative-dep-gen-1.h"
 
-// CHECK-BUILD: mod.pcm:
+// CHECK-BUILD: mod.pcm: {{.*}}Inputs{{[/\\]}}relative-dep-gen-1.h {{.*}}Inputs{{[/\\]}}relative-dep-gen-2.h
 // CHECK-BUILD:   {{[ \t]}}Inputs{{[/\\]}}relative-dep-gen{{(-cwd)?}}.modulemap
-// CHECK-BUILD:   {{[ \t]}}Inputs{{[/\\]}}relative-dep-gen-1.h
-// CHECK-BUILD:   {{[ \t]}}Inputs{{[/\\]}}relative-dep-gen-2.h
 // CHECK-USE: use.o:
 // CHECK-USE-DAG:   {{[ \t]}}relative-dep-gen.cpp
 // CHECK-EXPLICIT-DAG:   mod.pcm
 // CHECK-IMPLICIT-DAG:   {{[ \t]}}Inputs{{[/\\]}}relative-dep-gen-1.h