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.



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.


Thanks for taking a look!

Diff Detail

Event Timeline

Hi Erik, thanks for improving this. Comments below.


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


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


Remove this empty line

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


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.


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


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

vsapsai added inline comments.Oct 23 2018, 2:48 PM

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!


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


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

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


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.


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

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

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

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 *

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.

bruno resigned from this revision.May 20 2021, 5:59 PM