[PATCH] D27545: Don't assert when redefining a built-in macro in a PCH, PR29119
thakis created this revision. thakis added a reviewer: rnk. thakis added subscribers: cfe-commits, rsmith. PCH files store the macro history for a given macro, and the whole history list for one identifier is given to the Preprocessor at once via Preprocessor::setLoadedMacroDirective(). This contained an assert that no macro history exists yet for that identifier. That's usually true, but it's not true for builtin macros, which are created in Preprocessor() before flags and pchs are processed. Luckily, ASTWriter stops writing macro history lists at builtins (see shouldIgnoreMacro() in ASTWriter.cpp), so the head of the history list was missing for builtin macros. So make the assert weaker, and splice the history list to the existing single define for builtins. https://reviews.llvm.org/D27545 Files: include/clang/Lex/Preprocessor.h lib/Lex/PPMacroExpansion.cpp lib/Serialization/ASTReader.cpp test/PCH/builtin-macro.c Index: test/PCH/builtin-macro.c === --- test/PCH/builtin-macro.c +++ test/PCH/builtin-macro.c @@ -0,0 +1,33 @@ + +// Test this without pch. +// RUN: %clang_cc1 -D__DATE__= -D__TIMESTAMP__= -include %s -Wno-builtin-macro-redefined -fsyntax-only -verify %s + +// Test with pch. +// RUN: %clang_cc1 -D__DATE__= -D__TIMESTAMP__= -Wno-builtin-macro-redefined -emit-pch -o %t %s +// RUN: %clang_cc1 -D__DATE__= -D__TIMESTAMP__= -Wno-builtin-macro-redefined -include-pch %t -fsyntax-only -verify %s + +#if !defined(HEADER) +#define HEADER + +#define __TIME__ + +#undef __TIMESTAMP__ +#define __TIMESTAMP__ + +// FIXME: undefs don't work well with pchs yet, see PR31311 +// Once that's fixed, add -U__COUNTER__ to all command lines and check that +// an attempt to use __COUNTER__ at the bottom produces an error in both non-pch +// and pch case (works fine in the former case already). +// Same for #undef __FILE__ right here and a use of that at the bottom. +//#undef __FILE__ + +// Also spot-check a predefine +#undef __STDC_HOSTED__ + +#else + +const char s[] = __DATE__ " " __TIME__ " " __TIMESTAMP__; + +const int d = __STDC_HOSTED__; // expected-error{{use of undeclared identifier '__STDC_HOSTED__'}} + +#endif Index: lib/Serialization/ASTReader.cpp === --- lib/Serialization/ASTReader.cpp +++ lib/Serialization/ASTReader.cpp @@ -1953,7 +1953,7 @@ } if (Latest) -PP.setLoadedMacroDirective(II, Latest); +PP.setLoadedMacroDirective(II, Earliest, Latest); } ASTReader::InputFileInfo Index: lib/Lex/PPMacroExpansion.cpp === --- lib/Lex/PPMacroExpansion.cpp +++ lib/Lex/PPMacroExpansion.cpp @@ -92,12 +92,33 @@ } void Preprocessor::setLoadedMacroDirective(IdentifierInfo *II, + MacroDirective *ED, MacroDirective *MD) { + // Normally, when a macro is defined, it goes through appendMacroDirective() + // above, which chains a macro to previous defines, undefs, etc. + // However, in a pch, the whole macro history up to the end of the pch is + // stored, so ASTReader goes through this function instead. + // However, built-in macros are already registered in the Preprocessor + // ctor, and ASTWriter stops writing the macro chain at built-in macros, + // so in that case the chain from the pch needs to be spliced to the existing + // built-in. + assert(II && MD); MacroState &StoredMD = CurSubmoduleState->Macros[II]; - assert(!StoredMD.getLatest() && - "the macro history was modified before initializing it from a pch"); - StoredMD = MD; + + if (auto *OldMD = StoredMD.getLatest()) { +// FIXME: shouldIgnoreMacro() in ASTWriter also stops at macros from the +// predefines buffer in module builds. Do we need to splice to those here +// too? +assert(OldMD->getMacroInfo()->isBuiltinMacro() && + "only built-ins should have an entry here"); +assert(!OldMD->getPrevious() && "builtin should only have a singe entry"); +ED->setPrevious(OldMD); +StoredMD.setLatest(MD); + } else { +StoredMD = MD; + } + // Setup the identifier as having associated macro history. II->setHasMacroDefinition(true); if (!MD->isDefined() && LeafModuleMacros.find(II) == LeafModuleMacros.end()) Index: include/clang/Lex/Preprocessor.h === --- include/clang/Lex/Preprocessor.h +++ include/clang/Lex/Preprocessor.h @@ -887,7 +887,8 @@ return appendDefMacroDirective(II, MI, MI->getDefinitionLoc()); } /// \brief Set a MacroDirective that was loaded from a PCH file. - void setLoadedMacroDirective(IdentifierInfo *II, MacroDirective *MD); + void setLoadedMacroDirective(IdentifierInfo *II, MacroDirective *ED, + MacroDirective *MD); /// \brief Register an exported macro for a module
[PATCH] D27545: Don't assert when redefining a built-in macro in a PCH, PR29119
rnk added a comment. Fix seems reasonable. Comment at: lib/Lex/PPMacroExpansion.cpp:115 + "only built-ins should have an entry here"); +assert(!OldMD->getPrevious() && "builtin should only have a singe entry"); +ED->setPrevious(OldMD); "single" entry Comment at: test/PCH/builtin-macro.c:29 + +const char s[] = __DATE__ " " __TIME__ " " __TIMESTAMP__; + This test doesn't seem to fail if `__DATE__` expands to something. I removed `-D__DATE__=` from the command line and it passes. Can we find a way to make it fail? `int x = __DATE__ 42;`? https://reviews.llvm.org/D27545 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27545: Don't assert when redefining a built-in macro in a PCH, PR29119
thakis updated this revision to Diff 80687. thakis marked 2 inline comments as done. thakis added a comment. comments https://reviews.llvm.org/D27545 Files: include/clang/Lex/Preprocessor.h lib/Lex/PPMacroExpansion.cpp lib/Serialization/ASTReader.cpp test/PCH/builtin-macro.c Index: test/PCH/builtin-macro.c === --- test/PCH/builtin-macro.c +++ test/PCH/builtin-macro.c @@ -0,0 +1,35 @@ +// Test this without pch. +// RUN: %clang_cc1 -D__DATE__= -D__TIMESTAMP__= -include %s -Wno-builtin-macro-redefined -fsyntax-only -verify %s + +// Test with pch. +// RUN: %clang_cc1 -D__DATE__= -D__TIMESTAMP__= -Wno-builtin-macro-redefined -emit-pch -o %t %s +// RUN: %clang_cc1 -D__DATE__= -D__TIMESTAMP__= -Wno-builtin-macro-redefined -include-pch %t -fsyntax-only -verify %s + +#if !defined(HEADER) +#define HEADER + +#define __TIME__ + +#undef __TIMESTAMP__ +#define __TIMESTAMP__ + +// FIXME: undefs don't work well with pchs yet, see PR31311 +// Once that's fixed, add -U__COUNTER__ to all command lines and check that +// an attempt to use __COUNTER__ at the bottom produces an error in both non-pch +// and pch case (works fine in the former case already). +// Same for #undef __FILE__ right here and a use of that at the bottom. +//#undef __FILE__ + +// Also spot-check a predefine +#undef __STDC_HOSTED__ + +#else + +const char s[] = __DATE__ " " __TIME__ " " __TIMESTAMP__; + +// Check that we pick up __DATE__ from the -D flag: +int i = __DATE__ 4; + +const int d = __STDC_HOSTED__; // expected-error{{use of undeclared identifier '__STDC_HOSTED__'}} + +#endif Index: lib/Serialization/ASTReader.cpp === --- lib/Serialization/ASTReader.cpp +++ lib/Serialization/ASTReader.cpp @@ -1953,7 +1953,7 @@ } if (Latest) -PP.setLoadedMacroDirective(II, Latest); +PP.setLoadedMacroDirective(II, Earliest, Latest); } ASTReader::InputFileInfo Index: lib/Lex/PPMacroExpansion.cpp === --- lib/Lex/PPMacroExpansion.cpp +++ lib/Lex/PPMacroExpansion.cpp @@ -92,12 +92,33 @@ } void Preprocessor::setLoadedMacroDirective(IdentifierInfo *II, + MacroDirective *ED, MacroDirective *MD) { + // Normally, when a macro is defined, it goes through appendMacroDirective() + // above, which chains a macro to previous defines, undefs, etc. + // However, in a pch, the whole macro history up to the end of the pch is + // stored, so ASTReader goes through this function instead. + // However, built-in macros are already registered in the Preprocessor + // ctor, and ASTWriter stops writing the macro chain at built-in macros, + // so in that case the chain from the pch needs to be spliced to the existing + // built-in. + assert(II && MD); MacroState &StoredMD = CurSubmoduleState->Macros[II]; - assert(!StoredMD.getLatest() && - "the macro history was modified before initializing it from a pch"); - StoredMD = MD; + + if (auto *OldMD = StoredMD.getLatest()) { +// FIXME: shouldIgnoreMacro() in ASTWriter also stops at macros from the +// predefines buffer in module builds. Do we need to splice to those here +// too? +assert(OldMD->getMacroInfo()->isBuiltinMacro() && + "only built-ins should have an entry here"); +assert(!OldMD->getPrevious() && "builtin should only have a single entry"); +ED->setPrevious(OldMD); +StoredMD.setLatest(MD); + } else { +StoredMD = MD; + } + // Setup the identifier as having associated macro history. II->setHasMacroDefinition(true); if (!MD->isDefined() && LeafModuleMacros.find(II) == LeafModuleMacros.end()) Index: include/clang/Lex/Preprocessor.h === --- include/clang/Lex/Preprocessor.h +++ include/clang/Lex/Preprocessor.h @@ -887,7 +887,8 @@ return appendDefMacroDirective(II, MI, MI->getDefinitionLoc()); } /// \brief Set a MacroDirective that was loaded from a PCH file. - void setLoadedMacroDirective(IdentifierInfo *II, MacroDirective *MD); + void setLoadedMacroDirective(IdentifierInfo *II, MacroDirective *ED, + MacroDirective *MD); /// \brief Register an exported macro for a module and identifier. ModuleMacro *addModuleMacro(Module *Mod, IdentifierInfo *II, MacroInfo *Macro, ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27545: Don't assert when redefining a built-in macro in a PCH, PR29119
thakis added a comment. Thanks! Comment at: test/PCH/builtin-macro.c:29 + +const char s[] = __DATE__ " " __TIME__ " " __TIMESTAMP__; + rnk wrote: > This test doesn't seem to fail if `__DATE__` expands to something. I removed > `-D__DATE__=` from the command line and it passes. Can we find a way to make > it fail? `int x = __DATE__ 42;`? Oh, you mean to check that the __DATE__ value from -D is used, instead of the value of the builtin? Your suggestion works, good idea. https://reviews.llvm.org/D27545 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27545: Don't assert when redefining a built-in macro in a PCH, PR29119
rsmith added inline comments. Comment at: lib/Lex/PPMacroExpansion.cpp:110-112 +// FIXME: shouldIgnoreMacro() in ASTWriter also stops at macros from the +// predefines buffer in module builds. Do we need to splice to those here +// too? If I remember correctly, we shouldn't need to; we run this step before we start lexing the predefines buffer. However, that does mean that macros on the command line will *override* macros from the PCH, which seems like it's probably the wrong behavior... https://reviews.llvm.org/D27545 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27545: Don't assert when redefining a built-in macro in a PCH, PR29119
thakis added inline comments. Comment at: lib/Lex/PPMacroExpansion.cpp:110-112 +// FIXME: shouldIgnoreMacro() in ASTWriter also stops at macros from the +// predefines buffer in module builds. Do we need to splice to those here +// too? rsmith wrote: > If I remember correctly, we shouldn't need to; we run this step before we > start lexing the predefines buffer. However, that does mean that macros on > the command line will *override* macros from the PCH, which seems like it's > probably the wrong behavior... TL;DR: You're right, updated comment, filed PR31324 and PR31326 for (pre-existing) bugs I found along the way. From what I understand, the order is: FrontendAction::BeginSourceFile() builds PCH reader 1. built-ins (Preprocessor() ctor) 2. predefines predefines (clang::InitializePreprocessor in Frontend) 3. commandline predefines (later in InitializePreprocessor; InitOpts.Macros loop) 4. pch source is attached after those are set (but before parsing starts) 5. macros from PCH deserialized on-demand, usually through Lexer::LexIdentifier -> Preprocessor::LookUpIdentifierInfo -> ASTReader::get(llvm::StringRef) -> ~Deserializing RAII dtor -> FinishedDeserializing -> finishPendingActions -> resolvePendingMacro Aha, you're saying that when the preamble is parsed, when the preprocessor sees `#define __STD_HOSTED__ 1` from the preamble, it'll then read the history for `__STD_HOSTED__`, call this, and then…override it with the default value from the preamble? Doesn't that means that even the default values override the pch? Hm, no, this seems to work: ``` $ cat foo.h #define __STDC_HOSTED__ "hi" $ cat foo.c const char* s = __STDC_HOSTED__; $ bin/clang -cc1 -emit-pch -o foo.h.pch foo.h setting predefines foo.h:1:9: warning: '__STDC_HOSTED__' macro redefined #define __STDC_HOSTED__ "hi" ^ :307:9: note: previous definition is here #define __STDC_HOSTED__ 1 ^ 1 warning generated. $ bin/clang -cc1 -include-pch foo.h.pch foo.c -emit-llvm -o - ; ModuleID = 'foo.c' source_filename = "foo.c" target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128" target triple = "x86_64-apple-darwin14.5.0" @.str = private unnamed_addr constant [3 x i8] c"hi\00", align 1 ``` …ah, but I tested with a pch, not a module, and with a module `__STD_HOSTED__` is probably supposed to expand to the "default" value even if it's defined in some used module. How do I do the same with a module instead of a pch? …apparently something like this: ``` $ cat mod/module.map module foo { header "foo.h" } $ cat mod/foo.h #define __STDC_HOSTED__ "hi" $ cat foo.c const char* s = __STDC_HOSTED__; $ bin/clang -cc1 -fmodules -fmodule-name=foo -emit-module mod/module.map -o a.pcm While building module 'foo': In file included from :1: mod/foo.h:1:9: warning: '__STDC_HOSTED__' macro redefined #define __STDC_HOSTED__ "hi" ^ :307:9: note: previous definition is here #define __STDC_HOSTED__ 1 ^ 1 warning generated. $ bin/clang -cc1 -fmodules -fmodule-file=a.pcm -emit-obj foo.c foo.c:1:13: warning: incompatible integer to pointer conversion initializing 'const char *' with an expression of type 'int' const char* s = __STDC_HOSTED__; ^ ~~~ 1 warning generated. ``` So yes, looks like this does get the default value there. (Is there some less wordy thing to compile and use a module on the command line for testing? Do I have to make a module.map file like I did? Requires quite a bit more typing than with a pch…) Aha, this comment in ASTReader::get() explains things: // We don't need to do identifier table lookups in C++ modules (we preload // all interesting declarations, and don't need to use the scope for name // lookups). Perform the lookup in PCH files, though, since we don't build // a complete initial identifier table if we're carrying on from a PCH. So in modules we preload everything and then override from the predefines. I updated the comment. You're right that commandline define flags override values from a pch: ``` $ cat foo.h #define __STDC_HOSTED__ "hi" $ cat foo.c const char* s = __STDC_HOSTED__; $ bin/clang -cc1 -emit-pch -o foo.h.pch foo.h foo.h:1:9: warning: '__STDC_HOSTED__' macro redefined #define __STDC_HOSTED__ "hi" ^ :307:9: note: previous definition is here #define __STDC_HOSTED__ 1 ^ 1 warning generated. $ bin/clang -cc1 -D__STDC_HOSTED__=4 -include-pch foo.h.pch -emit-obj -o foo.o foo.c :1:9: warning: '__STDC_HOSTED__' macro redefined #define __STDC_HOSTED__ 4 ^ /Users/thakis/src/llvm-build/foo.h:1:9: note: previous definition is here #define __STDC_HOSTED__ "hi" ^ foo.c:1:13: warning: incompatible integer to pointer conversion initializing 'const char *' with an expression of type 'int' const char* s = __STDC_HOSTED__; ^ ~~~ 2 warnings generated. $ bin/clang -cc1 -D__STDC_HOSTED__=4 -include foo.h -emit-obj -o foo.o foo.c In file inc
[PATCH] D27545: Don't assert when redefining a built-in macro in a PCH, PR29119
thakis updated this revision to Diff 80849. thakis added a comment. update a comment https://reviews.llvm.org/D27545 Files: include/clang/Lex/Preprocessor.h lib/Lex/PPMacroExpansion.cpp lib/Serialization/ASTReader.cpp test/PCH/builtin-macro.c Index: test/PCH/builtin-macro.c === --- test/PCH/builtin-macro.c +++ test/PCH/builtin-macro.c @@ -0,0 +1,35 @@ +// Test this without pch. +// RUN: %clang_cc1 -D__DATE__= -D__TIMESTAMP__= -include %s -Wno-builtin-macro-redefined -fsyntax-only -verify %s + +// Test with pch. +// RUN: %clang_cc1 -D__DATE__= -D__TIMESTAMP__= -Wno-builtin-macro-redefined -emit-pch -o %t %s +// RUN: %clang_cc1 -D__DATE__= -D__TIMESTAMP__= -Wno-builtin-macro-redefined -include-pch %t -fsyntax-only -verify %s + +#if !defined(HEADER) +#define HEADER + +#define __TIME__ + +#undef __TIMESTAMP__ +#define __TIMESTAMP__ + +// FIXME: undefs don't work well with pchs yet, see PR31311 +// Once that's fixed, add -U__COUNTER__ to all command lines and check that +// an attempt to use __COUNTER__ at the bottom produces an error in both non-pch +// and pch case (works fine in the former case already). +// Same for #undef __FILE__ right here and a use of that at the bottom. +//#undef __FILE__ + +// Also spot-check a predefine +#undef __STDC_HOSTED__ + +#else + +const char s[] = __DATE__ " " __TIME__ " " __TIMESTAMP__; + +// Check that we pick up __DATE__ from the -D flag: +int i = __DATE__ 4; + +const int d = __STDC_HOSTED__; // expected-error{{use of undeclared identifier '__STDC_HOSTED__'}} + +#endif Index: lib/Serialization/ASTReader.cpp === --- lib/Serialization/ASTReader.cpp +++ lib/Serialization/ASTReader.cpp @@ -1953,7 +1953,7 @@ } if (Latest) -PP.setLoadedMacroDirective(II, Latest); +PP.setLoadedMacroDirective(II, Earliest, Latest); } ASTReader::InputFileInfo Index: lib/Lex/PPMacroExpansion.cpp === --- lib/Lex/PPMacroExpansion.cpp +++ lib/Lex/PPMacroExpansion.cpp @@ -92,12 +92,35 @@ } void Preprocessor::setLoadedMacroDirective(IdentifierInfo *II, + MacroDirective *ED, MacroDirective *MD) { + // Normally, when a macro is defined, it goes through appendMacroDirective() + // above, which chains a macro to previous defines, undefs, etc. + // However, in a pch, the whole macro history up to the end of the pch is + // stored, so ASTReader goes through this function instead. + // However, built-in macros are already registered in the Preprocessor + // ctor, and ASTWriter stops writing the macro chain at built-in macros, + // so in that case the chain from the pch needs to be spliced to the existing + // built-in. + assert(II && MD); MacroState &StoredMD = CurSubmoduleState->Macros[II]; - assert(!StoredMD.getLatest() && - "the macro history was modified before initializing it from a pch"); - StoredMD = MD; + + if (auto *OldMD = StoredMD.getLatest()) { +// shouldIgnoreMacro() in ASTWriter also stops at macros from the +// predefines buffer in module builds. However, in module builds, modules +// are loaded completely before predefines are processed, so StoredMD +// will be nullptr for them when they're loaded. StoredMD should only be +// non-nullptr for builtins read from a pch file. +assert(OldMD->getMacroInfo()->isBuiltinMacro() && + "only built-ins should have an entry here"); +assert(!OldMD->getPrevious() && "builtin should only have a single entry"); +ED->setPrevious(OldMD); +StoredMD.setLatest(MD); + } else { +StoredMD = MD; + } + // Setup the identifier as having associated macro history. II->setHasMacroDefinition(true); if (!MD->isDefined() && LeafModuleMacros.find(II) == LeafModuleMacros.end()) Index: include/clang/Lex/Preprocessor.h === --- include/clang/Lex/Preprocessor.h +++ include/clang/Lex/Preprocessor.h @@ -887,7 +887,8 @@ return appendDefMacroDirective(II, MI, MI->getDefinitionLoc()); } /// \brief Set a MacroDirective that was loaded from a PCH file. - void setLoadedMacroDirective(IdentifierInfo *II, MacroDirective *MD); + void setLoadedMacroDirective(IdentifierInfo *II, MacroDirective *ED, + MacroDirective *MD); /// \brief Register an exported macro for a module and identifier. ModuleMacro *addModuleMacro(Module *Mod, IdentifierInfo *II, MacroInfo *Macro, ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27545: Don't assert when redefining a built-in macro in a PCH, PR29119
thakis added a comment. (The reason that the predefines buffer doesn't override the value of `__STDC_HOSTED__` from the pch is that CompilerInstance::createPCHExternalASTSource overrides the predefines buffer with a different predefines buffer suggested by the ASTReader, usually an empty one.) https://reviews.llvm.org/D27545 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27545: Don't assert when redefining a built-in macro in a PCH, PR29119
rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm https://reviews.llvm.org/D27545 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27545: Don't assert when redefining a built-in macro in a PCH, PR29119
thakis closed this revision. thakis added a comment. r289228, thanks! https://reviews.llvm.org/D27545 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D27545: Don't assert when redefining a built-in macro in a PCH, PR29119
(I replied to comments about 1h ago, looks like phab is not in the mood for sending a mail for that for some reason. I did use the "Leap into action" submit button.) On Wed, Dec 7, 2016 at 9:19 PM, Richard Smith via Phabricator < revi...@reviews.llvm.org> wrote: > rsmith added inline comments. > > > > Comment at: lib/Lex/PPMacroExpansion.cpp:110-112 > +// FIXME: shouldIgnoreMacro() in ASTWriter also stops at macros from > the > +// predefines buffer in module builds. Do we need to splice to those > here > +// too? > > If I remember correctly, we shouldn't need to; we run this step before we > start lexing the predefines buffer. However, that does mean that macros on > the command line will *override* macros from the PCH, which seems like it's > probably the wrong behavior... > > > https://reviews.llvm.org/D27545 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits