Move WTF::SpinLock to base::SpinLock.

Move WTF::SpinLock to base::SpinLock.

This CL depends on https://codereview.chromium.org/2473153003 ("Move some
compiler intrinsic #defines to base/.").

BUG=632441
Committed: https://crrev.com/d9dbd59434791ddc998d2aeb24f7f1326431ed14
Cr-Commit-Position: refs/heads/master@{#433378}

[palmer, 2016-11-08 00:44:47]

palmer@chromium.org changed reviewers:
+ dcheng@chromium.org, esprehn@chromium.org, thakis@chromium.org

[palmer, 2016-11-08 00:46:28]

Question: when I don't have an explicit constructor, I get a chromium-style
error saying that I need one because SpinLock is a complex struct/class. But its
only member is a std::atomic_int, which is POD (with special magic for
reads/writes) — seems like maybe that isn't really a complex class?

Thanks for any clues!

[palmer, 2016-11-08 01:29:22]

Note additionally that when I give |SpinLock| the empty ctor, the next compile
problem I run into is:

third_party/WebKit/Source/wtf/allocator/PartitionAlloc.cpp:69:29: error:
declaration requires a global constructor [-Werror,-Wglobal-constructors]
SpinLock PartitionRootBase::gInitializedLock;
                            ^~~~~~~~~~~~~~~~

third_party/WebKit/Source/wtf/allocator/AddressSpaceRandomization.cpp:76:22:
error: declaration requires a global constructor [-Werror,-Wglobal-constructors]
static struct ranctx s_ranctx;
                     ^~~~~~~~

(|ranctx| has-a |SpinLock|)

It seems like a |LazyInstance| could fix this (right?); but, perhaps someday
|LazyInstance| will have-a |SpinLock|, and then we'd end up with a circular
dependency between the 2 classes?

[dcheng, 2016-11-08 02:06:03]

dcheng@chromium.org changed reviewers:
+ hans@chromium.org

[dcheng, 2016-11-08 02:06:03]

I think the problem is the clang plugin gets confused by templates, and so it
just assumes that std::atomic is a non-pod type (when in fact, it is).

Unfortunately, I don't think there's a good workaround atm. I'll hack together
something in the plugin to add an exception for std::atomic, but it'll take time
to roll the plugin.

+hans for FYI

[kinuko, 2016-11-08 05:15:42]

https://codereview.chromium.org/2484803003/diff/1/base/spin_lock.h
File base/spin_lock.h (right):

https://codereview.chromium.org/2484803003/diff/1/base/spin_lock.h#newcode1
base/spin_lock.h:1: /*
drive-by.  Should these files be maybe put under base/synchronization ?

Also could we use short copyright format?

[hans, 2016-11-08 17:43:33]

On 2016/11/08 02:06:03, dcheng wrote:
> I think the problem is the clang plugin gets confused by templates, and so it
> just assumes that std::atomic is a non-pod type (when in fact, it is).

I guess this is where it happens?
https://cs.chromium.org/chromium/src/tools/clang/plugins/FindBadConstructsCon...

> Unfortunately, I don't think there's a good workaround atm. I'll hack together
> something in the plugin to add an exception for std::atomic, but it'll take
time
> to roll the plugin.
> 
> +hans for FYI

Let me know if you get a fix out for the plugin. I'd be up for rolling it
out-of-band (i.e. without pulling in a new clang version).

[dcheng, 2016-11-08 17:56:41]

On 2016/11/08 17:43:33, hans wrote:
> On 2016/11/08 02:06:03, dcheng wrote:
> > I think the problem is the clang plugin gets confused by templates, and so
it
> > just assumes that std::atomic is a non-pod type (when in fact, it is).
> 
> I guess this is where it happens?
>
https://cs.chromium.org/chromium/src/tools/clang/plugins/FindBadConstructsCon...

Yeah. I was thinking we could actually make this check smarter and avoid doing
checks on templates until the template is actually instantiated. Not sure how
that'll interact with the current way the plugin is written though...

(Btw palmer@, if you're up for making this change, I'm happy to provide support
as well, it'd be good to distribute this knowledge a bit more, so it's not
bottlenecked on me)

> 
> > Unfortunately, I don't think there's a good workaround atm. I'll hack
together
> > something in the plugin to add an exception for std::atomic, but it'll take
> time
> > to roll the plugin.
> > 
> > +hans for FYI
> 
> Let me know if you get a fix out for the plugin. I'd be up for rolling it
> out-of-band (i.e. without pulling in a new clang version).

[palmer, 2016-11-10 23:35:12]

> drive-by.  Should these files be maybe put under base/synchronization ?

Done. Thanks!

> Also could we use short copyright format?

Done.

[palmer, 2016-11-15 00:13:48]

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

[commit-bot: I haz the power, 2016-11-15 00:15:18]

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

[palmer, 2016-11-15 00:16:08]

OK, I think this CL is ready to go. PTAL, thanks!

[esprehn, 2016-11-15 00:17:12]

lgtm

[commit-bot: I haz the power, 2016-11-15 02:17:13]

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

[commit-bot: I haz the power, 2016-11-15 02:17:14]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[dcheng, 2016-11-15 02:46:03]

https://codereview.chromium.org/2484803003/diff/40001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/2484803003/diff/40001/base/BUILD.gn#newcode1208
base/BUILD.gn:1208: # compiler doesn't allow.
Will we need to move things like LazyInstance and Singleton here as well?

[Nico, 2016-11-15 15:26:41]

should all the partitionalloc things go into base/partitionalloc (or similar),
so that it's easier to move around later if needed?

[palmer, 2016-11-15 19:21:07]

> Will we need to move things like LazyInstance and Singleton here as well?

If they ever end up depending on SpinLock, then yes. Currently they do not.

[palmer, 2016-11-15 19:28:15]

> should all the partitionalloc things go into base/partitionalloc (or similar),
so that it's easier to move around later if needed?

Although PA is the only thing that currently depends on SpinLock, I am hoping to
either make it the standard spin lock implementation and use it in favor of
other spin lock-like things; OR, to get rid of it and have PA use a different
lock. Note that SpinLock comes from Source/wtf, and not Source/wtf/allocator — I
think the intention was that SpinLock would be generally useful.

That said, if we are sure it won't be generally useful, I'm happy to bundle it
up with PA.

[Primiano Tucci (use gerrit), 2016-11-16 13:07:13]

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

[Primiano Tucci (use gerrit), 2016-11-16 13:07:14]

Accidentally passing by here while I was having a conversation on this topic
with +alexclarke.

can we use the LINKER_INITIALIZED + explicit empty ctor pattern here, similar to
LazyInstance? Or at very least add a warning in the header saying "THIS SHOULD
BE USED ONLY AS A GLOBAL OBJECT OR BAD THINGS WILL HAPPEN".

This class as-is is quite dangerous (like the current WTF::SpinLock is) if
somebody tries to use it as a member field, as the lock_ won't be initialized.

[alex clarke (OOO till 29th), 2016-11-16 13:36:13]

On 2016/11/16 13:07:14, Primiano - slow(travelling) wrote:
> Accidentally passing by here while I was having a conversation on this topic
> with +alexclarke.
> 
> can we use the LINKER_INITIALIZED + explicit empty ctor pattern here, similar
to
> LazyInstance? Or at very least add a warning in the header saying "THIS SHOULD
> BE USED ONLY AS A GLOBAL OBJECT OR BAD THINGS WILL HAPPEN".
> 
> This class as-is is quite dangerous (like the current WTF::SpinLock is) if
> somebody tries to use it as a member field, as the lock_ won't be initialized.

+1 I would prefer the LINKER_INITIALIZED explicit empty ctor pattern.  I got
bitten by
this when trying to use a non-global WTF::SpinLock in the blink scheduler.

[dcheng, 2016-11-16 13:48:01]

On 2016/11/16 13:36:13, alex clarke wrote:
> On 2016/11/16 13:07:14, Primiano - slow(travelling) wrote:
> > Accidentally passing by here while I was having a conversation on this topic
> > with +alexclarke.
> > 
> > can we use the LINKER_INITIALIZED + explicit empty ctor pattern here,
similar
> to
> > LazyInstance? Or at very least add a warning in the header saying "THIS
SHOULD
> > BE USED ONLY AS A GLOBAL OBJECT OR BAD THINGS WILL HAPPEN".
> > 
> > This class as-is is quite dangerous (like the current WTF::SpinLock is) if
> > somebody tries to use it as a member field, as the lock_ won't be
initialized.
> 
> +1 I would prefer the LINKER_INITIALIZED explicit empty ctor pattern.  I got
> bitten by
> this when trying to use a non-global WTF::SpinLock in the blink scheduler.

Sounds good: let's do this before we move it into base then.

Btw... Is there a reason people would want to use this directly? I guess the
default choice should be Lock, right? Wondering if we want to stick this in
base::subtle.

[dcheng, 2016-11-16 13:49:13]

On 2016/11/16 13:48:01, dcheng wrote:
> On 2016/11/16 13:36:13, alex clarke wrote:
> > On 2016/11/16 13:07:14, Primiano - slow(travelling) wrote:
> > > Accidentally passing by here while I was having a conversation on this
topic
> > > with +alexclarke.
> > > 
> > > can we use the LINKER_INITIALIZED + explicit empty ctor pattern here,
> similar
> > to
> > > LazyInstance? Or at very least add a warning in the header saying "THIS
> SHOULD
> > > BE USED ONLY AS A GLOBAL OBJECT OR BAD THINGS WILL HAPPEN".
> > > 
> > > This class as-is is quite dangerous (like the current WTF::SpinLock is) if
> > > somebody tries to use it as a member field, as the lock_ won't be
> initialized.
> > 
> > +1 I would prefer the LINKER_INITIALIZED explicit empty ctor pattern.  I got
> > bitten by
> > this when trying to use a non-global WTF::SpinLock in the blink scheduler.
> 
> Sounds good: let's do this before we move it into base then.
> 
> Btw... Is there a reason people would want to use this directly? I guess the
> default choice should be Lock, right? Wondering if we want to stick this in
> base::subtle.

I guess my reply isn't phrased well, but basically, I'm wondering if people
should default to Lock unless there's a good reason to do otherwise.

[Primiano Tucci (use gerrit), 2016-11-16 14:00:27]

On 2016/11/16 13:49:13, dcheng wrote:
> I guess my reply isn't phrased well, but basically, I'm wondering if people
> should default to Lock unless there's a good reason to do otherwise.

+1 to subtle, or people will start getting into neverending threads like: I
wanted to use a spinlock but my reviewer told me to use a lock and now we are
bikeshedding.

In the specific alexclarke@ was looking into that for the blink scheduler, which
IMHO falls in the category of the few things like messageloop where it should be
socially acceptable to use a spinlock.
In other words, I think spinlock should remain something to build other
base-like blocks on top, not something to be used in random places.

[palmer, 2016-11-16 21:16:13]

> Accidentally passing by here while I was having a conversation on this topic
with +alexclarke.
> 
> can we use the LINKER_INITIALIZED + explicit empty ctor pattern here, similar
to LazyInstance? Or at very least add a warning in the header saying "THIS
SHOULD BE USED ONLY AS A GLOBAL OBJECT OR BAD THINGS WILL HAPPEN".

I'm not sure what you mean; LazyInstance does not use LINKER_INITIALIZED:

// LazyInstance uses its own struct initializer-list style static
// initialization, as base's LINKER_INITIALIZED requires a constructor and on
// some compilers (notably gcc 4.4) this still ends up needing runtime
// initialization.
#define LAZY_INSTANCE_INITIALIZER {0}

thread_local_storage.h cargo-cults the same pattern and comment.

> This class as-is is quite dangerous (like the current WTF::SpinLock is) if
somebody tries to use it as a member field, as the lock_ won't be initialized.

I have moved it to base::subtle::, and added the warning comment. (In an
upcoming patchset, to be uploaded after I go eat.)

However, from what I can tell — and I could just be confused — nobody seems to
actually use the LINKER_INITIALIZED/explicit constructor pattern in call sites.
Additionally, using it causes a compile failure due to the

    using Guard = std::lock_guard<SpinLock>;

declaration; there is no matching 0-argument constructor for |SpinLock|. Maybe
(likely) I'm misunderstanding something.

[Primiano Tucci (use gerrit), 2016-11-16 22:36:05]

On 2016/11/16 21:16:13, palmer wrote:
> > Accidentally passing by here while I was having a conversation on this topic
> with +alexclarke.
> > 
> > can we use the LINKER_INITIALIZED + explicit empty ctor pattern here,
similar
> to LazyInstance? Or at very least add a warning in the header saying "THIS
> SHOULD BE USED ONLY AS A GLOBAL OBJECT OR BAD THINGS WILL HAPPEN".
> 
> I'm not sure what you mean; LazyInstance does not use LINKER_INITIALIZED:
> 
> // LazyInstance uses its own struct initializer-list style static
> // initialization, as base's LINKER_INITIALIZED requires a constructor and on
> // some compilers (notably gcc 4.4) this still ends up needing runtime
> // initialization.
> #define LAZY_INSTANCE_INITIALIZER {0}
> 
> thread_local_storage.h cargo-cults the same pattern and comment.
> 
> > This class as-is is quite dangerous (like the current WTF::SpinLock is) if
> somebody tries to use it as a member field, as the lock_ won't be initialized.
> 
> I have moved it to base::subtle::, and added the warning comment. (In an
> upcoming patchset, to be uploaded after I go eat.)
> 
> However, from what I can tell — and I could just be confused — nobody seems to
> actually use the LINKER_INITIALIZED/explicit constructor pattern in call
sites.
> Additionally, using it causes a compile failure due to the
> 
>     using Guard = std::lock_guard<SpinLock>;
> 
> declaration; there is no matching 0-argument constructor for |SpinLock|. Maybe
> (likely) I'm misunderstanding something.

Uhm, I guess I should have re-checked the code before. I was quite convinced
that LazyInstance and other things were using the explicit LinkerInizialized
ctor like this:
https://cs.chromium.org/chromium/src/third_party/tcmalloc/vendor/src/base/spi...
I guess it was the case in the past and that changed at some point because of
the initializers-related surprises mentioned in the comments your pointed out.
You are right it doesn't seem to be the case anymore, the only trace I could
find today is that third_party/tcmalloc/.
Given the comments you pointed out I guess we found that it plays bad with some
compilers.
So, nevermind, ignore my comment. It seems that a warning is all we can afford.

[palmer, 2016-11-16 23:32:13]

OK, here's that new patchset.

[palmer, 2016-11-16 23:36:11]

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

[commit-bot: I haz the power, 2016-11-16 23:37:17]

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

[commit-bot: I haz the power, 2016-11-17 01:43:11]

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

[commit-bot: I haz the power, 2016-11-17 01:43:12]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dcheng, 2016-11-18 19:43:03]

lgtm

[palmer, 2016-11-18 21:30:38]

The CQ bit was checked by palmer@chromium.org

[palmer, 2016-11-18 21:30:38]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2484803003/#ps60001
(title: "base::SpinLock → base::subtle::SpinLock, plus a warning comment.")

[commit-bot: I haz the power, 2016-11-18 21:32:14]

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

[commit-bot: I haz the power, 2016-11-18 23:57:24]

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

[commit-bot: I haz the power, 2016-11-18 23:57:25]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[palmer, 2016-11-19 00:33:49]

The CQ bit was checked by palmer@chromium.org

[commit-bot: I haz the power, 2016-11-19 00:34:25]

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

[commit-bot: I haz the power, 2016-11-19 02:03:03]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-19 02:08:17]

Description was changed from

==========
Move WTF::SpinLock to base::SpinLock.

This CL depends on https://codereview.chromium.org/2473153003 ("Move some
compiler intrinsic #defines to base/.").

BUG=632441
==========

to

==========
Move WTF::SpinLock to base::SpinLock.

This CL depends on https://codereview.chromium.org/2473153003 ("Move some
compiler intrinsic #defines to base/.").

BUG=632441

Committed: https://crrev.com/d9dbd59434791ddc998d2aeb24f7f1326431ed14
Cr-Commit-Position: refs/heads/master@{#433378}
==========

[commit-bot: I haz the power, 2016-11-19 02:08:18]

Patchset 4 (id:??) landed as
https://crrev.com/d9dbd59434791ddc998d2aeb24f7f1326431ed14
Cr-Commit-Position: refs/heads/master@{#433378}

[kapishnikov, 2016-11-21 02:58:07]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2517033002/ by kapishnikov@chromium.org.

The reason for reverting is: Broke Android Cronet ARMv6 Builder:
https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20ARM....