[clang][HeaderSearch] Fix implicit module when using header maps

[ivanmurashko]

Summary

Previously, if a header was found via in a header map, and not just remapped.
we wouldn't also find the module it maps to when using implicit modules (for
module maps that were explicitly loaded).

This diff just updates these code paths to also locate the owning module via
findUsableModuleForHeader.

[andrewjcg]

add tests

[andrewjcg]

We were hitting this in our build environment when mixing header maps with clang module maps, where the use of the former would prevent properly associated an included header with it's module via the module map.

[andrewjcg]

clang/test/Modules/implicit-module-header-maps.cpp

28

This include will fail if modules weren't used.

The include name itself doesn't exist and relies on the header map to remap it to the real header.

[andrewjcg]

lint

[andrewjcg]

rebase

[andrewjcg]

Hmm, I can't repro the module test failures locally.

[bruno]

Thanks for working on this, comments inline. @vsapsai @jansvoboda11 @dexonsmith any headermap related concerns on your side?

clang/lib/Lex/HeaderSearch.cpp

445

I see, looks like this matches what clang already does in the non-hmaps path by calling getFileAndSuggestModule above, nice!

454

Since this is the same as above, can this logic be put into FixupSearchPath? If so, it could be renamed to FixupSearchPathAndSuggestModule?

clang/test/Modules/implicit-module-header-maps.cpp

2

Maybe using %t would fix the bot errors?

28

Can you add a comment like that to the test?

[andrewjcg]

feedback

[jansvoboda11]

I don't have any comments regarding header maps.

clang/test/Modules/implicit-module-header-maps.cpp

18

The Windows CI is failing here, you should use a separator in sed that can't appear in %t.

[andrewjcg]

fix sed for windows test

[dexonsmith]

In D103930#2820725, @bruno wrote:

Thanks for working on this, comments inline. @vsapsai @jansvoboda11 @dexonsmith any headermap related concerns on your side?

@jansvoboda11, I think it'd be prudent for us to test this patch out internally before it's landed, since I don't really trust that the existing unit tests cover all the interactions between header maps and modules. Might you be able to coordinate something with @arphaman?

clang/test/Modules/implicit-module-header-maps.cpp

28

Do you really mean if modules aren't used, or do you mean if header maps aren't used?

(I think we want to find the same headers on disk whether or not modules are on... if this patch changes that, then I guess I'm not totally understanding why...)

[andrewjcg]

clang/test/Modules/implicit-module-header-maps.cpp

28

Ahh, meant if header maps aren't used. Will fix.

[andrewjcg]

clang/test/Modules/implicit-module-header-maps.cpp

28

Ah no wait. The include should fail if either header maps or modules isn't used (header maps required for the remapping and modules required to prevent the FOO define from propagating into the included header and triggering the #error).

Will update the comment.

[andrewjcg]

fix comment

[dexonsmith]

clang/test/Modules/implicit-module-header-maps.cpp

28

Got it, thanks, the comment is clear now!

[bruno]

@dexonsmith and @jansvoboda11 thanks for the fast reply and the extra testing.

clang/lib/Lex/HeaderSearch.cpp

421

Can you capitalize the first letter in the param?

[andrewjcg]

capitalize param

[jansvoboda11]

In D103930#2825181, @dexonsmith wrote:

In D103930#2820725, @bruno wrote:

Thanks for working on this, comments inline. @vsapsai @jansvoboda11 @dexonsmith any headermap related concerns on your side?

@jansvoboda11, I think it'd be prudent for us to test this patch out internally before it's landed, since I don't really trust that the existing unit tests cover all the interactions between header maps and modules. Might you be able to coordinate something with @arphaman?

I'm OOO until July 7. @arphaman, can you take a look?

[arphaman]

I've started testing this change. I'll let you know how it looks in a few days.

[andrewjcg]

Thanks!

[arphaman]

Looks like this patch causes a number of issues for us, I will work with @jansvoboda11 to see if there's some way to resolve them.

[ivanmurashko]

@arphaman, @andrewjcg, what's the status of the diff?
The functionality is important for me and I want to get it landed.

BTW: @andrewjcg I can commander the diff if you don't mind.

[bruno]

Looks like this patch causes a number of issues for us, I will work with @jansvoboda11 to see if there's some way to resolve them.

If you can share a reproducer I'm pretty sure @ivanmurashko can help make it work for y'all too. Thanks!

[andrewjcg]

@ivanmurashko: Sorry for the delay getting back to you here. Feel free to commandeer, as I don't have plans to get to this soon.

[ivanmurashko]

Commandeer the diff from @andrewjcg and made some changes at the code (get it compatible with latest clang source code) and at the tests (move modules artefacts to temp folder to make the test execution more stable).

[ivanmurashko]

Rebase

[ivanmurashko]

Windows constrains added

[ivanmurashko]

Friendly ping

@arphaman, @jansvoboda11, I have made the patch buildable on all platforms and have all tests passed. There was also a small fix (temp path for modules artefact) at the test that could fix its run on some platforms. Could you look at it? Does it have any issues on your side?

[dexonsmith]

In D103930#4310061, @ivanmurashko wrote:

Friendly ping

@arphaman, @jansvoboda11, I have made the patch buildable on all platforms and have all tests passed. There was also a small fix (temp path for modules artefact) at the test that could fix its run on some platforms. Could you look at it? Does it have any issues on your side?

Alternatively, maybe @vsapsai, @ributzka, or @iana...?

Given that header maps are somewhat Apple-specific and unit test coverage is a bit lacking for these sorts of interactions, it'd be nice for someone to check this with Apple-internal stuff. But if you're okay with it landing, and then you triage internal issues not covered by public tests later, please say that as well!

[bruno]

Adding code owners and more relevant folks

[bruno]

Thanks Duncan.

Given that header maps are somewhat Apple-specific

Some non-obvious background: It began Apple specific, but Meta uses them at scale as well, pretty important for us to get this right.

and unit test coverage is a bit lacking for these sorts of interactions, it'd be nice for someone to check this with Apple-internal stuff.

+1, we got some feedback from Apple folks some time ago, trying to revive this so @ivanmurashko and others can kill more internal technical debt.

[ChuanqiXu]

clang/test/Modules/Inputs/implicit-module-header-maps/a.h

2–4

It would be better to use split-file to merge the segments of the tests. You can find the example in the Modules directory.

[benlangmuir]

In D103930#4310061, @ivanmurashko wrote:

Friendly ping

@arphaman, @jansvoboda11, I have made the patch buildable on all platforms and have all tests passed. There was also a small fix (temp path for modules artefact) at the test that could fix its run on some platforms. Could you look at it? Does it have any issues on your side?

I was actually looking at this issue myself today, and @jansvoboda11 mentioned this patch. I'll kick off some testing on our side and see what it turns up.

[ivanmurashko]

split-file was used for the lit-test simplification (see @ChuanqiXu comment)

[ivanmurashko]

small fix at the lit-test

[ivanmurashko]

rebase

[ivanmurashko]

windows marked as non supported

[benlangmuir]

We have several new build failures with this change that I'm looking through. So far, a common one is an error of the form

/source/module.modulemap: error: redefinition of module
/build/Foo.framework/Modules/module.modulemap: note: previously defined here

ie. we're now finding the same module in two places where we didn't before. It's possible we could avoid this for framework modules if we ignore framework modules that are not actually in framework directories, but we have seen this with non-frameworks modules as well.

Another class of errors is where the module we find does not compile in the current context, or it doesn't provide what the includer wanted when built as a module. Basically we silently depended on it being a textual include.

Still looking at issues and not sure whether these are blockers or not.

[iana]

clang/test/Modules/implicit-module-header-maps.cpp

53

usesd -> used

[iana]

I wonder if clang should have better module interaction with the header maps produced by Xcode, or if Xcode should produce better header maps to work with clang. Or are you having problems with header maps outside of Xcode?

[ivanmurashko]

typo fixed + rebase

[ivanmurashko]

Still looking at issues and not sure whether these are blockers or not.

Friendly reminder:
@benlangmuir, do you need any assistance from my side with it?

[ivanmurashko]

rebase to latest LLVM main branch

[benlangmuir]

@benlangmuir, do you need any assistance from my side with it?

@ivanmurashko thanks for your patience. I discussed this with some colleagues and we're in favour of making this fix. LGTM

clang/lib/Lex/HeaderSearch.cpp

Optional<FileEntryRef> DirectoryLookup::LookupFile(
   SmallString<1024> Path;
   StringRef Dest = HM->lookupFilename(Filename, Path);
   if (Dest.empty())
     return None;
 
   IsInHeaderMap = true;
 
   auto FixupSearchPath = [&]() {
     if (SearchPath) {
       StringRef SearchPathRef(getName());
       SearchPath->clear();
       SearchPath->append(SearchPathRef.begin(), SearchPathRef.end());
     }
     if (RelativePath) {
       RelativePath->clear();
       RelativePath->append(Filename.begin(), Filename.end());
     }
   };
 
   // Check if the headermap maps the filename to a framework include
   // ("Foo.h" -> "Foo/Foo.h"), in which case continue header lookup using the
   // framework include.
   if (llvm::sys::path::is_relative(Dest)) {
     MappedName.append(Dest.begin(), Dest.end());
     Filename = StringRef(MappedName.begin(), MappedName.size());
     Optional<FileEntryRef> Result = HM->LookupFile(Filename, HS.getFileMgr());
     if (Result) {
       FixupSearchPath();
+      if (!HS.findUsableModuleForHeader(
+              &Result->getFileEntry(), Result->getFileEntry().getDir(),
+              RequestingModule, SuggestedModule, isSystemHeaderDirectory())) {
+        return None;
+      }
       return *Result;
     }
   } else if (auto Res = HS.getFileMgr().getOptionalFileRef(Dest)) {
     FixupSearchPath();
+    if (!HS.findUsableModuleForHeader(
+            &Res->getFileEntry(), Res->getFileEntry().getDir(),
+            RequestingModule, SuggestedModule, isSystemHeaderDirectory())) {
+      return None;
+    }
     return *Res;
   }
 
   return None;
 }
 
 /// Given a framework directory, find the top-most framework directory.
 ///

clang/test/Modules/Inputs/implicit-module-header-maps/a.h

+#ifdef FOO
+#error foo
+#endif

clang/test/Modules/Inputs/implicit-module-header-maps/a.hmap.json

+{
+  "mappings" :
+    {
+     "Before/Mapping.h" : "After/Mapping.h",
+     "After/Mapping.h" : "After/Mapping.h"
+    }
+}

clang/test/Modules/Inputs/implicit-module-header-maps/a.module.modulemap

+module a {
+  header "After/Mapping.h"
+}

clang/test/Modules/Inputs/implicit-module-header-maps/b.hmap.json

+{
+  "mappings" :
+    {
+     "Before/Mapping.h" : "OUTPUTS_DIR/After/Mapping.h"
+    }
+}

clang/test/Modules/implicit-module-header-maps.cpp

+// RUN: rm -rf %T
+// RUN: mkdir %T
+// RUN: cd %T
+//
+// RUN: %hmaptool write %S/Inputs/implicit-module-header-maps/a.hmap.json %T/hmap
+//
+// RUN: mkdir -p %T/After
+// RUN: cp %S/Inputs/implicit-module-header-maps/a.h %T/After/Mapping.h
+// RUN: cp %S/Inputs/implicit-module-header-maps/a.module.modulemap %T/module.modulemap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules -fimplicit-module-maps -fmodule-map-file=%T/module.modulemap -fsyntax-only %s -I %T/hmap
+//
+// RUN: rm -rf %T
+// RUN: mkdir %T
+// RUN: cd %T
+//
+// RUN: sed -e "s:OUTPUTS_DIR:%T:g" %S/Inputs/implicit-module-header-maps/b.hmap.json > %T/hmap.json
+// RUN: %hmaptool write %T/hmap.json %T/hmap
+//
+// RUN: mkdir -p %T/After
+// RUN: cp %S/Inputs/implicit-module-header-maps/a.h %T/After/Mapping.h
+// RUN: cp %S/Inputs/implicit-module-header-maps/a.module.modulemap %T/module.modulemap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules -fimplicit-module-maps -fmodule-map-file=%T/module.modulemap -fsyntax-only %s -I %T/hmap
+
+#define FOO
+#include "Before/Mapping.h"