This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ivanmurashko on Jun 8 2021, 2:49 PM.

Details

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.

Diff Detail

Event Timeline

andrewjcg created this revision.Jun 8 2021, 2:49 PM
andrewjcg updated this revision to Diff 350757.Jun 8 2021, 6:51 PM

add tests

andrewjcg published this revision for review.Jun 8 2021, 6:55 PM
andrewjcg edited the summary of this revision. (Show Details)
andrewjcg added reviewers: bruno, rsmith.
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2021, 6:55 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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 added a subscriber: sugak.Jun 8 2021, 6:56 PM
andrewjcg added inline comments.Jun 8 2021, 6:57 PM
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.

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

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

clang/lib/Lex/HeaderSearch.cpp
523

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

527

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 marked 4 inline comments as done.Jun 15 2021, 7:24 PM

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 updated this revision to Diff 352740.Jun 17 2021, 8:35 AM

fix sed for windows test

andrewjcg marked an inline comment as done.Jun 17 2021, 8:36 AM

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 added inline comments.Jun 17 2021, 12:27 PM
clang/test/Modules/implicit-module-header-maps.cpp
28

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

andrewjcg added inline comments.Jun 17 2021, 12:28 PM
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.

fix comment

andrewjcg marked an inline comment as done.Jun 17 2021, 12:32 PM
dexonsmith added inline comments.Jun 17 2021, 12:42 PM
clang/test/Modules/implicit-module-header-maps.cpp
28

Got it, thanks, the comment is clear now!

bruno added a comment.Jun 17 2021, 2:33 PM

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

clang/lib/Lex/HeaderSearch.cpp
495

Can you capitalize the first letter in the param?

andrewjcg updated this revision to Diff 352898.Jun 17 2021, 7:11 PM

capitalize param

andrewjcg marked 2 inline comments as done.Jun 17 2021, 7:12 PM

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?

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

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.

Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2022, 8:56 AM
rmaz added a subscriber: rmaz.May 16 2022, 9:16 AM

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

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!

@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 commandeered this revision.Apr 26 2023, 6:58 AM
ivanmurashko added a reviewer: andrewjcg.
ivanmurashko updated this revision to Diff 517159.EditedApr 26 2023, 7:05 AM

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

Windows constrains added

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?

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!

Adding code owners and more relevant folks

bruno added a comment.May 4 2023, 10:55 AM

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 added inline comments.May 4 2023, 7:02 PM
clang/test/Modules/Inputs/implicit-module-header-maps/a.h
1–3 ↗(On Diff #517323)

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

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 updated this revision to Diff 520062.EditedMay 6 2023, 3:02 AM

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

small fix at the lit-test

windows marked as non supported

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 added inline comments.May 10 2023, 10:26 AM
clang/test/Modules/implicit-module-header-maps.cpp
53

usesd -> used

iana added a comment.May 10 2023, 10:30 AM

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?

typo fixed + rebase

ivanmurashko marked an inline comment as done.May 12 2023, 1:29 AM

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?

rebase to latest LLVM main branch

benlangmuir accepted this revision.May 25 2023, 10:29 AM

@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

This revision is now accepted and ready to land.May 25 2023, 10:29 AM