llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-modules Author: Qiongsi Wu (qiongsiwu) <details> <summary>Changes</summary> …ule with config macros (#<!-- -->174034) The revised PR now has a test case that works well on `swift/next`. The test now passes absolute paths to `-include-pch`. 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 things: 1. The logic that checks for config macros is not aware of any command line macros passed through the PCH ([here](https://github.com/llvm/llvm-project/blob/7976ac990000a58a7474269a3ca95e16aed8c35b/clang/lib/Frontend/CompilerInstance.cpp#L1562)). 2. `clang` _replaces_ the predefined macros on the command line with the predefined macros from the PCH, which does not include any builtins ([here](https://github.com/llvm/llvm-project/blob/7976ac990000a58a7474269a3ca95e16aed8c35b/clang/lib/Frontend/CompilerInstance.cpp#L679)). 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. The config macro itself works fine, and it is only the error check that needs fixing. rdar://95261458 (cherry picked from commit 71925cbdf80d4fc88ef844e726f89aa71c64bceb) --- Full diff: https://github.com/llvm/llvm-project/pull/177078.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 (+29) - (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 5adc45a19ca79..7bd4f253d97e3 100644 --- a/clang/include/clang/Lex/Preprocessor.h +++ b/clang/include/clang/Lex/Preprocessor.h @@ -1180,6 +1180,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; @@ -1379,6 +1383,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 { + 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 bb5230b4d22f6..0f94e60bd3bf9 100644 --- a/clang/lib/Frontend/CompilerInstance.cpp +++ b/clang/lib/Frontend/CompilerInstance.cpp @@ -1563,7 +1563,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..09cf778938427 100644 --- a/clang/lib/Lex/PPMacroExpansion.cpp +++ b/clang/lib/Lex/PPMacroExpansion.cpp @@ -125,6 +125,35 @@ 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(); + + if (SourceMgr.isWrittenInCommandLineFile(MDLoc)) { + 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..67a63dbe1286e --- /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 %t/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 %t/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(); +} `````````` </details> https://github.com/llvm/llvm-project/pull/177078 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
