[PATCH] D27545: Don't assert when redefining a built-in macro in a PCH, PR29119

[Nico Weber via Phabricator via cfe-commits]

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

[PATCH] D27545: Don't assert when redefining a built-in macro in a PCH, PR29119

[Reid Kleckner via Phabricator via cfe-commits]

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

[Nico Weber via Phabricator via cfe-commits]

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

[Nico Weber via Phabricator via cfe-commits]

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

[Nico Weber via Phabricator via cfe-commits]

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 

Re: [PATCH] D27545: Don't assert when redefining a built-in macro in a PCH, PR29119

[Nico Weber via cfe-commits]

(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

[PATCH] D27545: Don't assert when redefining a built-in macro in a PCH, PR29119

[Richard Smith via Phabricator via cfe-commits]

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

[Nico Weber via Phabricator via cfe-commits]

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

[Nico Weber via Phabricator via cfe-commits]

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

[Reid Kleckner via Phabricator via cfe-commits]

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

[Nico Weber via Phabricator via cfe-commits]

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  = 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 and