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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

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


https://reviews.llvm.org/D53522



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: clang/lib/Lex/ModuleMap.cpp:1198-1203
+// 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);
+}

erik.pilkington wrote:
> vsapsai wrote:
> > vsapsai wrote:
> > > 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?
> > Looks like you have 3 cases:
> > 
> > 1. Add all files in module map to dependencies, even if a file isn't 
> > `#include`d 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.
> 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.
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.


https://reviews.llvm.org/D53522



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added inline comments.



Comment at: clang/lib/Lex/ModuleMap.cpp:1198-1203
+// 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);
+}

vsapsai wrote:
> vsapsai wrote:
> > 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?
> Looks like you have 3 cases:
> 
> 1. Add all files in module map to dependencies, even if a file isn't 
> `#include`d 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.
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.


https://reviews.llvm.org/D53522



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: clang/lib/Lex/ModuleMap.cpp:1198-1203
+// 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);
+}

vsapsai wrote:
> 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?
Looks like you have 3 cases:

1. Add all files in module map to dependencies, even if a file isn't 
`#include`d 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.


https://reviews.llvm.org/D53522



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: clang/lib/Frontend/DependencyFile.cpp:100
+
+DepCollector.maybeAddDependency(HeaderPath, /*FromModule=*/false, IsSystem,
+/*IsModuleFile*/ false,

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



Comment at: clang/lib/Frontend/DependencyFile.cpp:105-109
+  void moduleMapAddUmbrellaHeader(FileManager *FileMgr, const FileEntry 
*Header,
+  bool IsSystem, bool Imported) override {
+DepCollectorMMCallbacks::moduleMapAddHeader(Header->getName(), IsSystem,
+Imported);
+  }

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.



Comment at: clang/lib/Lex/ModuleMap.cpp:1198-1203
+// 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);
+}

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?


https://reviews.llvm.org/D53522



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

Ping!


https://reviews.llvm.org/D53522



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added inline comments.



Comment at: clang/include/clang/Lex/ModuleMap.h:649-650
+  ///This can differ from \c Header's name due to symlinks.
   void addHeader(Module *Mod, Module::Header Header,
- ModuleHeaderRole Role, bool Imported = false);
+ ModuleHeaderRole Role, StringRef OrigHeaderName = "",
+ bool Imported = false);

vsapsai wrote:
> 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.
Oh, good point. I think NameAsWritten is what we're looking for here. Thanks!



Comment at: clang/lib/Frontend/DependencyFile.cpp:94
+  void moduleMapAddHeader(StringRef HeaderPath, bool IsSystem) override {
+if (!llvm::sys::path::is_absolute(HeaderPath))
+  return;

bruno wrote:
> Can you add a testecase for when `HeaderPath` isn't absolute? 
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.


https://reviews.llvm.org/D53522



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington updated this revision to Diff 171719.
erik.pilkington marked 7 inline comments as done.
erik.pilkington added a comment.

Address review comments. Thanks!


https://reviews.llvm.org/D53522

Files:
  clang/include/clang/Lex/ModuleMap.h
  clang/lib/Frontend/DependencyFile.cpp
  clang/lib/Frontend/ModuleDependencyCollector.cpp
  clang/lib/Lex/ModuleMap.cpp
  clang/test/Modules/Inputs/dfg-symlinks.modulemap
  clang/test/Modules/dependency-file-symlinks.c
  clang/test/Modules/dependency-gen-pch.m
  clang/test/Modules/dependency-gen.m
  clang/test/Modules/relative-dep-gen.cpp

Index: clang/test/Modules/relative-dep-gen.cpp
===
--- clang/test/Modules/relative-dep-gen.cpp
+++ clang/test/Modules/relative-dep-gen.cpp
@@ -29,10 +29,8 @@
 
 #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
Index: clang/test/Modules/dependency-gen.m
===
--- clang/test/Modules/dependency-gen.m
+++ clang/test/Modules/dependency-gen.m
@@ -4,19 +4,19 @@
 // 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
Index: clang/test/Modules/dependency-gen-pch.m
===
--- clang/test/Modules/dependency-gen-pch.m
+++ clang/test/Modules/dependency-gen-pch.m
@@ -5,9 +5,9 @@
 // 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"
Index: clang/test/Modules/dependency-file-symlinks.c
===
--- /dev/null
+++ clang/test/Modules/dependency-file-symlinks.c
@@ -0,0 +1,55 @@
+// 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 = 

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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: clang/test/Modules/dependency-file-symlinks.c:3
+
+// RUN: rm -fr %t
+// RUN: mkdir -p %t/cache

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


Repository:
  rC Clang

https://reviews.llvm.org/D53522



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

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




Comment at: clang/include/clang/Lex/ModuleMap.h:649-650
+  ///This can differ from \c Header's name due to symlinks.
   void addHeader(Module *Mod, Module::Header Header,
- ModuleHeaderRole Role, bool Imported = false);
+ ModuleHeaderRole Role, StringRef OrigHeaderName = "",
+ bool Imported = false);

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.



Comment at: clang/lib/Lex/ModuleMap.cpp:1200-1201
+
+SmallString<128> FullPath(Mod->Directory->getName());
+llvm::sys::path::append(FullPath, OrigHeaderName);
+

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



Comment at: clang/test/Modules/dependency-file-symlinks.c:20-21
+
+// Run clang again, to make sure that we get identical output with the cached
+// modules.
+

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


Repository:
  rC Clang

https://reviews.llvm.org/D53522



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Hi Erik, thanks for improving this. Comments below.




Comment at: clang/include/clang/Lex/ModuleMap.h:650
   void addHeader(Module *Mod, Module::Header Header,
- ModuleHeaderRole Role, bool Imported = false);
+ ModuleHeaderRole Role, StringRef OrigHeaderName = "",
+ bool Imported = false);

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



Comment at: clang/lib/Frontend/DependencyFile.cpp:94
+  void moduleMapAddHeader(StringRef HeaderPath, bool IsSystem) override {
+if (!llvm::sys::path::is_absolute(HeaderPath))
+  return;

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



Comment at: clang/lib/Lex/ModuleMap.cpp:1196
+// to the actual file. The callback should be notified of both.
+
+if (OrigHeaderName.empty())

Remove this empty line


Repository:
  rC Clang

https://reviews.llvm.org/D53522



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington created this revision.
erik.pilkington added reviewers: bruno, rsmith.
Herald added a subscriber: dexonsmith.

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


Repository:
  rC Clang

https://reviews.llvm.org/D53522

Files:
  clang/include/clang/Lex/ModuleMap.h
  clang/lib/Frontend/DependencyFile.cpp
  clang/lib/Frontend/ModuleDependencyCollector.cpp
  clang/lib/Lex/ModuleMap.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/test/Modules/Inputs/dfg-symlinks.modulemap
  clang/test/Modules/dependency-file-symlinks.c
  clang/test/Modules/dependency-gen-pch.m
  clang/test/Modules/dependency-gen.m
  clang/test/Modules/dependency-gen.modulemap

Index: clang/test/Modules/dependency-gen.modulemap
===
--- clang/test/Modules/dependency-gen.modulemap
+++ clang/test/Modules/dependency-gen.modulemap
@@ -27,6 +27,7 @@
 // For an explicit use of a module via -fmodule-file=, the other module maps
 // and included headers are not dependencies of this target (they are instead
 // dependencies of the explicitly-specified .pcm input).
+// FIXME: Shouldn't we include all transitive dependencies here?
 //
 // EXPLICIT: {{^}}explicit.pcm:
 // EXPLICIT-NOT: dependency-gen-
Index: clang/test/Modules/dependency-gen.m
===
--- clang/test/Modules/dependency-gen.m
+++ clang/test/Modules/dependency-gen.m
@@ -4,19 +4,19 @@
 // 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
Index: clang/test/Modules/dependency-gen-pch.m
===
--- clang/test/Modules/dependency-gen-pch.m
+++ clang/test/Modules/dependency-gen-pch.m
@@ -5,9 +5,9 @@
 // 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"
Index: clang/test/Modules/dependency-file-symlinks.c
===
--- /dev/null
+++ clang/test/Modules/dependency-file-symlinks.c
@@ -0,0 +1,37 @@
+// REQUIRES: shell
+
+// RUN: rm -fr %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 %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 %s
+
+#include "misc.h"
+
+int main() {
+  void *p = foo;
+}
+
+// CHECK:  dependency-file-symlinks.o:
+// CHECK-NEXT:   {{.*}}dependency-file-symlinks.c
+// CHECK-NEXT:   {{.*}}link.h
+// CHECK-NEXT:   {{.*}}misc.h
+// CHECK-NEXT:   {{.*}}module.modulemap
+// CHECK-NEXT:   {{.*}}target.h
Index: