Require SK_DECLARE_STATIC_LAZY_PTR is used in global scope.

Require SK_DECLARE_STATIC_LAZY_PTR is used in global scope.

Function- or method- local scope isn't threadsafe; the pointer is generally
zero-initialized on first use in function scope (i.e. lazily... we have to go
deeper), but for globals we can be pretty sure the linker will do that for us.

BUG=skia:

No public API changes.
TBR=reed@google.com

Committed: https://skia.googlesource.com/skia/+/148ec59001ca7d7e54aec580a048c6dd2a085991

[mtklein, 2014-10-13 15:52:35]

mtklein@google.com changed reviewers:
+ bungeman@google.com, mtklein@google.com, reed@google.com

[mtklein, 2014-10-13 15:52:35]

mtklein@google.com changed required reviewers:
+ bungeman@google.com, reed@google.com

[bungeman-skia, 2014-10-13 16:31:31]

lgtm, do we have any more XXX_STATIC_XXX macros that need to be treated this way
as well?

[mtklein, 2014-10-13 17:53:12]

On 2014/10/13 16:31:31, bungeman1 wrote:
> lgtm, do we have any more XXX_STATIC_XXX macros that need to be treated this
way
> as well?

Yes, one more, SK_DECLARE_STATIC_ONCE.  Will follow up with that.

[mtklein, 2014-10-13 18:02:10]

On 2014/10/13 17:53:12, mtklein wrote:
> On 2014/10/13 16:31:31, bungeman1 wrote:
> > lgtm, do we have any more XXX_STATIC_XXX macros that need to be treated this
> way
> > as well?
> 
> Yes, one more, SK_DECLARE_STATIC_ONCE.  Will follow up with that.

FWIW, SK_DECLARE_STATIC_ONCE is used in only a couple of places, but used in
ways that are harder to just force into global space than this one.  The follow
up may take a while.

[mtklein, 2014-10-13 18:33:35]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-10-13 18:38:01]

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

[commit-bot: I haz the power, 2014-10-13 18:40:11]

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

[commit-bot: I haz the power, 2014-10-13 18:40:12]

Try jobs failed on following builders:
  Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Android-Trybot on client.skia.compile
(http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu13.10-GC...)
  Build-Ubuntu13.10-GCC4.8-x86_64-Release-Trybot on client.skia.compile
(http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu13.10-GC...)

[mtklein, 2014-10-13 19:06:50]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-10-13 19:07:33]

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

[commit-bot: I haz the power, 2014-10-13 19:10:45]

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

[commit-bot: I haz the power, 2014-10-13 19:10:46]

Try jobs failed on following builders:
  Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Android-Trybot on client.skia.compile
(http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu13.10-GC...)

[mtklein, 2014-10-13 19:12:40]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-10-13 19:13:17]

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

[commit-bot: I haz the power, 2014-10-13 19:39:52]

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

[commit-bot: I haz the power, 2014-10-13 19:39:53]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on client.skia
(http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x...)

[mtklein, 2014-10-13 19:46:26]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-10-13 19:46:46]

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

[commit-bot: I haz the power, 2014-10-13 20:00:29]

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

[commit-bot: I haz the power, 2014-10-13 20:00:30]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on client.skia
(http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x...)

[mtklein, 2014-10-13 20:08:39]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-10-13 20:09:00]

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

[commit-bot: I haz the power, 2014-10-13 20:18:00]

Committed patchset #5 (id:550001) as 148ec59001ca7d7e54aec580a048c6dd2a085991

[brucedawson, 2015-02-09 18:38:00]

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

[brucedawson, 2015-02-09 18:38:01]

I noticed this change because of some new variable shadowing warnings in the
VC++ /analyze build. These warnings point out that skmessagebus.h's macros
declare a global variable named 'bus' and then some local variables named 'bus'.
The global variable should be renamed to avoid these warnings and namespace
pollution in general. Here are the warnings:

src\third_party\skia\src\core\skmessagebus.h(104) : warning C6244: Local
declaration of 'bus' hides previous declaration
src\third_party\skia\src\core\skmessagebus.h(64) : warning C6244: Local
declaration of 'bus' hides previous declaration
src\third_party\skia\src\core\skmessagebus.h(72) : warning C6244: Local
declaration of 'bus' hides previous declaration

I'm also not sure of the premise of this change. Function local statics are, I'm
pretty sure, zero initialized by the loader, just like global variables. Any
run-time initialization of function statics is racy, but this change doesn't
affect that since you are run-time initialization a global instead of a local
static. That suggests an alternate fix to the variable shadowing warnings --
move the 'bus' global back into the function where it belongs.

[bungeman-skia, 2015-02-09 19:13:56]

On 2015/02/09 18:38:01, brucedawson wrote:
> I noticed this change because of some new variable shadowing warnings in the
> VC++ /analyze build. These warnings point out that skmessagebus.h's macros
> declare a global variable named 'bus' and then some local variables named
'bus'.
> The global variable should be renamed to avoid these warnings and namespace
> pollution in general. Here are the warnings:
> 
> src\third_party\skia\src\core\skmessagebus.h(104) : warning C6244: Local
> declaration of 'bus' hides previous declaration
> src\third_party\skia\src\core\skmessagebus.h(64) : warning C6244: Local
> declaration of 'bus' hides previous declaration
> src\third_party\skia\src\core\skmessagebus.h(72) : warning C6244: Local
> declaration of 'bus' hides previous declaration
> 
> I'm also not sure of the premise of this change. Function local statics are,
I'm
> pretty sure, zero initialized by the loader, just like global variables.

Nope. The fact that they aren't is why this change was made.

> Any
> run-time initialization of function statics is racy, but this change doesn't
> affect that since you are run-time initialization a global instead of a local
> static.

SK_DECLARE_STATIC_LAZY_PTR does an actual atomic compare swap for creating the
object and delete and non-winners (and a consume_load, though that doesn't
really turn into anything on a platforms we support). Regular static
initialization will not handle this.

> That suggests an alternate fix to the variable shadowing warnings --
> move the 'bus' global back into the function where it belongs.

As stated above, almost no function level statics do what we actually want.
However, it would probably be good to give all of these globals different (and
namespaced) names. Especially those in headers. (I think src/core/SkMessageBus.h
is the only one really in violation of this.)

mtklein, would you be interested in this?

[Nico, 2015-02-09 19:30:24]

On 2015/02/09 19:13:56, bungeman1 wrote:
> On 2015/02/09 18:38:01, brucedawson wrote:
> > I noticed this change because of some new variable shadowing warnings in the
> > VC++ /analyze build. These warnings point out that skmessagebus.h's macros
> > declare a global variable named 'bus' and then some local variables named
> 'bus'.
> > The global variable should be renamed to avoid these warnings and namespace
> > pollution in general. Here are the warnings:
> > 
> > src\third_party\skia\src\core\skmessagebus.h(104) : warning C6244: Local
> > declaration of 'bus' hides previous declaration
> > src\third_party\skia\src\core\skmessagebus.h(64) : warning C6244: Local
> > declaration of 'bus' hides previous declaration
> > src\third_party\skia\src\core\skmessagebus.h(72) : warning C6244: Local
> > declaration of 'bus' hides previous declaration
> > 
> > I'm also not sure of the premise of this change. Function local statics are,
> I'm
> > pretty sure, zero initialized by the loader, just like global variables.
> 
> Nope. The fact that they aren't is why this change was made.

Are you sure they aren't? Do you have examples where this isn't true? I would've
expected this too. (And in the few simple cases I just tried, it was true.)

[mtklein, 2015-02-09 19:30:40]

On 2015/02/09 19:13:56, bungeman1 wrote:
> On 2015/02/09 18:38:01, brucedawson wrote:
> > I noticed this change because of some new variable shadowing warnings in the
> > VC++ /analyze build. These warnings point out that skmessagebus.h's macros
> > declare a global variable named 'bus' and then some local variables named
> 'bus'.
> > The global variable should be renamed to avoid these warnings and namespace
> > pollution in general. Here are the warnings:
> > 
> > src\third_party\skia\src\core\skmessagebus.h(104) : warning C6244: Local
> > declaration of 'bus' hides previous declaration
> > src\third_party\skia\src\core\skmessagebus.h(64) : warning C6244: Local
> > declaration of 'bus' hides previous declaration
> > src\third_party\skia\src\core\skmessagebus.h(72) : warning C6244: Local
> > declaration of 'bus' hides previous declaration
> > 
> > I'm also not sure of the premise of this change. Function local statics are,
> I'm
> > pretty sure, zero initialized by the loader, just like global variables.
> 
> Nope. The fact that they aren't is why this change was made.

Right, we did see synchronization added by the compiler, which is worrying.  I
agree, if we're just zeroing things, it logically can happen before runtime. 
But it doesn't seem to actually be happening that way.  As I understand it, it
just has to happen before it's first used.

> > Any
> > run-time initialization of function statics is racy, but this change doesn't
> > affect that since you are run-time initialization a global instead of a
local
> > static.
> 
> SK_DECLARE_STATIC_LAZY_PTR does an actual atomic compare swap for creating the
> object and delete and non-winners (and a consume_load, though that doesn't
> really turn into anything on a platforms we support). Regular static
> initialization will not handle this.

Woah, woah, SK_DECLARE_STATIC_LAZY_PTR does nothing.  It declares a POD struct,
implicitly zeroed given that we're forcing it into a global scope.
The implicit zeroing is not essential here... we could always write '= {0}'
after this if we wanted to allow this in non-global scopes.
Mostly we do not trust compilers to make sure that zeroing is done safely for
locals.

Its .get() method does what you described, starting from the zeroed state.

> 
> > That suggests an alternate fix to the variable shadowing warnings --
> > move the 'bus' global back into the function where it belongs.
> 
> As stated above, almost no function level statics do what we actually want.
> However, it would probably be good to give all of these globals different (and
> namespaced) names. Especially those in headers. (I think
src/core/SkMessageBus.h
> is the only one really in violation of this.)
> 
> mtklein, would you be interested in this?

Sure, it's probably fine if you want to rename it something like gBus##Message. 
There will never sanely be two SkMessageBus<T> for a given T.

[brucedawson, 2015-02-09 19:32:38]

That's odd that the compiler was adding synchronization -- go figure.

Anyway, this is the list of warnings of this type in skia -- four different
variables are affected. Not a bug, just potentially confusing.

third_party\skia\include\core\skonce.h(124) : warning C6244: Local declaration
of 'once' hides previous declaration at line '121' of
'third_party\skia\src\ports\skglobalinitialization_chromium.cpp'.
third_party\skia\include\core\skonce.h(124) : warning C6244: Local declaration
of 'once' hides previous declaration at line '44' of
'third_party\skia\src\utils\win\skdwrite.cpp'.
third_party\skia\include\core\skonce.h(124) : warning C6244: Local declaration
of 'once' hides previous declaration at line '54' of
'third_party\skia\src\utils\skeventtracer.cpp'.
third_party\skia\src\core\skglyphcache.cpp(484) : warning C6244: Local
declaration of 'globals' hides previous declaration at line '28' of
'third_party\skia\src\core\skglyphcache.cpp'.
third_party\skia\src\core\skglyphcache.cpp(542) : warning C6244: Local
declaration of 'globals' hides previous declaration at line '28' of
'third_party\skia\src\core\skglyphcache.cpp'.
third_party\skia\src\core\skmessagebus.h(104) : warning C6244: Local declaration
of 'bus' hides previous declaration at line '43' of
'third_party\skia\src\core\skpicture.cpp'.
third_party\skia\src\core\skmessagebus.h(64) : warning C6244: Local declaration
of 'bus' hides previous declaration at line '17' of
'third_party\skia\src\gpu\grresourcecache2.cpp'.
third_party\skia\src\core\skmessagebus.h(72) : warning C6244: Local declaration
of 'bus' hides previous declaration at line '17' of
'third_party\skia\src\gpu\grresourcecache2.cpp'.
third_party\skia\src\pdf\skpdfgraphicstate.cpp(203) : warning C6244: Local
declaration of 'invertFunction' hides previous declaration at line '173' of
'third_party\skia\src\pdf\skpdfgraphicstate.cpp'.

[Nico, 2015-02-09 19:41:09]

On 2015/02/09 19:30:40, mtklein wrote:
> On 2015/02/09 19:13:56, bungeman1 wrote:
> > On 2015/02/09 18:38:01, brucedawson wrote:
> > > I noticed this change because of some new variable shadowing warnings in
the
> > > VC++ /analyze build. These warnings point out that skmessagebus.h's macros
> > > declare a global variable named 'bus' and then some local variables named
> > 'bus'.
> > > The global variable should be renamed to avoid these warnings and
namespace
> > > pollution in general. Here are the warnings:
> > > 
> > > src\third_party\skia\src\core\skmessagebus.h(104) : warning C6244: Local
> > > declaration of 'bus' hides previous declaration
> > > src\third_party\skia\src\core\skmessagebus.h(64) : warning C6244: Local
> > > declaration of 'bus' hides previous declaration
> > > src\third_party\skia\src\core\skmessagebus.h(72) : warning C6244: Local
> > > declaration of 'bus' hides previous declaration
> > > 
> > > I'm also not sure of the premise of this change. Function local statics
are,
> > I'm
> > > pretty sure, zero initialized by the loader, just like global variables.
> > 
> > Nope. The fact that they aren't is why this change was made.
> 
> Right, we did see synchronization added by the compiler, which is worrying.  I
> agree, if we're just zeroing things, it logically can happen before runtime. 
> But it doesn't seem to actually be happening that way.  As I understand it, it
> just has to happen before it's first used.

So there's two parts here:

1. The storage for statics is the same as for globals I believe – if they can be
load-time zero-initialized, they end up in the bss section and will be 0 at
program start, reliably, before any code runs.

2. The actual initialization of the local static. Intuitively, dthe compiler
adds a bool that is 0 at program start, and then when the function runs it
checks if it's 0, and if not it runs actual initialization of the static (if any
– say you have a ` = rhs()` dynamic initializer). This part is not thread-safe
(except you use 2015's magic statics, or you don't pass
-fno-threadsafe-statics).

It sounds like you saw the init bool from 2. While this might be true that this
is present, it should also be harmless as your .get() function does a
thread-safe cas and your static should be safely initialized to 0 at program
start.

> Mostly we do not trust compilers to make sure that zeroing is done safely for
> locals.

But static locals are really globals. I think you can trust the compiler for
static locals just as much as you trust it for globals.

[mtklein, 2015-02-09 19:53:19]

> 2. The actual initialization of the local static. Intuitively, dthe compiler
> adds a bool that is 0 at program start, and then when the function runs it
> checks if it's 0, and if not it runs actual initialization of the static (if
any
> – say you have a ` = rhs()` dynamic initializer). This part is not thread-safe
> (except you use 2015's magic statics, or you don't pass
> -fno-threadsafe-statics).

Right, I think we were seeing 2.  We want to avoid the bool and in particular
any non-relaxed load on it, as the fast-path of the .get() function can
otherwise be safely barrier-free (we're reading the lazy data through CAS'd
pointer, so we get that ordering for free via data dependency).

That said, I don't really see why 2 should be happening.  The initializer we'd
want to use is very not-dynamic: " = { NULL } ".

I imagine the behavior of this might be C++98/C++11 sensitive.  We recently
updated all our bots and GYP configs to build with -std=c++11.  Think that could
have any effect?

It might be worth a shot to put the initializer back and move some of these
globals back in as function locals.  That's where they logically belong, and
where I wrote them all at first.  That said, I don't really mind keeping the
restriction that they're globals.  I don't think I've ever seen a spot where it
made the code confusing.  And it keeps some consistency across our various
SK_DECLARE_STATIC_... macros that they all must be global-scoped (though perhaps
none of them need that restriction?).

[Nico, 2015-02-09 19:57:12]

On 2015/02/09 19:53:19, mtklein wrote:
> > 2. The actual initialization of the local static. Intuitively, dthe compiler
> > adds a bool that is 0 at program start, and then when the function runs it
> > checks if it's 0, and if not it runs actual initialization of the static (if
> any
> > – say you have a ` = rhs()` dynamic initializer). This part is not
thread-safe
> > (except you use 2015's magic statics, or you don't pass
> > -fno-threadsafe-statics).
> 
> Right, I think we were seeing 2.  We want to avoid the bool and in particular
> any non-relaxed load on it, as the fast-path of the .get() function can
> otherwise be safely barrier-free (we're reading the lazy data through CAS'd
> pointer, so we get that ordering for free via data dependency).
> 
> That said, I don't really see why 2 should be happening.  The initializer we'd
> want to use is very not-dynamic: " = { NULL } ".

Did you see this in release builds, or in debug builds only?

[mtklein, 2015-02-09 20:02:08]

On 2015/02/09 19:57:12, Nico wrote:
> On 2015/02/09 19:53:19, mtklein wrote:
> > > 2. The actual initialization of the local static. Intuitively, dthe
compiler
> > > adds a bool that is 0 at program start, and then when the function runs it
> > > checks if it's 0, and if not it runs actual initialization of the static
(if
> > any
> > > – say you have a ` = rhs()` dynamic initializer). This part is not
> thread-safe
> > > (except you use 2015's magic statics, or you don't pass
> > > -fno-threadsafe-statics).
> > 
> > Right, I think we were seeing 2.  We want to avoid the bool and in
particular
> > any non-relaxed load on it, as the fast-path of the .get() function can
> > otherwise be safely barrier-free (we're reading the lazy data through CAS'd
> > pointer, so we get that ordering for free via data dependency).
> > 
> > That said, I don't really see why 2 should be happening.  The initializer
we'd
> > want to use is very not-dynamic: " = { NULL } ".
> 
> Did you see this in release builds, or in debug builds only?

Hmm.  Not sure, but I'd guess a release build.  I'm not usually brave enough to
profile or disassemble debug builds, so I don't think I'd have noticed there.

[Nico, 2015-02-09 21:00:47]

On 2015/02/09 20:02:08, mtklein wrote:
> On 2015/02/09 19:57:12, Nico wrote:
> > On 2015/02/09 19:53:19, mtklein wrote:
> > > > 2. The actual initialization of the local static. Intuitively, dthe
> compiler
> > > > adds a bool that is 0 at program start, and then when the function runs
it
> > > > checks if it's 0, and if not it runs actual initialization of the static
> (if
> > > any
> > > > – say you have a ` = rhs()` dynamic initializer). This part is not
> > thread-safe
> > > > (except you use 2015's magic statics, or you don't pass
> > > > -fno-threadsafe-statics).
> > > 
> > > Right, I think we were seeing 2.  We want to avoid the bool and in
> particular
> > > any non-relaxed load on it, as the fast-path of the .get() function can
> > > otherwise be safely barrier-free (we're reading the lazy data through
CAS'd
> > > pointer, so we get that ordering for free via data dependency).
> > > 
> > > That said, I don't really see why 2 should be happening.  The initializer
> we'd
> > > want to use is very not-dynamic: " = { NULL } ".
> > 
> > Did you see this in release builds, or in debug builds only?
> 
> Hmm.  Not sure, but I'd guess a release build.  I'm not usually brave enough
to
> profile or disassemble debug builds, so I don't think I'd have noticed there.

That's a bit surprising, but I'd believe that. So this CL isn't about making
anything more thread-safe, but about performance, yes?

[mtklein, 2015-02-09 21:09:09]

On 2015/02/09 21:00:47, Nico wrote:
> On 2015/02/09 20:02:08, mtklein wrote:
> > On 2015/02/09 19:57:12, Nico wrote:
> > > On 2015/02/09 19:53:19, mtklein wrote:
> > > > > 2. The actual initialization of the local static. Intuitively, dthe
> > compiler
> > > > > adds a bool that is 0 at program start, and then when the function
runs
> it
> > > > > checks if it's 0, and if not it runs actual initialization of the
static
> > (if
> > > > any
> > > > > – say you have a ` = rhs()` dynamic initializer). This part is not
> > > thread-safe
> > > > > (except you use 2015's magic statics, or you don't pass
> > > > > -fno-threadsafe-statics).
> > > > 
> > > > Right, I think we were seeing 2.  We want to avoid the bool and in
> > particular
> > > > any non-relaxed load on it, as the fast-path of the .get() function can
> > > > otherwise be safely barrier-free (we're reading the lazy data through
> CAS'd
> > > > pointer, so we get that ordering for free via data dependency).
> > > > 
> > > > That said, I don't really see why 2 should be happening.  The
initializer
> > we'd
> > > > want to use is very not-dynamic: " = { NULL } ".
> > > 
> > > Did you see this in release builds, or in debug builds only?
> > 
> > Hmm.  Not sure, but I'd guess a release build.  I'm not usually brave enough
> to
> > profile or disassemble debug builds, so I don't think I'd have noticed
there.
> 
> That's a bit surprising, but I'd believe that. So this CL isn't about making
> anything more thread-safe, but about performance, yes?

In hindsight, yes.  I think at the time we were mixing up whether the pointer
was zeroed with whether the compiler-generated boolean said it had been zeroed,
and so weren't certain about threadsafety for MSVC or Chromium.  If that wasn't
a problem, this does just boil down to removing that compiler-generated boolean
altogether.

[Nico, 2015-02-09 23:42:10]

I'd be really interested in seeing an example where this guard variable is
initialized, by the way. (I asked a local compiler expert who thought that no
compiler (out of gcc, clang, cl) would add a guard bool for zero-initialized
local statics.)