Page MenuHomePhabricator

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

Authored by erik.pilkington on Oct 22 2018, 12:46 PM.

Details

Reviewers
bruno
rsmith
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

Diff Detail

Event Timeline

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

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 added inline comments.Oct 23 2018, 2:48 PM
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 marked 7 inline comments as done.

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.

vsapsai added inline comments.Nov 7 2018, 6:33 PM
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 added inline comments.Nov 7 2018, 7:27 PM
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 added inline comments.Nov 9 2018, 3:29 PM
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 added inline comments.Nov 9 2018, 7:37 PM
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.

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.