DCHECK_ALWAYS_ON and Release mode only code

[Ryan Sleevi]

This is somewhat of a follow-up from
https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thread/2b71990726d9eef4
.

Currently, we have two useful classes for checking thread-affinity/
ownership: base::ThreadChecker and base::NonThreadSafe. The former is
used for membership-based checks (eg: uses composition), the latter is
used for inheritance based checks (eg: ... uses inheritance).
NonThreadSafe is implemented in terms of ThreadChecker, so it's
"mostly" syntactic fluff. However...

ThreadChecker is currently gated on !defined(NDEBUG) ||
defined(DCHECK_ALWAYS_ON) ( http://crrev.com/116350 ). NonThreadSafe
is gated on simply !defined(NDEBUG). This means that, on the Release
bots (which define DCHECK_ALWAYS_ON), ThreadChecker code is enabled,
but NonThreadSafe checks (which use ThreadChecker) are not.

Two questions:
1) Since the APIs are parallel, should NonThreadSafe also be checking
based on DCHECK_ALWAYS_ON?
- Arguments for: More checks for programmer error as early as
possible, the better.
- Arguments against: The overhead involved with the checking may
impact timing (thus hiding bugs otherwise caught by release/debug
split) or may cause perf regressions (which are methodology issues,
since Release != Official)
2) If we have any general rule about such things for, within Chromium
code (eg: not counting third-party code), when it's appropriate to
only check #ifndef NDEBUG. For example, base/memory/ref_counted.h has
a DCHECK on a member "in_dtor_", to check for possible double-frees/U-
A-F, but that code is gated on NDEBUG. Various other DCHECKs
throughout the code are similarly guarded. The previous thread
proposed something similar to DCHECK_IS_ON, but did not reach any
conclusion.

[John Abd-El-Malek]

I hit a similar issue with base::ThreadRestrictions: they were only checking for debug builds, which means violations would happily go through the default bots which are release only to break the main tree. I switched them to !OFFICIAL_BUILD instead. If I had known about DCHECK_IS_ON, I would have used that :)

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
   http://groups.google.com/a/chromium.org/group/chromium-dev