Enable ThreadChecker in non-debug builds if DCHECK_ALWAYS_ON is

Enable ThreadChecker in non-debug builds if DCHECK_ALWAYS_ON is
defined.

This brings thread checking to e.g. the *_rel trybots.

BUG=108227
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=116350

[joth, 2011-12-21 14:17:15]

drive-by (no need to wait for me on any follow-ups)

http://codereview.chromium.org/9020008/diff/3001/base/threading/thread_checke...
File base/threading/thread_checker_unittest.cc (right):

http://codereview.chromium.org/9020008/diff/3001/base/threading/thread_checke...
base/threading/thread_checker_unittest.cc:13: // good citizens there and undef
the macro.
yeah the duplication is unfortunate but I can't see a cleaner way (and the test
would fail anyway if this was incorrect... although whether we'd run them in
that failing config is another question!)

FWIW you could simplify it both places to
  #define ENABLE_THREAD_CHECKER (!defined(NDEBUG) || defined(DCHECK_ALWAYS_ON))

(Personally I think a much better approach would be to have a project wide
define and use that in place of NDEBUG everywhere we care if DCHECK is compiled
in, to avoid this inelegant expression cropping up numerous places. No doubt
sometimes with !OFFICIAL_BUILD sprinkled in too).

[Jói, 2011-12-21 14:58:27]

> FWIW you could simplify it both places to>  #define ENABLE_THREAD_CHECKER
(!defined(NDEBUG) ||> defined(DCHECK_ALWAYS_ON))
That's much nicer :)  Uploaded a new patch.

On Wed, Dec 21, 2011 at 2:17 PM,  <joth@chromium.org> wrote:
> drive-by (no need to wait for me on any follow-ups)
>
>
>
http://codereview.chromium.org/9020008/diff/3001/base/threading/thread_checke...
> File base/threading/thread_checker_unittest.cc (right):
>
>
http://codereview.chromium.org/9020008/diff/3001/base/threading/thread_checke...
> base/threading/thread_checker_unittest.cc:13: // good citizens there and
> undef the macro.
> yeah the duplication is unfortunate but I can't see a cleaner way (and
> the test would fail anyway if this was incorrect... although whether
> we'd run them in that failing config is another question!)
>
> FWIW you could simplify it both places to
>  #define ENABLE_THREAD_CHECKER (!defined(NDEBUG) ||
> defined(DCHECK_ALWAYS_ON))
>
> (Personally I think a much better approach would be to have a project
> wide define and use that in place of NDEBUG everywhere we care if DCHECK
> is compiled in, to avoid this inelegant expression cropping up numerous
> places. No doubt sometimes with !OFFICIAL_BUILD sprinkled in too).
>
> http://codereview.chromium.org/9020008/

[Jói, 2011-12-21 16:22:34]

So there was a test asserting debug death (and non-death in release mode) which
no longer matches the times ThreadChecker is enabled.

Please see the new patch, which adds a ThreadChecker::IsEnabled function.

I'm thinking it might just be cleaner to expose ENABLE_THREAD_CHECKER from the
thread_checker.h header (maybe rename it THREAD_CHECKER_ENABLED), or cleaner yet
to expose something from logging.h that is essentially the same as
ENABLE_THREAD_CHECKER, i.e. in the current configuration, DCHECKs are always
enabled.

WDYT?

Cheers,
Jói

[Nico, 2011-12-21 18:10:11]

I think this change is a great idea.

It looks like most of the complexity here comes from the ASSERT_DEBUG_DEATH
tests – i'll posit that these are just a Bad Idea, and that we should either
convert that DCHECK to a CHECK and have a real death test, or remove that test.
Then this CL gets a lot simpler – wdyt?

http://codereview.chromium.org/9020008/diff/7003/base/message_pump_libevent_u...
File base/message_pump_libevent_unittest.cc (left):

http://codereview.chromium.org/9020008/diff/7003/base/message_pump_libevent_u...
base/message_pump_libevent_unittest.cc:70:
ASSERT_DEBUG_DEATH(io_loop()->WatchFileDescriptor(
How is ASSERT_DEBUG_DEATH a useful test? I think we should just kill this test.

[Jói, 2011-12-27 14:13:11]

Hi Nico,

Agreed the ASSERT_*_DEATH tests are often not that useful.  In this case the
test is verifying that ThreadChecker will DCHECK on a particular error.  Not
sure whether killing the test is necessarily the right thing to do, I've added
dmaclach@ who seems to have authored this test.

I've simplified the change, removing the IsEnabled thing and just using
!defined(NDEBUG) to turn on the death test (which then is just ASSERT_DEATH
rather than ASSERT_DEBUG_DEATH).  I'd like to just commit it this way, and then
we can deal with the question of death tests separately and more
comprehensively.  Let me know what you think.

Cheers,
Jói

[dmac, 2011-12-28 05:38:09]

IIRC I had a case where I was watching a file descriptor off of a wrong thread.
It caused me a lot of pain, so I added the DCHECK to help prevent people from
doing it in the future. It can be turned into a CHECK if it simplifies things. I
left it as a DCHECK initially because I figured that it having the FD be watched
from the wrong thread was a design flaw that the DCHECK would notify developers
about pretty much as soon as they exercised the code they were writing, and in
release mode the code just wouldn't work.

[Jói, 2011-12-28 11:12:07]

Dave, in my opinion the test is fine.  At this point it probably just
verifies that ThreadChecker works correctly (already unit tested
elsewhere) and that your code is calling CalledOnValidThread in the
appropriate places (a valid test of your code).  I only wish we had a
more efficient way to test that DCHECKs are hit, than by using a death
test.

Nico, if you agree I will check in as is.  The complexity is gone, I think.

Cheers,
Jói



On Wed, Dec 28, 2011 at 5:38 AM,  <dmaclach@chromium.org> wrote:
> IIRC I had a case where I was watching a file descriptor off of a wrong
> thread.
> It caused me a lot of pain, so I added the DCHECK to help prevent people
> from
> doing it in the future. It can be turned into a CHECK if it simplifies
> things. I
> left it as a DCHECK initially because I figured that it having the FD be
> watched
> from the wrong thread was a design flaw that the DCHECK would notify
> developers
> about pretty much as soon as they exercised the code they were writing, and
> in
> release mode the code just wouldn't work.
>
> http://codereview.chromium.org/9020008/

[Nico, 2012-01-02 23:42:36]

(sorry for the delay; new year's and stuff)

http://codereview.chromium.org/9020008/diff/8001/base/message_pump_libevent_u...
File base/message_pump_libevent_unittest.cc (right):

http://codereview.chromium.org/9020008/diff/8001/base/message_pump_libevent_u...
base/message_pump_libevent_unittest.cc:62: #if GTEST_HAS_DEATH_TEST &&
!defined(NDEBUG)
Isn't this now also a test that won't fail on the (rel) trybots?

[Jói, 2012-01-04 09:29:29]

Thanks Nico.  New Year's etc. delays on this end as well :)

Cheers,
Jói

http://codereview.chromium.org/9020008/diff/8001/base/message_pump_libevent_u...
File base/message_pump_libevent_unittest.cc (right):

http://codereview.chromium.org/9020008/diff/8001/base/message_pump_libevent_u...
base/message_pump_libevent_unittest.cc:62: #if GTEST_HAS_DEATH_TEST &&
!defined(NDEBUG)
On 2012/01/02 23:42:37, Nico wrote:
> Isn't this now also a test that won't fail on the (rel) trybots?

True, but it's also a test that's not likely to break.

The options here seem to be:
a) Leave it as is;
b) Remove the test altogether (easiest, but it's not an empty test, it tests
that ThreadChecker is used correctly in this class);
c) Use the same preprocessor sequence here as in ThreadChecker, i.e.
!defined(NDEBUG) || defined(DCHECK_ALWAYS_ON) (some code duplication)
d) Expose ENABLE_THREAD_CHECKER to users of base/threading/thread_checker.h and
use it here;
e) Bring back the IsEnabled thing I had in a previous patch set.

I would propose either (a) or (d).  I'm no longer a fan of (e) since what you
really need is a preprocessor check rather than an at-runtime check, plus (e) is
slightly more complex.

[Nico, 2012-01-04 16:15:10]

http://codereview.chromium.org/9020008/diff/8001/base/message_pump_libevent_u...
File base/message_pump_libevent_unittest.cc (right):

http://codereview.chromium.org/9020008/diff/8001/base/message_pump_libevent_u...
base/message_pump_libevent_unittest.cc:62: #if GTEST_HAS_DEATH_TEST &&
!defined(NDEBUG)
On 2012/01/04 09:29:32, Jói wrote:
> True, but it's also a test that's not likely to break.

Famous last words :-)

> The options here seem to be:
> a) Leave it as is;
> b) Remove the test altogether (easiest, but it's not an empty test, it tests
> that ThreadChecker is used correctly in this class);

(Saying both "safe, because unlikely to break" and "can't remove this test, it
checks something important" sounds a bit contradictory to me :-) )

> c) Use the same preprocessor sequence here as in ThreadChecker, i.e.
> !defined(NDEBUG) || defined(DCHECK_ALWAYS_ON) (some code duplication)
> d) Expose ENABLE_THREAD_CHECKER to users of base/threading/thread_checker.h
and
> use it here;
> e) Bring back the IsEnabled thing I had in a previous patch set.

f) Turn the thread checker check into a CHECK() (right?)

But ok, let's go with (a) for now and see how it goes. LGTM.

[Jói, 2012-01-04 16:23:11]

tl;dr:  OK, will go with the current version of the change and see how it goes.

> (Saying both "safe, because unlikely to break" and "can't remove this> test,
it checks something important" sounds a bit contradictory to me> :-) )
You're right, hehe :)

> f) Turn the thread checker check into a CHECK() (right?)

I'm assuming you mean doing that and removing all preprocessor
conditionals around the test except the "has death test" one?

That would also imply using ThreadCheckerImpl in builds where DCHECK
is not enabled, whereas today we use the much faster
ThreadCheckerDoNothing.  I think the desire is (or at least was) to
avoid the overhead of checking thread usage in "fast" builds.

Cheers,
Jói



On Wed, Jan 4, 2012 at 4:15 PM,  <thakis@chromium.org> wrote:
>
>
http://codereview.chromium.org/9020008/diff/8001/base/message_pump_libevent_u...
> File base/message_pump_libevent_unittest.cc (right):
>
>
http://codereview.chromium.org/9020008/diff/8001/base/message_pump_libevent_u...
> base/message_pump_libevent_unittest.cc:62: #if GTEST_HAS_DEATH_TEST &&
> !defined(NDEBUG)
> On 2012/01/04 09:29:32, Jói wrote:
>>
>> True, but it's also a test that's not likely to break.
>
>
> Famous last words :-)
>
>
>> The options here seem to be:
>> a) Leave it as is;
>> b) Remove the test altogether (easiest, but it's not an empty test, it
>
> tests
>>
>> that ThreadChecker is used correctly in this class);
>
>
> (Saying both "safe, because unlikely to break" and "can't remove this
> test, it checks something important" sounds a bit contradictory to me
> :-) )
>
>
>> c) Use the same preprocessor sequence here as in ThreadChecker, i.e.
>> !defined(NDEBUG) || defined(DCHECK_ALWAYS_ON) (some code duplication)
>> d) Expose ENABLE_THREAD_CHECKER to users of
>
> base/threading/thread_checker.h and
>>
>> use it here;
>> e) Bring back the IsEnabled thing I had in a previous patch set.
>
>
> f) Turn the thread checker check into a CHECK() (right?)
>
> But ok, let's go with (a) for now and see how it goes. LGTM.
>
> http://codereview.chromium.org/9020008/

[commit-bot: I haz the power, 2012-01-04 16:24:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/9020008/16001

[commit-bot: I haz the power, 2012-01-04 16:25:04]

Presubmit check for 9020008-16001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
base/threading/thread_checker.h,base/message_pump_libevent_unittest.cc,base/threading/thread_checker_unittest.cc

Presubmit checks took 2.7s to calculate.

[Jói, 2012-01-04 16:28:00]

+jar for base/OWNERS approval.

Cheers,
Jói

[jar (doing other things), 2012-01-04 17:06:28]

lgtm

[commit-bot: I haz the power, 2012-01-04 17:36:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/9020008/16001

[commit-bot: I haz the power, 2012-01-04 19:02:40]

Change committed as 116350