[clang] [clang] Diagnose config_macros before building modules (PR #83641)
https://github.com/Bigcheese closed https://github.com/llvm/llvm-project/pull/83641 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Diagnose config_macros before building modules (PR #83641)
@@ -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
vsapsai wrote:
That works. I like how for other cases we have the *intention* of the test
written down. And I don't think it would lead to the proliferation of
"config_error2.m", "config_error3.m".
https://github.com/llvm/llvm-project/pull/83641
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Diagnose config_macros before building modules (PR #83641)
@@ -2006,6 +2021,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(
vsapsai wrote:
Thanks for checking this case!
https://github.com/llvm/llvm-project/pull/83641
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Diagnose config_macros before building modules (PR #83641)
https://github.com/vsapsai approved this pull request. https://github.com/llvm/llvm-project/pull/83641 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Diagnose config_macros before building modules (PR #83641)
https://github.com/vsapsai edited https://github.com/llvm/llvm-project/pull/83641 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Diagnose config_macros before building modules (PR #83641)
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
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 4443073775..ec4e68209d657d 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -1591,6 +1591,14 @@ static void checkConfigMacro(Preprocessor , StringRef
ConfigMacro,
}
}
+static void checkConfigMacros(Preprocessor , 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..00
--- 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
[clang] [clang] Diagnose config_macros before building modules (PR #83641)
@@ -2006,6 +2021,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(
Bigcheese wrote:
Yeah, given how `Preprocessor::HandleHeaderIncludeOrImport` works, you will
never hit this case.
https://github.com/llvm/llvm-project/pull/83641
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Diagnose config_macros before building modules (PR #83641)
@@ -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';}}
Bigcheese wrote:
There were already other tests that check the exact wording, intent here was
just to make it easier to reword if ever needed, as this specific test doesn't
care about the exact wording.
https://github.com/llvm/llvm-project/pull/83641
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Diagnose config_macros before building modules (PR #83641)
@@ -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
Bigcheese wrote:
Not needed, that was left over from when I tried to use a single file for all
the cases.
https://github.com/llvm/llvm-project/pull/83641
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Diagnose config_macros before building modules (PR #83641)
@@ -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
Bigcheese wrote:
I can just add comments explaining what it's testing above the run line.
https://github.com/llvm/llvm-project/pull/83641
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Diagnose config_macros before building modules (PR #83641)
@@ -1591,6 +1591,14 @@ static void checkConfigMacro(Preprocessor , StringRef
ConfigMacro,
}
}
+static void checkConfigMacros(Preprocessor , Module *M,
+ SourceLocation ImportLoc) {
+ clang::Module *TopModule = M->getTopLevelModule();
+ for (unsigned I = 0, N = TopModule->ConfigMacros.size(); I != N; ++I) {
dwblaikie wrote:
range based for loop?
https://github.com/llvm/llvm-project/pull/83641
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Diagnose config_macros before building modules (PR #83641)
@@ -2006,6 +2021,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(
Bigcheese wrote:
Hmm, I assumed it wasn't needed because this is just `-fmodule-name=` which
causes textual inclusion, but it looks like the original code would warn in
that case, even though they actually would apply if it was the first inclusion.
I'll check and see if it matters.
https://github.com/llvm/llvm-project/pull/83641
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Diagnose config_macros before building modules (PR #83641)
@@ -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
vsapsai wrote:
I was thinking about having a more descriptive name because it's not the only
way to get an error. Does "config_macro_required_not_on_command_line.m" capture
the purpose of the test?
https://github.com/llvm/llvm-project/pull/83641
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Diagnose config_macros before building modules (PR #83641)
@@ -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];
vsapsai wrote:
Don't think it is worth verifying in the test but we have a diagnostic
```
error: use of undeclared identifier 'SOME_VALUE'
```
which is still useful when we have no corresponding macro defined anywhere.
https://github.com/llvm/llvm-project/pull/83641
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Diagnose config_macros before building modules (PR #83641)
@@ -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';}}
vsapsai wrote:
Not sure if it is worth testing the part `pass '-DSOME_VALUE=...' on the
command line to configure the module` but can confirm there is an actionable
recommendation here.
https://github.com/llvm/llvm-project/pull/83641
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Diagnose config_macros before building modules (PR #83641)
@@ -2006,6 +2021,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(
vsapsai wrote:
Do we need to `checkConfigMacros` for a module in this case? It is that we have
```
if
check
else if
no check <- comment about this block
else
check in the called method
```
Don't know the code good enough to tell if it is required or not, just noticed
an inconsistency.
https://github.com/llvm/llvm-project/pull/83641
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Diagnose config_macros before building modules (PR #83641)
@@ -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
vsapsai wrote:
What is the point of wrapping everything in `TEST_ERROR`?
https://github.com/llvm/llvm-project/pull/83641
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Diagnose config_macros before building modules (PR #83641)
https://github.com/Bigcheese updated
https://github.com/llvm/llvm-project/pull/83641
>From c2445d426e374592522ac392254c9909ab52fc40 Mon Sep 17 00:00:00 2001
From: Michael Spencer
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| 29 ++
clang/test/Modules/Inputs/config.h | 7
clang/test/Modules/Inputs/module.modulemap | 5 ---
clang/test/Modules/config_macros.m | 45 --
4 files changed, 63 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 4443073775..378f940d8da2cf 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -1591,6 +1591,14 @@ static void checkConfigMacro(Preprocessor , StringRef
ConfigMacro,
}
}
+static void checkConfigMacros(Preprocessor , 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,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,6 +2021,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 +2166,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..00
--- 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
[clang] [clang] Diagnose config_macros before building modules (PR #83641)
llvmbot wrote:
@llvm/pr-subscribers-clang-modules
Author: Michael Spencer (Bigcheese)
Changes
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
---
Full diff: https://github.com/llvm/llvm-project/pull/83641.diff
4 Files Affected:
- (modified) clang/lib/Frontend/CompilerInstance.cpp (+20-8)
- (removed) clang/test/Modules/Inputs/config.h (-7)
- (modified) clang/test/Modules/Inputs/module.modulemap (-5)
- (modified) clang/test/Modules/config_macros.m (+42-3)
``diff
diff --git a/clang/lib/Frontend/CompilerInstance.cpp
b/clang/lib/Frontend/CompilerInstance.cpp
index 4443073775..14fd140f03cc36 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -1591,6 +1591,14 @@ static void checkConfigMacro(Preprocessor , StringRef
ConfigMacro,
}
}
+static void checkConfigMacros(Preprocessor , 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..00
--- 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
[clang] [clang] Diagnose config_macros before building modules (PR #83641)
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: Michael Spencer (Bigcheese)
Changes
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
---
Full diff: https://github.com/llvm/llvm-project/pull/83641.diff
4 Files Affected:
- (modified) clang/lib/Frontend/CompilerInstance.cpp (+20-8)
- (removed) clang/test/Modules/Inputs/config.h (-7)
- (modified) clang/test/Modules/Inputs/module.modulemap (-5)
- (modified) clang/test/Modules/config_macros.m (+42-3)
``diff
diff --git a/clang/lib/Frontend/CompilerInstance.cpp
b/clang/lib/Frontend/CompilerInstance.cpp
index 4443073775..14fd140f03cc36 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -1591,6 +1591,14 @@ static void checkConfigMacro(Preprocessor , StringRef
ConfigMacro,
}
}
+static void checkConfigMacros(Preprocessor , 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..00
--- 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
[clang] [clang] Diagnose config_macros before building modules (PR #83641)
https://github.com/Bigcheese created
https://github.com/llvm/llvm-project/pull/83641
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
>From 4edc58151460ae21baa312a91aa5a7955857c8a5 Mon Sep 17 00:00:00 2001
From: Michael Spencer
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| 28 ++
clang/test/Modules/Inputs/config.h | 7
clang/test/Modules/Inputs/module.modulemap | 5 ---
clang/test/Modules/config_macros.m | 45 --
4 files changed, 62 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 4443073775..14fd140f03cc36 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -1591,6 +1591,14 @@ static void checkConfigMacro(Preprocessor , StringRef
ConfigMacro,
}
}
+static void checkConfigMacros(Preprocessor , 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..00
--- 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
---
