Page MenuHomePhabricator

[modules] Fix 37878; Autoload subdirectory modulemaps with specific LangOpts
ClosedPublic

Authored by yamaguchi on Jun 20 2018, 7:31 AM.

Details

Summary

Reproducer and errors:
https://bugs.llvm.org/show_bug.cgi?id=37878

lookupModule was falling back to loadSubdirectoryModuleMaps when it couldn't
find ModuleName in (proper) search paths. This was causing iteration over all
files in the search path subdirectories for example "/usr/include/foobar" in
bugzilla case.

Users don't expect Clang to load modulemaps in subdirectories implicitly, and
also the disk access is not cheap.

if (AllowExtraModuleMapSearch) true with ObjC with @import ModuleName.

Event Timeline

yamaguchi created this revision.Jun 20 2018, 7:31 AM
yamaguchi updated this revision to Diff 152086.Jun 20 2018, 7:41 AM

Update commmit message and comment, add ObjC2

yamaguchi edited the summary of this revision. (Show Details)Jun 20 2018, 7:42 AM
aprantl added inline comments.Jun 20 2018, 9:16 AM
clang/lib/Lex/HeaderSearch.cpp
285–286

Are these flags also enabled in Objective-C++ mode?

bruno added a comment.Jun 20 2018, 9:52 AM

Hi Yuka, thanks for working on this.

clang/lib/Lex/HeaderSearch.cpp
285–286

Looks like all this logic was introduced in r177621 to allow the names of modules to differ from the name of their subdirectory in the include path.

Instead of having this to be based on the language, it's probably better if we have it based on @import name lookup, which is the scenario where we actually currently look more aggressively, did you try that path?

This is also lacking a testcase, can you create one?

yamaguchi updated this revision to Diff 152240.Jun 21 2018, 2:00 AM

Add a test case

yamaguchi added inline comments.Jun 21 2018, 2:03 AM
clang/lib/Lex/HeaderSearch.cpp
285–286

Are these flags also enabled in Objective-C++ mode?

I think so from FrontendOptions.h:190
bool isObjectiveC() const { return Lang == ObjC || Lang == ObjCXX; }

it's probably better if we have it based on @import name lookup

I don't think I understood what you're saying, could you explain a bit more?

This is also lacking a testcase, can you create one?

Sure!

bruno added inline comments.Jun 21 2018, 9:42 AM
clang/lib/Lex/HeaderSearch.cpp
285–286

I don't think I understood what you're saying, could you explain a bit more?

This extra call to loadSubdirectoryModuleMaps was introduced in r177621 to allow @import SomeName to work even if SomeName doesn't match a subdirectory name. See the original commit for more details.

This means that it was never supposed to be called when the module is built via #include or #import, which is what you are getting. That said, I believe it's better if the condition for calling loadSubdirectoryModuleMaps checks somehow if the lookup is coming from of an @import instead of checking the language (we too don't want it to be called for ObjC when #include or #import are used).

clang/test/Modules/autoload-subdirectory.cpp
1

Instead of %clang, please use %clang_cc1 and -verify here.

yamaguchi added inline comments.Jun 25 2018, 4:47 AM
clang/lib/Lex/HeaderSearch.cpp
285–286

we too don't want it to be called for ObjC when #include or #import are used

https://gist.github.com/yamaguchi1024/27caba1897eb813b297a8c4785adc11d
This is one thing I could think of, it excludes #include but #import and @import are still treated in the same way. Do you have any idea how to do this? Is Clang differenciating the module behavior based on #import or @import in ObjC?

clang/test/Modules/autoload-subdirectory.cpp
1

Sure

yamaguchi updated this revision to Diff 152664.Jun 25 2018, 4:55 AM

use %clang_cc1 instead of %clang

bruno added inline comments.Jun 25 2018, 2:45 PM
clang/lib/Lex/HeaderSearch.cpp
285–286

https://gist.github.com/yamaguchi1024/27caba1897eb813b297a8c4785adc11d

Something along these lines seems fine. I'd reverse the condition of IsInclusionDirective to be true by default (which is the most common case), and you pass false when coming from @imports. Maybe also rename it to something more meaningful for the change in question, around the lines of AllowExtraModuleMapSearch.

This is one thing I could think of, it excludes #include but #import and @import are still treated in the same way.

#import and #include should be treated the same way (extra searches shouldn't happen for both), @import is the only one different.

Do you have any idea how to do this? Is Clang differenciating the module behavior based on #import or @import in ObjC?

#import works very much like #include, the difference is that #import's have implicit #pragma once like behavior.

yamaguchi updated this revision to Diff 152875.Jun 26 2018, 5:51 AM

Add #import test and add branch AllowExtraModuleMapSearch when ModuleName was from @import

clang/lib/Lex/HeaderSearch.cpp
285–286

#import and #include should be treated the same way

I'll add a test for #import as well.

yamaguchi updated this revision to Diff 152876.Jun 26 2018, 5:53 AM

Delete a.out)

yamaguchi edited the summary of this revision. (Show Details)Jun 26 2018, 5:55 AM
bruno accepted this revision.Jun 28 2018, 11:00 AM

Thanks for working on this, LGTM

This revision is now accepted and ready to land.Jun 28 2018, 11:00 AM
This revision was automatically updated to reflect the committed changes.