llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Qiongsi Wu (qiongsiwu) <details> <summary>Changes</summary> When a PCH is compiled with macro definitions on the command line, such as `-DCONFIG1`, an unexpected warning can occur if the macro definitions happen to belong to an imported module's config macros. The warning may look like the following: ``` definition of configuration macro 'CONFIG1' has no effect on the import of 'Mod1'; pass '-DCONFIG1=...' on the command line to configure the module ``` while `-DCONFIG1` is clearly on the command line when `clang` compiles the source that uses the PCH and the module. The reason this can happen is a combination of two actions: 1. The logic that checks for config macros is not aware of any macros passed through the PCH. 2. `clang` _replaces_ the predefined macros on the command line with the predefined macros from the PCH, which does not include any builtins. This PR teaches the preprocessor to recognize the command line macro definitions passed transitively through the PCH, so that the error check does not miss these definitions by mistake. --- Full diff: https://github.com/llvm/llvm-project/pull/174034.diff 6 Files Affected: - (modified) clang/include/clang/Lex/Preprocessor.h (+11) - (modified) clang/lib/Frontend/CompilerInstance.cpp (+3-1) - (modified) clang/lib/Lex/PPMacroExpansion.cpp (+32) - (added) clang/test/Modules/Inputs/pch-config-macros/include/Mod1.h (+5) - (added) clang/test/Modules/Inputs/pch-config-macros/include/module.modulemap (+4) - (added) clang/test/Modules/pch-config-macros.c (+48) ``````````diff diff --git a/clang/include/clang/Lex/Preprocessor.h b/clang/include/clang/Lex/Preprocessor.h index b1c648e647f41..ecd1585830f46 100644 --- a/clang/include/clang/Lex/Preprocessor.h +++ b/clang/include/clang/Lex/Preprocessor.h @@ -1119,6 +1119,10 @@ class Preprocessor { /// The file ID for the PCH through header. FileID PCHThroughHeaderFileID; + /// The file ID for the predefines that come from the PCH. + /// This is only set when modules are in effect. + FileID PCHPredefinesFileID; + /// Whether tokens are being skipped until a #pragma hdrstop is seen. bool SkippingUntilPragmaHdrStop = false; @@ -1315,6 +1319,13 @@ class Preprocessor { /// Returns the FileID for the preprocessor predefines. FileID getPredefinesFileID() const { return PredefinesFileID; } + /// Returns the FileID for the predefines loaded from the PCH. + FileID getPCHPredefinesFileID(/*const IdentifierInfo *II*/) const { + assert(getLangOpts().Modules && + "PCHPredefinedFileID is only set when modules is in effect!"); + return PCHPredefinesFileID; + } + /// \{ /// Accessors for preprocessor callbacks. /// diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp index f7f0ea3317932..ac27ae692b2a2 100644 --- a/clang/lib/Frontend/CompilerInstance.cpp +++ b/clang/lib/Frontend/CompilerInstance.cpp @@ -1548,7 +1548,9 @@ static void checkConfigMacro(Preprocessor &PP, StringRef ConfigMacro, for (auto *MD = LatestLocalMD; MD; MD = MD->getPrevious()) { // We only care about the predefines buffer. FileID FID = SourceMgr.getFileID(MD->getLocation()); - if (FID.isInvalid() || FID != PP.getPredefinesFileID()) + if (FID.isInvalid()) + continue; + if (FID != PP.getPredefinesFileID() && FID != PP.getPCHPredefinesFileID()) continue; if (auto *DMD = dyn_cast<DefMacroDirective>(MD)) CmdLineDefinition = DMD->getMacroInfo(); diff --git a/clang/lib/Lex/PPMacroExpansion.cpp b/clang/lib/Lex/PPMacroExpansion.cpp index 5efa4b5b3f872..6b6ef9fe9f3ed 100644 --- a/clang/lib/Lex/PPMacroExpansion.cpp +++ b/clang/lib/Lex/PPMacroExpansion.cpp @@ -125,6 +125,38 @@ void Preprocessor::setLoadedMacroDirective(IdentifierInfo *II, II->setHasMacroDefinition(true); if (!MD->isDefined() && !LeafModuleMacros.contains(II)) II->setHasMacroDefinition(false); + + if (getLangOpts().Modules) { + // When both modules and a PCH are used, we may run into the following + // situation: + // - the PCH is compiled with macro definitions on the command line. + // - the modules are compiled with the same set of macros on the command + // line. + // In this case, clang needs to know that some predefined macros exist + // over the command line transitively through the PCH and some are passed + // directly over the command line. The preprocessor stores + // PCHPredefinesFileID so later it is aware of macros defined transitively + // through the PCH's compilation. + auto MDLoc = MD->getLocation(); + + // The PCH loads the command line macros into the <built-in> buffer. + // This is not the same as SourceMgr.isWrittenInBuiltinFile(MDLoc), + // since we are not looking at the Presumed location. + if (SourceMgr.getBufferName(MDLoc) == "<built-in>") { + auto MDFileID = SourceMgr.getFileID(MDLoc); + if (PCHPredefinesFileID.isInvalid()) + PCHPredefinesFileID = MDFileID; + else { + // The PCH and all the chain of headers it includes must be + // compiled with the exact same set of macros defined over the + // command line. No different macros should be defined over + // different command line invocations. This means that all the macros' + // source locations should have the same MDFileID. + assert(MDFileID == PCHPredefinesFileID && + "PCHBuiltinFileID must be consistent!"); + } + } + } } ModuleMacro *Preprocessor::addModuleMacro(Module *Mod, IdentifierInfo *II, diff --git a/clang/test/Modules/Inputs/pch-config-macros/include/Mod1.h b/clang/test/Modules/Inputs/pch-config-macros/include/Mod1.h new file mode 100644 index 0000000000000..3b8f33877dcd2 --- /dev/null +++ b/clang/test/Modules/Inputs/pch-config-macros/include/Mod1.h @@ -0,0 +1,5 @@ +#if CONFIG1 +int foo() { return 42; } +#else +int foo() { return 43; } +#endif diff --git a/clang/test/Modules/Inputs/pch-config-macros/include/module.modulemap b/clang/test/Modules/Inputs/pch-config-macros/include/module.modulemap new file mode 100644 index 0000000000000..9d5f2e714ec88 --- /dev/null +++ b/clang/test/Modules/Inputs/pch-config-macros/include/module.modulemap @@ -0,0 +1,4 @@ +module Mod1 { + header "Mod1.h" + config_macros CONFIG1,CONFIG2 +} diff --git a/clang/test/Modules/pch-config-macros.c b/clang/test/Modules/pch-config-macros.c new file mode 100644 index 0000000000000..129c0451741d6 --- /dev/null +++ b/clang/test/Modules/pch-config-macros.c @@ -0,0 +1,48 @@ +// RUN: rm -rf %t +// RUN: split-file %s %t +// RUN: cd %t + +// This test builds two PCHs. bridging.h.pch depends on h1.h.pch. +// Then the test uses bridiging.h.pch in a source file that imports +// a module with config macros. +// This is a normal use case and no warnings should be issued. +// RUN: %clang_cc1 -fmodules \ +// RUN: -fmodule-map-file=%S/Inputs/pch-config-macros/include/module.modulemap \ +// RUN: -fmodules-cache-path=%t/cache -I %S/Inputs/pch-config-macros/include \ +// RUN: h1.h -emit-pch -o h1.h.pch -DCONFIG1 -DCONFIG2 +// RUN: %clang_cc1 -fmodules \ +// RUN: -fmodule-map-file=%S/Inputs/pch-config-macros/include/module.modulemap \ +// RUN: -fmodules-cache-path=%t/cache -I %S/Inputs/pch-config-macros/include \ +// RUN: -include-pch h1.h.pch bridging.h -emit-pch -o bridging.h.pch \ +// RUN: -DCONFIG1 -DCONFIG2 +// RUN: %clang_cc1 -fmodules \ +// RUN: -fmodule-map-file=%S/Inputs/pch-config-macros/include/module.modulemap \ +// RUN: -fmodules-cache-path=%t/cache -I %S/Inputs/pch-config-macros/include \ +// RUN: -emit-obj -o main.o main.c -include-pch bridging.h.pch \ +// RUN: -DCONFIG1 -DCONFIG2 -verify + +//--- h1.h +#if CONFIG1 +int bar1() { return 42; } +#else +int bar2() { return 43; } +#endif + +//--- bridging.h +#if CONFIG1 +int bar() { return bar1(); } +#else +int bar() { return bar2(); } +#endif + +#if CONFIG2 +int baz() { return 77; } +#endif + +//--- main.c +#include "Mod1.h" +// expected-no-diagnostics + +int main_func() { + return foo() + bar(); +} \ No newline at end of file `````````` </details> https://github.com/llvm/llvm-project/pull/174034 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
