Fix thread unsafe mutex initialization.

Fix thread unsafe mutex initialization.

BUG=skia:2779
Committed: https://skia.googlesource.com/skia/+/d6aeb6dc8fe21066f1a2c4813a4256a3acd3edf5

Committed: https://skia.googlesource.com/skia/+/1ef960b01b3d76d9dac19a0d38d71dd03bbb9f21

[bungeman-skia, 2014-07-24 23:00:20]

A bit early as I haven't really reviewed this myself, but it seems to work.

[mtklein, 2014-07-25 12:35:34]

lgtm, with some questions

https://codereview.chromium.org/419113002/diff/1/include/core/SkInstCnt.h
File include/core/SkInstCnt.h (right):

https://codereview.chromium.org/419113002/diff/1/include/core/SkInstCnt.h#new...
include/core/SkInstCnt.h:80: static SkMutex* childrenMutex =
(SkMutex*)className::SkInstanceCountHelper::nil; \
Just curious, how come static SkMutex* childrenMutex; is no good?  Or = NULL?

https://codereview.chromium.org/419113002/diff/1/include/core/SkInstCnt.h#new...
include/core/SkInstCnt.h:81: SK_DECLARE_STATIC_ONCE(once);                      
            \
Can't we still use SK_DECLARE_STATIC_MUTEX to make the mutex a static member?

https://codereview.chromium.org/419113002/diff/1/src/ports/SkMutex_pthread.h
File src/ports/SkMutex_pthread.h (right):

https://codereview.chromium.org/419113002/diff/1/src/ports/SkMutex_pthread.h#...
src/ports/SkMutex_pthread.h:92: #define SK_DECLARE_STATIC_MUTEX(name) static
void SK_MACRO_APPEND_LINE(name)(); static SkBaseMutex name = SK_BASE_MUTEX_INIT
Add a comment about the function declaration here too?

[bsalomon, 2014-07-25 13:01:20]

Looks like you'll have to either remove the static from the function, define it,
or otherwise suppress the static-func-declared-but-not-defined warning.

Mike, I think the difficulty with having a static member is that we'd need a
separate macro to define it in the cpp.

[mtklein, 2014-07-25 13:14:02]

> Mike, I think the difficulty with having a static member is that we'd need a
> separate macro to define it in the cpp.

Won't the extra static method declaration be fine there as part of the class?

[bsalomon, 2014-07-25 13:56:51]

On 2014/07/25 13:14:02, mtklein wrote:
> > Mike, I think the difficulty with having a static member is that we'd need a
> > separate macro to define it in the cpp.
> 
> Won't the extra static method declaration be fine there as part of the class?

Yeah the static method decl should be fine. But the mutex will require a
definition outside of the class definition.

[bungeman-skia, 2014-07-25 14:40:40]

bsalomon pretty much already said this out of line, but I'd already written
these inline, and it seemed like a waste not to use them.

https://codereview.chromium.org/419113002/diff/1/include/core/SkInstCnt.h
File include/core/SkInstCnt.h (right):

https://codereview.chromium.org/419113002/diff/1/include/core/SkInstCnt.h#new...
include/core/SkInstCnt.h:80: static SkMutex* childrenMutex =
(SkMutex*)className::SkInstanceCountHelper::nil; \
On 2014/07/25 12:35:33, mtklein wrote:
> Just curious, how come static SkMutex* childrenMutex; is no good?  Or = NULL?

Because that would require an out of line definition (only static const
primitives are allowed to be defined in a class). We did a bunch of work before
to get away from the error prone two part declare and define of this instance
counting class, and I'd like to avoid it for just this. One would think to get
around this with intptr_t, but it would still have to be static const and the
compiler can ignore where it's used and use the class value directly.

https://codereview.chromium.org/419113002/diff/1/include/core/SkInstCnt.h#new...
include/core/SkInstCnt.h:81: SK_DECLARE_STATIC_ONCE(once);                      
            \
On 2014/07/25 12:35:33, mtklein wrote:
> Can't we still use SK_DECLARE_STATIC_MUTEX to make the mutex a static member?

See above, it's the same situation as the pointer, requiring a top level
definition.

https://codereview.chromium.org/419113002/diff/1/src/ports/SkMutex_pthread.h
File src/ports/SkMutex_pthread.h (right):

https://codereview.chromium.org/419113002/diff/1/src/ports/SkMutex_pthread.h#...
src/ports/SkMutex_pthread.h:92: #define SK_DECLARE_STATIC_MUTEX(name) static
void SK_MACRO_APPEND_LINE(name)(); static SkBaseMutex name = SK_BASE_MUTEX_INIT
On 2014/07/25 12:35:33, mtklein wrote:
> Add a comment about the function declaration here too?

Hmmm... this one is actually safe (well, in practice though not according to
spec). Since this is POD all existing compilers will just compile time init such
a thing. Maybe the comment should go in SkThread.h, though that is a bit far
from the actual 'definition' of this macro.

[bungeman-skia, 2014-07-25 15:44:07]

https://codereview.chromium.org/419113002/diff/40001/include/core/SkInstCnt.h
File include/core/SkInstCnt.h (right):

https://codereview.chromium.org/419113002/diff/40001/include/core/SkInstCnt.h...
include/core/SkInstCnt.h:83: return *childrenMutex;                             
            \
Making this body look like

SK_DECLARE_STATIC_LAZY_PTR(SkMutex, childrenMutex);
return *childrenMutex.get();

seems like it would work even better, but this file is in include/core and
SkLazyPtr.h is in src/core, which might not be on the search path. I think that
would make the include something like #include "../../src/core/SkLazyPtr.h"
which requires that 'src' and 'include' be next to each other.

[bsalomon, 2014-07-25 17:45:31]

lgtm

[bungeman-skia, 2014-07-25 18:40:49]

The CQ bit was checked by bungeman@google.com

[commit-bot: I haz the power, 2014-07-25 18:41:12]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/bungeman@google.com/419113002/60001

[commit-bot: I haz the power, 2014-07-25 18:52:53]

Change committed as d6aeb6dc8fe21066f1a2c4813a4256a3acd3edf5

[bungeman-skia, 2014-07-25 21:58:13]

The CQ bit was checked by bungeman@google.com

[commit-bot: I haz the power, 2014-07-25 21:58:44]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/bungeman@google.com/419113002/80001

[commit-bot: I haz the power, 2014-07-25 22:11:08]

Change committed as 1ef960b01b3d76d9dac19a0d38d71dd03bbb9f21