Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2215433002:
Rework default GN symbol handling for win goma builds. (Closed)
|
Created:
4 years, 4 months ago by Dirk Pranke Modified:
4 years, 3 months ago CC:
chromium-reviews, chrisha Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRework default GN symbol handling for win goma builds.
Win goma builds do not support PDB files, so trying to build with
full symbols is very painful. Previously we would default to "full
symbols" but silently turn off symbols if using goma; this CL
restructures things so that we default to "minimal symbols" if
using goma (and visual studio's compiler).
R=brucedawson@chromium.org, brettw@chromium.org, thakis@chromium.org
BUG=634184
Committed: https://crrev.com/64fea5e29655036e2c5410b903cd43f832d04e93
Cr-Commit-Position: refs/heads/master@{#415325}
Patch Set 1 : fix typo #
Total comments: 3
Patch Set 2 : merge #Patch Set 3 : fix long string literal #
Total comments: 4
Patch Set 4 : merge, clean up w/ clang review feedback #Messages
Total messages: 47 (17 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
dpranke@chromium.org changed reviewers: + scottmg@chromium.org
I think some change like this is a good idea (and an improvement over what we had before), but I'm not sure if this particular patch is the best way to do this. Maybe the assert is too strict? What do y'all think?
https://codereview.chromium.org/2215433002/diff/40001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2215433002/diff/40001/build/config/compiler/B... build/config/compiler/BUILD.gn:1482: "Cannot generate full symbols w/ goma, use symbol_level=1") Oh, we don't have win_z7 in GN? I guess goma-win is mostly a bot thing by this point?
(fine with me though)
https://codereview.chromium.org/2215433002/diff/40001/build/config/compiler/c... File build/config/compiler/compiler.gni (right): https://codereview.chromium.org/2215433002/diff/40001/build/config/compiler/c... build/config/compiler/compiler.gni:87: # compilation because otherwise the redundant debug information Don't we want this for non win too?
On 2016/08/03 23:37:24, scottmg wrote: > https://codereview.chromium.org/2215433002/diff/40001/build/config/compiler/B... > File build/config/compiler/BUILD.gn (right): > > https://codereview.chromium.org/2215433002/diff/40001/build/config/compiler/B... > build/config/compiler/BUILD.gn:1482: "Cannot generate full symbols w/ goma, use > symbol_level=1") > Oh, we don't have win_z7 in GN? I guess goma-win is mostly a bot thing by this > point? Good question. We had win_z7 in GYP, but it eventually turned out that that was only used on one builder (syzyasan) and that was by accident. So, I intentionally didn't port it into GN, and we get this logic instead. We can add win_z7 back in, but there doesn't seem to be a great reason to do so. Even if goma-win is used for local devs (and it is, sometimes), we can't use /Zi, and it's not clear if /Z7 is a net win over a full-symbol non-goma build.
dpranke@chromium.org changed reviewers: + jam@chromium.org
+jam, since this CL might grow a bit in scope and have broader impact on cycle times ... https://codereview.chromium.org/2215433002/diff/40001/build/config/compiler/c... File build/config/compiler/compiler.gni (right): https://codereview.chromium.org/2215433002/diff/40001/build/config/compiler/c... build/config/compiler/compiler.gni:87: # compilation because otherwise the redundant debug information On 2016/08/03 23:42:16, Nico wrote: > Don't we want this for non win too? I'm not sure what you're asking. goma + full symbols works fine (though slower) on the other platforms, so we don't *have* to do this change. And, I believe there are a fair number of people that build locally with full symbols + goma on at least linux. So, I don't think we should prohibit goma+full_symbols on mac/linux/android/ios. Are you asking whether we should change the default so that non-win release builds also pick up minimal symbols? These days, most (non-win) trybots are explicitly set to minimal_symbols, but release waterfall builds pick up the defaults (so, full symbols on mac, no symbols on linux). We can change things so that releases default to minimal_symbols; it would make stack traces worse on mac but possibly speed up cycle times, and it would slow build times on linux (locally, and on waterfall linux builders) for waterfall linux builders. But, it would make things a lot more consistent. So, making that change might be better done in a separate CL so we can isolate the effects on cycle times and load better.
On 2016/08/03 23:54:36, Dirk Pranke wrote: > +jam, since this CL might grow a bit in scope and have broader impact on cycle > times ... > > https://codereview.chromium.org/2215433002/diff/40001/build/config/compiler/c... > File build/config/compiler/compiler.gni (right): > > https://codereview.chromium.org/2215433002/diff/40001/build/config/compiler/c... > build/config/compiler/compiler.gni:87: # compilation because otherwise the > redundant debug information > On 2016/08/03 23:42:16, Nico wrote: > > Don't we want this for non win too? > > I'm not sure what you're asking. > > goma + full symbols works fine (though slower) on the other platforms, so > we don't *have* to do this change. And, I believe there are a fair number of > people that build locally with full symbols + goma on at least linux. So, I > don't think we should prohibit goma+full_symbols on mac/linux/android/ios. > > Are you asking whether we should change the default so that non-win > release builds also pick up minimal symbols? > > These days, most (non-win) trybots are explicitly set to minimal_symbols, but > release > waterfall builds pick up the defaults (so, full symbols on mac, no symbols on > linux). > > We can change things so that releases default to minimal_symbols; > it would make stack traces worse on mac but possibly speed up cycle times, and > it would slow build times on linux (locally, and on waterfall linux builders) > for > waterfall linux builders. But, it would make things a lot more consistent. Agree this sounds like an improvement. To repeat to make sure I'm understanding correctly, this won't affect the CQ right? My hunch would be adding minimal symbols for linux waterfall builds won't slow them down much. The compiles are already very fast so we have a lot of margin. > > So, making that change might be better done in a separate CL so we can isolate > the effects on cycle times and load better.
On 2016/08/03 23:54:36, Dirk Pranke wrote: > +jam, since this CL might grow a bit in scope and have broader impact on cycle > times ... > > https://codereview.chromium.org/2215433002/diff/40001/build/config/compiler/c... > File build/config/compiler/compiler.gni (right): > > https://codereview.chromium.org/2215433002/diff/40001/build/config/compiler/c... > build/config/compiler/compiler.gni:87: # compilation because otherwise the > redundant debug information > On 2016/08/03 23:42:16, Nico wrote: > > Don't we want this for non win too? > > I'm not sure what you're asking. > > goma + full symbols works fine (though slower) on the other platforms, so > we don't *have* to do this change. And, I believe there are a fair number of > people that build locally with full symbols + goma on at least linux. So, I > don't think we should prohibit goma+full_symbols on mac/linux/android/ios. > > Are you asking whether we should change the default so that non-win > release builds also pick up minimal symbols? > > These days, most (non-win) trybots are explicitly set to minimal_symbols, but > release > waterfall builds pick up the defaults (so, full symbols on mac, no symbols on > linux). Ah right, the non-trybots don't do this. > We can change things so that releases default to minimal_symbols; > it would make stack traces worse on mac but possibly speed up cycle times, and > it would slow build times on linux (locally, and on waterfall linux builders) > for > waterfall linux builders. But, it would make things a lot more consistent. As a general guideline, I think it's good to make compiler flags match as much as possible between platforms. But there'll probably always be some one-off differences, and I could see this being one. But I think it might be worth trying to set this flag uniformly.
lgtm
On 2016/08/04 22:05:11, Nico wrote: > As a general guideline, I think it's good to make compiler flags match as much > as possible between platforms. But there'll probably always be some one-off > differences, and I could see this being one. But I think it might be worth > trying to set this flag uniformly. I think I'm inclined to agree. However, like I said, let's do that as a separate CL.
The CQ bit was checked by dpranke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
rnk@chromium.org changed reviewers: + rnk@chromium.org
https://codereview.chromium.org/2215433002/diff/80001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2215433002/diff/80001/build/config/compiler/B... build/config/compiler/BUILD.gn:1558: !(is_win && use_goma), I'd like it if you tossed in '&& !is_clang' so that you can build with 'is_clang=true use_goma=true symbol_level=2' and get a full symbols. Goma will not make linking clang-built objects any slower or faster, since clang aliases /Zi to /Z7. Our link times with /Z7 aren't particularly good, but they aren't as bad as MSVC's, probably because we emit less stuff.
@brucedawson, what do you think of just removing the assert altogether? https://codereview.chromium.org/2215433002/diff/80001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2215433002/diff/80001/build/config/compiler/B... build/config/compiler/BUILD.gn:1558: !(is_win && use_goma), On 2016/08/13 01:55:27, Reid Kleckner wrote: > I'd like it if you tossed in '&& !is_clang' so that you can build with > 'is_clang=true use_goma=true symbol_level=2' and get a full symbols. > > Goma will not make linking clang-built objects any slower or faster, since clang > aliases /Zi to /Z7. Our link times with /Z7 aren't particularly good, but they > aren't as bad as MSVC's, probably because we emit less stuff. Okay, will do. At the moment, I'm actually leaning to removing the assert altogether and seeing what happens.
On 2016/08/13 01:55:27, Reid Kleckner wrote: > https://codereview.chromium.org/2215433002/diff/80001/build/config/compiler/B... > File build/config/compiler/BUILD.gn (right): > > https://codereview.chromium.org/2215433002/diff/80001/build/config/compiler/B... > build/config/compiler/BUILD.gn:1558: !(is_win && use_goma), > I'd like it if you tossed in '&& !is_clang' so that you can build with > 'is_clang=true use_goma=true symbol_level=2' and get a full symbols. > > Goma will not make linking clang-built objects any slower or faster, since clang > aliases /Zi to /Z7. Our link times with /Z7 aren't particularly good, but they > aren't as bad as MSVC's, probably because we emit less stuff. I'd also like something like that, however when goma sees /Zi is always falls back to a local build. We need to pass /Z7 to have goma working.
On 2016/08/14 23:34:58, Dirk Pranke wrote: > @brucedawson, what do you think of just removing the assert altogether? Sorry for the delayed reply. Removing the assert is probably fine since the problem will only hit people who use VC++ and goma and explicitly request symbol_level = 2. That won't be zero people, but it will be extremely close.
https://codereview.chromium.org/2215433002/diff/80001/build/config/compiler/c... File build/config/compiler/compiler.gni (right): https://codereview.chromium.org/2215433002/diff/80001/build/config/compiler/c... build/config/compiler/compiler.gni:85: } else if (is_win && use_goma) { like rnk says, this should probably grow a !is_clang too https://codereview.chromium.org/2215433002/diff/80001/build/config/compiler/c... build/config/compiler/compiler.gni:93: # debug info doesn't make link.exe run for hours. (i removed this a while ago, please don't accidentally add it back in this cl)
Description was changed from ========== Rework default GN symbol handling for win goma builds. Win goma builds do not support PDB files, so trying to build with full symbols is very painful. Previously we would default to "full symbols" but silently turn off symbols if using goma; this CL restructures things so that we default to "minimal symbols" if using goma, and assert out if full symbols is explicitly requested. R=brucedawson@chromium.org, brettw@chromium.org, thakis@chromium.org BUG=634184 ========== to ========== Rework default GN symbol handling for win goma builds. Win goma builds do not support PDB files, so trying to build with full symbols is very painful. Previously we would default to "full symbols" but silently turn off symbols if using goma; this CL restructures things so that we default to "minimal symbols" if using goma (and visual studio's compiler). R=brucedawson@chromium.org, brettw@chromium.org, thakis@chromium.org BUG=634184 ==========
@thakis - does this latest version look good to you?
The CQ bit was checked by dpranke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm (Note to self: After this, if you don't explicitly set symbol_level but do set use_goma, you don't get debug info with cl but do with clang-cl. This will affect compile time comparisons. Bots all set symbol_level and should be unaffected though.)
The CQ bit was checked by dpranke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org Link to the patchset: https://codereview.chromium.org/2215433002/#ps100001 (title: "merge, clean up w/ clang review feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Rework default GN symbol handling for win goma builds. Win goma builds do not support PDB files, so trying to build with full symbols is very painful. Previously we would default to "full symbols" but silently turn off symbols if using goma; this CL restructures things so that we default to "minimal symbols" if using goma (and visual studio's compiler). R=brucedawson@chromium.org, brettw@chromium.org, thakis@chromium.org BUG=634184 ========== to ========== Rework default GN symbol handling for win goma builds. Win goma builds do not support PDB files, so trying to build with full symbols is very painful. Previously we would default to "full symbols" but silently turn off symbols if using goma; this CL restructures things so that we default to "minimal symbols" if using goma (and visual studio's compiler). R=brucedawson@chromium.org, brettw@chromium.org, thakis@chromium.org BUG=634184 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Rework default GN symbol handling for win goma builds. Win goma builds do not support PDB files, so trying to build with full symbols is very painful. Previously we would default to "full symbols" but silently turn off symbols if using goma; this CL restructures things so that we default to "minimal symbols" if using goma (and visual studio's compiler). R=brucedawson@chromium.org, brettw@chromium.org, thakis@chromium.org BUG=634184 ========== to ========== Rework default GN symbol handling for win goma builds. Win goma builds do not support PDB files, so trying to build with full symbols is very painful. Previously we would default to "full symbols" but silently turn off symbols if using goma; this CL restructures things so that we default to "minimal symbols" if using goma (and visual studio's compiler). R=brucedawson@chromium.org, brettw@chromium.org, thakis@chromium.org BUG=634184 Committed: https://crrev.com/64fea5e29655036e2c5410b903cd43f832d04e93 Cr-Commit-Position: refs/heads/master@{#415325} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/64fea5e29655036e2c5410b903cd43f832d04e93 Cr-Commit-Position: refs/heads/master@{#415325}
Message was sent while issue was closed.
On 2016/08/30 16:58:25, commit-bot: I haz the power wrote: > Patchset 4 (id:??) landed as > https://crrev.com/64fea5e29655036e2c5410b903cd43f832d04e93 > Cr-Commit-Position: refs/heads/master@{#415325} I can no longer build locally on Windows. "gn gen" returns "undefined identifier" pointing at "use_goma" in line 85, in compiler.gni. I'm planning on reverting this CL. Objections?
Message was sent while issue was closed.
On 2016/08/30 18:15:16, mmenke wrote: > On 2016/08/30 16:58:25, commit-bot: I haz the power wrote: > > Patchset 4 (id:??) landed as > > https://crrev.com/64fea5e29655036e2c5410b903cd43f832d04e93 > > Cr-Commit-Position: refs/heads/master@{#415325} > > I can no longer build locally on Windows. "gn gen" returns "undefined > identifier" pointing at "use_goma" in line 85, in compiler.gni. I'm planning on > reverting this CL. Objections? (Note that this is off corp, not using goma)
Message was sent while issue was closed.
Description was changed from ========== Rework default GN symbol handling for win goma builds. Win goma builds do not support PDB files, so trying to build with full symbols is very painful. Previously we would default to "full symbols" but silently turn off symbols if using goma; this CL restructures things so that we default to "minimal symbols" if using goma (and visual studio's compiler). R=brucedawson@chromium.org, brettw@chromium.org, thakis@chromium.org BUG=634184 Committed: https://crrev.com/64fea5e29655036e2c5410b903cd43f832d04e93 Cr-Commit-Position: refs/heads/master@{#415325} ========== to ========== Rework default GN symbol handling for win goma builds. Win goma builds do not support PDB files, so trying to build with full symbols is very painful. Previously we would default to "full symbols" but silently turn off symbols if using goma; this CL restructures things so that we default to "minimal symbols" if using goma (and visual studio's compiler). R=brucedawson@chromium.org, brettw@chromium.org, thakis@chromium.org BUG=634184 Committed: https://crrev.com/64fea5e29655036e2c5410b903cd43f832d04e93 Cr-Commit-Position: refs/heads/master@{#415325} ==========
Message was sent while issue was closed.
chrisha reported the same on internal windows list. I think it probably only needs an import of build\toolchain\goma.gni, yes, sounds fine to me to revert/reland.
Message was sent while issue was closed.
If this breaks your build, it likely breaks others, please revert! (it's likely missing an include goma.gni at the top - i guess the bots don't catch this 'cause they all explicitly set use_goma)
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:100001) has been created in https://codereview.chromium.org/2291293002/ by dpranke@chromium.org. The reason for reverting is: reverting, this breaks non-goma win builds since use_goma is undefined (i.e., I missed an import)..
Message was sent while issue was closed.
On 2016/08/30 18:20:01, Nico wrote: > If this breaks your build, it likely breaks others, please revert! > > (it's likely missing an include goma.gni at the top - i guess the bots don't > catch this 'cause they all explicitly set use_goma) Revert CQed
Message was sent while issue was closed.
On 2016/08/30 18:24:21, mmenke wrote: > On 2016/08/30 18:20:01, Nico wrote: > > If this breaks your build, it likely breaks others, please revert! > > > > (it's likely missing an include goma.gni at the top - i guess the bots don't > > catch this 'cause they all explicitly set use_goma) > > Revert CQed CQed, bypassing trybots, that is. And I verified reverting fixes the issue locally.
Message was sent while issue was closed.
On 2016/08/30 18:24:48, mmenke wrote: > On 2016/08/30 18:24:21, mmenke wrote: > > On 2016/08/30 18:20:01, Nico wrote: > > > If this breaks your build, it likely breaks others, please revert! > > > > > > (it's likely missing an include goma.gni at the top - i guess the bots don't > > > catch this 'cause they all explicitly set use_goma) > > > > Revert CQed > > CQed, bypassing trybots, that is. And I verified reverting fixes the issue > locally. let's see which revert wins :). |
