Add use_rtti flag to control Run-Time Type Identification

Add use_rtti flag to control Run-Time Type Identification

BUG=webrtc:6468
TESTED=Compiled all of WebRTC in Debug+Release with use_rtti=true

[kjellander_chromium, 2017-01-02 07:46:24]

Description was changed from

==========
Add use_rtti flag to control Run-Time Type Identification

BUG=webrtc:6468
==========

to

==========
Add use_rtti flag to control Run-Time Type Identification

BUG=webrtc:6468
TESTED=Compiled all of WebRTC in Debug+Release with use_rtti=true
==========

[kjellander_chromium, 2017-01-02 07:46:24]

kjellander@chromium.org changed reviewers:
+ dpranke@chromium.org

[kjellander_chromium, 2017-01-02 07:47:08]

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

[commit-bot: I haz the power, 2017-01-02 07:47:14]

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

[kjellander_chromium, 2017-01-02 07:47:15]

https://codereview.chromium.org/2603183002/diff/1/build/config/BUILDCONFIG.gn
File build/config/BUILDCONFIG.gn (right):

https://codereview.chromium.org/2603183002/diff/1/build/config/BUILDCONFIG.gn...
build/config/BUILDCONFIG.gn:167: use_rtti = false
Let me know if there's a better place to put this.

[tsniatowski, 2017-01-02 09:11:16]

On 2017/01/02 07:47:15, kjellander_chromium wrote:
> https://codereview.chromium.org/2603183002/diff/1/build/config/BUILDCONFIG.gn
> File build/config/BUILDCONFIG.gn (right):
> 
>
https://codereview.chromium.org/2603183002/diff/1/build/config/BUILDCONFIG.gn...
> build/config/BUILDCONFIG.gn:167: use_rtti = false
> Let me know if there's a better place to put this.

This will break targets that want to explicitly have rtti enabled always and do 
  configs -= [ "no_rtti" ]
  configs += [ "rtti" ]
because now when use_rtti is true there won't be "no_rtti" on the config list.

I think https://codereview.chromium.org/2607903002/ (that I coincidentally
uploaded just a few days ago) is better. It reuses existing "sometimes rtti"
approach a bunch of   sanitizers need, and adds an arg to control it explicitly.

[kjellander_chromium, 2017-01-02 09:19:45]

On 2017/01/02 09:11:16, tsniatowski wrote:
> On 2017/01/02 07:47:15, kjellander_chromium wrote:
> >
https://codereview.chromium.org/2603183002/diff/1/build/config/BUILDCONFIG.gn
> > File build/config/BUILDCONFIG.gn (right):
> > 
> >
>
https://codereview.chromium.org/2603183002/diff/1/build/config/BUILDCONFIG.gn...
> > build/config/BUILDCONFIG.gn:167: use_rtti = false
> > Let me know if there's a better place to put this.
> 
> This will break targets that want to explicitly have rtti enabled always and
do 
>   configs -= [ "no_rtti" ]
>   configs += [ "rtti" ]
> because now when use_rtti is true there won't be "no_rtti" on the config list.

That's right, but shouldn't it be better to just update those targets instead?
IMO you shouldn't have to do things like that to enable/disable a feature.

> I think https://codereview.chromium.org/2607903002/ (that I coincidentally
> uploaded just a few days ago) is better. It reuses existing "sometimes rtti"
> approach a bunch of   sanitizers need, and adds an arg to control it
explicitly.

I'm open to whatever solution works for everyone, but won't you need to add a
dependency on the //build/config/compiler:rtti config in addition to setting
use_rtti in order to use it?
When I tried enabling rtti for WebRTC by adding the config to all our own
targets (via our templates in
https://cs.chromium.org/chromium/src/third_party/webrtc/build/webrtc.gni), I
still got compile errors since libraries like gtest wasn't built with rtti
enabled. That's why I think it needs to be added on a global scope as well, for
users that needs it enabled everywhere and not just sometimes...

[kjellander_chromium, 2017-01-02 09:20:22]

+tsniatowski since I responded to your comment a moment ago.

[tsniatowski, 2017-01-02 09:27:24]

On 2017/01/02 09:19:45, kjellander_chromium wrote:
> On 2017/01/02 09:11:16, tsniatowski wrote:
> > On 2017/01/02 07:47:15, kjellander_chromium wrote:
> > >
> https://codereview.chromium.org/2603183002/diff/1/build/config/BUILDCONFIG.gn
> > > File build/config/BUILDCONFIG.gn (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2603183002/diff/1/build/config/BUILDCONFIG.gn...
> > > build/config/BUILDCONFIG.gn:167: use_rtti = false
> > > Let me know if there's a better place to put this.
> > 
> > This will break targets that want to explicitly have rtti enabled always and
> do 
> >   configs -= [ "no_rtti" ]
> >   configs += [ "rtti" ]
> > because now when use_rtti is true there won't be "no_rtti" on the config
list.
> 
> That's right, but shouldn't it be better to just update those targets instead?
> IMO you shouldn't have to do things like that to enable/disable a feature.

I think it's not an improvement if targets that want rtti always enabled have to
do an "if (use_rtti)" together with the config -= +=.

> 
> > I think https://codereview.chromium.org/2607903002/ (that I coincidentally
> > uploaded just a few days ago) is better. It reuses existing "sometimes rtti"
> > approach a bunch of   sanitizers need, and adds an arg to control it
> explicitly.
> 
> I'm open to whatever solution works for everyone, but won't you need to add a
> dependency on the //build/config/compiler:rtti config in addition to setting
> use_rtti in order to use it?
> When I tried enabling rtti for WebRTC by adding the config to all our own
> targets (via our templates in
> https://cs.chromium.org/chromium/src/third_party/webrtc/build/webrtc.gni), I
> still got compile errors since libraries like gtest wasn't built with rtti
> enabled. That's why I think it needs to be added on a global scope as well,
for
> users that needs it enabled everywhere and not just sometimes...

Rtti is enabled by default in the toolchains I use, so neutering the "no_rtti"
config so that it doesn't disable rtii is enough to have rtti enabled everywhere
-- without actually adding the "rtti" config. That seems to be good enough for
sanitizer builds, which can be picky about consistent rtti state. Does it work
for you?

[commit-bot: I haz the power, 2017-01-02 10:03:45]

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

[commit-bot: I haz the power, 2017-01-02 10:03:46]

Dry run: This issue passed the CQ dry run.

[Dirk Pranke, 2017-01-04 02:42:54]

lgtm if/when kjellander@ is happy with it.

[Dirk Pranke, 2017-01-04 02:56:02]

On 2017/01/04 02:42:54, Dirk Pranke wrote:
> lgtm if/when kjellander@ is happy with it.

Also, does this replace https://codereview.chromium.org/2607903002/ ?

[tsniatowski, 2017-01-04 07:00:46]

On 2017/01/04 02:56:02, Dirk Pranke wrote:
> On 2017/01/04 02:42:54, Dirk Pranke wrote:
> > lgtm if/when kjellander@ is happy with it.
> 
> Also, does this replace https://codereview.chromium.org/2607903002/ ?

I don't fancy the change here because it means 
* there will be two separate paths that disable rtti 
** the no_rtti config is added by default or is not added, and it actually
disables rtti or doesn't do anything
* use_rtti=true when building all of chromium with rtti on globally will still
need more patching since various places will break
**
https://cs.chromium.org/search/?q=compiler:no_rtti&sq=package:chromium&type=cs
** angle, libc++, breakpad, icu and khronos bits will need fixing

So basically this here is not usable for me without further patching, and IMO
it's bad that it's inconsistent with how some sanitizers ensure rtti stays on.

[Dirk Pranke, 2017-01-05 02:30:40]

On 2017/01/04 07:00:46, tsniatowski wrote:
> On 2017/01/04 02:56:02, Dirk Pranke wrote:
> > On 2017/01/04 02:42:54, Dirk Pranke wrote:
> > > lgtm if/when kjellander@ is happy with it.
> > 
> > Also, does this replace https://codereview.chromium.org/2607903002/ ?
> 
> I don't fancy the change here because it means 
> * there will be two separate paths that disable rtti 
> ** the no_rtti config is added by default or is not added, and it actually
> disables rtti or doesn't do anything
> * use_rtti=true when building all of chromium with rtti on globally will still
> need more patching since various places will break
> **
> https://cs.chromium.org/search/?q=compiler:no_rtti&sq=package:chromium&type=cs
> ** angle, libc++, breakpad, icu and khronos bits will need fixing
> 
> So basically this here is not usable for me without further patching, and IMO
> it's bad that it's inconsistent with how some sanitizers ensure rtti stays on.

Okay, so, this was actually kjellander's patch and so my review comment makes no
sense :).

That, plus tsniatowski's reply means I'm actually going to look at these two CLs
and form an opinion. 

I will try to get to this tomorrow; apologies for the delays and confusion.

[Dirk Pranke, 2017-01-09 06:33:20]

On 2017/01/05 02:30:40, Dirk Pranke wrote:
> On 2017/01/04 07:00:46, tsniatowski wrote:
> > On 2017/01/04 02:56:02, Dirk Pranke wrote:
> > > On 2017/01/04 02:42:54, Dirk Pranke wrote:
> > > > lgtm if/when kjellander@ is happy with it.
> > > 
> > > Also, does this replace https://codereview.chromium.org/2607903002/ ?
> > 
> > I don't fancy the change here because it means 
> > * there will be two separate paths that disable rtti 
> > ** the no_rtti config is added by default or is not added, and it actually
> > disables rtti or doesn't do anything
> > * use_rtti=true when building all of chromium with rtti on globally will
still
> > need more patching since various places will break
> > **
> >
https://cs.chromium.org/search/?q=compiler:no_rtti&sq=package:chromium&type=cs
> > ** angle, libc++, breakpad, icu and khronos bits will need fixing
> > 
> > So basically this here is not usable for me without further patching, and
IMO
> > it's bad that it's inconsistent with how some sanitizers ensure rtti stays
on.
> 
> Okay, so, this was actually kjellander's patch and so my review comment makes
no
> sense :).
> 
> That, plus tsniatowski's reply means I'm actually going to look at these two
CLs
> and form an opinion. 
> 
> I will try to get to this tomorrow; apologies for the delays and confusion.

I agree that tsniatowski's CL is the way we should go. Let's close this and land
that one instead.

[kjellander_chromium, 2017-01-09 07:05:16]

On 2017/01/09 06:33:20, Dirk Pranke wrote:
> On 2017/01/05 02:30:40, Dirk Pranke wrote:
> > On 2017/01/04 07:00:46, tsniatowski wrote:
> > > On 2017/01/04 02:56:02, Dirk Pranke wrote:
> > > > On 2017/01/04 02:42:54, Dirk Pranke wrote:
> > > > > lgtm if/when kjellander@ is happy with it.
> > > > 
> > > > Also, does this replace https://codereview.chromium.org/2607903002/ ?
> > > 
> > > I don't fancy the change here because it means 
> > > * there will be two separate paths that disable rtti 
> > > ** the no_rtti config is added by default or is not added, and it actually
> > > disables rtti or doesn't do anything
> > > * use_rtti=true when building all of chromium with rtti on globally will
> still
> > > need more patching since various places will break
> > > **
> > >
> https://cs.chromium.org/search/?q=compiler:no_rtti&sq=package:chromium&type=cs
> > > ** angle, libc++, breakpad, icu and khronos bits will need fixing
> > > 
> > > So basically this here is not usable for me without further patching, and
> IMO
> > > it's bad that it's inconsistent with how some sanitizers ensure rtti stays
> on.
> > 
> > Okay, so, this was actually kjellander's patch and so my review comment
makes
> no
> > sense :).
> > 
> > That, plus tsniatowski's reply means I'm actually going to look at these two
> CLs
> > and form an opinion. 
> > 
> > I will try to get to this tomorrow; apologies for the delays and confusion.
> 
> I agree that tsniatowski's CL is the way we should go. Let's close this and
land
> that one instead.

Yes that's what I expected too. I don't mind. Closing....