Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(370)

Issue 2140373002: Launch CFI for virtual calls on Linux x86-64. (Closed)

Created:
4 years, 5 months ago by krasin
Modified:
4 years, 5 months ago
Reviewers:
krasin1, esprehn, Nico
CC:
chromium-reviews, kcc2, pcc1
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Launch CFI for virtual calls on Linux x86-64. This is the second incremental step towards the full CFI launch. In the first step, we enabled LinkTimeOptimization (LTO) for the official Chrome builds. In this step we add Control Flow Integrity checks for all virtual calls. The remaining part is to add bad-cast checks to ensure the forward-edge Control Flow Integrity works as planned. That remaining part will require more work on reducing the overhead for size and speed by these CFI checks, so we don't enable them right away. The expected Perf impact by this CL: - Chrome binary size is increased by 5%, - Some of the benchmarks are slowed down by up to 3.5%. Note that before making it slower, we made it faster by implementing virtual const propagation and a number of heuristics for automatic devirtualization in LLVM which sped up some layout benchmarks by up to 7% (see https://crbug.com/580389 and https://crbug.com/617283) If there's a higher (negative) impact, we'll be willing to roll this feature back, but please allow the Perf bots to work for a day or two to collect more detailed statistics on the regressions, as it will help us to identify ways to speed it up (most likely, by inventing new ways for automatic devirtualization). BUG=464797 Committed: https://crrev.com/01f474c48200a1e556a4cf668e2b5dbda0f38a6f Cr-Commit-Position: refs/heads/master@{#405894}

Patch Set 1 #

Patch Set 2 : add missing gn include #

Unified diffs Side-by-side diffs Delta from patch set Stats (+6 lines, -3 lines) Patch
M build/common.gypi View 1 1 chunk +1 line, -1 line 0 comments Download
M build/config/sanitizers/sanitizers.gni View 1 2 chunks +5 lines, -2 lines 0 comments Download

Messages

Total messages: 25 (15 generated)
krasin1
Hi Nico, please, approve this launch CL for the subset of CFI (only virtual calls). ...
4 years, 5 months ago (2016-07-12 23:18:44 UTC) #2
Nico
lgtm, but you might want to mention in the cl description that the perf regression ...
4 years, 5 months ago (2016-07-12 23:23:47 UTC) #3
krasin1
On 2016/07/12 23:23:47, Nico wrote: > lgtm, but you might want to mention in the ...
4 years, 5 months ago (2016-07-12 23:29:26 UTC) #5
krasin
Elliott, may I get your LGTM on this? As discussed, you have the right to ...
4 years, 5 months ago (2016-07-15 17:59:42 UTC) #9
esprehn
On 2016/07/15 at 17:59:42, krasin wrote: > Elliott, > > may I get your LGTM ...
4 years, 5 months ago (2016-07-15 21:14:12 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2140373002/20001
4 years, 5 months ago (2016-07-15 23:14:42 UTC) #19
commit-bot: I haz the power
Committed patchset #2 (id:20001)
4 years, 5 months ago (2016-07-15 23:20:23 UTC) #21
commit-bot: I haz the power
CQ bit was unchecked.
4 years, 5 months ago (2016-07-15 23:20:26 UTC) #22
commit-bot: I haz the power
Patchset 2 (id:??) landed as https://crrev.com/01f474c48200a1e556a4cf668e2b5dbda0f38a6f Cr-Commit-Position: refs/heads/master@{#405894}
4 years, 5 months ago (2016-07-15 23:23:14 UTC) #24
krasin1
4 years, 5 months ago (2016-07-16 17:58:26 UTC) #25
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2154993002/ by krasin@chromium.org.

The reason for reverting is: Too many blink_perf.layout benchmarks regressed by
~3.5%. While the regressions are within predicted upper bound, there're too many
of them to ignore:

https://chromeperf.appspot.com/report?sid=ebf0165d8c96c7a70c790d179a9bdc1f9e5...

We will need to reevaluate the reason for such consistent slowdown and will make
another attempt after it's cleared..

Powered by Google App Engine
This is Rietveld 408576698