[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"
        ^
<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)


https://reviews.llvm.org/D27545