This is an archive of the discontinued LLVM Phabricator instance.

[Modules] Move modulemaps to header search directories. NFC intended.
ClosedPublic

Authored by vsapsai on Apr 19 2023, 8:05 PM.

Details

Summary

In code we use #include "llvm/Lib/Header.h" which is located in
"llvm/include/llvm/Lib/Header.h", so we use "llvm/include/" as a header
search path. We should put modulemaps in the same directory and
shouldn't rely on clang to search in immediate subdirectories.

rdar://106677321

Diff Detail

Event Timeline

vsapsai created this revision.Apr 19 2023, 8:05 PM
Herald added a project: Restricted Project. · View Herald Transcript
vsapsai requested review of this revision.Apr 19 2023, 8:05 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
vsapsai added inline comments.Apr 19 2023, 8:07 PM
clang/lib/Lex/HeaderSearch.cpp
358–373 ↗(On Diff #515192)

Disabling the code with the macro is not intended. Want to show which code is responsible for module map search in subdirectories and which code to disable in case you want to test locally.

vsapsai updated this revision to Diff 515194.Apr 19 2023, 8:09 PM

Don't touch HeaderSearch.cpp as the actual change in header search will be a separate patch.

Ping. Want to remind everyone that it is OK to review only a subset of files. The change spans multiple sub-projects and it is understandable nobody is comfortable reviewing all of them.

aprantl accepted this revision.May 1 2023, 8:49 AM
This revision is now accepted and ready to land.May 1 2023, 8:49 AM
ributzka accepted this revision.May 2 2023, 2:24 PM
This revision was landed with ongoing or failed builds.May 3 2023, 1:08 PM
This revision was automatically updated to reflect the committed changes.

Got a link to a design discussion motivating this change?

I'd have thought it made sense to put modulemaps in subdirectories - since they cover the whole directory, putting them in the root of an include path would be problematic if there are multiple distinct projects in that same path. And I didn't think this involved searching through subdirectories, but walking up parent directories from the included file...

Got a link to a design discussion motivating this change?

No design discussion. I though that doing less work is not contentious.

I'd have thought it made sense to put modulemaps in subdirectories - since they cover the whole directory, putting them in the root of an include path would be problematic if there are multiple distinct projects in that same path. And I didn't think this involved searching through subdirectories, but walking up parent directories from the included file...

When we look for a module map covering a header - you are right, we are walking up parent directories from the included file. But when we are looking up a module by name (for example, when we load a module), there is no included file and we need to look for a module top-down instead of bottom-up.

Your suggested approach to put module maps in corresponding subdirectories works when the names are the same, for example, module "blue" in directory "blue", that remains unchanged. But, for example, module "LLVM_DebugInfo" in directory "llvm" is not obvious. And there is no indication it must be in directory "llvm" and not in "llvm-c" or in some other directory, so clang checks all subdirectories.

llvm/include/llvm/module.extern.modulemap