|
|
DescriptionGN: Use icf=all instead of icf=safe
BUG=628441
Committed: https://crrev.com/b2eb9de87515796cf9b7c17fd31a34d92427eb8b
Cr-Commit-Position: refs/heads/master@{#405909}
Patch Set 1 #
Total comments: 5
Patch Set 2 : cros only #Messages
Total messages: 22 (6 generated)
stevenjb@chromium.org changed reviewers: + agrieve@chromium.org, dpranke@chromium.org, llozano@chromium.org, mbjorge@chromium.org
crbug.com/576197 is marked resoved. 'safe' vs. 'all' increases the chrome on chromeos binary size for x86 by ~5%.
The CQ bit was checked by stevenjb@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...
https://codereview.chromium.org/2153223002/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2153223002/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:362: } else { won't this still break linux/gcc x86? From my reading of 576197, the underlying gold issue hasn't been fixed yet.
https://codereview.chromium.org/2153223002/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2153223002/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:362: } else { On 2016/07/15 21:08:42, Dirk Pranke wrote: > won't this still break linux/gcc x86? From my reading of 576197, the underlying > gold issue hasn't been fixed yet. I didn't read through all of it, I assumed that since it was resolved it was, well, fixed. If it isn't resolved on linux (which I assume includes chromeos?) we need to reopen the issue or open another issue, update this comment, and prioritize it, since a 5% size increase is pretty awful.
https://codereview.chromium.org/2153223002/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2153223002/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:362: } else { On 2016/07/15 21:08:42, Dirk Pranke wrote: > won't this still break linux/gcc x86? From my reading of 576197, the underlying > gold issue hasn't been fixed yet. this has been in use by chromeos for a while but it has not been tested on linux or Android. I would prefer we figure out why the option was overriden by GN and allow to make it a ChromeOS thing only.
So, is this the change we want?
https://codereview.chromium.org/2153223002/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2153223002/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:362: } else { I agree that we should update the comment, possibly to refer to the binutils bug instead of the chromium bug (https://sourceware.org/bugzilla/show_bug.cgi?id=17704). It looks like the bug was reported after we had switched the linux build to GN, so I'm guessing we just never applied the "fix" to the GYP files, and so the GYP-based CrOS builds were unaffected and stayed on --icf=all. I'm fine w/ you changing the code so that you get --icf=all on CrOS x86 if you think it's safe to do so, just don't break the other platforms where we still think it isn't safe :). (and, no pun intended).
https://codereview.chromium.org/2153223002/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2153223002/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:362: } else { On 2016/07/15 21:30:05, Dirk Pranke wrote: > I agree that we should update the comment, possibly to refer to the binutils bug > instead of the chromium bug > (https://sourceware.org/bugzilla/show_bug.cgi?id=17704). > > It looks like the bug was reported after we had switched the linux build to GN, > so I'm guessing we just never applied the "fix" to the GYP files, and so the > GYP-based CrOS builds were unaffected and stayed on --icf=all. > > I'm fine w/ you changing the code so that you get --icf=all on CrOS x86 if you > think it's safe to do so, just don't break the other platforms where we still > think it isn't safe :). (and, no pun intended). So, I don't really understand the problem or the solution well enough to meaningfully update the comment. llozano@, if you or someone on your team could provide a better patch that would be great, otherwise someone can l-g-t-m this, just whatever it takes to get this fix in before Chrome OS 54 goes to Beta, thanks!
lgtm.
llozano@google.com changed reviewers: + llozano@google.com
lgtm
The CQ bit was checked by stevenjb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I was looking at this in more detail and the icf bug is not fixed yet. No one in ChromeOS has reported this ... So we may have to disable icf on ChromeOS too until there is a fix.. Your CL is ok in the sense that it brings us back to the previous state in terms of code size but now I need to look at this problem. I will file a different CL for this. Luis On Fri, Jul 15, 2016 at 3:27 PM, commit-bot@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > CQ is trying da patch. Follow status at > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > > https://codereview.chromium.org/2153223002/ > -- Luis A. Lozano | Software Engineer | llozano@google.com | +1 (408)431-5164 -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== GN: Use icf=all instead of icf=safe BUG=628441 ========== to ========== GN: Use icf=all instead of icf=safe BUG=628441 Committed: https://crrev.com/b2eb9de87515796cf9b7c17fd31a34d92427eb8b Cr-Commit-Position: refs/heads/master@{#405909} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/b2eb9de87515796cf9b7c17fd31a34d92427eb8b Cr-Commit-Position: refs/heads/master@{#405909} |