Implement use_cfi_cast to optionally enable cast checks.

Implement use_cfi_cast to optionally enable cast checks.

This is to allow launching cfi-vcal first, and follow up with additional strictness later.

BUG=626794, 464797
Committed: https://crrev.com/59d3718ac1328d46fac147720f078c53f63070cf
Cr-Commit-Position: refs/heads/master@{#404956}

[krasin, 2016-07-08 21:39:45]

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

[commit-bot: I haz the power, 2016-07-08 21:40:05]

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

[krasin, 2016-07-08 21:46:03]

The CQ bit was checked by krasin@google.com

[commit-bot: I haz the power, 2016-07-08 21:46:39]

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-07-08 21:46:40]

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

[commit-bot: I haz the power, 2016-07-08 21:46:41]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[krasin, 2016-07-08 21:48:43]

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

[commit-bot: I haz the power, 2016-07-08 21:49:14]

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

[krasin, 2016-07-08 22:00:10]

The CQ bit was unchecked by krasin@google.com

[krasin1, 2016-07-09 00:01:33]

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

[krasin1, 2016-07-09 00:01:34]

Peter, Dirk,

please, review this CL. It's a step forward gradual CFI launch: we'll only
launch CFI vcall check first, and follow up with unrelated-cast and derived-cast
checks in the next round.

[pcc1, 2016-07-11 20:42:05]

On 2016/07/09 00:01:34, krasin1 wrote:
> Peter, Dirk,
> 
> please, review this CL. It's a step forward gradual CFI launch: we'll only
> launch CFI vcall check first, and follow up with unrelated-cast and
derived-cast
> checks in the next round.

The test ToolsSanityTest.BadCast is failing. I would suggest changing that test
to perform a bad vcall instead.

[krasin1, 2016-07-11 21:50:43]

> 
> The test ToolsSanityTest.BadCast is failing. I would suggest changing that
test
> to perform a bad vcall instead.

Yes, I completely agree. I have added ToolsSanityTest.BadVirtualCall and guarded
ToolsSanityTest.BadCast with CFI_CAST_CHECK define.

Please, take another look.

[krasin1, 2016-07-11 21:50:47]

The CQ bit was checked by krasin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-11 21:51:20]

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

[pcc1, 2016-07-11 21:56:46]

pcc@chromium.org changed reviewers:
+ pcc@chromium.org

[pcc1, 2016-07-11 21:56:47]

https://codereview.chromium.org/2131423002/diff/60001/base/tools_sanity_unitt...
File base/tools_sanity_unittest.cc (right):

https://codereview.chromium.org/2131423002/diff/60001/base/tools_sanity_unitt...
base/tools_sanity_unittest.cc:378: EXPECT_DEATH(a_ptr->f(), "ILL_ILLOPN");
I'm not sure if this is quite the test we need here, because I would have
expected whole-program vtable optimization to devirtualize this call (and remove
the check here). If this test is passing locally for you, we should figure out
why.

[Dirk Pranke, 2016-07-11 22:56:03]

lgtm

[commit-bot: I haz the power, 2016-07-11 22:59:04]

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

[commit-bot: I haz the power, 2016-07-11 22:59:05]

Dry run: 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_...)

[krasin1, 2016-07-12 03:33:56]

https://codereview.chromium.org/2131423002/diff/60001/base/tools_sanity_unitt...
File base/tools_sanity_unittest.cc (right):

https://codereview.chromium.org/2131423002/diff/60001/base/tools_sanity_unitt...
base/tools_sanity_unittest.cc:378: EXPECT_DEATH(a_ptr->f(), "ILL_ILLOPN");
I've played a bit with the code, and the compiler gets confused with 

*reinterpret_cast<void**>(a_ptr) = *reinterpret_cast<void**>(&b);

all the time (with any possible outcome: not devirtualized, devirtualized,
incorrectly devirtualized). I have modified the code to make sure it could not
be devirtualized. Please, take a look.

[pcc1, 2016-07-12 18:54:28]

lgtm

https://codereview.chromium.org/2131423002/diff/80001/base/tools_sanity_unitt...
File base/tools_sanity_unittest.cc (right):

https://codereview.chromium.org/2131423002/diff/80001/base/tools_sanity_unitt...
base/tools_sanity_unittest.cc:375: __attribute__((noinline)) void
KillVptrAndCall(A *obj) {
Use NOINLINE macro here.

[krasin1, 2016-07-12 20:30:06]

Peter: all done, thanks!

Nico, can you please take a look? In particular, it wants your LGTM for
base/tools_sanity_unittest.cc.

https://codereview.chromium.org/2131423002/diff/80001/base/tools_sanity_unitt...
File base/tools_sanity_unittest.cc (right):

https://codereview.chromium.org/2131423002/diff/80001/base/tools_sanity_unitt...
base/tools_sanity_unittest.cc:375: __attribute__((noinline)) void
KillVptrAndCall(A *obj) {
On 2016/07/12 18:54:28, pcc1 wrote:
> Use NOINLINE macro here.

Done.

[krasin1, 2016-07-12 20:30:14]

The CQ bit was checked by krasin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-12 20:31:27]

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

[krasin1, 2016-07-12 21:09:55]

krasin@chromium.org changed reviewers:
+ thakis@chromium.org

[krasin1, 2016-07-12 21:09:55]

Nico, can you please take a look? In particular, it wants your LGTM for
base/tools_sanity_unittest.cc.

(repeating the request, as I forgot to add your email to the list of the
reviewers in the first time)

[krasin1, 2016-07-12 22:01:49]

Things are looking good so far in the CQ dry run. Pending the LGTM for
base/tools_sanity_unittests.cc from Nico, we should be good for the launch
tomorrow.

[Nico, 2016-07-12 23:23:59]

https://codereview.chromium.org/2131423002/diff/100001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/2131423002/diff/100001/build/common.gypi#newc...
build/common.gypi:6248: 'CFI_CAST_CHECK',
do we really have to globally add a define to every translation unit when the
define is only used in a single cc file? could this be on the base_unittests
target instead?

https://codereview.chromium.org/2131423002/diff/100001/build/config/sanitizer...
File build/config/sanitizers/BUILD.gn (right):

https://codereview.chromium.org/2131423002/diff/100001/build/config/sanitizer...
build/config/sanitizers/BUILD.gn:271: defines += [ "CFI_CAST_CHECK" ]
likewise

[commit-bot: I haz the power, 2016-07-12 23:36:11]

The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-12 23:36:12]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[krasin1, 2016-07-13 00:02:51]

https://codereview.chromium.org/2131423002/diff/100001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/2131423002/diff/100001/build/common.gypi#newc...
build/common.gypi:6248: 'CFI_CAST_CHECK',
On 2016/07/12 23:23:59, Nico wrote:
> do we really have to globally add a define to every translation unit when the
> define is only used in a single cc file? could this be on the base_unittests
> target instead?

Done.

https://codereview.chromium.org/2131423002/diff/100001/build/config/sanitizer...
File build/config/sanitizers/BUILD.gn (right):

https://codereview.chromium.org/2131423002/diff/100001/build/config/sanitizer...
build/config/sanitizers/BUILD.gn:271: defines += [ "CFI_CAST_CHECK" ]
On 2016/07/12 23:23:59, Nico wrote:
> likewise

Done.

[krasin1, 2016-07-13 00:03:00]

The CQ bit was checked by krasin@chromium.org

[krasin1, 2016-07-13 00:03:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, pcc@chromium.org, krasin@chromium.org
Link to the patchset: https://codereview.chromium.org/2131423002/#ps120001
(title: "Localize CFI_CAST_CHECK define")

[commit-bot: I haz the power, 2016-07-13 00:07:28]

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

[Nico, 2016-07-13 00:22:12]

lgtm

[commit-bot: I haz the power, 2016-07-13 02:16:21]

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

[commit-bot: I haz the power, 2016-07-13 02:16:23]

Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[krasin, 2016-07-13 03:01:00]

The CQ bit was checked by krasin@google.com

[commit-bot: I haz the power, 2016-07-13 03:02:32]

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-07-13 03:24:48]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-13 03:25:05]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 03:28:15]

Description was changed from

==========
Implement use_cfi_cast to optionally enable cast checks.

This is to allow launching cfi-vcal first, and follow up with additional
strictness later.

BUG=626794,464797
==========

to

==========
Implement use_cfi_cast to optionally enable cast checks.

This is to allow launching cfi-vcal first, and follow up with additional
strictness later.

BUG=626794,464797

Committed: https://crrev.com/59d3718ac1328d46fac147720f078c53f63070cf
Cr-Commit-Position: refs/heads/master@{#404956}
==========

[commit-bot: I haz the power, 2016-07-13 03:28:18]

Patchset 7 (id:??) landed as
https://crrev.com/59d3718ac1328d46fac147720f078c53f63070cf
Cr-Commit-Position: refs/heads/master@{#404956}