Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2106313002:
Enable whole-program vtable opt when CFI is enabled. (Closed)
DescriptionEnable whole-program vtable opt when CFI is enabled.
Following the roll to clang r273760, the compiler can now support
both of these features simultaneously.
Discussion thread for CFI: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pbJqt6ccMII/7iJC2oklCAAJ
BUG=464797, 580389
R=thakis@chromium.org,krasin@chromium.org
Committed: https://crrev.com/4811641ea017bef631852cc0ac1aa8f7ac85385d
Cr-Commit-Position: refs/heads/master@{#403599}
Patch Set 1 #
Messages
Total messages: 22 (4 generated)
On 2016/06/30 03:12:22, pcc1 wrote: LGTM++ Thank you Peter! Please, allow the CFI trybot to succeed before landing this patch.
Ugh. Turns out, 'CFI Linux' buildbot is red, as of now. I highly recommend to make the bot green before submitting this CL. https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux/builds/5926
lgtm (but krasin's suggestion makes sense to me). Please point to the chromium-dev discussion thread for this in the CL description. Does this have a big impact on build time?
On 2016/06/30 17:06:42, Nico wrote: > lgtm (but krasin's suggestion makes sense to me). Okay, I'll take a look at the failures first. > Please point to the > chromium-dev discussion thread for this in the CL description. The discussion thread for CFI? Will do. The whole-program vtable opt feature was discussed in crbug.com/580389 , not on chromium-dev. I'll also add that bug to the CL description. > Does this have a big impact on build time? I don't think so (this CL doesn't affect whether we use LTO anywhere).
Description was changed from ========== Enable whole-program vtable opt when CFI is enabled. Following the roll to clang r273760, the compiler can now support both of these features simultaneously. BUG=464797 R=thakis@chromium.org,krasin@chromium.org ========== to ========== Enable whole-program vtable opt when CFI is enabled. Following the roll to clang r273760, the compiler can now support both of these features simultaneously. Discussion thread for CFI: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pbJqt6ccMII/7iJC2... BUG=464797,580389 R=thakis@chromium.org,krasin@chromium.org ==========
On 2016/06/30 18:06:45, pcc1 wrote: > On 2016/06/30 17:06:42, Nico wrote: > > lgtm (but krasin's suggestion makes sense to me). > > Okay, I'll take a look at the failures first. > > > Please point to the > > chromium-dev discussion thread for this in the CL description. > > The discussion thread for CFI? Will do. > > The whole-program vtable opt feature was discussed in crbug.com/580389 , not on > chromium-dev. I'll also add that bug to the CL description. Oh, I somehow halucinated we had a thread on this as well. Might be a good idea to send an "intent to implement" for this as well, just to make sure people are (vaguely) aware of this.
On 2016/06/30 18:09:08, Nico wrote: > On 2016/06/30 18:06:45, pcc1 wrote: > > On 2016/06/30 17:06:42, Nico wrote: > > > lgtm (but krasin's suggestion makes sense to me). > > > > Okay, I'll take a look at the failures first. > > > > > Please point to the > > > chromium-dev discussion thread for this in the CL description. > > > > The discussion thread for CFI? Will do. > > > > The whole-program vtable opt feature was discussed in crbug.com/580389 , not > on > > chromium-dev. I'll also add that bug to the CL description. > > Oh, I somehow halucinated we had a thread on this as well. Might be a good idea > to send an "intent to implement" for this as well, just to make sure people are > (vaguely) aware of this. Well, we've already rolled out whole-program vtable opt in official Linux builds, so I'm not sure if sending an "intent to implement" makes sense at this point.
I'm confused :-) Is the state of the world that we ship chrome/linux with LTO and vtable opts, but we don't ship CFI yet, and this enables turning on CFI in addition to LTO / vtable opts (while before this, turning on CFI would disable vtable opts)? If so, then I agree that sending email about this doesn't make sense.
On 2016/06/30 18:16:30, Nico wrote: > I'm confused :-) > > Is the state of the world that we ship chrome/linux with LTO and vtable opts, > but we don't ship CFI yet, and this enables turning on CFI in addition to LTO / > vtable opts (while before this, turning on CFI would disable vtable opts)? Yes. (Just to be clear, I'm not changing any defaults here. CFI still needs to be explicitly turned on with a build flag.)
Ok, lgtm, sorry about being confused.
The failures on the bot are: https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux/builds/5924/st... https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux/builds/5924/st... https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux/builds/5924/st... Sent fixes for the first two: https://codereview.chromium.org/2118503004/ https://codereview.chromium.org/2119683002/ The last one would seem to be unrelated to CFI, but I can't seem to find a non-CFI bot on which it is failing. Taking a closer look.
> https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux/builds/5924/st... Can reproduce locally without CFI. Whatever's going on here, it has nothing to do with CFI and is more likely related to this particular bot's configuration.
On 2016/06/30 22:30:24, pcc1 wrote: > > > https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux/builds/5924/st... > > Can reproduce locally without CFI. Whatever's going on here, it has nothing to > do with CFI and is more likely related to this particular bot's configuration. Notified the author of the CL: https://codereview.chromium.org/1998653002/
Thank you, Peter! (sorry for a slow reply, I am still away)
The only remaining redness on CFI Linux is because of the views_unittests failure, which is known to be unrelated to CFI. I'll go ahead and land this and keep an eye on the bot to make sure we don't regress anything else.
The CQ bit was checked by pcc@chromium.org
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 ========== Enable whole-program vtable opt when CFI is enabled. Following the roll to clang r273760, the compiler can now support both of these features simultaneously. Discussion thread for CFI: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pbJqt6ccMII/7iJC2... BUG=464797,580389 R=thakis@chromium.org,krasin@chromium.org ========== to ========== Enable whole-program vtable opt when CFI is enabled. Following the roll to clang r273760, the compiler can now support both of these features simultaneously. Discussion thread for CFI: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pbJqt6ccMII/7iJC2... BUG=464797,580389 R=thakis@chromium.org,krasin@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Enable whole-program vtable opt when CFI is enabled. Following the roll to clang r273760, the compiler can now support both of these features simultaneously. Discussion thread for CFI: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pbJqt6ccMII/7iJC2... BUG=464797,580389 R=thakis@chromium.org,krasin@chromium.org ========== to ========== Enable whole-program vtable opt when CFI is enabled. Following the roll to clang r273760, the compiler can now support both of these features simultaneously. Discussion thread for CFI: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pbJqt6ccMII/7iJC2... BUG=464797,580389 R=thakis@chromium.org,krasin@chromium.org Committed: https://crrev.com/4811641ea017bef631852cc0ac1aa8f7ac85385d Cr-Commit-Position: refs/heads/master@{#403599} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/4811641ea017bef631852cc0ac1aa8f7ac85385d Cr-Commit-Position: refs/heads/master@{#403599} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
