Enable CFI for virtual calls on Linux x86-64 official builds.

Enable CFI for virtual calls on Linux x86-64 official builds.

This is the second incremental step towards the full CFI launch.
In the first step, we enabled LinkTimeOptimization (LTO) for the
official Chrome builds, which allowed us to devirtualize
51491 site calls pointing to 23149 virtual methods:
https://storage.googleapis.com/cfi-stats/2016-08-15/devirt-methods.html

That sped up a few layout benchmarks by up to 7%
(see https://crbug.com/580389 and https://crbug.com/617283) and
more by 2%-3%.

In the current step, we add Control Flow Integrity checks for
virtual calls. As of now, some functions are excluded from CFI for
performance reasons by either tools/cfi/blacklist.txt or
DISABLE_CFI_PERF attribute.

Once we have proven that there're no perf regressions, we'll be
working on the compiler optimizations to allow reenabling CFI
on the currently suppressed functions.

The remaining part would be to add bad-cast checks to ensure the
forward-edge Control Flow Integrity works as planned. That 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%.

If we see any slowdown, the regressed microbenchmarks will be profiled,
and a few top methods will have CFI disabled on them. This is
the safety valve we intend to use until Clang is ready to generate
more efficient code in these cases.

BUG=464797
Committed: https://crrev.com/e4f3427521c7d96fb1772bd74c7938e39346d280
Cr-Commit-Position: refs/heads/master@{#413252}

[krasin, 2016-08-19 17:02:09]

krasin@google.com changed reviewers:
+ dpranke@chromium.org, esprehn@chromium.org

[krasin, 2016-08-19 17:02:09]

Hi Dirk,

please, approve this CL to enable CFI on the Linux x86-64 official builds. As in
the previous time, we'll be willing to revert if anything is slowed down too
much, but now we have a safety valve and a precise profiler for CFI checks, so
we might also just disable CFI on a few methods instead. We'll see.

This is still not an official launch, and when the performance impact is
evaluated (and possibly iterated on), we might need to revert it again, just to
comply with processes. But without trying, we can get any data on the slowdowns:
the local testing shows nothing.

Elliott: you have the "revert-for-no-reason" power as in the previous time.

[krasin, 2016-08-19 17:12:10]

The CQ bit was checked by krasin@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 17:12:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 18:16:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-19 18:16:20]

Dry run: This issue passed the CQ dry run.

[Dirk Pranke, 2016-08-19 20:22:10]

https://codereview.chromium.org/2259293002/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/2259293002/diff/1/build/common.gypi#newcode851
build/common.gypi:851: 'cfi_vptr%': 1,
you don't really need to change GYP at this point.

https://codereview.chromium.org/2259293002/diff/1/build/config/sanitizers/san...
File build/config/sanitizers/sanitizers.gni (right):

https://codereview.chromium.org/2259293002/diff/1/build/config/sanitizers/san...
build/config/sanitizers/sanitizers.gni:57: is_chrome_branded &&
is_official_build
Should we move this to //build/config/compiler/compiler.gni or toolchain.gni,
since this'll be being used in non-sanitizer builds?

[krasin, 2016-08-19 20:30:46]

PTAL

https://codereview.chromium.org/2259293002/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/2259293002/diff/1/build/common.gypi#newcode851
build/common.gypi:851: 'cfi_vptr%': 1,
On 2016/08/19 20:22:09, Dirk Pranke wrote:
> you don't really need to change GYP at this point.

That's useful to know. Reverted.

I am very glad that killing GYP is so close.

https://codereview.chromium.org/2259293002/diff/1/build/config/sanitizers/san...
File build/config/sanitizers/sanitizers.gni (right):

https://codereview.chromium.org/2259293002/diff/1/build/config/sanitizers/san...
build/config/sanitizers/sanitizers.gni:57: is_chrome_branded &&
is_official_build
On 2016/08/19 20:22:09, Dirk Pranke wrote:
> Should we move this to //build/config/compiler/compiler.gni or toolchain.gni,
> since this'll be being used in non-sanitizer builds?

Yes, theoretically it's worth discussing. But given that this CL has high
chances to be reverted, I would prefer to delay the discussion until later to
avoid complication aforementioned revert.

Let me know if you feel strongly, I am personally fine to move the setting
around.

[krasin, 2016-08-19 20:31:01]

The CQ bit was checked by krasin@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 20:31:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dirk Pranke, 2016-08-19 20:56:19]

okay. I think we should move the flags, but it's fine to do that in a follow-up
after this change sticks.

Maybe add a TODO?

lgtm.

[krasin, 2016-08-19 20:58:36]

On 2016/08/19 20:56:19, Dirk Pranke wrote:
> okay. I think we should move the flags, but it's fine to do that in a
follow-up
> after this change sticks.
> 
> Maybe add a TODO?
> 
> lgtm.

Filed https://crbug.com/639445 and assigned to myself.

Thank you!

[krasin, 2016-08-19 21:00:54]

The CQ bit was unchecked by krasin@google.com

[krasin, 2016-08-19 21:01:00]

The CQ bit was checked by krasin@google.com

[commit-bot: I haz the power, 2016-08-19 21:01:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 21:42:44]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-19 21:46:47]

Description was changed from

==========
Enable CFI for virtual calls on Linux x86-64 official builds.

This is the second incremental step towards the full CFI launch.
In the first step, we enabled LinkTimeOptimization (LTO) for the
official Chrome builds, which allowed us to devirtualize
51491 site calls pointing to 23149 virtual methods:
https://storage.googleapis.com/cfi-stats/2016-08-15/devirt-methods.html

That sped up a few layout benchmarks by up to 7%
(see https://crbug.com/580389 and https://crbug.com/617283) and
more by 2%-3%.

In the current step, we add Control Flow Integrity checks for
virtual calls. As of now, some functions are excluded from CFI for
performance reasons by either tools/cfi/blacklist.txt or
DISABLE_CFI_PERF attribute.

Once we have proven that there're no perf regressions, we'll be
working on the compiler optimizations to allow reenabling CFI
on the currently suppressed functions.

The remaining part would be to add bad-cast checks to ensure the
forward-edge Control Flow Integrity works as planned. That 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%.

If we see any slowdown, the regressed microbenchmarks will be profiled,
and a few top methods will have CFI disabled on them. This is
the safety valve we intend to use until Clang is ready to generate
more efficient code in these cases.

BUG=464797
==========

to

==========
Enable CFI for virtual calls on Linux x86-64 official builds.

This is the second incremental step towards the full CFI launch.
In the first step, we enabled LinkTimeOptimization (LTO) for the
official Chrome builds, which allowed us to devirtualize
51491 site calls pointing to 23149 virtual methods:
https://storage.googleapis.com/cfi-stats/2016-08-15/devirt-methods.html

That sped up a few layout benchmarks by up to 7%
(see https://crbug.com/580389 and https://crbug.com/617283) and
more by 2%-3%.

In the current step, we add Control Flow Integrity checks for
virtual calls. As of now, some functions are excluded from CFI for
performance reasons by either tools/cfi/blacklist.txt or
DISABLE_CFI_PERF attribute.

Once we have proven that there're no perf regressions, we'll be
working on the compiler optimizations to allow reenabling CFI
on the currently suppressed functions.

The remaining part would be to add bad-cast checks to ensure the
forward-edge Control Flow Integrity works as planned. That 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%.

If we see any slowdown, the regressed microbenchmarks will be profiled,
and a few top methods will have CFI disabled on them. This is
the safety valve we intend to use until Clang is ready to generate
more efficient code in these cases.

BUG=464797

Committed: https://crrev.com/e4f3427521c7d96fb1772bd74c7938e39346d280
Cr-Commit-Position: refs/heads/master@{#413252}
==========

[commit-bot: I haz the power, 2016-08-19 21:46:47]

Patchset 2 (id:??) landed as
https://crrev.com/e4f3427521c7d96fb1772bd74c7938e39346d280
Cr-Commit-Position: refs/heads/master@{#413252}