Create rtc::AtomicInt POD struct.

Create rtc::AtomicInt POD struct.

Prevents accidental non-atomic reads, increments and stores since
"volatile int" doesn't enforce atomic usage.

BUG=
R=kwiberg@webrtc.org, tommi@webrtc.org

Committed: https://crrev.com/b27f590ece487819c3d1fda400315e582fb975b6
Cr-Commit-Position: refs/heads/master@{#10657}

[pbos-webrtc, 2015-11-02 10:49:19]

Description was changed from

==========
Create AtomicInt struct.

Prevents accidental non-atomic reads, increments and stores since
"volatile int" doesn't enforce atomic usage.

BUG=
R=kwiberg@webrtc.org, tommi@webrtc.org
==========

to

==========
Create rtc::AtomicInt POD struct.

Prevents accidental non-atomic reads, increments and stores since
"volatile int" doesn't enforce atomic usage.

BUG=
R=kwiberg@webrtc.org, tommi@webrtc.org
==========

[pbos-webrtc, 2015-11-02 10:56:12]

remove copy constructor

[pbos-webrtc, 2015-11-02 11:01:12]

fix non-atomic use of playing_ and recording_ on iOS

[pbos-webrtc, 2015-11-02 11:06:18]

const for AcquireLoad

[pbos-webrtc, 2015-11-02 11:38:39]

PTAL, this found a bug in webrtc/modules/audio_device/ios/audio_device_ios.mm.
Plan's to then use this AtomicInt and remove Atomic32 from system_wrappers
(rebuild https://codereview.webrtc.org/1347793005/ on top of this).

[pbos-webrtc, 2015-11-02 11:39:41]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-02 11:39:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1420043008/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1420043008/60001

[commit-bot: I haz the power, 2015-11-02 11:44:05]

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

[commit-bot: I haz the power, 2015-11-02 11:44:06]

Dry run: Try jobs failed on following builders:
  win_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_compile_dbg/builds/869)

[pbos-webrtc, 2015-11-02 11:55:45]

decrement, oops

[pbos-webrtc, 2015-11-02 11:57:47]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-02 11:57:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1420043008/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1420043008/80001

[commit-bot: I haz the power, 2015-11-02 12:01:48]

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

[commit-bot: I haz the power, 2015-11-02 12:01:49]

Dry run: Try jobs failed on following builders:
  win_drmemory_light on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/buil...)
  win_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/10543)

[kwiberg-webrtc, 2015-11-02 12:41:50]

https://codereview.webrtc.org/1420043008/diff/80001/webrtc/base/atomicops.h
File webrtc/base/atomicops.h (right):

https://codereview.webrtc.org/1420043008/diff/80001/webrtc/base/atomicops.h#n...
webrtc/base/atomicops.h:69: // the operations are atomic.
Maybe "Functions are static, so that the AtomicInt:: prefix must be present in
the code, clearly labeling the operations as being atomic." for the last
sentence?

Also, do you plan to migrate all current users of AtomicOps?

https://codereview.webrtc.org/1420043008/diff/80001/webrtc/base/atomicops.h#n...
webrtc/base/atomicops.h:91: }
I'd like a short comment on each of these, explaining what it does and what it
returns.

[pbos-webrtc, 2015-11-02 15:21:07]

feedback

[pbos-webrtc, 2015-11-02 15:21:15]

https://codereview.webrtc.org/1420043008/diff/80001/webrtc/base/atomicops.h
File webrtc/base/atomicops.h (right):

https://codereview.webrtc.org/1420043008/diff/80001/webrtc/base/atomicops.h#n...
webrtc/base/atomicops.h:69: // the operations are atomic.
On 2015/11/02 12:41:49, kwiberg-webrtc wrote:
> Maybe "Functions are static, so that the AtomicInt:: prefix must be present in
> the code, clearly labeling the operations as being atomic." for the last
> sentence?
> 
> Also, do you plan to migrate all current users of AtomicOps?

Done, yep that's the plan. I'm not sure if that should entail removing AtomicOps
though.

https://codereview.webrtc.org/1420043008/diff/80001/webrtc/base/atomicops.h#n...
webrtc/base/atomicops.h:91: }
On 2015/11/02 12:41:50, kwiberg-webrtc wrote:
> I'd like a short comment on each of these, explaining what it does and what it
> returns.

Done.

[pbos-webrtc, 2015-11-02 15:22:10]

newlines

[pbos-webrtc, 2015-11-02 15:22:22]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-02 15:22:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1420043008/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1420043008/120001

[pbos-webrtc, 2015-11-02 15:23:20]

PTAL, motivation for this one right now is that it happened to find existing
racy use of volatile int.

[commit-bot: I haz the power, 2015-11-02 17:04:43]

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

[commit-bot: I haz the power, 2015-11-02 17:04:45]

Dry run: This issue passed the CQ dry run.

[kwiberg-webrtc, 2015-11-02 19:42:02]

lgtm, but consider adding the constructor I suggested

https://codereview.webrtc.org/1420043008/diff/120001/webrtc/base/atomicops.h
File webrtc/base/atomicops.h (right):

https://codereview.webrtc.org/1420043008/diff/120001/webrtc/base/atomicops.h#...
webrtc/base/atomicops.h:82: // Atomically increment |i|, returning the resulting
incremented value.
Third-person singular, present tense: increments, returns

https://codereview.webrtc.org/1420043008/diff/120001/webrtc/base/refcount.h
File webrtc/base/refcount.h (right):

https://codereview.webrtc.org/1420043008/diff/120001/webrtc/base/refcount.h#n...
webrtc/base/refcount.h:32: RefCountedObject() : ref_count_({0}) {}
Actually, why not add a constructor that takes an int? That way, you could omit
all (or most of) these braces. (The type would still be a POD.)

[pbos-webrtc, 2015-11-05 18:29:17]

rebase

[pbos-webrtc, 2015-11-05 18:32:18]

fix comments

[pbos-webrtc, 2015-11-05 18:32:22]

Tommi PTAL

https://codereview.webrtc.org/1420043008/diff/120001/webrtc/base/atomicops.h
File webrtc/base/atomicops.h (right):

https://codereview.webrtc.org/1420043008/diff/120001/webrtc/base/atomicops.h#...
webrtc/base/atomicops.h:82: // Atomically increment |i|, returning the resulting
incremented value.
On 2015/11/02 19:42:02, kwiberg-webrtc wrote:
> Third-person singular, present tense: increments, returns

Done.

https://codereview.webrtc.org/1420043008/diff/120001/webrtc/base/refcount.h
File webrtc/base/refcount.h (right):

https://codereview.webrtc.org/1420043008/diff/120001/webrtc/base/refcount.h#n...
webrtc/base/refcount.h:32: RefCountedObject() : ref_count_({0}) {}
On 2015/11/02 19:42:02, kwiberg-webrtc wrote:
> Actually, why not add a constructor that takes an int? That way, you could
omit
> all (or most of) these braces. (The type would still be a POD.)

I'm not confident that all compilers eliminate the constructor, {}
initialization seems more likely to not have these conflicts.

Also I like that atomics happen to look different, but that's on a different
note.

[tommi, 2015-11-06 22:40:34]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-06 22:40:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1420043008/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1420043008/160001

[tommi, 2015-11-06 22:53:30]

On 2015/11/05 18:32:22, pbos-webrtc wrote:
> Tommi PTAL
> 
> https://codereview.webrtc.org/1420043008/diff/120001/webrtc/base/atomicops.h
> File webrtc/base/atomicops.h (right):
> 
>
https://codereview.webrtc.org/1420043008/diff/120001/webrtc/base/atomicops.h#...
> webrtc/base/atomicops.h:82: // Atomically increment |i|, returning the
resulting
> incremented value.
> On 2015/11/02 19:42:02, kwiberg-webrtc wrote:
> > Third-person singular, present tense: increments, returns
> 
> Done.
> 
> https://codereview.webrtc.org/1420043008/diff/120001/webrtc/base/refcount.h
> File webrtc/base/refcount.h (right):
> 
>
https://codereview.webrtc.org/1420043008/diff/120001/webrtc/base/refcount.h#n...
> webrtc/base/refcount.h:32: RefCountedObject() : ref_count_({0}) {}
> On 2015/11/02 19:42:02, kwiberg-webrtc wrote:
> > Actually, why not add a constructor that takes an int? That way, you could
> omit
> > all (or most of) these braces. (The type would still be a POD.)
> 
> I'm not confident that all compilers eliminate the constructor, {}
> initialization seems more likely to not have these conflicts.
> 
> Also I like that atomics happen to look different, but that's on a different
> note.

I would also like the ctor rather than brace initialization. What gives you
confidence in that compiler behavior?

[tommi, 2015-11-06 22:53:36]

https://codereview.webrtc.org/1420043008/diff/160001/talk/session/media/srtpf...
File talk/session/media/srtpfilter.cc (right):

https://codereview.webrtc.org/1420043008/diff/160001/talk/session/media/srtpf...
talk/session/media/srtpfilter.cc:485: rtc::GlobalLockPod SrtpSession::lock_ =
{{0}};
why this change?

https://codereview.webrtc.org/1420043008/diff/160001/webrtc/base/atomicops.h
File webrtc/base/atomicops.h (right):

https://codereview.webrtc.org/1420043008/diff/160001/webrtc/base/atomicops.h#...
webrtc/base/atomicops.h:70: // copy constructor needs to be present for brace
initialization.
why do we need brace initialization? (are you sure it's supported by all
toolsets?)

https://codereview.webrtc.org/1420043008/diff/160001/webrtc/base/atomicops.h#...
webrtc/base/atomicops.h:73: // TODO(pbos): When MSVC allows brace initialization
(or we move to
ah... so, how can we use it?

https://codereview.webrtc.org/1420043008/diff/160001/webrtc/base/atomicops.h#...
webrtc/base/atomicops.h:79: // directly.
The documentation doesn't clearly explain why we actually need this beyond
issues with compilers.  Having the member variable public doesn't feel ideal.

https://codereview.webrtc.org/1420043008/diff/160001/webrtc/base/criticalsect...
File webrtc/base/criticalsection.cc (right):

https://codereview.webrtc.org/1420043008/diff/160001/webrtc/base/criticalsect...
webrtc/base/criticalsection.cc:156: GlobalLock::GlobalLock() :
GlobalLockPod({{0}}) {}
I guess you've talked about having a default ctor?

[commit-bot: I haz the power, 2015-11-06 23:43:41]

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

[commit-bot: I haz the power, 2015-11-06 23:43:43]

Dry run: This issue passed the CQ dry run.

[pbos-webrtc, 2015-11-10 15:59:24]

I want static initialization to end up in the data or bss segment so that we
don't generate any constructor code, hence brace initializers (which at least in
C usually does this), do you think that the constructor will get inlined and end
up in .bss/.data? It feels likelier that a constructor with code would end up
with code in the end?

[tommi, 2015-11-10 17:02:35]

On 2015/11/10 15:59:24, pbos-webrtc wrote:
> I want static initialization to end up in the data or bss segment so that we
> don't generate any constructor code, hence brace initializers (which at least
in
> C usually does this), do you think that the constructor will get inlined and
end
> up in .bss/.data? It feels likelier that a constructor with code would end up
> with code in the end?

I don't think braces or parenthesis really control that per se as language
features.
If the code requires construction, construction code will be added.  Consider
for example:

MyClass::MyClass::() : my_int_() {}

MyClass foo;

vs

MyIntStruct struct = { GetMyInt() };

The second one can't go into .bss but the first one can.

Similarly if there are external dependencies that the compiler doesn't have
access too (e.g. external variables in another object file), there will be code
generated.  Arrays of arrays for example where the arrays aren't compiled into
the same object file.  E.g.

const char* kArrayArray[] = {
  &kMyExternalArray[0],
};

[pbos-webrtc, 2015-11-10 18:05:57]

ctor version, let's pray they are put in .data or .bss

[pbos-webrtc, 2015-11-10 18:06:14]

Let's give it a shot and see if we need to revert it. PTAL.

[pbos-webrtc, 2015-11-10 18:06:47]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-10 18:07:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1420043008/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1420043008/180001

[commit-bot: I haz the power, 2015-11-10 18:11:09]

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

[commit-bot: I haz the power, 2015-11-10 18:11:10]

Dry run: Try jobs failed on following builders:
  android_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_dbg/builds/6810)

[pbos-webrtc, 2015-11-10 19:00:34]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-10 19:00:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1420043008/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1420043008/180001

[commit-bot: I haz the power, 2015-11-10 20:16:32]

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

[commit-bot: I haz the power, 2015-11-10 20:16:33]

Dry run: This issue passed the CQ dry run.

[tommi, 2015-11-11 15:58:37]

lgtm.  crazy idea though: since all call sites initially set the atomic to 0,
what about having a default ctor that does that?

https://codereview.webrtc.org/1420043008/diff/180001/webrtc/base/atomicops.h
File webrtc/base/atomicops.h (right):

https://codereview.webrtc.org/1420043008/diff/180001/webrtc/base/atomicops.h#...
webrtc/base/atomicops.h:67: // POD struct  version of AtomicOps, prevents
accidental non-atomic operator
nit: unintentional whitespace of doom

[pbos-webrtc, 2015-11-16 18:07:54]

default ctor

[pbos-webrtc, 2015-11-16 18:08:34]

intentional obliteration of the unintentional whitespace of doom

[pbos-webrtc, 2015-11-16 18:10:42]

https://codereview.webrtc.org/1420043008/diff/180001/webrtc/base/atomicops.h
File webrtc/base/atomicops.h (right):

https://codereview.webrtc.org/1420043008/diff/180001/webrtc/base/atomicops.h#...
webrtc/base/atomicops.h:67: // POD struct  version of AtomicOps, prevents
accidental non-atomic operator
On 2015/11/11 15:58:37, tommi (webrtc) wrote:
> nit: unintentional whitespace of doom

Done.

[pbos-webrtc, 2015-11-16 18:10:47]

The CQ bit was checked by pbos@webrtc.org

[pbos-webrtc, 2015-11-16 18:10:47]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org,
tommi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1420043008/#ps220001 (title:
"intentional obliteration of the unintentional whitespace of doom")

[commit-bot: I haz the power, 2015-11-16 18:10:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1420043008/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1420043008/220001

[commit-bot: I haz the power, 2015-11-16 19:03:07]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-11-16 19:03:14]

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

[pbos-webrtc, 2015-11-17 11:20:42]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.webrtc.org/1453093002/ by pbos@webrtc.org.

The reason for reverting is: Caused static initializers.

BUG=chromium:556866
TBR=tommi@webrtc.org.