Add cfi_diag build flag.

Add cfi_diag build flag.

This flag causes the CFI instrumentation to print an error message to
stderr when a check fails, rather than terminating the process.

BUG=457523
R=thakis@chromium.org

Committed: https://crrev.com/080d985e11e8f9b0fd3c10f938404698c3b021ae
Cr-Commit-Position: refs/heads/master@{#339642}

[Nico, 2015-07-09 22:17:32]

The usual questions: Is this flag needed? Should we always use diag mode
unconditionally?

Which clang revision does this need?

[pcc1, 2015-07-09 22:39:57]

On 2015/07/09 22:17:32, Nico wrote:
> The usual questions: Is this flag needed? Should we always use diag mode
> unconditionally?

The purpose of the CFI instrumentation is to provide additional security by
terminating the process when a CFI violation occurs. Diagnostic mode will not
only enable diagnostics but disable terminate-on-error. This is useful during
development, as it allows developers to see multiple CFI violations at once,
together with the details of each, but we would generally want this feature
disabled in other scenarios, as it would defeat the purpose of the
instrumentation, and introduces binary size overhead in the form of RTTI and
additional runtime libraries.

> Which clang revision does this need?

The feature landed in r240109, which is beyond our current roll.

[Nico, 2015-07-10 03:22:42]

On 2015/07/09 22:39:57, pcc1 wrote:
> On 2015/07/09 22:17:32, Nico wrote:
> > The usual questions: Is this flag needed? Should we always use diag mode
> > unconditionally?
> 
> The purpose of the CFI instrumentation is to provide additional security by
> terminating the process when a CFI violation occurs. Diagnostic mode will not
> only enable diagnostics but disable terminate-on-error. This is useful during
> development, as it allows developers to see multiple CFI violations at once,
> together with the details of each, but we would generally want this feature
> disabled in other scenarios, as it would defeat the purpose of the
> instrumentation, and introduces binary size overhead in the form of RTTI and
> additional runtime libraries.

Maybe this could be keyed off buildtype?

> > Which clang revision does this need?
> 
> The feature landed in r240109, which is beyond our current roll.

I just landed a roll that includes this revision. We'll see if it sticks.

[pcc1, 2015-07-10 03:37:15]

On 2015/07/10 03:22:42, Nico wrote:
> On 2015/07/09 22:39:57, pcc1 wrote:
> > On 2015/07/09 22:17:32, Nico wrote:
> > > The usual questions: Is this flag needed? Should we always use diag mode
> > > unconditionally?
> > 
> > The purpose of the CFI instrumentation is to provide additional security by
> > terminating the process when a CFI violation occurs. Diagnostic mode will
not
> > only enable diagnostics but disable terminate-on-error. This is useful
during
> > development, as it allows developers to see multiple CFI violations at once,
> > together with the details of each, but we would generally want this feature
> > disabled in other scenarios, as it would defeat the purpose of the
> > instrumentation, and introduces binary size overhead in the form of RTTI and
> > additional runtime libraries.
> 
> Maybe this could be keyed off buildtype?

I'd prefer not to, as buildtype=Official changes the browser's behavior, and
when debugging the browser it would be useful to be able to change this flag
independently of the behavioral changes. (Say there is a CFI violation that only
happens in official builds.)

It does make sense to me for this flag's default to be based on the buildtype
though.

[pcc1, 2015-07-20 12:40:47]

On 2015/07/10 03:37:15, pcc1 wrote:
> On 2015/07/10 03:22:42, Nico wrote:
> > On 2015/07/09 22:39:57, pcc1 wrote:
> > > On 2015/07/09 22:17:32, Nico wrote:
> > > > The usual questions: Is this flag needed? Should we always use diag mode
> > > > unconditionally?
> > > 
> > > The purpose of the CFI instrumentation is to provide additional security
by
> > > terminating the process when a CFI violation occurs. Diagnostic mode will
> not
> > > only enable diagnostics but disable terminate-on-error. This is useful
> during
> > > development, as it allows developers to see multiple CFI violations at
once,
> > > together with the details of each, but we would generally want this
feature
> > > disabled in other scenarios, as it would defeat the purpose of the
> > > instrumentation, and introduces binary size overhead in the form of RTTI
and
> > > additional runtime libraries.
> > 
> > Maybe this could be keyed off buildtype?
> 
> I'd prefer not to, as buildtype=Official changes the browser's behavior, and
> when debugging the browser it would be useful to be able to change this flag
> independently of the behavioral changes. (Say there is a CFI violation that
only
> happens in official builds.)
> 
> It does make sense to me for this flag's default to be based on the buildtype
> though.

This now defaults to 0 in official builds and 1 in non-official builds.

[Nico, 2015-07-20 16:17:02]

Sorry, this fell out of my inbox.

I'll try one last time: In official builds, a CFI failure is the same as a crash
conceptually, right? So I'm not sure it makes sense to have a flag to treat it
differently. And in dev builds, I'm not sure if this is useful enough to make it
behave differently from official builds. People who know about this flag can use
the debug_extra_cflags / release_extra_cflags gyp_defines to set this, and
people who don't know about the flag won't find this gyp define either. So I
think I'd still prefer to not add Yet Another Build Flag for this.

If you still don't find this convincing, lgtm, I don't feel super strongly. I do
think we have way too many flags though, and I'm not sure if adding more helps
with that :-) (I'll point out that this adds the flag to gyp only, not gn, but
gn tries pretty hard to have fewer build flags, so that's probably a good thing)

[pcc1, 2015-07-21 11:47:00]

On 2015/07/20 16:17:02, Nico wrote:
> Sorry, this fell out of my inbox.
> 
> I'll try one last time: In official builds, a CFI failure is the same as a
crash
> conceptually, right? So I'm not sure it makes sense to have a flag to treat it
> differently. And in dev builds, I'm not sure if this is useful enough to make
it
> behave differently from official builds. People who know about this flag can
use
> the debug_extra_cflags / release_extra_cflags gyp_defines to set this, and
> people who don't know about the flag won't find this gyp define either. So I
> think I'd still prefer to not add Yet Another Build Flag for this.
> 
> If you still don't find this convincing, lgtm, I don't feel super strongly. I
do
> think we have way too many flags though, and I'm not sure if adding more helps
> with that :-) (I'll point out that this adds the flag to gyp only, not gn, but
> gn tries pretty hard to have fewer build flags, so that's probably a good
thing)

I did take a look to see whether it would work for these flags to be supplied
using *_extra_cflags, but it doesn't look like it will because the flags also
need to be in ldflags (on non-Windows) for the right runtime libs to be linked.
So I'm sorry, but it does look like this needs to be a build flag. I do plan to
document the flag on
https://sites.google.com/a/chromium.org/dev/developers/testing/control-flow-i...
so people will hopefully know about the flag if they need it.

I agree though that having a different default for official vs non-official
doesn't seem all that useful. So I've gone back to the default being always 0.

I don't anticipate adding any more flags for CFI, or at least I cannot think of
any CFI flag right now that would provide more value than the 2 I've already
added (cfi_vptr and cfi_diag). I guess I'll have to add them to GN at some point
later.

[pcc1, 2015-07-21 11:47:34]

The CQ bit was checked by pcc@chromium.org

[pcc1, 2015-07-21 11:47:34]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1231793002/#ps40001
(title: "Change default back")

[commit-bot: I haz the power, 2015-07-21 11:47:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1231793002/40001

[commit-bot: I haz the power, 2015-07-21 13:00:45]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-07-21 13:01:53]

Patchset 3 (id:??) landed as
https://crrev.com/080d985e11e8f9b0fd3c10f938404698c3b021ae
Cr-Commit-Position: refs/heads/master@{#339642}

[Nico, 2015-07-21 15:02:26]

On 2015/07/21 11:47:00, pcc1 wrote:
> On 2015/07/20 16:17:02, Nico wrote:
> > Sorry, this fell out of my inbox.
> > 
> > I'll try one last time: In official builds, a CFI failure is the same as a
> crash
> > conceptually, right? So I'm not sure it makes sense to have a flag to treat
it
> > differently. And in dev builds, I'm not sure if this is useful enough to
make
> it
> > behave differently from official builds. People who know about this flag can
> use
> > the debug_extra_cflags / release_extra_cflags gyp_defines to set this, and
> > people who don't know about the flag won't find this gyp define either. So I
> > think I'd still prefer to not add Yet Another Build Flag for this.
> > 
> > If you still don't find this convincing, lgtm, I don't feel super strongly.
I
> do
> > think we have way too many flags though, and I'm not sure if adding more
helps
> > with that :-) (I'll point out that this adds the flag to gyp only, not gn,
but
> > gn tries pretty hard to have fewer build flags, so that's probably a good
> thing)
> 
> I did take a look to see whether it would work for these flags to be supplied
> using *_extra_cflags, but it doesn't look like it will because the flags also
> need to be in ldflags (on non-Windows) for the right runtime libs to be
linked.

If that's the reasoning, I'd rather we add an extra_ldflags too.

> So I'm sorry, but it does look like this needs to be a build flag. I do plan
to
> document the flag on
>
https://sites.google.com/a/chromium.org/dev/developers/testing/control-flow-i...
> so people will hopefully know about the flag if they need it.
> 
> I agree though that having a different default for official vs non-official
> doesn't seem all that useful. So I've gone back to the default being always 0.
> 
> I don't anticipate adding any more flags for CFI, or at least I cannot think
of
> any CFI flag right now that would provide more value than the 2 I've already
> added (cfi_vptr and cfi_diag). I guess I'll have to add them to GN at some
point
> later.

[pcc1, 2015-07-22 16:11:08]

On 2015/07/21 15:02:26, Nico (hiding) wrote:
> On 2015/07/21 11:47:00, pcc1 wrote:
> > On 2015/07/20 16:17:02, Nico wrote:
> > > Sorry, this fell out of my inbox.
> > > 
> > > I'll try one last time: In official builds, a CFI failure is the same as a
> > crash
> > > conceptually, right? So I'm not sure it makes sense to have a flag to
treat
> it
> > > differently. And in dev builds, I'm not sure if this is useful enough to
> make
> > it
> > > behave differently from official builds. People who know about this flag
can
> > use
> > > the debug_extra_cflags / release_extra_cflags gyp_defines to set this, and
> > > people who don't know about the flag won't find this gyp define either. So
I
> > > think I'd still prefer to not add Yet Another Build Flag for this.
> > > 
> > > If you still don't find this convincing, lgtm, I don't feel super
strongly.
> I
> > do
> > > think we have way too many flags though, and I'm not sure if adding more
> helps
> > > with that :-) (I'll point out that this adds the flag to gyp only, not gn,
> but
> > > gn tries pretty hard to have fewer build flags, so that's probably a good
> > thing)
> > 
> > I did take a look to see whether it would work for these flags to be
supplied
> > using *_extra_cflags, but it doesn't look like it will because the flags
also
> > need to be in ldflags (on non-Windows) for the right runtime libs to be
> linked.
> 
> If that's the reasoning, I'd rather we add an extra_ldflags too.

I also remembered that we need to remove some flags when diagnostic mode is
enabled (https://codereview.chromium.org/1243373003). Is there a way to remove
flags with something like *_extra_*flags?