[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 &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..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 { hea
[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 &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) { 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 &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,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 -DWA
[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 &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..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
[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 &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..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 -fm
[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 &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..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/Mod