Remove CHECK around duplicate FLAG lookup.

Remove CHECK around duplicate FLAG lookup.

It causes an asan initialization-order-fiasco in trying to read the
names of other globally constructed data:

==21449==ERROR: AddressSanitizer: initialization-order-fiasco on address 0x7f6f297bc5e8 at pc 0x7f6f26b332a7 bp 0x7ffd479f8cb0 sp 0x7ffd479f8ca8
READ of size 8 at 0x7f6f297bc5e8 thread T0
    #0 0x7f6f26b332a6 in name
webrtc/base/flags.h:83:38
    #1 0x7f6f26b332a6 in Lookup
webrtc/base/flags.cc:133
    #2 0x7f6f26b332a6 in rtc::FlagList::Register(rtc::Flag*)
webrtc/base/flags.cc:260
    #3 0x7f6f2529972b in __cxx_global_var_init.1

BUG=

Committed: https://crrev.com/73ab917d27b6e66f075802c473cdec3d605ef5e5
Cr-Commit-Position: refs/heads/master@{#13479}

[noahric, 2016-06-30 17:21:30]

noahric@chromium.org changed reviewers:
+ mflodman@webrtc.org, pthatcher@google.com

[noahric, 2016-06-30 17:21:31]

Hey guys, let me know if this looks ok. It's required to run things under asan
if they use webrtc/base/flags.

[pthatcher1, 2016-06-30 18:25:58]

On 2016/06/30 17:21:31, noahric wrote:
> Hey guys, let me know if this looks ok. It's required to run things under asan
> if they use webrtc/base/flags.
 
I'd like to be sure about what asan is complaining about to make sure we're
fixing the root problem. 

It seems like what the code is doing is:

DEFINE_bool -> DEFINE_FLAG -> rtc::Flag(..., name, ...) ->
FlagList::Register(Flag*) -> FlagList::Lookup -> list_->name_.

So why is it not OK to lookup list_->name_ there?  Because it might be read
before list_ is set to NULL on line 108?

[noahric, 2016-06-30 18:53:53]

On 2016/06/30 18:25:58, pthatcher1 wrote:
> On 2016/06/30 17:21:31, noahric wrote:
> > Hey guys, let me know if this looks ok. It's required to run things under
asan
> > if they use webrtc/base/flags.
>  
> I'd like to be sure about what asan is complaining about to make sure we're
> fixing the root problem. 
> 
> It seems like what the code is doing is:
> 
> DEFINE_bool -> DEFINE_FLAG -> rtc::Flag(..., name, ...) ->
> FlagList::Register(Flag*) -> FlagList::Lookup -> list_->name_.
> 
> So why is it not OK to lookup list_->name_ there?  Because it might be read
> before list_ is set to NULL on line 108?

I don't claim to fully understand the rules about c++ initialization of globals,
but my general understanding is that the relative order of initialization of
globals in different compilation units is undefined. So if you have flags in two
source files, the relative order of initialization is undefined. And the problem
isn't that one flag may come before another, it's that the individual parts are
also not defined relative to each other. So though it's likely practically
impossible, you could have A.cc's Flag created and observable from B.cc's Flag,
but the char* name of A.cc's Flag hasn't been initialized yet.

[pthatcher1, 2016-06-30 20:49:39]

pthatcher@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[pthatcher1, 2016-06-30 20:49:39]

http://m.memegen.com/esml86.jpg

https://codereview.webrtc.org/2110963004/diff/1/webrtc/base/flags.cc
File webrtc/base/flags.cc (left):

https://codereview.webrtc.org/2110963004/diff/1/webrtc/base/flags.cc#oldcode261
webrtc/base/flags.cc:261: << " declared twice";
Then can we at least leave a comment here, perhaps something like this?

Warning: Don't call Lookup within Register because it accesses list_->name()
which may not be initialized yet.

[noahric, 2016-07-01 21:44:50]

On 2016/06/30 20:49:39, pthatcher1 wrote:
> http://m.memegen.com/esml86.jpg
> 
> https://codereview.webrtc.org/2110963004/diff/1/webrtc/base/flags.cc
> File webrtc/base/flags.cc (left):
> 
>
https://codereview.webrtc.org/2110963004/diff/1/webrtc/base/flags.cc#oldcode261
> webrtc/base/flags.cc:261: << " declared twice";
> Then can we at least leave a comment here, perhaps something like this?
> 
> Warning: Don't call Lookup within Register because it accesses list_->name()
> which may not be initialized yet.

Done. Also gave more info about the asan part.

[pthatcher1, 2016-07-01 22:05:55]

lgtm

[noahric, 2016-07-01 22:09:58]

The CQ bit was checked by noahric@chromium.org

[commit-bot: I haz the power, 2016-07-01 22:10:00]

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

[commit-bot: I haz the power, 2016-07-01 23:45:16]

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

[commit-bot: I haz the power, 2016-07-01 23:45:18]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[noahric, 2016-07-07 19:53:52]

The CQ bit was checked by noahric@chromium.org

[commit-bot: I haz the power, 2016-07-07 19:54:05]

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

[commit-bot: I haz the power, 2016-07-07 21:54:44]

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

[commit-bot: I haz the power, 2016-07-07 21:54:45]

Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[noahric, 2016-07-07 22:58:48]

The CQ bit was checked by noahric@chromium.org

[commit-bot: I haz the power, 2016-07-07 22:58:54]

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

[commit-bot: I haz the power, 2016-07-08 00:59:20]

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

[commit-bot: I haz the power, 2016-07-08 00:59:21]

Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[noahric, 2016-07-14 22:50:25]

The CQ bit was checked by noahric@chromium.org

[commit-bot: I haz the power, 2016-07-14 22:50:28]

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

[commit-bot: I haz the power, 2016-07-15 01:21:17]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-15 01:21:20]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-15 01:21:24]

Description was changed from

==========
Remove CHECK around duplicate FLAG lookup.

It causes an asan initialization-order-fiasco in trying to read the
names of other globally constructed data:

==21449==ERROR: AddressSanitizer: initialization-order-fiasco on address
0x7f6f297bc5e8 at pc 0x7f6f26b332a7 bp 0x7ffd479f8cb0 sp 0x7ffd479f8ca8
READ of size 8 at 0x7f6f297bc5e8 thread T0
    #0 0x7f6f26b332a6 in name
webrtc/base/flags.h:83:38
    #1 0x7f6f26b332a6 in Lookup
webrtc/base/flags.cc:133
    #2 0x7f6f26b332a6 in rtc::FlagList::Register(rtc::Flag*)
webrtc/base/flags.cc:260
    #3 0x7f6f2529972b in __cxx_global_var_init.1

BUG=
==========

to

==========
Remove CHECK around duplicate FLAG lookup.

It causes an asan initialization-order-fiasco in trying to read the
names of other globally constructed data:

==21449==ERROR: AddressSanitizer: initialization-order-fiasco on address
0x7f6f297bc5e8 at pc 0x7f6f26b332a7 bp 0x7ffd479f8cb0 sp 0x7ffd479f8ca8
READ of size 8 at 0x7f6f297bc5e8 thread T0
    #0 0x7f6f26b332a6 in name
webrtc/base/flags.h:83:38
    #1 0x7f6f26b332a6 in Lookup
webrtc/base/flags.cc:133
    #2 0x7f6f26b332a6 in rtc::FlagList::Register(rtc::Flag*)
webrtc/base/flags.cc:260
    #3 0x7f6f2529972b in __cxx_global_var_init.1

BUG=

Committed: https://crrev.com/73ab917d27b6e66f075802c473cdec3d605ef5e5
Cr-Commit-Position: refs/heads/master@{#13479}
==========

[commit-bot: I haz the power, 2016-07-15 01:21:25]

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