Page MenuHomePhabricator

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

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

Details

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

Unit TestsFailed

TimeTest
2,110 msx64 debian > Clang.Modules::Werror.m
Script: -- : 'RUN: at line 1'; rm -rf /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Modules/Output/Werror.m.tmp
1,210 msx64 debian > Clang.Modules::explicit-build-flags.cpp
Script: -- : 'RUN: at line 1'; rm -rf /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Modules/Output/explicit-build-flags.cpp.tmp
1,200 msx64 debian > Clang.Modules::explicit-build-missing-files.cpp
Script: -- : 'RUN: at line 1'; rm -rf /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Modules/Output/explicit-build-missing-files.cpp.tmp
960 msx64 debian > Clang.Modules::fmodules-validate-once-per-build-session.c
Script: -- : 'RUN: at line 4'; rm -rf /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Modules/Output/fmodules-validate-once-per-build-session.c.tmp
440 msx64 debian > Clang.Modules::ignored_macros.m
Script: -- : 'RUN: at line 2'; rm -rf /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Modules/Output/ignored_macros.m.tmp.modules
View Full Test Results (9 Failed)

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.
clang/test/Modules/implicit-module-header-maps.cpp
27

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
445

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

455

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?

27

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
421

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.