[PATCH] D148776: [Modules] Move modulemaps to header search directories. NFC intended.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

In D148776#4328425 , @dblaikie wrote:

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148776/new/

https://reviews.llvm.org/D148776

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148776: [Modules] Move modulemaps to header search directories. NFC intended.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148776/new/

https://reviews.llvm.org/D148776

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148776: [Modules] Move modulemaps to header search directories. NFC intended.

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG24f36a215b4e: [Modules] Move modulemaps to header search 
directories. NFC intended. (authored by vsapsai).

Changed prior to commit:
  https://reviews.llvm.org/D148776?vs=515194=519224#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148776/new/

https://reviews.llvm.org/D148776

Files:
  clang/include/clang-c/module.modulemap
  clang/include/clang/module.modulemap
  clang/include/module.modulemap
  lldb/include/lldb/module.modulemap
  lldb/include/module.modulemap
  llvm/CMakeLists.txt
  llvm/include/CMakeLists.txt
  llvm/include/llvm-c/module.modulemap
  llvm/include/llvm/CMakeLists.txt
  llvm/include/llvm/module.extern.modulemap
  llvm/include/llvm/module.install.modulemap
  llvm/include/llvm/module.modulemap
  llvm/include/llvm/module.modulemap.build
  llvm/include/module.extern.modulemap
  llvm/include/module.install.modulemap
  llvm/include/module.modulemap
  llvm/include/module.modulemap.build

Index: llvm/include/module.modulemap.build
===
--- llvm/include/module.modulemap.build
+++ llvm/include/module.modulemap.build
@@ -1,13 +1,13 @@
 // This is copied into the build area for a $src != $build compilation.
 module LLVM_Support_DataTypes {
-  header "Support/DataTypes.h"
+  header "llvm/Support/DataTypes.h"
   export *
 }
 module LLVM_Config_ABI_Breaking {
-  header "Config/abi-breaking.h"
+  header "llvm/Config/abi-breaking.h"
   export *
 }
 module LLVM_Config_Config {
-  header "Config/llvm-config.h"
+  header "llvm/Config/llvm-config.h"
   export *
 }
Index: llvm/include/module.modulemap
===
--- /dev/null
+++ llvm/include/module.modulemap
@@ -0,0 +1,428 @@
+module LLVM_C {
+  umbrella "llvm-c"
+  module * { export * }
+}
+
+module LLVM_Analysis {
+  requires cplusplus
+  umbrella "llvm/Analysis"
+  module * { export * }
+
+  // This is intended for (repeated) textual inclusion.
+  textual header "llvm/Analysis/ScalarFuncs.def"
+  textual header "llvm/Analysis/TargetLibraryInfo.def"
+  textual header "llvm/Analysis/VecFuncs.def"
+}
+
+module LLVM_AsmParser {
+  requires cplusplus
+  umbrella "llvm/AsmParser"
+  module * { export * }
+}
+
+module LLVM_CodeGenTypes {
+  requires cplusplus
+
+  module LLT {
+header "llvm/CodeGen/LowLevelType.h" export *
+  }
+  module MVT {
+header "llvm/CodeGen/MachineValueType.h" export *
+extern module LLVM_Extern_CodeGenTypes_Gen "module.extern.modulemap"
+  }
+}
+
+// A module covering CodeGen/ and Target/. These are intertwined
+// and codependent, and thus notionally form a single module.
+module LLVM_Backend {
+  requires cplusplus
+
+  module CodeGen {
+umbrella "llvm/CodeGen"
+module * { export * }
+
+// Exclude these; they're intended to be included into only a single
+// translation unit (or none) and aren't part of this module.
+exclude header "llvm/CodeGen/LinkAllAsmWriterComponents.h"
+exclude header "llvm/CodeGen/LinkAllCodegenComponents.h"
+
+exclude header "llvm/CodeGen/CodeGenPassBuilder.h"
+
+// These are intended for (repeated) textual inclusion.
+textual header "llvm/CodeGen/DIEValue.def"
+textual header "llvm/CodeGen/MachinePassRegistry.def"
+  }
+}
+
+// FIXME: Make this as a submodule of LLVM_Backend again.
+//Doing so causes a linker error in clang-format.
+module LLVM_Backend_Target {
+  umbrella "llvm/Target"
+  module * { export * }
+}
+
+module LLVM_Bitcode {
+ requires cplusplus
+ umbrella "llvm/Bitcode"
+ module * { export * }
+}
+
+module LLVM_Bitstream {
+ requires cplusplus
+ umbrella "llvm/Bitstream"
+ module * { export * }
+}
+
+module LLVM_BinaryFormat {
+requires cplusplus
+umbrella "llvm/BinaryFormat" module * { export * }
+textual header "llvm/BinaryFormat/Dwarf.def"
+textual header "llvm/BinaryFormat/DXContainerConstants.def"
+textual header "llvm/BinaryFormat/DynamicTags.def"
+textual header "llvm/BinaryFormat/MachO.def"
+textual header "llvm/BinaryFormat/MinidumpConstants.def"
+textual header "llvm/BinaryFormat/Swift.def"
+textual header "llvm/BinaryFormat/ELFRelocs/AArch64.def"
+textual header "llvm/BinaryFormat/ELFRelocs/AMDGPU.def"
+textual header "llvm/BinaryFormat/ELFRelocs/ARM.def"
+textual header "llvm/BinaryFormat/ELFRelocs/ARC.def"
+textual header "llvm/BinaryFormat/ELFRelocs/AVR.def"
+textual header "llvm/BinaryFormat/ELFRelocs/BPF.def"
+textual header "llvm/BinaryFormat/ELFRelocs/CSKY.def"
+textual header "llvm/BinaryFormat/ELFRelocs/Hexagon.def"
+textual header "llvm/BinaryFormat/ELFRelocs/i386.def"
+textual header "llvm/BinaryFormat/ELFRelocs/Lanai.def"
+textual header "llvm/BinaryFormat/ELFRelocs/LoongArch.def"
+textual header 

[PATCH] D148776: [Modules] Move modulemaps to header search directories. NFC intended.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148776/new/

https://reviews.llvm.org/D148776

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148776: [Modules] Move modulemaps to header search directories. NFC intended.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 515194.
vsapsai added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148776/new/

https://reviews.llvm.org/D148776

Files:
  clang/include/clang-c/module.modulemap
  clang/include/clang/module.modulemap
  clang/include/module.modulemap
  lldb/include/lldb/module.modulemap
  lldb/include/module.modulemap
  llvm/CMakeLists.txt
  llvm/include/CMakeLists.txt
  llvm/include/llvm-c/module.modulemap
  llvm/include/llvm/CMakeLists.txt
  llvm/include/llvm/module.extern.modulemap
  llvm/include/llvm/module.install.modulemap
  llvm/include/llvm/module.modulemap
  llvm/include/llvm/module.modulemap.build
  llvm/include/module.extern.modulemap
  llvm/include/module.install.modulemap
  llvm/include/module.modulemap
  llvm/include/module.modulemap.build

Index: llvm/include/module.modulemap.build
===
--- llvm/include/module.modulemap.build
+++ llvm/include/module.modulemap.build
@@ -1,13 +1,13 @@
 // This is copied into the build area for a $src != $build compilation.
 module LLVM_Support_DataTypes {
-  header "Support/DataTypes.h"
+  header "llvm/Support/DataTypes.h"
   export *
 }
 module LLVM_Config_ABI_Breaking {
-  header "Config/abi-breaking.h"
+  header "llvm/Config/abi-breaking.h"
   export *
 }
 module LLVM_Config_Config {
-  header "Config/llvm-config.h"
+  header "llvm/Config/llvm-config.h"
   export *
 }
Index: llvm/include/module.modulemap
===
--- /dev/null
+++ llvm/include/module.modulemap
@@ -0,0 +1,481 @@
+module LLVM_C {
+  umbrella "llvm-c"
+  module * { export * }
+}
+
+module LLVM_Analysis {
+  requires cplusplus
+  umbrella "llvm/Analysis"
+  module * { export * }
+
+  // This is intended for (repeated) textual inclusion.
+  textual header "llvm/Analysis/ScalarFuncs.def"
+  textual header "llvm/Analysis/TargetLibraryInfo.def"
+  textual header "llvm/Analysis/VecFuncs.def"
+}
+
+module LLVM_AsmParser {
+  requires cplusplus
+  umbrella "llvm/AsmParser"
+  module * { export * }
+}
+
+// A module covering CodeGen/ and Target/. These are intertwined
+// and codependent, and thus notionally form a single module.
+module LLVM_Backend {
+  requires cplusplus
+
+  module CodeGen {
+umbrella "llvm/CodeGen"
+module * { export * }
+
+// Exclude these; they're intended to be included into only a single
+// translation unit (or none) and aren't part of this module.
+exclude header "llvm/CodeGen/LinkAllAsmWriterComponents.h"
+exclude header "llvm/CodeGen/LinkAllCodegenComponents.h"
+
+exclude header "llvm/CodeGen/CodeGenPassBuilder.h"
+
+// These are intended for (repeated) textual inclusion.
+textual header "llvm/CodeGen/DIEValue.def"
+textual header "llvm/CodeGen/MachinePassRegistry.def"
+  }
+}
+
+// FIXME: Make this as a submodule of LLVM_Backend again.
+//Doing so causes a linker error in clang-format.
+module LLVM_Backend_Target {
+  umbrella "llvm/Target"
+  module * { export * }
+}
+
+module LLVM_Bitcode {
+ requires cplusplus
+ umbrella "llvm/Bitcode"
+ module * { export * }
+}
+
+module LLVM_Bitstream {
+ requires cplusplus
+ umbrella "llvm/Bitstream"
+ module * { export * }
+}
+
+module LLVM_BinaryFormat {
+requires cplusplus
+umbrella "llvm/BinaryFormat" module * { export * }
+textual header "llvm/BinaryFormat/Dwarf.def"
+textual header "llvm/BinaryFormat/DXContainerConstants.def"
+textual header "llvm/BinaryFormat/DynamicTags.def"
+textual header "llvm/BinaryFormat/MachO.def"
+textual header "llvm/BinaryFormat/MinidumpConstants.def"
+textual header "llvm/BinaryFormat/Swift.def"
+textual header "llvm/BinaryFormat/ELFRelocs/AArch64.def"
+textual header "llvm/BinaryFormat/ELFRelocs/AMDGPU.def"
+textual header "llvm/BinaryFormat/ELFRelocs/ARM.def"
+textual header "llvm/BinaryFormat/ELFRelocs/ARC.def"
+textual header "llvm/BinaryFormat/ELFRelocs/AVR.def"
+textual header "llvm/BinaryFormat/ELFRelocs/BPF.def"
+textual header "llvm/BinaryFormat/ELFRelocs/CSKY.def"
+textual header "llvm/BinaryFormat/ELFRelocs/Hexagon.def"
+textual header "llvm/BinaryFormat/ELFRelocs/i386.def"
+textual header "llvm/BinaryFormat/ELFRelocs/Lanai.def"
+textual header "llvm/BinaryFormat/ELFRelocs/LoongArch.def"
+textual header "llvm/BinaryFormat/ELFRelocs/M68k.def"
+textual header "llvm/BinaryFormat/ELFRelocs/Mips.def"
+textual header "llvm/BinaryFormat/ELFRelocs/MSP430.def"
+textual header "llvm/BinaryFormat/ELFRelocs/PowerPC64.def"
+textual header "llvm/BinaryFormat/ELFRelocs/PowerPC.def"
+textual header "llvm/BinaryFormat/ELFRelocs/RISCV.def"
+textual header "llvm/BinaryFormat/ELFRelocs/Sparc.def"
+textual header "llvm/BinaryFormat/ELFRelocs/SystemZ.def"
+

[PATCH] D148776: [Modules] Move modulemaps to header search directories. NFC intended.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: clang/lib/Lex/HeaderSearch.cpp:358-373
+#if ALLOW_SUBDIRECTORY_MODULE_MAP_SEARCH
 // If we've already performed the exhaustive search for module maps in this
 // search directory, don't do it again.
 if (Dir.haveSearchedAllModuleMaps())
   continue;
 
 // Load all module maps in the immediate subdirectories of this search

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148776/new/

https://reviews.llvm.org/D148776

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148776: [Modules] Move modulemaps to header search directories. NFC intended.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai created this revision.
vsapsai added reviewers: EricWF, aprantl, chapuni.
Herald added subscribers: ributzka, s.egerton, simoncook, asb, fedor.sergeev, 
dschuff.
Herald added a reviewer: deadalnix.
Herald added a project: All.
vsapsai requested review of this revision.
Herald added subscribers: llvm-commits, lldb-commits, cfe-commits, jplehr, 
pcwang-thead, sstefan1, aheejin.
Herald added a reviewer: jdoerfert.
Herald added projects: clang, LLDB, LLVM.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148776

Files:
  clang/include/clang-c/module.modulemap
  clang/include/clang/module.modulemap
  clang/include/module.modulemap
  clang/lib/Lex/HeaderSearch.cpp
  lldb/include/lldb/module.modulemap
  lldb/include/module.modulemap
  llvm/CMakeLists.txt
  llvm/include/CMakeLists.txt
  llvm/include/llvm-c/module.modulemap
  llvm/include/llvm/CMakeLists.txt
  llvm/include/llvm/module.extern.modulemap
  llvm/include/llvm/module.install.modulemap
  llvm/include/llvm/module.modulemap
  llvm/include/llvm/module.modulemap.build
  llvm/include/module.extern.modulemap
  llvm/include/module.install.modulemap
  llvm/include/module.modulemap
  llvm/include/module.modulemap.build

Index: llvm/include/module.modulemap.build
===
--- llvm/include/module.modulemap.build
+++ llvm/include/module.modulemap.build
@@ -1,13 +1,13 @@
 // This is copied into the build area for a $src != $build compilation.
 module LLVM_Support_DataTypes {
-  header "Support/DataTypes.h"
+  header "llvm/Support/DataTypes.h"
   export *
 }
 module LLVM_Config_ABI_Breaking {
-  header "Config/abi-breaking.h"
+  header "llvm/Config/abi-breaking.h"
   export *
 }
 module LLVM_Config_Config {
-  header "Config/llvm-config.h"
+  header "llvm/Config/llvm-config.h"
   export *
 }
Index: llvm/include/module.modulemap
===
--- /dev/null
+++ llvm/include/module.modulemap
@@ -0,0 +1,481 @@
+module LLVM_C {
+  umbrella "llvm-c"
+  module * { export * }
+}
+
+module LLVM_Analysis {
+  requires cplusplus
+  umbrella "llvm/Analysis"
+  module * { export * }
+
+  // This is intended for (repeated) textual inclusion.
+  textual header "llvm/Analysis/ScalarFuncs.def"
+  textual header "llvm/Analysis/TargetLibraryInfo.def"
+  textual header "llvm/Analysis/VecFuncs.def"
+}
+
+module LLVM_AsmParser {
+  requires cplusplus
+  umbrella "llvm/AsmParser"
+  module * { export * }
+}
+
+// A module covering CodeGen/ and Target/. These are intertwined
+// and codependent, and thus notionally form a single module.
+module LLVM_Backend {
+  requires cplusplus
+
+  module CodeGen {
+umbrella "llvm/CodeGen"
+module * { export * }
+
+// Exclude these; they're intended to be included into only a single
+// translation unit (or none) and aren't part of this module.
+exclude header "llvm/CodeGen/LinkAllAsmWriterComponents.h"
+exclude header "llvm/CodeGen/LinkAllCodegenComponents.h"
+
+exclude header "llvm/CodeGen/CodeGenPassBuilder.h"
+
+// These are intended for (repeated) textual inclusion.
+textual header "llvm/CodeGen/DIEValue.def"
+textual header "llvm/CodeGen/MachinePassRegistry.def"
+  }
+}
+
+// FIXME: Make this as a submodule of LLVM_Backend again.
+//Doing so causes a linker error in clang-format.
+module LLVM_Backend_Target {
+  umbrella "llvm/Target"
+  module * { export * }
+}
+
+module LLVM_Bitcode {
+ requires cplusplus
+ umbrella "llvm/Bitcode"
+ module * { export * }
+}
+
+module LLVM_Bitstream {
+ requires cplusplus
+ umbrella "llvm/Bitstream"
+ module * { export * }
+}
+
+module LLVM_BinaryFormat {
+requires cplusplus
+umbrella "llvm/BinaryFormat" module * { export * }
+textual header "llvm/BinaryFormat/Dwarf.def"
+textual header "llvm/BinaryFormat/DXContainerConstants.def"
+textual header "llvm/BinaryFormat/DynamicTags.def"
+textual header "llvm/BinaryFormat/MachO.def"
+textual header "llvm/BinaryFormat/MinidumpConstants.def"
+textual header "llvm/BinaryFormat/Swift.def"
+textual header "llvm/BinaryFormat/ELFRelocs/AArch64.def"
+textual header "llvm/BinaryFormat/ELFRelocs/AMDGPU.def"
+textual header "llvm/BinaryFormat/ELFRelocs/ARM.def"
+textual header "llvm/BinaryFormat/ELFRelocs/ARC.def"
+textual header "llvm/BinaryFormat/ELFRelocs/AVR.def"
+textual header "llvm/BinaryFormat/ELFRelocs/BPF.def"
+textual header "llvm/BinaryFormat/ELFRelocs/CSKY.def"
+textual header "llvm/BinaryFormat/ELFRelocs/Hexagon.def"
+textual header "llvm/BinaryFormat/ELFRelocs/i386.def"
+textual header "llvm/BinaryFormat/ELFRelocs/Lanai.def"
+