Add AtomicFlag class to base/......

Add AtomicFlag class to base/...
This class is intended to replace wrong synchronization
via boolean like those in the bugs listed below.
TEST=./sconsbuild/Debug/base_unittests --gtest_filter="*AtomicFlag*"
BUG=21468, 22520, 24419

[Timur Iskhodzhanov, 2009-10-13 19:13:25]

Hi Darin,

Can you please review the AtomicFlag class we were talking about?
Can you give me some guidance on how to fix the death test for WIN?

Thank you,
Timur Iskhodzhanov

[darin (slow to review), 2009-10-14 06:25:45]

http://codereview.chromium.org/276002/diff/4001/5003
File base/atomic_flag.cc (right):

http://codereview.chromium.org/276002/diff/4001/5003#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
nit: just 2009

http://codereview.chromium.org/276002/diff/4001/5003#newcode8
Line 8: #include "base/atomic_flag.h"
nit: please list this header first per the style guide

http://codereview.chromium.org/276002/diff/4001/5004
File base/atomic_flag.h (right):

http://codereview.chromium.org/276002/diff/4001/5004#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
nit: just 2009

http://codereview.chromium.org/276002/diff/4001/5004#newcode11
Line 11: // of a single event.
nit: just make this a paragraph without the newlines after each "."

http://codereview.chromium.org/276002/diff/4001/5004#newcode19
Line 19: class AtomicFlag {
please put this class in the "base" namespace.

http://codereview.chromium.org/276002/diff/4001/5004#newcode34
Line 34: 
nit: only one newline here please

http://codereview.chromium.org/276002/diff/4001/5001
File base/atomic_flag_unittest.cc (right):

http://codereview.chromium.org/276002/diff/4001/5001#newcode18
Line 18: namespace {
nit: add a new line after this line

http://codereview.chromium.org/276002/diff/4001/5001#newcode57
Line 57: // Checks that Notify() can't be called more than once.
s/Notify/Set/

[Timur Iskhodzhanov, 2009-10-14 14:43:44]

Thank you for your review.

Is there any better way to disable a specific test if DEBUG_MODE is not 1?

Currently, I do the following:
// AtomicFlag should die on a DCHECK if Set() is called more than once.
// DCHECK works only if DEBUG_MODE is equal to 1.
#if DEBUG_MODE
#define DOUBLE_SET_DEATH_TEST_NAME DoubleSetDeathTest
#else
#define DOUBLE_SET_DEATH_TEST_NAME DISABLED_DoubleSetDeathTest
#endif
TEST_F(AtomicFlagTest, DOUBLE_SET_DEATH_TEST_NAME) {
  ...
}

http://codereview.chromium.org/276002/diff/4001/5003
File base/atomic_flag.cc (right):

http://codereview.chromium.org/276002/diff/4001/5003#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
On 2009/10/14 06:25:45, darin wrote:
> nit: just 2009

Done.

http://codereview.chromium.org/276002/diff/4001/5003#newcode8
Line 8: #include "base/atomic_flag.h"
On 2009/10/14 06:25:45, darin wrote:
> nit: please list this header first per the style guide

Done.

http://codereview.chromium.org/276002/diff/4001/5004
File base/atomic_flag.h (right):

http://codereview.chromium.org/276002/diff/4001/5004#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
On 2009/10/14 06:25:45, darin wrote:
> nit: just 2009

Done.

http://codereview.chromium.org/276002/diff/4001/5004#newcode11
Line 11: // of a single event.
On 2009/10/14 06:25:45, darin wrote:
> nit: just make this a paragraph without the newlines after each "."

Done.

http://codereview.chromium.org/276002/diff/4001/5004#newcode19
Line 19: class AtomicFlag {
On 2009/10/14 06:25:45, darin wrote:
> please put this class in the "base" namespace.

Done.

http://codereview.chromium.org/276002/diff/4001/5004#newcode34
Line 34: 
On 2009/10/14 06:25:45, darin wrote:
> nit: only one newline here please

Done.

http://codereview.chromium.org/276002/diff/4001/5001
File base/atomic_flag_unittest.cc (right):

http://codereview.chromium.org/276002/diff/4001/5001#newcode18
Line 18: namespace {
On 2009/10/14 06:25:45, darin wrote:
> nit: add a new line after this line

Done.

http://codereview.chromium.org/276002/diff/4001/5001#newcode57
Line 57: // Checks that Notify() can't be called more than once.
On 2009/10/14 06:25:45, darin wrote:
> s/Notify/Set/

Done.

[darin (slow to review), 2009-10-14 16:27:24]

On Wed, Oct 14, 2009 at 7:43 AM, <timurrrr@chromium.org> wrote:

> Thank you for your review.
>
> Is there any better way to disable a specific test if DEBUG_MODE is not 1?
>
> Currently, I do the following:
> // AtomicFlag should die on a DCHECK if Set() is called more than once.
> // DCHECK works only if DEBUG_MODE is equal to 1.
> #if DEBUG_MODE
> #define DOUBLE_SET_DEATH_TEST_NAME DoubleSetDeathTest
> #else
> #define DOUBLE_SET_DEATH_TEST_NAME DISABLED_DoubleSetDeathTest
> #endif
> TEST_F(AtomicFlagTest, DOUBLE_SET_DEATH_TEST_NAME) {
>  ...
> }
>
>
something like this might be better for readability:

#if DEBUG_MODE
#define DISABLED_IN_RELEASE(x) x
#else
#define DISABLED_IN_RELEASE(x) DISABLED_##x
#endif

TEST_F(AtomicFlag, DISABLED_IN_RELEASE(DoubleSetDeathTest))

you could also call it DISABLED_IN_NDEBUG and use the NDEBUG define instead
of DEBUG_MODE.

-darin





>
> http://codereview.chromium.org/276002
>

[Timur Iskhodzhanov, 2009-10-14 18:10:22]

On 2009/10/14 16:27:24, darin wrote:
> On Wed, Oct 14, 2009 at 7:43 AM, <mailto:timurrrr@chromium.org> wrote:
> 
> > Thank you for your review.
> >
> > Is there any better way to disable a specific test if DEBUG_MODE is not 1?
> >
> > Currently, I do the following:
> > // AtomicFlag should die on a DCHECK if Set() is called more than once.
> > // DCHECK works only if DEBUG_MODE is equal to 1.
> > #if DEBUG_MODE
> > #define DOUBLE_SET_DEATH_TEST_NAME DoubleSetDeathTest
> > #else
> > #define DOUBLE_SET_DEATH_TEST_NAME DISABLED_DoubleSetDeathTest
> > #endif
> > TEST_F(AtomicFlagTest, DOUBLE_SET_DEATH_TEST_NAME) {
> >  ...
> > }
> >
> >
> something like this might be better for readability:
> 
> #if DEBUG_MODE
> #define DISABLED_IN_RELEASE(x) x
> #else
> #define DISABLED_IN_RELEASE(x) DISABLED_##x
> #endif
> 
> TEST_F(AtomicFlag, DISABLED_IN_RELEASE(DoubleSetDeathTest))
> 
> you could also call it DISABLED_IN_NDEBUG and use the NDEBUG define instead
> of DEBUG_MODE.
> 
> -darin
Unfortunately, both approaches don't work.

* DEBUG_MODE is an enum constant, so #if doesn't catch it
* NDEBUG and DCHECK are _both_ not defined on Windows bot...
Any suggestions?

> >
> > http://codereview.chromium.org/276002
> >
>

[darin (slow to review), 2009-10-14 18:20:37]

On Wed, Oct 14, 2009 at 11:10 AM, <timurrrr@chromium.org> wrote:

> On 2009/10/14 16:27:24, darin wrote:
>
>  On Wed, Oct 14, 2009 at 7:43 AM, <mailto:timurrrr@chromium.org> wrote:
>>
>
>  > Thank you for your review.
>> >
>> > Is there any better way to disable a specific test if DEBUG_MODE is not
>> 1?
>> >
>> > Currently, I do the following:
>> > // AtomicFlag should die on a DCHECK if Set() is called more than once.
>> > // DCHECK works only if DEBUG_MODE is equal to 1.
>> > #if DEBUG_MODE
>> > #define DOUBLE_SET_DEATH_TEST_NAME DoubleSetDeathTest
>> > #else
>> > #define DOUBLE_SET_DEATH_TEST_NAME DISABLED_DoubleSetDeathTest
>> > #endif
>> > TEST_F(AtomicFlagTest, DOUBLE_SET_DEATH_TEST_NAME) {
>> >  ...
>> > }
>> >
>> >
>> something like this might be better for readability:
>>
>
>  #if DEBUG_MODE
>> #define DISABLED_IN_RELEASE(x) x
>> #else
>> #define DISABLED_IN_RELEASE(x) DISABLED_##x
>> #endif
>>
>
>  TEST_F(AtomicFlag, DISABLED_IN_RELEASE(DoubleSetDeathTest))
>>
>
>  you could also call it DISABLED_IN_NDEBUG and use the NDEBUG define
>> instead
>> of DEBUG_MODE.
>>
>
>  -darin
>>
> Unfortunately, both approaches don't work.
>
> * DEBUG_MODE is an enum constant, so #if doesn't catch it
>

oh, right.  NDEBUG is the way to go then.




> * NDEBUG and DCHECK are _both_ not defined on Windows bot...
> Any suggestions?


that doesn't make sense.



>
>
>  >
>> > http://codereview.chromium.org/276002
>> >
>>
>
>
>
>
> http://codereview.chromium.org/276002
>

[Timur Iskhodzhanov, 2009-10-14 19:17:40]

> > * NDEBUG and DCHECK are _both_ not defined on Windows bot...
> > Any suggestions?
> that doesn't make sense.
That doesn't make sense for me either, but:

(added the following dummy test in patch set 11)
 44 TEST_F(AtomicFlagTest, TestDCHECK) {
 45 #ifdef NDEBUG
 46   LOG(INFO) << "NDEBUG is defined";
 47 #else
 48   LOG(INFO) << "NDEBUG is not defined";
 49 #endif
 50   DCHECK(return_true());
 51 }

On Linux and Mac:
[----------] 5 tests from AtomicFlagTest
[ RUN      ] AtomicFlagTest.TestDCHECK
[39704:267:1014/114726:1826527949878585:INFO:/b/slave/mac/build/src/base/atomic_flag_unittest.cc(48)]
NDEBUG is not defined
[39704:267:1014/114726:1826527949921430:INFO:/b/slave/mac/build/src/base/atomic_flag_unittest.cc(40)]
return_true was called
[       OK ] AtomicFlagTest.TestDCHECK (1 ms)
[ RUN      ] AtomicFlagTest.SimpleSingleThreadedTest
[       OK ] AtomicFlagTest.SimpleSingleThreadedTest (0 ms)
[ RUN      ] AtomicFlagTest.SimpleSingleThreadedTestPrenotified
[       OK ] AtomicFlagTest.SimpleSingleThreadedTestPrenotified (0 ms)
[ RUN      ] AtomicFlagTest.DoubleSetDeathTest
[       OK ] AtomicFlagTest.DoubleSetDeathTest (14335 ms)
[ RUN      ] AtomicFlagTest.SimpleThreadedTest
[       OK ] AtomicFlagTest.SimpleThreadedTest (89 ms)
[----------] 5 tests from AtomicFlagTest (14426 ms total)

On Windows:
[----------] 5 tests from AtomicFlagTest
[ RUN      ] AtomicFlagTest.TestDCHECK
[       OK ] AtomicFlagTest.TestDCHECK (0 ms)
[ RUN      ] AtomicFlagTest.SimpleSingleThreadedTest
[       OK ] AtomicFlagTest.SimpleSingleThreadedTest (0 ms)
[ RUN      ] AtomicFlagTest.SimpleSingleThreadedTestPrenotified
[       OK ] AtomicFlagTest.SimpleSingleThreadedTestPrenotified (0 ms)
[ RUN      ] AtomicFlagTest.DoubleSetDeathTest
c:\b\slave\win\build\src\base\atomic_flag_unittest.cc(78): error: Death test:
flag.Set()
    Result: failed to die.
 Error msg:
[  FAILED  ] AtomicFlagTest.DoubleSetDeathTest (78 ms)
[ RUN      ] AtomicFlagTest.SimpleThreadedTest
[       OK ] AtomicFlagTest.SimpleThreadedTest (62 ms)
[----------] 5 tests from AtomicFlagTest (140 ms total)

[Timur Iskhodzhanov, 2009-10-14 19:20:55]

Forgot to mention
 39 bool return_true() {
 40   LOG(INFO) << "return_true was called";
 41   return true;
 42 }

[darin (slow to review), 2009-10-14 19:31:56]

logging on windows does not go to stdout.  it goes to a log file.-darin

On Wed, Oct 14, 2009 at 12:17 PM, <timurrrr@chromium.org> wrote:

> > * NDEBUG and DCHECK are _both_ not defined on Windows bot...
>> > Any suggestions?
>> that doesn't make sense.
>>
> That doesn't make sense for me either, but:
>
> (added the following dummy test in patch set 11)
>  44 TEST_F(AtomicFlagTest, TestDCHECK) {
>  45 #ifdef NDEBUG
>  46   LOG(INFO) << "NDEBUG is defined";
>  47 #else
>  48   LOG(INFO) << "NDEBUG is not defined";
>  49 #endif
>  50   DCHECK(return_true());
>  51 }
>
> On Linux and Mac:
> [----------] 5 tests from AtomicFlagTest
> [ RUN      ] AtomicFlagTest.TestDCHECK
>
>
[39704:267:1014/114726:1826527949878585:INFO:/b/slave/mac/build/src/base/atomic_flag_unittest.cc(48)]
> NDEBUG is not defined
>
>
[39704:267:1014/114726:1826527949921430:INFO:/b/slave/mac/build/src/base/atomic_flag_unittest.cc(40)]
> return_true was called
> [       OK ] AtomicFlagTest.TestDCHECK (1 ms)
> [ RUN      ] AtomicFlagTest.SimpleSingleThreadedTest
> [       OK ] AtomicFlagTest.SimpleSingleThreadedTest (0 ms)
> [ RUN      ] AtomicFlagTest.SimpleSingleThreadedTestPrenotified
> [       OK ] AtomicFlagTest.SimpleSingleThreadedTestPrenotified (0 ms)
> [ RUN      ] AtomicFlagTest.DoubleSetDeathTest
> [       OK ] AtomicFlagTest.DoubleSetDeathTest (14335 ms)
> [ RUN      ] AtomicFlagTest.SimpleThreadedTest
> [       OK ] AtomicFlagTest.SimpleThreadedTest (89 ms)
> [----------] 5 tests from AtomicFlagTest (14426 ms total)
>
> On Windows:
> [----------] 5 tests from AtomicFlagTest
> [ RUN      ] AtomicFlagTest.TestDCHECK
> [       OK ] AtomicFlagTest.TestDCHECK (0 ms)
> [ RUN      ] AtomicFlagTest.SimpleSingleThreadedTest
> [       OK ] AtomicFlagTest.SimpleSingleThreadedTest (0 ms)
> [ RUN      ] AtomicFlagTest.SimpleSingleThreadedTestPrenotified
> [       OK ] AtomicFlagTest.SimpleSingleThreadedTestPrenotified (0 ms)
> [ RUN      ] AtomicFlagTest.DoubleSetDeathTest
> c:\b\slave\win\build\src\base\atomic_flag_unittest.cc(78): error: Death
> test:
> flag.Set()
>    Result: failed to die.
>  Error msg:
> [  FAILED  ] AtomicFlagTest.DoubleSetDeathTest (78 ms)
> [ RUN      ] AtomicFlagTest.SimpleThreadedTest
> [       OK ] AtomicFlagTest.SimpleThreadedTest (62 ms)
> [----------] 5 tests from AtomicFlagTest (140 ms total)
>
>
> http://codereview.chromium.org/276002
>

[darin (slow to review), 2009-10-14 19:32:42]

at least, that is the case for LOG(INFO).  try LOG(ERROR).

On Wed, Oct 14, 2009 at 12:31 PM, Darin Fisher <darin@chromium.org> wrote:

> logging on windows does not go to stdout.  it goes to a log file.-darin
>
>
> On Wed, Oct 14, 2009 at 12:17 PM, <timurrrr@chromium.org> wrote:
>
>> > * NDEBUG and DCHECK are _both_ not defined on Windows bot...
>>> > Any suggestions?
>>> that doesn't make sense.
>>>
>> That doesn't make sense for me either, but:
>>
>> (added the following dummy test in patch set 11)
>>  44 TEST_F(AtomicFlagTest, TestDCHECK) {
>>  45 #ifdef NDEBUG
>>  46   LOG(INFO) << "NDEBUG is defined";
>>  47 #else
>>  48   LOG(INFO) << "NDEBUG is not defined";
>>  49 #endif
>>  50   DCHECK(return_true());
>>  51 }
>>
>> On Linux and Mac:
>> [----------] 5 tests from AtomicFlagTest
>> [ RUN      ] AtomicFlagTest.TestDCHECK
>>
>>
[39704:267:1014/114726:1826527949878585:INFO:/b/slave/mac/build/src/base/atomic_flag_unittest.cc(48)]
>> NDEBUG is not defined
>>
>>
[39704:267:1014/114726:1826527949921430:INFO:/b/slave/mac/build/src/base/atomic_flag_unittest.cc(40)]
>> return_true was called
>> [       OK ] AtomicFlagTest.TestDCHECK (1 ms)
>> [ RUN      ] AtomicFlagTest.SimpleSingleThreadedTest
>> [       OK ] AtomicFlagTest.SimpleSingleThreadedTest (0 ms)
>> [ RUN      ] AtomicFlagTest.SimpleSingleThreadedTestPrenotified
>> [       OK ] AtomicFlagTest.SimpleSingleThreadedTestPrenotified (0 ms)
>> [ RUN      ] AtomicFlagTest.DoubleSetDeathTest
>> [       OK ] AtomicFlagTest.DoubleSetDeathTest (14335 ms)
>> [ RUN      ] AtomicFlagTest.SimpleThreadedTest
>> [       OK ] AtomicFlagTest.SimpleThreadedTest (89 ms)
>> [----------] 5 tests from AtomicFlagTest (14426 ms total)
>>
>> On Windows:
>> [----------] 5 tests from AtomicFlagTest
>> [ RUN      ] AtomicFlagTest.TestDCHECK
>> [       OK ] AtomicFlagTest.TestDCHECK (0 ms)
>> [ RUN      ] AtomicFlagTest.SimpleSingleThreadedTest
>> [       OK ] AtomicFlagTest.SimpleSingleThreadedTest (0 ms)
>> [ RUN      ] AtomicFlagTest.SimpleSingleThreadedTestPrenotified
>> [       OK ] AtomicFlagTest.SimpleSingleThreadedTestPrenotified (0 ms)
>> [ RUN      ] AtomicFlagTest.DoubleSetDeathTest
>> c:\b\slave\win\build\src\base\atomic_flag_unittest.cc(78): error: Death
>> test:
>> flag.Set()
>>    Result: failed to die.
>>  Error msg:
>> [  FAILED  ] AtomicFlagTest.DoubleSetDeathTest (78 ms)
>> [ RUN      ] AtomicFlagTest.SimpleThreadedTest
>> [       OK ] AtomicFlagTest.SimpleThreadedTest (62 ms)
>> [----------] 5 tests from AtomicFlagTest (140 ms total)
>>
>>
>> http://codereview.chromium.org/276002
>>
>
>

[Timur Iskhodzhanov, 2009-10-14 20:36:08]

I've created a separate try-CL
http://codereview.chromium.org/277007
Seems like the issue is connected with threadsafe-Death_tests.
I'll investigate it more thoroughly tomorrow on a real Win machine...

On 2009/10/14 19:32:42, darin wrote:
> at least, that is the case for LOG(INFO).  try LOG(ERROR).
> 
> On Wed, Oct 14, 2009 at 12:31 PM, Darin Fisher <mailto:darin@chromium.org>
wrote:
> 
> > logging on windows does not go to stdout.  it goes to a log file.-darin
> >
> >
> > On Wed, Oct 14, 2009 at 12:17 PM, <mailto:timurrrr@chromium.org> wrote:
> >
> >> > * NDEBUG and DCHECK are _both_ not defined on Windows bot...
> >>> > Any suggestions?
> >>> that doesn't make sense.
> >>>
> >> That doesn't make sense for me either, but:
> >>
> >> (added the following dummy test in patch set 11)
> >>  44 TEST_F(AtomicFlagTest, TestDCHECK) {
> >>  45 #ifdef NDEBUG
> >>  46   LOG(INFO) << "NDEBUG is defined";
> >>  47 #else
> >>  48   LOG(INFO) << "NDEBUG is not defined";
> >>  49 #endif
> >>  50   DCHECK(return_true());
> >>  51 }
> >>
> >> On Linux and Mac:
> >> [----------] 5 tests from AtomicFlagTest
> >> [ RUN      ] AtomicFlagTest.TestDCHECK
> >>
> >>
>
[39704:267:1014/114726:1826527949878585:INFO:/b/slave/mac/build/src/base/atomic_flag_unittest.cc(48)]
> >> NDEBUG is not defined
> >>
> >>
>
[39704:267:1014/114726:1826527949921430:INFO:/b/slave/mac/build/src/base/atomic_flag_unittest.cc(40)]
> >> return_true was called
> >> [       OK ] AtomicFlagTest.TestDCHECK (1 ms)
> >> [ RUN      ] AtomicFlagTest.SimpleSingleThreadedTest
> >> [       OK ] AtomicFlagTest.SimpleSingleThreadedTest (0 ms)
> >> [ RUN      ] AtomicFlagTest.SimpleSingleThreadedTestPrenotified
> >> [       OK ] AtomicFlagTest.SimpleSingleThreadedTestPrenotified (0 ms)
> >> [ RUN      ] AtomicFlagTest.DoubleSetDeathTest
> >> [       OK ] AtomicFlagTest.DoubleSetDeathTest (14335 ms)
> >> [ RUN      ] AtomicFlagTest.SimpleThreadedTest
> >> [       OK ] AtomicFlagTest.SimpleThreadedTest (89 ms)
> >> [----------] 5 tests from AtomicFlagTest (14426 ms total)
> >>
> >> On Windows:
> >> [----------] 5 tests from AtomicFlagTest
> >> [ RUN      ] AtomicFlagTest.TestDCHECK
> >> [       OK ] AtomicFlagTest.TestDCHECK (0 ms)
> >> [ RUN      ] AtomicFlagTest.SimpleSingleThreadedTest
> >> [       OK ] AtomicFlagTest.SimpleSingleThreadedTest (0 ms)
> >> [ RUN      ] AtomicFlagTest.SimpleSingleThreadedTestPrenotified
> >> [       OK ] AtomicFlagTest.SimpleSingleThreadedTestPrenotified (0 ms)
> >> [ RUN      ] AtomicFlagTest.DoubleSetDeathTest
> >> c:\b\slave\win\build\src\base\atomic_flag_unittest.cc(78): error: Death
> >> test:
> >> flag.Set()
> >>    Result: failed to die.
> >>  Error msg:
> >> [  FAILED  ] AtomicFlagTest.DoubleSetDeathTest (78 ms)
> >> [ RUN      ] AtomicFlagTest.SimpleThreadedTest
> >> [       OK ] AtomicFlagTest.SimpleThreadedTest (62 ms)
> >> [----------] 5 tests from AtomicFlagTest (140 ms total)
> >>
> >>
> >> http://codereview.chromium.org/276002
> >>
> >
> >
>

[Timur Iskhodzhanov, 2009-10-15 11:48:23]

I've updated the changelist.
I've decided to disable the  death test on windows due to
http://code.google.com/p/chromium/issues/detail?id=24885

[darin (slow to review), 2009-10-15 18:05:54]

LGTM w/ one nit:

http://codereview.chromium.org/276002/diff/12/1022
File base/atomic_flag_unittest.cc (right):

http://codereview.chromium.org/276002/diff/12/1022#newcode48
Line 48: #if GTEST_OS_WINDOWS
please use #if defined(OS_WIN) instead