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