[GN]: Precompiled header support for GCC.

[GN]: Precompiled header support for GCC.

+ Adds `gcc` as an option for `precompiled_header_type`
+ Fixes a bug where build targets had superfluous pch deps.
+ GCH files are compiled using the `-x <header lang>` and used via the `-header` flag. Since the two are mutually exclusive and we didn’t want to add `-header` to every build target line, the global cflags_* vars contain `-include` and each pch build target includes its own copy of the cflags_* values while replacing `-include` with `-x <header lang>`

BUG=297681
Committed: https://crrev.com/1ae777a027b5243924f2b3ba774067a7bf5926cf
Cr-Commit-Position: refs/heads/master@{#349518}

[Bons, 2015-08-20 17:20:52]

andybons@chromium.org changed reviewers:
+ brettw@chromium.org, dpranke@chromium.org, mark@chromium.org,
thakis@chromium.org

[Bons, 2015-08-20 17:20:52]

WIP: Progress for GCC precompiled header support. Most importantly, I’d like to
know if this is the desired output (provided here but also in the added unit
test):
defines =
include_dirs =
cflags =
cflags_cc =
target_output_name = pch_target

build withpch/obj/build/pch_target.precompile.cc.o: cxx ../../build/precompile.h
  cflags_cc = ${cflags_cc} -x c++-header

build withpch/obj/foo/pch_target.input1.o: cxx ../../foo/input1.cc |
withpch/obj/build/pch_target.precompile.cc.o

build withpch/obj/foo/pch_target.stamp: stamp
withpch/obj/foo/pch_target.input1.o withpch/obj/build/pch_target.precompile.cc.o

Before I submit I also need to update the documentation and fix the TODO I
added.

[Mark Mentovai, 2015-08-20 20:52:03]

Take care: a precompiled header is normally only valid when created with the
same compiler options that the translation unit that includes it is compiled
with. (Some options don’t make a difference, but since you probably don’t want
to be in the business of deciding which do and which don’t, it’s safest to
require that they all be the same.)

I don’t know if it does or if it doesn’t, but if gn allows individual files to
specify their own compiler options, and those might differ from the options used
to build the precompiled header, then the results could be unexpected.

Otherwise, you’re basically looking at running the compiler on a header to
precompile it, and from that point on, you can inject it into subsequent
compilations with -include. Make sure that the precompiled header is rebuilt
when anything it depends on changes. ninja ought to give you this for free.

[brettw, 2015-08-21 05:09:31]

Sorry, I'm OOO Friday but will try to look at this soonish.

To answer Mark's question, all #defines and flags are per-target, not
per-source-file.

[Bons, 2015-08-21 17:29:43]

On 2015/08/21 05:09:31, brettw wrote:
> Sorry, I'm OOO Friday but will try to look at this soonish.
> 
> To answer Mark's question, all #defines and flags are per-target, not
> per-source-file.

Note that these are my current assumptions. Please correct me if I’m wrong:
+ The output file does not need to conform to the foo.h.gch naming convention
since it is explicitly specified as a dependency (as opposed to gcc looking for
bar.h.gch if it sees #include "bar.h").
+ The -include flag isn't needed based on a cursory glance of gyp's output,
which appears to override it in cflags_pch_* when GCC_PRECOMPILE_PREFIX_HEADER
is set to 1. – https://gist.github.com/andybons/01c3fdb91459681c0829

[brettw, 2015-08-24 19:36:05]

https://codereview.chromium.org/1292983004/diff/40001/tools/gn/ninja_binary_t...
File tools/gn/ninja_binary_target_writer.cc (right):

https://codereview.chromium.org/1292983004/diff/40001/tools/gn/ninja_binary_t...
tools/gn/ninja_binary_target_writer.cc:195: const char* lang_suffix =
GetPCHSuffixForToolType(tool_type);
I'm having a hard time trying to compute the intersection of what you're doing
and what the GCC docs say to do.

Can you paste an example ninja file generated by your code for me to look at?

I thought that the output name needed to be <header>.gch. From my reading of
this code, this will generate a .o file and link to it. On Windows you need .obj
files for precompiled headers, but in GCC you don't (to my knowledge).

[Bons, 2015-08-24 19:53:57]

https://codereview.chromium.org/1292983004/diff/40001/tools/gn/ninja_binary_t...
File tools/gn/ninja_binary_target_writer.cc (right):

https://codereview.chromium.org/1292983004/diff/40001/tools/gn/ninja_binary_t...
tools/gn/ninja_binary_target_writer.cc:195: const char* lang_suffix =
GetPCHSuffixForToolType(tool_type);
On 2015/08/24 19:36:05, brettw wrote:
> I'm having a hard time trying to compute the intersection of what you're doing
> and what the GCC docs say to do.
> 
> Can you paste an example ninja file generated by your code for me to look at?
defines =
include_dirs =
cflags =
cflags_cc =
target_output_name = pch_target

build withpch/obj/build/pch_target.precompile.cc.o: cxx ../../build/precompile.h
  cflags_cc = ${cflags_cc} -x c++-header

build withpch/obj/foo/pch_target.input1.o: cxx ../../foo/input1.cc |
withpch/obj/build/pch_target.precompile.cc.o

build withpch/obj/foo/pch_target.stamp: stamp
withpch/obj/foo/pch_target.input1.o withpch/obj/build/pch_target.precompile.cc.o

 
> I thought that the output name needed to be <header>.gch. From my reading of
> this code, this will generate a .o file and link to it. On Windows you need
.obj
> files for precompiled headers, but in GCC you don't (to my knowledge).
You’re correct. The output should be <header>.gch, not .o

[Nico, 2015-08-25 00:40:06]

On 2015/08/21 17:29:43, Bons wrote:
> On 2015/08/21 05:09:31, brettw wrote:
> > Sorry, I'm OOO Friday but will try to look at this soonish.
> > 
> > To answer Mark's question, all #defines and flags are per-target, not
> > per-source-file.
> 
> Note that these are my current assumptions. Please correct me if I’m wrong:
> + The output file does not need to conform to the foo.h.gch naming convention
> since it is explicitly specified as a dependency (as opposed to gcc looking
for
> bar.h.gch if it sees #include "bar.h").

This isn't correct. Dependencies are for ninja, but clang explicitly looks for
file.h.gch when processing a -include switch. (clang doesn't use gch files for
#includes.)

> + The -include flag isn't needed based on a cursory glance of gyp's output,
> which appears to override it in cflags_pch_* when GCC_PRECOMPILE_PREFIX_HEADER
> is set to 1. – https://gist.github.com/andybons/01c3fdb91459681c0829

The -include flag is needed.

[Nico, 2015-08-25 00:42:25]

I didn't look at the code in detail, but it seems to do roughly the right thing
:-) I'd like to look at a snippet of generated ninja code for this too.

[Bons, 2015-08-25 00:51:51]

On 2015/08/25 00:42:25, Nico wrote:
> I didn't look at the code in detail, but it seems to do roughly the right
thing
> :-) I'd like to look at a snippet of generated ninja code for this too.

The example ninja code generated by this is in both
https://codereview.chromium.org/1292983004/#msg2 and
https://codereview.chromium.org/1292983004/#msg7

[Nico, 2015-08-25 04:48:38]

On 2015/08/25 00:51:51, Bons wrote:
> On 2015/08/25 00:42:25, Nico wrote:
> > I didn't look at the code in detail, but it seems to do roughly the right
> thing
> > :-) I'd like to look at a snippet of generated ninja code for this too.
> 
> The example ninja code generated by this is in both
> https://codereview.chromium.org/1292983004/#msg2 and
> https://codereview.chromium.org/1292983004/#msg7

Ah, thanks. Yes, the output needs to be .gch (and I think it must not have the
pch_target. prefix either), and the cflags for the target need to have a
`-include precompile.h` flag (which must not be added when the gch itself is
built).

Here's what gyp produces, you probably want to get the same output:

thakis$ cat tmp.gyp
{
  'targets': [
    {
      'target_name': 'foo',
      'type': 'executable',
      'sources': [ 'foo.cc' ],
      'xcode_settings': {
        'GCC_PREFIX_HEADER': 'header.h',
        'GCC_PRECOMPILE_PREFIX_HEADER': 'YES',
      },
    },
  ],
}
thakis$ ~/src/chrome/src/tools/gyp/gyp -f ninja --depth . tmp.gyp
thakis$ head -13 out/Default/build.ninja 
cc = cc
cxx = c++
ld = $cc
ldxx = $cxx
ar = ar

pool link_pool
  depth = 4

rule cc
  command = $cc -MMD -MF $out.d $defines $includes $cflags $cflags_c
$cflags_pch_c -c $in -o $out
  description = CC $out
  depfile = $out.d

thakis$  cat out/Default/obj/foo.ninja 
defines = 
includes = 
cflags_pch_c = -include obj/foo.header.h-c
cflags_pch_cc = -include obj/foo.header.h-cc
cflags_pch_objc = -include obj/foo.header.h-m
cflags_pch_objcc = -include obj/foo.header.h-mm
cflags = -fasm-blocks -mpascal-strings -Os -gdwarf-2 -arch x86_64
cflags_c = 
cflags_cc = 
cflags_objc = $cflags_c
cflags_objcc = $cflags_cc
arflags = 

build obj/foo.foo.o: cxx ../../foo.cc | obj/foo.header.h-cc.gch
build obj/foo.header.h-c.gch: cc ../../header.h
  cflags_pch_c = -x c-header
build obj/foo.header.h-cc.gch: cxx ../../header.h
  cflags_pch_cc = -x c++-header
build obj/foo.header.h-m.gch: objc ../../header.h
  cflags_pch_objc = -x objective-c-header
build obj/foo.header.h-mm.gch: objcxx ../../header.h
  cflags_pch_objcc = -x objective-c++-header

ldflags = -arch x86_64 -L.
libs = 
build foo: link obj/foo.foo.o
  ld = $ldxx



So the cc rule grows an additional $cflags_pch_c parameter, and targets that
have a pch set that to `cflags_pch_c = -include obj/foo.header.h-c` (the -c is
so that the compiler picks up the precompiled header – one is needed per
language; how exactly this is done could be different in gn), and the edge
building the gch file itself sets it to `-x c-header` instead.

[Bons, 2015-08-27 15:41:16]

OK, please take another look. I changed it to better emulate gyps's behavior.
Here is an example ninja output:

defines =
include_dirs =
cflags =
cflags_cc =
cflags_pch_cc = -include build/precompile.h-cc
target_output_name = pch_target

build obj/build/pch_target.precompile.h-cc.gch: cxx ../../build/precompile.h
  cflags_pch_cc = -x c++-header

build obj/foo/pch_target.input1.o: cxx ../../foo/input1.cc |
obj/build/pch_target.precompile.h-cc.gch

build obj/foo/pch_target.stamp: stamp obj/foo/pch_target.input1.o
obj/build/pch_target.precompile.h-cc.gch

The Windows PCH code has also been updated to use this same method.

This will require toolchains to include a {{cflags_pch_*}} build var as it does
within generated ninja files from gyp. For example:

rule cc
  command = $cc -MMD -MF $out.d $defines $includes $cflags $cflags_c
$cflags_pch_c -c $in -o $out

Dirk, Brett: I was uncertain how to address this within variables.cc from a
documentation standpoint so any suggestions are welcome there.

[Bons, 2015-08-27 15:43:53]

On 2015/08/27 15:41:16, Bons wrote:
> OK, please take another look. I changed it to better emulate gyps's behavior.
> Here is an example ninja output:
> 
> defines =
> include_dirs =
> cflags =
> cflags_cc =
> cflags_pch_cc = -include build/precompile.h-cc
> target_output_name = pch_target
> 
> build obj/build/pch_target.precompile.h-cc.gch: cxx ../../build/precompile.h
>   cflags_pch_cc = -x c++-header
> 
> build obj/foo/pch_target.input1.o: cxx ../../foo/input1.cc |
> obj/build/pch_target.precompile.h-cc.gch
> 
> build obj/foo/pch_target.stamp: stamp obj/foo/pch_target.input1.o
> obj/build/pch_target.precompile.h-cc.gch
> 
> The Windows PCH code has also been updated to use this same method.
> 
> This will require toolchains to include a {{cflags_pch_*}} build var as it
does
> within generated ninja files from gyp. For example:
> 
> rule cc
>   command = $cc -MMD -MF $out.d $defines $includes $cflags $cflags_c
> $cflags_pch_c -c $in -o $out
> 
> Dirk, Brett: I was uncertain how to address this within variables.cc from a
> documentation standpoint so any suggestions are welcome there.

just noticed that build obj/build/pch_target.precompile.h-cc.gch should be build
obj/build/precompile.h-cc.gch or cflags_pch_cc should be -include
obj/build/pch_target.precompile.h-cc

working on that. in the meantime please take a look regardless.

[Bons, 2015-08-27 21:49:03]

On 2015/08/27 15:43:53, Bons wrote:
> On 2015/08/27 15:41:16, Bons wrote:
> > OK, please take another look. I changed it to better emulate gyps's
behavior.
> > Here is an example ninja output:
> > 
> > defines =
> > include_dirs =
> > cflags =
> > cflags_cc =
> > cflags_pch_cc = -include build/precompile.h-cc
> > target_output_name = pch_target
> > 
> > build obj/build/pch_target.precompile.h-cc.gch: cxx ../../build/precompile.h
> >   cflags_pch_cc = -x c++-header
> > 
> > build obj/foo/pch_target.input1.o: cxx ../../foo/input1.cc |
> > obj/build/pch_target.precompile.h-cc.gch
> > 
> > build obj/foo/pch_target.stamp: stamp obj/foo/pch_target.input1.o
> > obj/build/pch_target.precompile.h-cc.gch
> > 
> > The Windows PCH code has also been updated to use this same method.
> > 
> > This will require toolchains to include a {{cflags_pch_*}} build var as it
> does
> > within generated ninja files from gyp. For example:
> > 
> > rule cc
> >   command = $cc -MMD -MF $out.d $defines $includes $cflags $cflags_c
> > $cflags_pch_c -c $in -o $out
> > 
> > Dirk, Brett: I was uncertain how to address this within variables.cc from a
> > documentation standpoint so any suggestions are welcome there.
> 
> just noticed that build obj/build/pch_target.precompile.h-cc.gch should be
build
> obj/build/precompile.h-cc.gch or cflags_pch_cc should be -include
> obj/build/pch_target.precompile.h-cc
> 
> working on that. in the meantime please take a look regardless.

fixed now

[brettw, 2015-08-28 20:09:30]

Do we really need the separate set of flags for pch? The reason for having to
specify what type of PCH you're doing (gcc vs msvc) is that GN has to go
manually hacking up your compiler flags. I got Windows working without this, and
I think we can do the same here.

It's difficult for me to give better advice just looking at this patch. Do you
have a patch anywhere that shows what hooking this up means in terms of BUILD
files?

[Bons, 2015-09-01 01:43:13]

On 2015/08/28 20:09:30, brettw wrote:
> Do we really need the separate set of flags for pch? The reason for having to
> specify what type of PCH you're doing (gcc vs msvc) is that GN has to go
> manually hacking up your compiler flags. I got Windows working without this,
and
> I think we can do the same here.
> 
> It's difficult for me to give better advice just looking at this patch. Do you
> have a patch anywhere that shows what hooking this up means in terms of BUILD
> files?

I’ve updated the BUILD files. The primary change is made to the toolchains. I
don‘t have an issue with losing the flag, but I was mirroring with what GYP did
¯\_(ツ)_/¯

FYI if you end up patching this in I need to fix a linker issue where it’s
treating the .gch file as a .o file which breaks things, but you can see how
things would change from looking at the latest patch.

[Bons, 2015-09-03 15:44:52]

On 2015/09/01 01:43:13, Bons wrote:
> On 2015/08/28 20:09:30, brettw wrote:
> > Do we really need the separate set of flags for pch? The reason for having
to
> > specify what type of PCH you're doing (gcc vs msvc) is that GN has to go
> > manually hacking up your compiler flags. I got Windows working without this,
> and
> > I think we can do the same here.
> > 
> > It's difficult for me to give better advice just looking at this patch. Do
you
> > have a patch anywhere that shows what hooking this up means in terms of
BUILD
> > files?
> 
> I’ve updated the BUILD files. The primary change is made to the toolchains. I
> don‘t have an issue with losing the flag, but I was mirroring with what GYP
did
> ¯\_(ツ)_/¯
> 
> FYI if you end up patching this in I need to fix a linker issue where it’s
> treating the .gch file as a .o file which breaks things, but you can see how
> things would change from looking at the latest patch.

I updated the patch to not treat .gch files as object files. However, now I
believe I understand the reasoning behind gyp's decision to use the cflags_pch_*

The toolchain commands for Objective-C and Objective-C++ include not only
cflags_objc and cflags_objcc, but also cflags_c and cflags_cc. For example:

tool("objcxx") {
      command = "$cxx -MMD -MF $depfile {{defines}} {{include_dirs}} {{cflags}}
{{cflags_cc}} {{cflags_objcc}} -c {{source}} -o {{output}}"
      ...
}

This is just emulating what Xcode does, but it causes issues when multiple tools
are used in a target. Even if we naively overwrite the flags for each gch build
rule, there are still issues due to this overlap. Here is a simplified example
of /base.ninja:

cflags_c = -include obj/base/base/precompile.h-c
cflags_cc = -include obj/base/base/precompile.h-cc
cflags_objcc = -include obj/base/base/precompile.h-mm
root_out_dir = .
target_out_dir = obj/base
target_output_name = base

build obj/base/base/precompile.h-c.gch: cc ../../build/precompile.h
  cflags_c = -x c-header

build obj/base/base/precompile.h-cc.gch: cxx ../../build/precompile.h
  cflags_cc = -x c++-header

build obj/base/base/precompile.h-mm.gch: objcxx ../../build/precompile.h
  cflags_objcc = -x objective-c++-header

<targets that use objcxx, cc, cxx, etc>

Because the objcxx tool includes both the cflags_cc and cflags_objcc, the build
rule for obj/base/base/precompile.h-mm.gch has an "-include
obj/base/base/precompile.h-cc", which causes a compilation error (the file may
not exist), and is not an explicit dep of obj/base/base/precompile.h-mm.gch (nor
should it be).

The cflags_pch_* rules were most likely added to solve this issue in a
relatively simplistic way by setting aside a set of variables that were not only
safe to be overwritten by the build step, but also specific to each build tool.

Any suggestions from someone with more Ninja knowledge (Nico?) would be
appreciated.

[Bons, 2015-09-14 18:31:08]

On 2015/09/03 15:44:52, Bons wrote:
> On 2015/09/01 01:43:13, Bons wrote:
> > On 2015/08/28 20:09:30, brettw wrote:
> > > Do we really need the separate set of flags for pch? The reason for having
> to
> > > specify what type of PCH you're doing (gcc vs msvc) is that GN has to go
> > > manually hacking up your compiler flags. I got Windows working without
this,
> > and
> > > I think we can do the same here.
> > > 
> > > It's difficult for me to give better advice just looking at this patch. Do
> you
> > > have a patch anywhere that shows what hooking this up means in terms of
> BUILD
> > > files?
> > 
> > I’ve updated the BUILD files. The primary change is made to the toolchains.
I
> > don‘t have an issue with losing the flag, but I was mirroring with what GYP
> did
> > ¯\_(ツ)_/¯
> > 
> > FYI if you end up patching this in I need to fix a linker issue where it’s
> > treating the .gch file as a .o file which breaks things, but you can see how
> > things would change from looking at the latest patch.
> 
> I updated the patch to not treat .gch files as object files. However, now I
> believe I understand the reasoning behind gyp's decision to use the
cflags_pch_*
> 
> The toolchain commands for Objective-C and Objective-C++ include not only
> cflags_objc and cflags_objcc, but also cflags_c and cflags_cc. For example:
> 
> tool("objcxx") {
>       command = "$cxx -MMD -MF $depfile {{defines}} {{include_dirs}}
{{cflags}}
> {{cflags_cc}} {{cflags_objcc}} -c {{source}} -o {{output}}"
>       ...
> }
> 
> This is just emulating what Xcode does, but it causes issues when multiple
tools
> are used in a target. Even if we naively overwrite the flags for each gch
build
> rule, there are still issues due to this overlap. Here is a simplified example
> of /base.ninja:
> 
> cflags_c = -include obj/base/base/precompile.h-c
> cflags_cc = -include obj/base/base/precompile.h-cc
> cflags_objcc = -include obj/base/base/precompile.h-mm
> root_out_dir = .
> target_out_dir = obj/base
> target_output_name = base
> 
> build obj/base/base/precompile.h-c.gch: cc ../../build/precompile.h
>   cflags_c = -x c-header
> 
> build obj/base/base/precompile.h-cc.gch: cxx ../../build/precompile.h
>   cflags_cc = -x c++-header
> 
> build obj/base/base/precompile.h-mm.gch: objcxx ../../build/precompile.h
>   cflags_objcc = -x objective-c++-header
> 
> <targets that use objcxx, cc, cxx, etc>
> 
> Because the objcxx tool includes both the cflags_cc and cflags_objcc, the
build
> rule for obj/base/base/precompile.h-mm.gch has an "-include
> obj/base/base/precompile.h-cc", which causes a compilation error (the file may
> not exist), and is not an explicit dep of obj/base/base/precompile.h-mm.gch
(nor
> should it be).
> 
> The cflags_pch_* rules were most likely added to solve this issue in a
> relatively simplistic way by setting aside a set of variables that were not
only
> safe to be overwritten by the build step, but also specific to each build
tool.
> 
> Any suggestions from someone with more Ninja knowledge (Nico?) would be
> appreciated.

OK please take a look. I addressed comments per the discussion at
https://groups.google.com/a/chromium.org/forum/#!topic/gn-dev/JCDcgsuEXZ4

[brettw, 2015-09-14 23:56:28]

https://codereview.chromium.org/1292983004/diff/410001/tools/gn/ninja_binary_...
File tools/gn/ninja_binary_target_writer.cc (right):

https://codereview.chromium.org/1292983004/diff/410001/tools/gn/ninja_binary_...
tools/gn/ninja_binary_target_writer.cc:194: if (extension_offset ==
std::string::npos) {
This new code will crash if there is no extension. I don't think it should do
that. Probably it should just return early and not fill in any output files.

https://codereview.chromium.org/1292983004/diff/410001/tools/gn/ninja_binary_...
tools/gn/ninja_binary_target_writer.cc:210: //   obj/foo/target_name.header.o ->
".o" -> ".h"

https://codereview.chromium.org/1292983004/diff/410001/tools/gn/ninja_binary_...
tools/gn/ninja_binary_target_writer.cc:687: output_suffix += ".h-";
I need to get into this a little bit more, I didn't have a chance to review all
of this in detail. I feel like the precompiled header names are being computed
multiple times in multiple places. Can this be factored out a bit and shared?

[Bons, 2015-09-15 15:08:02]

https://codereview.chromium.org/1292983004/diff/410001/tools/gn/ninja_binary_...
File tools/gn/ninja_binary_target_writer.cc (right):

https://codereview.chromium.org/1292983004/diff/410001/tools/gn/ninja_binary_...
tools/gn/ninja_binary_target_writer.cc:194: if (extension_offset ==
std::string::npos) {
On 2015/09/14 23:56:27, brettw wrote:
> This new code will crash if there is no extension. I don't think it should do
> that. Probably it should just return early and not fill in any output files.

Done.

https://codereview.chromium.org/1292983004/diff/410001/tools/gn/ninja_binary_...
tools/gn/ninja_binary_target_writer.cc:210: //   obj/foo/target_name.header.o ->
On 2015/09/14 23:56:28, brettw wrote:
> ".o" -> ".h"

Done.

https://codereview.chromium.org/1292983004/diff/410001/tools/gn/ninja_binary_...
tools/gn/ninja_binary_target_writer.cc:687: output_suffix += ".h-";
On 2015/09/14 23:56:28, brettw wrote:
> I need to get into this a little bit more, I didn't have a chance to review
all
> of this in detail. I feel like the precompiled header names are being computed
> multiple times in multiple places. Can this be factored out a bit and shared?

Done.

[brettw, 2015-09-15 17:17:12]

https://codereview.chromium.org/1292983004/diff/430001/tools/gn/ninja_binary_...
File tools/gn/ninja_binary_target_writer.cc (right):

https://codereview.chromium.org/1292983004/diff/430001/tools/gn/ninja_binary_...
tools/gn/ninja_binary_target_writer.cc:176: return result.c_str();
This will crash.

Can you test this and paste in what the ninja code looks like for a small target
with precompiled headers?

[Bons, 2015-09-15 18:16:24]

https://codereview.chromium.org/1292983004/diff/430001/tools/gn/ninja_binary_...
File tools/gn/ninja_binary_target_writer.cc (right):

https://codereview.chromium.org/1292983004/diff/430001/tools/gn/ninja_binary_...
tools/gn/ninja_binary_target_writer.cc:176: return result.c_str();
On 2015/09/15 17:17:12, brettw wrote:
> This will crash.
Can you be more specific? I’ve been testing both with the unit tests and with
chromium targets.

 
> Can you test this and paste in what the ninja code looks like for a small
target
> with precompiled headers?

A snip of base.ninja:

cflags_c = -std=c99 -include obj/base/base/precompile.h-c
cflags_cc = -fno-threadsafe-statics -fvisibility-inlines-hidden
-Wno-undefined-bool-conversion -Wno-tautological-undefined-compare -std=c++11
-fno-rtti -fno-exceptions -include obj/base/base/precompile.h-cc
cflags_objcc = -fobjc-call-cxx-cdtors -fno-threadsafe-statics
-fvisibility-inlines-hidden -Wno-undefined-bool-conversion
-Wno-tautological-undefined-compare -std=c++11 -fno-rtti -fno-exceptions
-include obj/base/base/precompile.h-mm
root_out_dir = .
target_out_dir = obj/base
target_output_name = base

build obj/base/base/precompile.h-c.gch: cc ../../build/precompile.h
  cflags_c = -std=c99 -x c-header

build obj/base/base/precompile.h-cc.gch: cxx ../../build/precompile.h
  cflags_cc = -fno-threadsafe-statics -fvisibility-inlines-hidden
-Wno-undefined-bool-conversion -Wno-tautological-undefined-compare -std=c++11
-fno-rtti -fno-exceptions -x c++-header

build obj/base/base/precompile.h-mm.gch: objcxx ../../build/precompile.h
  cflags_objcc = -fobjc-call-cxx-cdtors -fno-threadsafe-statics
-fvisibility-inlines-hidden -Wno-undefined-bool-conversion
-Wno-tautological-undefined-compare -std=c++11 -fno-rtti -fno-exceptions -x
objective-c++-header

build obj/base/base/allocator_extension.o: cxx
../../base/allocator/allocator_extension.cc | obj/base/base/precompile.h-cc.gch
build obj/base/base/async_socket_io_handler_posix.o: cxx
../../base/async_socket_io_handler_posix.cc | obj/base/base/precompile.h-cc.gch
build obj/base/base/at_exit.o: cxx ../../base/at_exit.cc |
obj/base/base/precompile.h-cc.gch
build obj/base/base/barrier_closure.o: cxx ../../base/barrier_closure.cc |
obj/base/base/precompile.h-cc.gch
build obj/base/base/base64.o: cxx ../../base/base64.cc |
obj/base/base/precompile.h-cc.gch
build obj/base/base/big_endian.o: cxx ../../base/big_endian.cc |
obj/base/base/precompile.h-cc.gch
build obj/base/base/bind_helpers.o: cxx ../../base/bind_helpers.cc |
obj/base/base/precompile.h-cc.gch
...

[Bons, 2015-09-15 20:11:40]

https://codereview.chromium.org/1292983004/diff/430001/tools/gn/ninja_binary_...
File tools/gn/ninja_binary_target_writer.cc (right):

https://codereview.chromium.org/1292983004/diff/430001/tools/gn/ninja_binary_...
tools/gn/ninja_binary_target_writer.cc:176: return result.c_str();
On 2015/09/15 18:16:24, Bons wrote:
> On 2015/09/15 17:17:12, brettw wrote:
> > This will crash.
> Can you be more specific? I’ve been testing both with the unit tests and with
> chromium targets.
> 
>  
> > Can you test this and paste in what the ninja code looks like for a small
> target
> > with precompiled headers?
> 
> A snip of base.ninja:
> 
> cflags_c = -std=c99 -include obj/base/base/precompile.h-c
> cflags_cc = -fno-threadsafe-statics -fvisibility-inlines-hidden
> -Wno-undefined-bool-conversion -Wno-tautological-undefined-compare -std=c++11
> -fno-rtti -fno-exceptions -include obj/base/base/precompile.h-cc
> cflags_objcc = -fobjc-call-cxx-cdtors -fno-threadsafe-statics
> -fvisibility-inlines-hidden -Wno-undefined-bool-conversion
> -Wno-tautological-undefined-compare -std=c++11 -fno-rtti -fno-exceptions
> -include obj/base/base/precompile.h-mm
> root_out_dir = .
> target_out_dir = obj/base
> target_output_name = base
> 
> build obj/base/base/precompile.h-c.gch: cc ../../build/precompile.h
>   cflags_c = -std=c99 -x c-header
> 
> build obj/base/base/precompile.h-cc.gch: cxx ../../build/precompile.h
>   cflags_cc = -fno-threadsafe-statics -fvisibility-inlines-hidden
> -Wno-undefined-bool-conversion -Wno-tautological-undefined-compare -std=c++11
> -fno-rtti -fno-exceptions -x c++-header
> 
> build obj/base/base/precompile.h-mm.gch: objcxx ../../build/precompile.h
>   cflags_objcc = -fobjc-call-cxx-cdtors -fno-threadsafe-statics
> -fvisibility-inlines-hidden -Wno-undefined-bool-conversion
> -Wno-tautological-undefined-compare -std=c++11 -fno-rtti -fno-exceptions -x
> objective-c++-header
> 
> build obj/base/base/allocator_extension.o: cxx
> ../../base/allocator/allocator_extension.cc |
obj/base/base/precompile.h-cc.gch
> build obj/base/base/async_socket_io_handler_posix.o: cxx
> ../../base/async_socket_io_handler_posix.cc |
obj/base/base/precompile.h-cc.gch
> build obj/base/base/at_exit.o: cxx ../../base/at_exit.cc |
> obj/base/base/precompile.h-cc.gch
> build obj/base/base/barrier_closure.o: cxx ../../base/barrier_closure.cc |
> obj/base/base/precompile.h-cc.gch
> build obj/base/base/base64.o: cxx ../../base/base64.cc |
> obj/base/base/precompile.h-cc.gch
> build obj/base/base/big_endian.o: cxx ../../base/big_endian.cc |
> obj/base/base/precompile.h-cc.gch
> build obj/base/base/bind_helpers.o: cxx ../../base/bind_helpers.cc |
> obj/base/base/precompile.h-cc.gch
> ...

I see it's a use-after-free scenario. Will update to return std::string.

[Bons, 2015-09-16 15:02:43]

On 2015/09/15 20:11:40, Bons wrote:
>
https://codereview.chromium.org/1292983004/diff/430001/tools/gn/ninja_binary_...
> File tools/gn/ninja_binary_target_writer.cc (right):
> 
>
https://codereview.chromium.org/1292983004/diff/430001/tools/gn/ninja_binary_...
> tools/gn/ninja_binary_target_writer.cc:176: return result.c_str();
> On 2015/09/15 18:16:24, Bons wrote:
> > On 2015/09/15 17:17:12, brettw wrote:
> > > This will crash.
> > Can you be more specific? I’ve been testing both with the unit tests and
with
> > chromium targets.
> > 
> >  
> > > Can you test this and paste in what the ninja code looks like for a small
> > target
> > > with precompiled headers?
> > 
> > A snip of base.ninja:
> > 
> > cflags_c = -std=c99 -include obj/base/base/precompile.h-c
> > cflags_cc = -fno-threadsafe-statics -fvisibility-inlines-hidden
> > -Wno-undefined-bool-conversion -Wno-tautological-undefined-compare
-std=c++11
> > -fno-rtti -fno-exceptions -include obj/base/base/precompile.h-cc
> > cflags_objcc = -fobjc-call-cxx-cdtors -fno-threadsafe-statics
> > -fvisibility-inlines-hidden -Wno-undefined-bool-conversion
> > -Wno-tautological-undefined-compare -std=c++11 -fno-rtti -fno-exceptions
> > -include obj/base/base/precompile.h-mm
> > root_out_dir = .
> > target_out_dir = obj/base
> > target_output_name = base
> > 
> > build obj/base/base/precompile.h-c.gch: cc ../../build/precompile.h
> >   cflags_c = -std=c99 -x c-header
> > 
> > build obj/base/base/precompile.h-cc.gch: cxx ../../build/precompile.h
> >   cflags_cc = -fno-threadsafe-statics -fvisibility-inlines-hidden
> > -Wno-undefined-bool-conversion -Wno-tautological-undefined-compare
-std=c++11
> > -fno-rtti -fno-exceptions -x c++-header
> > 
> > build obj/base/base/precompile.h-mm.gch: objcxx ../../build/precompile.h
> >   cflags_objcc = -fobjc-call-cxx-cdtors -fno-threadsafe-statics
> > -fvisibility-inlines-hidden -Wno-undefined-bool-conversion
> > -Wno-tautological-undefined-compare -std=c++11 -fno-rtti -fno-exceptions -x
> > objective-c++-header
> > 
> > build obj/base/base/allocator_extension.o: cxx
> > ../../base/allocator/allocator_extension.cc |
> obj/base/base/precompile.h-cc.gch
> > build obj/base/base/async_socket_io_handler_posix.o: cxx
> > ../../base/async_socket_io_handler_posix.cc |
> obj/base/base/precompile.h-cc.gch
> > build obj/base/base/at_exit.o: cxx ../../base/at_exit.cc |
> > obj/base/base/precompile.h-cc.gch
> > build obj/base/base/barrier_closure.o: cxx ../../base/barrier_closure.cc |
> > obj/base/base/precompile.h-cc.gch
> > build obj/base/base/base64.o: cxx ../../base/base64.cc |
> > obj/base/base/precompile.h-cc.gch
> > build obj/base/base/big_endian.o: cxx ../../base/big_endian.cc |
> > obj/base/base/precompile.h-cc.gch
> > build obj/base/base/bind_helpers.o: cxx ../../base/bind_helpers.cc |
> > obj/base/base/precompile.h-cc.gch
> > ...
> 
> I see it's a use-after-free scenario. Will update to return std::string.

Done. Please take another look.

[Bons, 2015-09-16 22:19:23]

On 2015/09/16 15:02:43, Bons wrote:
> On 2015/09/15 20:11:40, Bons wrote:
> >
>
https://codereview.chromium.org/1292983004/diff/430001/tools/gn/ninja_binary_...
> > File tools/gn/ninja_binary_target_writer.cc (right):
> > 
> >
>
https://codereview.chromium.org/1292983004/diff/430001/tools/gn/ninja_binary_...
> > tools/gn/ninja_binary_target_writer.cc:176: return result.c_str();
> > On 2015/09/15 18:16:24, Bons wrote:
> > > On 2015/09/15 17:17:12, brettw wrote:
> > > > This will crash.
> > > Can you be more specific? I’ve been testing both with the unit tests and
> with
> > > chromium targets.
> > > 
> > >  
> > > > Can you test this and paste in what the ninja code looks like for a
small
> > > target
> > > > with precompiled headers?
> > > 
> > > A snip of base.ninja:
> > > 
> > > cflags_c = -std=c99 -include obj/base/base/precompile.h-c
> > > cflags_cc = -fno-threadsafe-statics -fvisibility-inlines-hidden
> > > -Wno-undefined-bool-conversion -Wno-tautological-undefined-compare
> -std=c++11
> > > -fno-rtti -fno-exceptions -include obj/base/base/precompile.h-cc
> > > cflags_objcc = -fobjc-call-cxx-cdtors -fno-threadsafe-statics
> > > -fvisibility-inlines-hidden -Wno-undefined-bool-conversion
> > > -Wno-tautological-undefined-compare -std=c++11 -fno-rtti -fno-exceptions
> > > -include obj/base/base/precompile.h-mm
> > > root_out_dir = .
> > > target_out_dir = obj/base
> > > target_output_name = base
> > > 
> > > build obj/base/base/precompile.h-c.gch: cc ../../build/precompile.h
> > >   cflags_c = -std=c99 -x c-header
> > > 
> > > build obj/base/base/precompile.h-cc.gch: cxx ../../build/precompile.h
> > >   cflags_cc = -fno-threadsafe-statics -fvisibility-inlines-hidden
> > > -Wno-undefined-bool-conversion -Wno-tautological-undefined-compare
> -std=c++11
> > > -fno-rtti -fno-exceptions -x c++-header
> > > 
> > > build obj/base/base/precompile.h-mm.gch: objcxx ../../build/precompile.h
> > >   cflags_objcc = -fobjc-call-cxx-cdtors -fno-threadsafe-statics
> > > -fvisibility-inlines-hidden -Wno-undefined-bool-conversion
> > > -Wno-tautological-undefined-compare -std=c++11 -fno-rtti -fno-exceptions
-x
> > > objective-c++-header
> > > 
> > > build obj/base/base/allocator_extension.o: cxx
> > > ../../base/allocator/allocator_extension.cc |
> > obj/base/base/precompile.h-cc.gch
> > > build obj/base/base/async_socket_io_handler_posix.o: cxx
> > > ../../base/async_socket_io_handler_posix.cc |
> > obj/base/base/precompile.h-cc.gch
> > > build obj/base/base/at_exit.o: cxx ../../base/at_exit.cc |
> > > obj/base/base/precompile.h-cc.gch
> > > build obj/base/base/barrier_closure.o: cxx ../../base/barrier_closure.cc |
> > > obj/base/base/precompile.h-cc.gch
> > > build obj/base/base/base64.o: cxx ../../base/base64.cc |
> > > obj/base/base/precompile.h-cc.gch
> > > build obj/base/base/big_endian.o: cxx ../../base/big_endian.cc |
> > > obj/base/base/precompile.h-cc.gch
> > > build obj/base/base/bind_helpers.o: cxx ../../base/bind_helpers.cc |
> > > obj/base/base/precompile.h-cc.gch
> > > ...
> > 
> > I see it's a use-after-free scenario. Will update to return std::string.
> 
> Done. Please take another look.

Ping.

[brettw, 2015-09-16 22:41:14]

Sorry this review is taking forever! Thanks for putting up with it...

https://codereview.chromium.org/1292983004/diff/430001/tools/gn/ninja_binary_...
File tools/gn/ninja_binary_target_writer.cc (right):

https://codereview.chromium.org/1292983004/diff/430001/tools/gn/ninja_binary_...
tools/gn/ninja_binary_target_writer.cc:176: return result.c_str();
Sorry I wasn't more specific, I thought it would crash right away.

https://codereview.chromium.org/1292983004/diff/450001/tools/gn/ninja_binary_...
File tools/gn/ninja_binary_target_writer.cc (right):

https://codereview.chromium.org/1292983004/diff/450001/tools/gn/ninja_binary_...
tools/gn/ninja_binary_target_writer.cc:161: result += lang_suffix;
This function is kind of confusing because it returns fairly different things
for MSVC vs GCC. In MSVC I would consider the PCH output extension to be ".pch"
(it also generates .pch files as a side effect). Instead, it returns the object
file names on Windows, and the .pch file name on GCC.

This might be fine, but it looks like the results are used in quite different
ways in all cases below except WritePCHCommand where the results are in turn
interpreted differently.

I wonder if it would be better to keep the old name GetWindowsPCHObjectFiles and
just write a new one GetGCCPCHFileName for when we need that.

https://codereview.chromium.org/1292983004/diff/450001/tools/gn/ninja_binary_...
tools/gn/ninja_binary_target_writer.cc:204: // Compute the tool. This must use
the tool type passed in rather than the
If you take the above advice, this first part of this function will be shared,
but it can be factored out into a helper function that computes the substitution
commands that is used by both the GCC and MSVC variants.

https://codereview.chromium.org/1292983004/diff/450001/tools/gn/ninja_binary_...
tools/gn/ninja_binary_target_writer.cc:480: return;
This flow here is a bit weird. This function goes through some effort not to
early return with the flag_values_written flag, except for this case.

My feeling is that RecursiveTargetConfigStringsToStream should be added to the
bottom of the PCH_MSVC arm above, and also in an "else" case at the bottom so
it's executed every way through here without the extra flag (it's just one
function call so doesn't seem so much like duplicated code to me).

Also, we should be careful about these returns. If this case is hit, we'll stop
writing a command in the middle and leave a corrupted ninja file. Before (I
could well have messed it up) there were certain errors that would give odd
commands, that would give compiler failures, but these would be a somewhat
obvious side effect of messing up the inputs (i.e. no extension on input gives
weirdly named output).

https://codereview.chromium.org/1292983004/diff/450001/tools/gn/ninja_binary_...
tools/gn/ninja_binary_target_writer.cc:555: bool
NinjaBinaryTargetWriter::WritePCHCommand(
I'm feeling like this function would be better split between two variants, one
for Windows, and one for GCC. There are only like 4 lines of shared code, and I
was questioning above whether GetPCHOutputFiles should be different between the
two. I suspect splitting would make each case easier to follow.

[Bons, 2015-09-17 16:53:40]

OK. It’s been updated to keep some of the msvc and gcc bits separate in an
effort to make it more understandable. I’m not quite certain if it's exactly
what you had in mind but happy to change if I simply misunderstood your
comments.

https://codereview.chromium.org/1292983004/diff/450001/tools/gn/ninja_binary_...
File tools/gn/ninja_binary_target_writer.cc (right):

https://codereview.chromium.org/1292983004/diff/450001/tools/gn/ninja_binary_...
tools/gn/ninja_binary_target_writer.cc:161: result += lang_suffix;
On 2015/09/16 22:41:13, brettw wrote:
> This function is kind of confusing because it returns fairly different things
> for MSVC vs GCC. In MSVC I would consider the PCH output extension to be
".pch"
> (it also generates .pch files as a side effect). Instead, it returns the
object
> file names on Windows, and the .pch file name on GCC.
> 
> This might be fine, but it looks like the results are used in quite different
> ways in all cases below except WritePCHCommand where the results are in turn
> interpreted differently.
> 
> I wonder if it would be better to keep the old name GetWindowsPCHObjectFiles
and
> just write a new one GetGCCPCHFileName for when we need that.

Done.

https://codereview.chromium.org/1292983004/diff/450001/tools/gn/ninja_binary_...
tools/gn/ninja_binary_target_writer.cc:204: // Compute the tool. This must use
the tool type passed in rather than the
On 2015/09/16 22:41:14, brettw wrote:
> If you take the above advice, this first part of this function will be shared,
> but it can be factored out into a helper function that computes the
substitution
> commands that is used by both the GCC and MSVC variants.

Done.

https://codereview.chromium.org/1292983004/diff/450001/tools/gn/ninja_binary_...
tools/gn/ninja_binary_target_writer.cc:480: return;
On 2015/09/16 22:41:14, brettw wrote:
> This flow here is a bit weird. This function goes through some effort not to
> early return with the flag_values_written flag, except for this case.
> 
> My feeling is that RecursiveTargetConfigStringsToStream should be added to the
> bottom of the PCH_MSVC arm above, and also in an "else" case at the bottom so
> it's executed every way through here without the extra flag (it's just one
> function call so doesn't seem so much like duplicated code to me).
> 
> Also, we should be careful about these returns. If this case is hit, we'll
stop
> writing a command in the middle and leave a corrupted ninja file. Before (I
> could well have messed it up) there were certain errors that would give odd
> commands, that would give compiler failures, but these would be a somewhat
> obvious side effect of messing up the inputs (i.e. no extension on input gives
> weirdly named output).

Done.

https://codereview.chromium.org/1292983004/diff/450001/tools/gn/ninja_binary_...
tools/gn/ninja_binary_target_writer.cc:555: bool
NinjaBinaryTargetWriter::WritePCHCommand(
On 2015/09/16 22:41:13, brettw wrote:
> I'm feeling like this function would be better split between two variants, one
> for Windows, and one for GCC. There are only like 4 lines of shared code, and
I
> was questioning above whether GetPCHOutputFiles should be different between
the
> two. I suspect splitting would make each case easier to follow.

Done.

[brettw, 2015-09-17 19:47:25]

I think this is almost it...

https://codereview.chromium.org/1292983004/diff/470001/tools/gn/ninja_binary_...
File tools/gn/ninja_binary_target_writer.cc (right):

https://codereview.chromium.org/1292983004/diff/470001/tools/gn/ninja_binary_...
tools/gn/ninja_binary_target_writer.cc:588: header_str.insert(0, "//");
Is this checking really correct? I think GCC is the same as MSVC here, in that
the precompiled_header is something that the compiler will resolve relative to
the include path, and the precompiled source is a file that GN knows about.

But for example in Blink, such includes would typically be relative to
third_party/WebKit, but the source file will need to be an absolute path. GN
doesn't even automatically put "//" on the include search path.

I think we can actually just delete this checking.

If we wanted this, we should check at the time the variables are defined so we
can properly blame the source code that added them. As you can see, basically
nothing in here fails because it's assume that everything ahs been set up
properly earlier.

https://codereview.chromium.org/1292983004/diff/470001/tools/gn/ninja_binary_...
tools/gn/ninja_binary_target_writer.cc:604: return false;
I think if we delete this, we can also make this function and the other one
return void and remove the error checking.

https://codereview.chromium.org/1292983004/diff/470001/tools/gn/ninja_binary_...
tools/gn/ninja_binary_target_writer.cc:625: // -include flag with the -x <header
lang> flag required for .gch targets.
You you add "implicitly generated" before "-include". I was originally looking
for code that searched for "-include" and replaced it, but really we're just
writing another version without it.

[Bons, 2015-09-17 20:36:12]

https://codereview.chromium.org/1292983004/diff/470001/tools/gn/ninja_binary_...
File tools/gn/ninja_binary_target_writer.cc (right):

https://codereview.chromium.org/1292983004/diff/470001/tools/gn/ninja_binary_...
tools/gn/ninja_binary_target_writer.cc:588: header_str.insert(0, "//");
On 2015/09/17 19:47:25, brettw wrote:
> Is this checking really correct? I think GCC is the same as MSVC here, in that
> the precompiled_header is something that the compiler will resolve relative to
> the include path, and the precompiled source is a file that GN knows about.
> 
> But for example in Blink, such includes would typically be relative to
> third_party/WebKit, but the source file will need to be an absolute path. GN
> doesn't even automatically put "//" on the include search path.
> 
> I think we can actually just delete this checking.
> 
> If we wanted this, we should check at the time the variables are defined so we
> can properly blame the source code that added them. As you can see, basically
> nothing in here fails because it's assume that everything ahs been set up
> properly earlier.

Done.

https://codereview.chromium.org/1292983004/diff/470001/tools/gn/ninja_binary_...
tools/gn/ninja_binary_target_writer.cc:604: return false;
On 2015/09/17 19:47:25, brettw wrote:
> I think if we delete this, we can also make this function and the other one
> return void and remove the error checking.

Done.

https://codereview.chromium.org/1292983004/diff/470001/tools/gn/ninja_binary_...
tools/gn/ninja_binary_target_writer.cc:625: // -include flag with the -x <header
lang> flag required for .gch targets.
On 2015/09/17 19:47:25, brettw wrote:
> You you add "implicitly generated" before "-include". I was originally looking
> for code that searched for "-include" and replaced it, but really we're just
> writing another version without it.

Done.

[brettw, 2015-09-17 21:32:31]

lgtm

[Bons, 2015-09-17 21:44:50]

The CQ bit was checked by andybons@chromium.org

[commit-bot: I haz the power, 2015-09-17 21:45:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1292983004/490001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1292983004/490001

[commit-bot: I haz the power, 2015-09-17 22:24:39]

Committed patchset #26 (id:490001)

[commit-bot: I haz the power, 2015-09-17 22:25:22]

Patchset 26 (id:??) landed as
https://crrev.com/1ae777a027b5243924f2b3ba774067a7bf5926cf
Cr-Commit-Position: refs/heads/master@{#349518}