|
|
Created:
5 years, 4 months ago by Bons Modified:
5 years, 3 months ago Reviewers:
Mark Mentovai, Dirk Pranke, Nico, brettw CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[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}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : switch statement. error handling. some initial docs. #
Total comments: 2
Patch Set 4 : use .gch suffix #Patch Set 5 : cflags_pch_* #Patch Set 6 : merge #Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #Patch Set 10 : fixed include #Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #Patch Set 14 : #Patch Set 15 : #Patch Set 16 : #Patch Set 17 : #Patch Set 18 : fix bug with superfluous pch deps. #Patch Set 19 : #Patch Set 20 : #Patch Set 21 : #Patch Set 22 : #
Total comments: 6
Patch Set 23 : #
Total comments: 4
Patch Set 24 : fix use-after-free bug #
Total comments: 8
Patch Set 25 : Address Brett’s comments... (hopefully) #
Total comments: 6
Patch Set 26 : #
Created: 5 years, 3 months ago
Messages
Total messages: 34 (2 generated)
andybons@chromium.org changed reviewers: + brettw@chromium.org, dpranke@chromium.org, mark@chromium.org, thakis@chromium.org
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.
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.
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.
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
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).
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
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.
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.
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
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.
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.
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.
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
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?
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.
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.
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
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?
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.
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?
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 ...
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.
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.
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.
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.
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.
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.
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.
lgtm
The CQ bit was checked by andybons@chromium.org
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
Message was sent while issue was closed.
Committed patchset #26 (id:490001)
Message was sent while issue was closed.
Patchset 26 (id:??) landed as https://crrev.com/1ae777a027b5243924f2b3ba774067a7bf5926cf Cr-Commit-Position: refs/heads/master@{#349518} |