[Modules] Don't fail when an unused textual header is missing. #138227
Conversation
According to the documentation > A header declaration that does not contain `exclude` nor `textual` specifies a header that contributes to the enclosing module. Which means that `exclude` and `textual` header don't contribute to the enclosing module and their presence isn't required to build such a module. The keywords tell clang how a header should be treated in a context of the module but they don't add headers to the module. When a textual header *is* used, clang still emits "file not found" error pointing to the location where the missing file is included.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Volodymyr Sapsai (vsapsai) ChangesAccording to the documentation Which means that When a textual header is used, clang still emits "file not found" error pointing to the location where the missing file is included. Full diff: https://github.com/llvm/llvm-project/pull/138227.diff 3 Files Affected:
diff --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp
index c2f13fa48d464..1ba02b919a22a 100644
--- a/clang/lib/Lex/ModuleMap.cpp
+++ b/clang/lib/Lex/ModuleMap.cpp
@@ -310,8 +310,10 @@ void ModuleMap::resolveHeader(Module *Mod,
} else if (Header.HasBuiltinHeader && !Header.Size && !Header.ModTime) {
// There's a builtin header but no corresponding on-disk header. Assume
// this was supposed to modularize the builtin header alone.
- } else if (Header.Kind == Module::HK_Excluded) {
- // Ignore missing excluded header files. They're optional anyway.
+ } else if ((Header.Kind == Module::HK_Excluded) ||
+ (Header.Kind == Module::HK_Textual)) {
+ // Ignore excluded and textual header files as a module can be built with
+ // such headers missing.
} else {
// If we find a module that has a missing header, we mark this module as
// unavailable and store the header directive for displaying diagnostics.
diff --git a/clang/test/Modules/Inputs/submodules/module.modulemap b/clang/test/Modules/Inputs/submodules/module.modulemap
index 1c1b76a08969e..9e8143b8101de 100644
--- a/clang/test/Modules/Inputs/submodules/module.modulemap
+++ b/clang/test/Modules/Inputs/submodules/module.modulemap
@@ -30,3 +30,7 @@ module missing_umbrella_with_inferred_submodules {
module * { export * }
export *
}
+
+module missing_textual_header {
+ textual header "missing_textual.h"
+}
diff --git a/clang/test/Modules/missing-header.m b/clang/test/Modules/missing-header.m
index c162e1b5f08b3..84d82e5ceda32 100644
--- a/clang/test/Modules/missing-header.m
+++ b/clang/test/Modules/missing-header.m
@@ -8,6 +8,9 @@
@import missing_unavailable_headers.not_missing; // OK
// CHECK-NOT: missing_unavailable_headers
+@import missing_textual_header; // OK
+// CHECK-NOT: missing_textual_header
+
@import missing_headers;
// CHECK: module.modulemap:15:27: error: header 'missing.h' not found
// CHECK: could not build module 'missing_headers'
|
|
@llvm/pr-subscribers-clang-modules Author: Volodymyr Sapsai (vsapsai) ChangesAccording to the documentation Which means that When a textual header is used, clang still emits "file not found" error pointing to the location where the missing file is included. Full diff: https://github.com/llvm/llvm-project/pull/138227.diff 3 Files Affected:
diff --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp
index c2f13fa48d464..1ba02b919a22a 100644
--- a/clang/lib/Lex/ModuleMap.cpp
+++ b/clang/lib/Lex/ModuleMap.cpp
@@ -310,8 +310,10 @@ void ModuleMap::resolveHeader(Module *Mod,
} else if (Header.HasBuiltinHeader && !Header.Size && !Header.ModTime) {
// There's a builtin header but no corresponding on-disk header. Assume
// this was supposed to modularize the builtin header alone.
- } else if (Header.Kind == Module::HK_Excluded) {
- // Ignore missing excluded header files. They're optional anyway.
+ } else if ((Header.Kind == Module::HK_Excluded) ||
+ (Header.Kind == Module::HK_Textual)) {
+ // Ignore excluded and textual header files as a module can be built with
+ // such headers missing.
} else {
// If we find a module that has a missing header, we mark this module as
// unavailable and store the header directive for displaying diagnostics.
diff --git a/clang/test/Modules/Inputs/submodules/module.modulemap b/clang/test/Modules/Inputs/submodules/module.modulemap
index 1c1b76a08969e..9e8143b8101de 100644
--- a/clang/test/Modules/Inputs/submodules/module.modulemap
+++ b/clang/test/Modules/Inputs/submodules/module.modulemap
@@ -30,3 +30,7 @@ module missing_umbrella_with_inferred_submodules {
module * { export * }
export *
}
+
+module missing_textual_header {
+ textual header "missing_textual.h"
+}
diff --git a/clang/test/Modules/missing-header.m b/clang/test/Modules/missing-header.m
index c162e1b5f08b3..84d82e5ceda32 100644
--- a/clang/test/Modules/missing-header.m
+++ b/clang/test/Modules/missing-header.m
@@ -8,6 +8,9 @@
@import missing_unavailable_headers.not_missing; // OK
// CHECK-NOT: missing_unavailable_headers
+@import missing_textual_header; // OK
+// CHECK-NOT: missing_textual_header
+
@import missing_headers;
// CHECK: module.modulemap:15:27: error: header 'missing.h' not found
// CHECK: could not build module 'missing_headers'
|
|
The change for |
|
Thanks for the review! |
|
We have bisected build breakages internally at google at this change. Ironically the compiler complains about a missing header while before it didn't. We do not have a reproducer yet. |
|
Here's a reproducer for our breakage: // RUN: rm -rf %t
// RUN: split-file %s %t
//
// First, build a module with a header.
//
// RUN: %clang_cc1 -fmodules -fmodule-map-file=%t/modules1.map -fmodule-name=a -emit-module -xc++ -fmodules-embed-all-files -o %t/a.pcm %t/modules1.map
// RUN: %clang_cc1 -fmodules -fmodule-map-file=%t/modules1.map -fmodule-map-file=%t/modules2.map -fmodule-name=b -emit-module \
// RUN: -fmodule-file=%t/a.pcm -xc++ -fmodules-embed-all-files -o %t/b.pcm %t/modules2.map
//
// Remove the header and check we can still build the code that finds it in a PCM.
//
// RUN: rm %t/foo.h
// RUN: %clang_cc1 -fmodules -fmodule-map-file=%t/modules2.map -fmodule-file=%t/b.pcm -fsyntax-only %t/use.cpp
//--- modules1.map
module a {
module foo {
header "foo.h"
export *
}
export *
}
//--- modules2.map
module all_textual {
module foo {
textual header "foo.h"
export *
}
module wrap_foo {
textual header "wrap_foo.h"
export *
}
export *
}
module b {
module wrap_foo {
private header "wrap_foo.h"
export *
}
export *
}
//--- foo.h
#ifndef FOO_H
#define FOO_H
void foo();
#endif
//--- wrap_foo.h
#include "foo.h"
//--- use.cpp
#include "wrap_foo.h"
void test() {
foo();
}It previously built fine because the llvm-project/clang/lib/Lex/ModuleMap.cpp Line 326 in 09fd8f0 and therefore it made us pick the module llvm-project/clang/lib/Lex/ModuleMap.cpp Line 618 in 09fd8f0 After the change, we no longer mark I know what we do is cheesy and we might need to reconsider how to approach it, but this is a breaking change and it would be great to find a resolution for it before committing to the new behavior. Could we revert this while we discuss the potential ways out of it? |
|
@vsapsai Please revert if there's no obvious fix. |
What is your timeframe for stopping putting the same header ("wrap_foo.h") into multiple modules? If you do that it was never guaranteed which module would be used for a header. I'm willing to unblock you but I'd like to know if you are committed to fixing the issue in the module maps. |
I don't think we can ever get rid of that pattern and we will keep relying on Clang supporting this. I am sure there are ways to meet both your needs and our needs. To reiterate, Clang supports having the same header in multiple module maps and we don't view our current setup as having at a high level. I just want to explicit that our stance is that there are no issues that need "fixing in the module maps". How we achieve this today is shaky (relying on availability doesn't look right), but we absolutely need:
|
According to the documentation
Which means that
excludeandtextualheader don't contribute to the enclosing module and their presence isn't required to build such a module. The keywords tell clang how a header should be treated in a context of the module but they don't add headers to the module.When a textual header is used, clang still emits "file not found" error pointing to the location where the missing file is included.