[clang] Diagnose config_macros before building modules #83641
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Michael Spencer (Bigcheese) ChangesBefore this patch, if a module fails to build because of a missing config_macro, the user will never see the config macro warning. This patch diagnoses this before building, and each subsequent time a module is imported. rdar://123921931 Full diff: https://github.com/llvm/llvm-project/pull/83641.diff 4 Files Affected:
diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index 444ffff3073775..14fd140f03cc36 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -1591,6 +1591,14 @@ static void checkConfigMacro(Preprocessor &PP, StringRef ConfigMacro,
}
}
+static void checkConfigMacros(Preprocessor &PP, Module *M,
+ SourceLocation ImportLoc) {
+ clang::Module *TopModule = M->getTopLevelModule();
+ for (unsigned I = 0, N = TopModule->ConfigMacros.size(); I != N; ++I) {
+ checkConfigMacro(PP, TopModule->ConfigMacros[I], M, ImportLoc);
+ }
+}
+
/// Write a new timestamp file with the given path.
static void writeTimestampFile(StringRef TimestampFile) {
std::error_code EC;
@@ -1829,6 +1837,12 @@ ModuleLoadResult CompilerInstance::findOrCompileModuleAndReadAST(
Module *M =
HS.lookupModule(ModuleName, ImportLoc, true, !IsInclusionDirective);
+ // Check for any configuration macros that have changed. This is done
+ // immediately before potentially building a module in case this module
+ // depends on having one of its configuration macros defined to successfully
+ // build. If this is not done the user will never see the warning.
+ checkConfigMacros(getPreprocessor(), M, ImportLoc);
+
// Select the source and filename for loading the named module.
std::string ModuleFilename;
ModuleSource Source =
@@ -2006,6 +2020,11 @@ CompilerInstance::loadModule(SourceLocation ImportLoc,
if (auto MaybeModule = MM.getCachedModuleLoad(*Path[0].first)) {
// Use the cached result, which may be nullptr.
Module = *MaybeModule;
+ // Config macros are already checked before building a module, but they need
+ // to be checked at each import location in case any of the config macros
+ // have a new value at the current `ImportLoc`.
+ if (Module)
+ checkConfigMacros(getPreprocessor(), Module, ImportLoc);
} else if (ModuleName == getLangOpts().CurrentModule) {
// This is the module we're building.
Module = PP->getHeaderSearchInfo().lookupModule(
@@ -2146,18 +2165,11 @@ CompilerInstance::loadModule(SourceLocation ImportLoc,
TheASTReader->makeModuleVisible(Module, Visibility, ImportLoc);
}
- // Check for any configuration macros that have changed.
- clang::Module *TopModule = Module->getTopLevelModule();
- for (unsigned I = 0, N = TopModule->ConfigMacros.size(); I != N; ++I) {
- checkConfigMacro(getPreprocessor(), TopModule->ConfigMacros[I],
- Module, ImportLoc);
- }
-
// Resolve any remaining module using export_as for this one.
getPreprocessor()
.getHeaderSearchInfo()
.getModuleMap()
- .resolveLinkAsDependencies(TopModule);
+ .resolveLinkAsDependencies(Module->getTopLevelModule());
LastModuleImportLoc = ImportLoc;
LastModuleImportResult = ModuleLoadResult(Module);
diff --git a/clang/test/Modules/Inputs/config.h b/clang/test/Modules/Inputs/config.h
deleted file mode 100644
index 4c124b0bf82b7c..00000000000000
--- a/clang/test/Modules/Inputs/config.h
+++ /dev/null
@@ -1,7 +0,0 @@
-#ifdef WANT_FOO
-int* foo(void);
-#endif
-
-#ifdef WANT_BAR
-char *bar(void);
-#endif
diff --git a/clang/test/Modules/Inputs/module.modulemap b/clang/test/Modules/Inputs/module.modulemap
index e7cb4b27bc08b9..47f6c5c1010d7d 100644
--- a/clang/test/Modules/Inputs/module.modulemap
+++ b/clang/test/Modules/Inputs/module.modulemap
@@ -260,11 +260,6 @@ module cxx_decls_merged {
header "cxx-decls-merged.h"
}
-module config {
- header "config.h"
- config_macros [exhaustive] WANT_FOO, WANT_BAR
-}
-
module diag_flags {
header "diag_flags.h"
}
diff --git a/clang/test/Modules/config_macros.m b/clang/test/Modules/config_macros.m
index 15e2c16606ba29..65dd2a89a05c32 100644
--- a/clang/test/Modules/config_macros.m
+++ b/clang/test/Modules/config_macros.m
@@ -1,3 +1,39 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -std=c99 -fmodules -fimplicit-module-maps -x objective-c -fmodules-cache-path=%t -DWANT_FOO=1 -emit-module -fmodule-name=config %t/module.modulemap
+// RUN: %clang_cc1 -std=c99 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %t -DWANT_FOO=1 %t/config.m -verify
+// RUN: %clang_cc1 -std=c99 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %t -DTEST_ERROR %t/config_error.m -verify
+
+//--- module.modulemap
+
+module config {
+ header "config.h"
+ config_macros [exhaustive] WANT_FOO, WANT_BAR
+}
+
+module config_error {
+ header "config_error.h"
+ config_macros SOME_VALUE
+}
+
+//--- config.h
+
+#ifdef WANT_FOO
+int* foo(void);
+#endif
+
+#ifdef WANT_BAR
+char *bar(void);
+#endif
+
+//--- config_error.h
+
+struct my_thing {
+ char buf[SOME_VALUE];
+};
+
+//--- config.m
+
@import config;
int *test_foo(void) {
@@ -22,7 +58,10 @@
#define WANT_BAR 1 // expected-note{{macro was defined here}}
@import config; // expected-warning{{definition of configuration macro 'WANT_BAR' has no effect on the import of 'config'; pass '-DWANT_BAR=...' on the command line to configure the module}}
-// RUN: rm -rf %t
-// RUN: %clang_cc1 -std=c99 -fmodules -fimplicit-module-maps -x objective-c -fmodules-cache-path=%t -DWANT_FOO=1 -emit-module -fmodule-name=config %S/Inputs/module.modulemap
-// RUN: %clang_cc1 -std=c99 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs -DWANT_FOO=1 %s -verify
+//--- config_error.m
+#ifdef TEST_ERROR
+#define SOME_VALUE 5 // expected-note{{macro was defined here}}
+@import config_error; // expected-error{{could not build module}} \
+ // expected-warning{{definition of configuration macro 'SOME_VALUE' has no effect on the import of 'config_error';}}
+#endif
|
|
@llvm/pr-subscribers-clang-modules Author: Michael Spencer (Bigcheese) ChangesBefore this patch, if a module fails to build because of a missing config_macro, the user will never see the config macro warning. This patch diagnoses this before building, and each subsequent time a module is imported. rdar://123921931 Full diff: https://github.com/llvm/llvm-project/pull/83641.diff 4 Files Affected:
diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index 444ffff3073775..14fd140f03cc36 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -1591,6 +1591,14 @@ static void checkConfigMacro(Preprocessor &PP, StringRef ConfigMacro,
}
}
+static void checkConfigMacros(Preprocessor &PP, Module *M,
+ SourceLocation ImportLoc) {
+ clang::Module *TopModule = M->getTopLevelModule();
+ for (unsigned I = 0, N = TopModule->ConfigMacros.size(); I != N; ++I) {
+ checkConfigMacro(PP, TopModule->ConfigMacros[I], M, ImportLoc);
+ }
+}
+
/// Write a new timestamp file with the given path.
static void writeTimestampFile(StringRef TimestampFile) {
std::error_code EC;
@@ -1829,6 +1837,12 @@ ModuleLoadResult CompilerInstance::findOrCompileModuleAndReadAST(
Module *M =
HS.lookupModule(ModuleName, ImportLoc, true, !IsInclusionDirective);
+ // Check for any configuration macros that have changed. This is done
+ // immediately before potentially building a module in case this module
+ // depends on having one of its configuration macros defined to successfully
+ // build. If this is not done the user will never see the warning.
+ checkConfigMacros(getPreprocessor(), M, ImportLoc);
+
// Select the source and filename for loading the named module.
std::string ModuleFilename;
ModuleSource Source =
@@ -2006,6 +2020,11 @@ CompilerInstance::loadModule(SourceLocation ImportLoc,
if (auto MaybeModule = MM.getCachedModuleLoad(*Path[0].first)) {
// Use the cached result, which may be nullptr.
Module = *MaybeModule;
+ // Config macros are already checked before building a module, but they need
+ // to be checked at each import location in case any of the config macros
+ // have a new value at the current `ImportLoc`.
+ if (Module)
+ checkConfigMacros(getPreprocessor(), Module, ImportLoc);
} else if (ModuleName == getLangOpts().CurrentModule) {
// This is the module we're building.
Module = PP->getHeaderSearchInfo().lookupModule(
@@ -2146,18 +2165,11 @@ CompilerInstance::loadModule(SourceLocation ImportLoc,
TheASTReader->makeModuleVisible(Module, Visibility, ImportLoc);
}
- // Check for any configuration macros that have changed.
- clang::Module *TopModule = Module->getTopLevelModule();
- for (unsigned I = 0, N = TopModule->ConfigMacros.size(); I != N; ++I) {
- checkConfigMacro(getPreprocessor(), TopModule->ConfigMacros[I],
- Module, ImportLoc);
- }
-
// Resolve any remaining module using export_as for this one.
getPreprocessor()
.getHeaderSearchInfo()
.getModuleMap()
- .resolveLinkAsDependencies(TopModule);
+ .resolveLinkAsDependencies(Module->getTopLevelModule());
LastModuleImportLoc = ImportLoc;
LastModuleImportResult = ModuleLoadResult(Module);
diff --git a/clang/test/Modules/Inputs/config.h b/clang/test/Modules/Inputs/config.h
deleted file mode 100644
index 4c124b0bf82b7c..00000000000000
--- a/clang/test/Modules/Inputs/config.h
+++ /dev/null
@@ -1,7 +0,0 @@
-#ifdef WANT_FOO
-int* foo(void);
-#endif
-
-#ifdef WANT_BAR
-char *bar(void);
-#endif
diff --git a/clang/test/Modules/Inputs/module.modulemap b/clang/test/Modules/Inputs/module.modulemap
index e7cb4b27bc08b9..47f6c5c1010d7d 100644
--- a/clang/test/Modules/Inputs/module.modulemap
+++ b/clang/test/Modules/Inputs/module.modulemap
@@ -260,11 +260,6 @@ module cxx_decls_merged {
header "cxx-decls-merged.h"
}
-module config {
- header "config.h"
- config_macros [exhaustive] WANT_FOO, WANT_BAR
-}
-
module diag_flags {
header "diag_flags.h"
}
diff --git a/clang/test/Modules/config_macros.m b/clang/test/Modules/config_macros.m
index 15e2c16606ba29..65dd2a89a05c32 100644
--- a/clang/test/Modules/config_macros.m
+++ b/clang/test/Modules/config_macros.m
@@ -1,3 +1,39 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -std=c99 -fmodules -fimplicit-module-maps -x objective-c -fmodules-cache-path=%t -DWANT_FOO=1 -emit-module -fmodule-name=config %t/module.modulemap
+// RUN: %clang_cc1 -std=c99 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %t -DWANT_FOO=1 %t/config.m -verify
+// RUN: %clang_cc1 -std=c99 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %t -DTEST_ERROR %t/config_error.m -verify
+
+//--- module.modulemap
+
+module config {
+ header "config.h"
+ config_macros [exhaustive] WANT_FOO, WANT_BAR
+}
+
+module config_error {
+ header "config_error.h"
+ config_macros SOME_VALUE
+}
+
+//--- config.h
+
+#ifdef WANT_FOO
+int* foo(void);
+#endif
+
+#ifdef WANT_BAR
+char *bar(void);
+#endif
+
+//--- config_error.h
+
+struct my_thing {
+ char buf[SOME_VALUE];
+};
+
+//--- config.m
+
@import config;
int *test_foo(void) {
@@ -22,7 +58,10 @@
#define WANT_BAR 1 // expected-note{{macro was defined here}}
@import config; // expected-warning{{definition of configuration macro 'WANT_BAR' has no effect on the import of 'config'; pass '-DWANT_BAR=...' on the command line to configure the module}}
-// RUN: rm -rf %t
-// RUN: %clang_cc1 -std=c99 -fmodules -fimplicit-module-maps -x objective-c -fmodules-cache-path=%t -DWANT_FOO=1 -emit-module -fmodule-name=config %S/Inputs/module.modulemap
-// RUN: %clang_cc1 -std=c99 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs -DWANT_FOO=1 %s -verify
+//--- config_error.m
+#ifdef TEST_ERROR
+#define SOME_VALUE 5 // expected-note{{macro was defined here}}
+@import config_error; // expected-error{{could not build module}} \
+ // expected-warning{{definition of configuration macro 'SOME_VALUE' has no effect on the import of 'config_error';}}
+#endif
|
Bigcheese
force-pushed
the
dev/warn-config-macros
branch
from
4edc581
to
c2445d4
Compare
vsapsai
reviewed
Mar 4, 2024
dwblaikie
reviewed
Mar 4, 2024
Before this patch, if a module fails to build because of a missing config_macro, the user will never see the config macro warning. This patch diagnoses this before building, and each subsequent time a module is imported. rdar://123921931
Bigcheese
force-pushed
the
dev/warn-config-macros
branch
from
c2445d4
to
e8993b5
Compare
vsapsai
approved these changes
Mar 4, 2024
Bigcheese
added a commit
to Bigcheese/llvm-project
that referenced
this pull request
Mar 5, 2024
Before this patch, if a module fails to build because of a missing config_macro, the user will never see the config macro warning. This patch diagnoses this before building, and each subsequent time a module is imported. rdar://123921931 llvm#83641 (cherry picked from commit ee044d5)
Bigcheese
added a commit
to apple/llvm-project
that referenced
this pull request
Mar 6, 2024
Before this patch, if a module fails to build because of a missing config_macro, the user will never see the config macro warning. This patch diagnoses this before building, and each subsequent time a module is imported. rdar://123921931 llvm#83641 (cherry picked from commit ee044d5)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this patch, if a module fails to build because of a missing config_macro, the user will never see the config macro warning. This patch diagnoses this before building, and each subsequent time a module is imported.
rdar://123921931