Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 1 month ago by pbos-webrtc Modified:
5 years, 1 month ago Reviewers:
tommi, kwiberg-webrtc CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), Andrew MacDonald, henrika_webrtc, stefan-webrtc, tterriberry_mozilla.com, andresp, peah-webrtc, the sun, perkj_webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionCreate 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}
Patch Set 1 #Patch Set 2 : remove copy constructor #Patch Set 3 : fix non-atomic use of playing_ and recording_ on iOS #Patch Set 4 : const for AcquireLoad #Patch Set 5 : decrement, oops #
Total comments: 4
Patch Set 6 : feedback #Patch Set 7 : newlines #
Total comments: 4
Patch Set 8 : rebase #Patch Set 9 : fix comments #
Total comments: 5
Patch Set 10 : ctor version, let's pray they are put in .data or .bss #
Total comments: 2
Patch Set 11 : default ctor #Patch Set 12 : intentional obliteration of the unintentional whitespace of doom #
Created: 5 years, 1 month ago
Messages
Total messages: 55 (15 generated)
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 ==========
remove copy constructor
fix non-atomic use of playing_ and recording_ on iOS
const for AcquireLoad
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).
The CQ bit was checked by pbos@webrtc.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
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)
decrement, oops
The CQ bit was checked by pbos@webrtc.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
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)
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.
feedback
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.
newlines
The CQ bit was checked by pbos@webrtc.org to run a CQ dry run
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
PTAL, motivation for this one right now is that it happened to find existing racy use of volatile int.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.)
rebase
fix comments
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.
The CQ bit was checked by tommi@webrtc.org to run a CQ dry run
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
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?
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?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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?
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],
};
ctor version, let's pray they are put in .data or .bss
Let's give it a shot and see if we need to revert it. PTAL.
The CQ bit was checked by pbos@webrtc.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
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)
The CQ bit was checked by pbos@webrtc.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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
default ctor
intentional obliteration of the unintentional whitespace of doom
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.
The CQ bit was checked by pbos@webrtc.org
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")
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
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/b27f590ece487819c3d1fda400315e582fb975b6 Cr-Commit-Position: refs/heads/master@{#10657}
Message was sent while issue was closed.
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. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
