Don't assert when redefining a built-in macro in a PCH, PR29119

[thakis]

Summary

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.

[rnk]

Fix seems reasonable.

lib/Lex/PPMacroExpansion.cpp

115

"single" entry

test/PCH/builtin-macro.c

29

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;?

[thakis]

comments

[thakis]

Thanks!

test/PCH/builtin-macro.c

29

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.

[rsmith]

lib/Lex/PPMacroExpansion.cpp

110–112

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...

[thakis]

update a comment

lib/Lex/PPMacroExpansion.cpp

110–112

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"
        ^
<built-in>: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 <module-includes>:1:
mod/foo.h:1:9: warning: '__STDC_HOSTED__' macro redefined
#define __STDC_HOSTED__ "hi"
        ^
<built-in>: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"
        ^
<built-in>: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
<built-in>: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 included from <built-in>:311:
<command line>:1:9: warning: '__STDC_HOSTED__' macro redefined
#define __STDC_HOSTED__ 4
        ^
<built-in>:307:9: note: previous definition is here
#define __STDC_HOSTED__ 1
        ^
In file included from <built-in>:1:
./foo.h:1:9: warning: '__STDC_HOSTED__' macro redefined
#define __STDC_HOSTED__ "hi"
        ^
<command line>:1:9: note: previous definition is here
#define __STDC_HOSTED__ 4
        ^
2 warnings generated.

I filed PR31324 for that.

(Also found an unrelated crasher, PR31326)

[thakis]

(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.)

[rnk]

lgtm

[thakis]

r289228, thanks!

include/clang/Lex/Preprocessor.h

DefMacroDirective *appendDefMacroDirective(IdentifierInfo *II, MacroInfo *MI,
     appendMacroDirective(II, MD);
     return MD;
   }
   DefMacroDirective *appendDefMacroDirective(IdentifierInfo *II,
                                              MacroInfo *MI) {
     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,
                               ArrayRef<ModuleMacro *> Overrides, bool &IsNew);
   ModuleMacro *getModuleMacro(Module *Mod, IdentifierInfo *II);
 
   /// \brief Get the list of leaf (non-overridden) module macros for a name.
   ArrayRef<ModuleMacro*> getLeafModuleMacros(const IdentifierInfo *II) const {

lib/Lex/PPMacroExpansion.cpp

void Preprocessor::appendMacroDirective(IdentifierInfo *II, MacroDirective *MD){
   II->setHasMacroDefinition(true);
   if (!MD->isDefined() && LeafModuleMacros.find(II) == LeafModuleMacros.end())
     II->setHasMacroDefinition(false);
   if (II->isFromAST())
     II->setChangedSinceDeserialization();
 }
 
 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");
+
+  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())
     II->setHasMacroDefinition(false);
 }
 
 ModuleMacro *Preprocessor::addModuleMacro(Module *Mod, IdentifierInfo *II,
                                           MacroInfo *Macro,

lib/Serialization/ASTReader.cpp

while (Idx < N) {
     if (!Latest)
       Latest = MD;
     if (Earliest)
       Earliest->setPrevious(MD);
     Earliest = MD;
   }
 
   if (Latest)
-    PP.setLoadedMacroDirective(II, Latest);
+    PP.setLoadedMacroDirective(II, Earliest, Latest);
 }
 
 ASTReader::InputFileInfo
 ASTReader::readInputFileInfo(ModuleFile &F, unsigned ID) {
   // Go find this input file.
   BitstreamCursor &Cursor = F.InputFilesCursor;
   SavedStreamPosition SavedPosition(Cursor);
   Cursor.JumpToBit(F.InputFileOffsets[ID-1]);

test/PCH/builtin-macro.c

+
+// 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