[clang] [clang] Diagnose config_macros before building modules (PR #83641)
Michael Spencer via cfe-commits
cfe-commits at lists.llvm.org
Mon Mar 4 14:47:38 PST 2024
https://github.com/Bigcheese updated https://github.com/llvm/llvm-project/pull/83641
>From e8993b51f0dcdecd2fcb72f91d7e4631e95c2c09 Mon Sep 17 00:00:00 2001
From: Michael Spencer <michael_spencer at apple.com>
Date: Fri, 1 Mar 2024 17:18:20 -0800
Subject: [PATCH] [clang] Diagnose config_macros before building modules
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
---
clang/lib/Frontend/CompilerInstance.cpp | 35 ++++++++++----
clang/test/Modules/Inputs/config.h | 7 ---
clang/test/Modules/Inputs/module.modulemap | 5 --
clang/test/Modules/config_macros.m | 54 ++++++++++++++++++++--
4 files changed, 78 insertions(+), 23 deletions(-)
delete mode 100644 clang/test/Modules/Inputs/config.h
diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index 444ffff3073775..ec4e68209d657d 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 (const StringRef ConMacro : TopModule->ConfigMacros) {
+ checkConfigMacro(PP, ConMacro, M, ImportLoc);
+ }
+}
+
/// Write a new timestamp file with the given path.
static void writeTimestampFile(StringRef TimestampFile) {
std::error_code EC;
@@ -1829,6 +1837,13 @@ 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.
+ if (M)
+ checkConfigMacros(getPreprocessor(), M, ImportLoc);
+
// Select the source and filename for loading the named module.
std::string ModuleFilename;
ModuleSource Source =
@@ -2006,12 +2021,23 @@ 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(
ModuleName, ImportLoc, /*AllowSearch*/ true,
/*AllowExtraModuleMapSearch*/ !IsInclusionDirective);
+ // Config macros do not need to be checked here for two reasons.
+ // * This will always be textual inclusion, and thus the config macros
+ // actually do impact the content of the header.
+ // * `Preprocessor::HandleHeaderIncludeOrImport` will never call this
+ // function as the `#include` or `#import` is textual.
+
MM.cacheModuleLoad(*Path[0].first, Module);
} else {
ModuleLoadResult Result = findOrCompileModuleAndReadAST(
@@ -2146,18 +2172,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..adb2a8f6e5ce34 100644
--- a/clang/test/Modules/config_macros.m
+++ b/clang/test/Modules/config_macros.m
@@ -1,3 +1,50 @@
+// This test verifies that config macro warnings are emitted when it looks like
+// the user expected a `#define` to impact the import of a module.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// Prebuild the `config` module so it's in the module cache.
+// 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
+
+// Verify that each time the `config` module is imported the current macro state
+// is checked.
+// RUN: %clang_cc1 -std=c99 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %t -DWANT_FOO=1 %t/config.m -verify
+
+// Verify that warnings are emitted before building a module in case the command
+// line macro state causes the module to fail to build.
+// RUN: %clang_cc1 -std=c99 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %t %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 +69,8 @@
#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
+#define SOME_VALUE 5 // expected-note{{macro was defined here}}
+ at 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';}}
More information about the cfe-commits
mailing list