Re: When should we use #if DCHECK_IS_ON()

223 views
Skip to first unread message

Nico Weber

unread,
Jun 30, 2021, 11:34:44 AM6/30/21
to Aaron Leventhal, cxx, Nektarios Paisios
+cxx

3 seems right to me. You want the first check so that you don't get an "unused function" warning. You shouldn't need to surround the DCHECK though. Do you have a link to a bot where that didn't work?

On Wed, Jun 30, 2021, 11:17 AM Aaron Leventhal <aleve...@google.com> wrote:
Hi Nico,

If you have a moment, Nektarios and I were debating which of these is better?

Version 1:
#if DCHECK_IS_ON()
bool IsValid() {
...
}
#endif

void MyClass::OnDataChanged() {
#if DCHECK_IS_ON()
  DCHECK(IsValid());
#endif
}

Version 2:
Same as #1 but remove #if DCHECK_IS_ON().
My concern with this one is whether our linkers on all platforms will be smart enough to not include IsValid().

Version 3:
Same as #1 but remove only the last #if DCHECK_IS_ON(). Keep the first one around IsValid().
I know this one isn't right, because i've tried it and it failed to compile on some platforms, which really surprised me? Why should I need #if DCHECK_IS_ON() around a DCHECK() ?

Thanks for any advice,
Aaron


dan...@chromium.org

unread,
Jun 30, 2021, 11:43:55 AM6/30/21
to Nico Weber, Aaron Leventhal, cxx, Nektarios Paisios
On Wed, Jun 30, 2021 at 11:34 AM Nico Weber <tha...@chromium.org> wrote:
+cxx

3 seems right to me. You want the first check so that you don't get an "unused function" warning. You shouldn't need to surround the DCHECK though. Do you have a link to a bot where that didn't work?

DCHECK marks the argument as used, even if disabled to avoid "unused variable" warnings. So that probably means IsValid() needs to exist.


You could put the #if DCHECK_IS_ON() inside the IsValid() function, or you can keep both DCHECK_IS_ON() or remove both.

In terms of LTO, the function should have no side effects, and its return value would be ignored. If that's sufficient to remove the function call then it could. If the function body isn't obvious enough to the compiler it may assume side effects?

I might err toward 1 in such cases.

Dana
 

On Wed, Jun 30, 2021, 11:17 AM Aaron Leventhal <aleve...@google.com> wrote:
Hi Nico,

If you have a moment, Nektarios and I were debating which of these is better?

Version 1:
#if DCHECK_IS_ON()
bool IsValid() {
...
}
#endif

void MyClass::OnDataChanged() {
#if DCHECK_IS_ON()
  DCHECK(IsValid());
#endif
}

Version 2:
Same as #1 but remove #if DCHECK_IS_ON().
My concern with this one is whether our linkers on all platforms will be smart enough to not include IsValid().

Version 3:
Same as #1 but remove only the last #if DCHECK_IS_ON(). Keep the first one around IsValid().
I know this one isn't right, because i've tried it and it failed to compile on some platforms, which really surprised me? Why should I need #if DCHECK_IS_ON() around a DCHECK() ?

Thanks for any advice,
Aaron


--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAMGbLiG5fVnmnhv0FZA8h8mZcdRT_jvGKdrDr7-1w2N5k-QM2g%40mail.gmail.com.

Aaron Leventhal

unread,
Jun 30, 2021, 12:04:09 PM6/30/21
to dan...@chromium.org, Nico Weber, cxx, Nektarios Paisios
I should be able to put up a build that shows the compilation error that I've seen in the past for #3.

Anton Bikineev

unread,
Jun 30, 2021, 12:09:56 PM6/30/21
to dan...@chromium.org, Nico Weber, Aaron Leventhal, cxx, Nektarios Paisios
Fwiw, once we had a regression in Oilpan that was caused by not wrapping

DCHECK_EQ(some_value, FreeListSize())

in #if DCHECK_IS_ON().

Even though FreeListSize() was fully inlinable and side-effect-free, the compiler decided to not drop it. What made this bug insidious is that on non-official release builds the compiler managed to optimize the call away. The problem was only reproducible on official release builds (wrong AFDO feedback?).

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaRqfZg_VqiHsV%2BvSgN7MaAeXXZpe88sV14DjV6TZouR0Q%40mail.gmail.com.

dan...@chromium.org

unread,
Jun 30, 2021, 12:14:31 PM6/30/21
to Anton Bikineev, Nico Weber, Aaron Leventhal, cxx, Nektarios Paisios
On Wed, Jun 30, 2021 at 12:09 PM Anton Bikineev <biki...@google.com> wrote:
Fwiw, once we had a regression in Oilpan that was caused by not wrapping

DCHECK_EQ(some_value, FreeListSize())

in #if DCHECK_IS_ON().

Even though FreeListSize() was fully inlinable and side-effect-free, the compiler decided to not drop it. What made this bug insidious is that on non-official release builds the compiler managed to optimize the call away. The problem was only reproducible on official release builds (wrong AFDO feedback?).

Maybe we should use some metaprogramming to avoid calling the function in this case, since "unused function" isn't a warning like "unused variable".

Anton Bikineev

unread,
Jun 30, 2021, 12:21:41 PM6/30/21
to dan...@chromium.org, Nico Weber, Aaron Leventhal, cxx, Nektarios Paisios
Maybe we should use some metaprogramming to avoid calling the function in this case, since "unused function" isn't a warning like "unused variable".

What we did in cppgc (new Oilpan) is we put the DCHECK expression in unevaluated context (as a decltype operand):

Maybe something like this could also work for base/logging.h.

dan...@chromium.org

unread,
Jun 30, 2021, 12:35:13 PM6/30/21
to Anton Bikineev, Nico Weber, Aaron Leventhal, cxx, Nektarios Paisios
On Wed, Jun 30, 2021 at 12:14 PM <dan...@chromium.org> wrote:
On Wed, Jun 30, 2021 at 12:09 PM Anton Bikineev <biki...@google.com> wrote:
Fwiw, once we had a regression in Oilpan that was caused by not wrapping

Hm! That regression was with DCHECK_EQ which is implemented differently. It actually uses each parameter twice even when disabled. First, converting them to strings, then passing the expression to EAT_CHECK_STREAM_PARAMS. This doesn't look necessary to me, and maybe that interferes with eliding the call too: https://chromium-review.googlesource.com/c/chromium/src/+/2997682/

dan...@chromium.org

unread,
Jun 30, 2021, 12:48:03 PM6/30/21
to Anton Bikineev, Nico Weber, Aaron Leventhal, cxx, Nektarios Paisios
On Wed, Jun 30, 2021 at 12:21 PM Anton Bikineev <biki...@google.com> wrote:
Maybe we should use some metaprogramming to avoid calling the function in this case, since "unused function" isn't a warning like "unused variable".

What we did in cppgc (new Oilpan) is we put the DCHECK expression in unevaluated context (as a decltype operand):

Maybe something like this could also work for base/logging.h.

That looks like a great idea, something we couldn't do when this code was first written, but could now.

dan...@chromium.org

unread,
Jun 30, 2021, 1:58:33 PM6/30/21
to Anton Bikineev, Nico Weber, Aaron Leventhal, cxx, Nektarios Paisios
On Wed, Jun 30, 2021 at 12:47 PM <dan...@chromium.org> wrote:
On Wed, Jun 30, 2021 at 12:21 PM Anton Bikineev <biki...@google.com> wrote:
Maybe we should use some metaprogramming to avoid calling the function in this case, since "unused function" isn't a warning like "unused variable".

What we did in cppgc (new Oilpan) is we put the DCHECK expression in unevaluated context (as a decltype operand):

Maybe something like this could also work for base/logging.h.

That looks like a great idea, something we couldn't do when this code was first written, but could now.

I guess that cppgc DCHECKs are never written with lambdas inside them because of this decision. Applying it to base fails in a lot of places, where the DCHECK expression has a lambda in it.

For example:
../../cc/metrics/frame_sorter.cc:102:9: error: lambda expression in an unevaluated operand
        [](const viz::BeginFrameArgs& one, const viz::BeginFrameArgs& two) {

But in the process, I learnt that the compiler is definitely not using the (expr) in EAT_CHECK_STREAM_PARAMS(expr), as there's many callers to the macro that do not pass an expr. I suspect then the regression was because of the difference in DCHECK_OP, where it built a string out of the lhs and rhs as well.

Anton Bikineev

unread,
Jun 30, 2021, 3:31:42 PM6/30/21
to dan...@chromium.org, Nico Weber, Aaron Leventhal, cxx, Nektarios Paisios
I guess that cppgc DCHECKs are never written with lambdas inside them because of this decision. 

Ah, indeed. Lambdas are not allowed in the unevaluated context (until C++20, I think). Thanks for checking!

Nektarios Paisios

unread,
Jun 30, 2021, 4:50:14 PM6/30/21
to Anton Bikineev, dan...@chromium.org, Nico Weber, Aaron Leventhal, cxx
So, it sounds like number 1 should be the preferred way to write this because the compiler might not optimize the function away in all cases?
Nektarios.

dan...@chromium.org

unread,
Jun 30, 2021, 5:19:34 PM6/30/21
to Nektarios Paisios, Anton Bikineev, Nico Weber, Aaron Leventhal, cxx
On Wed, Jun 30, 2021 at 4:50 PM Nektarios Paisios <nek...@google.com> wrote:
So, it sounds like number 1 should be the preferred way to write this because the compiler might not optimize the function away in all cases?
Nektarios.

I tried further hiding the DCHECK_OP string generation from runtime, but it had an exactly 0-byte effect on binary size in an official build, so it does seem that anything inside the DCHECK macro itself will be properly elided.

The example given where it was not caught included code written outside the DCHECK. So, AFAICT option 2 should be fine. If anyone can show that the linker is not removing unused functions then that'd be good to fix, but sounds unrelated to DCHECKs.

Anton Bikineev

unread,
Jun 30, 2021, 7:35:22 PM6/30/21
to dan...@chromium.org, Nektarios Paisios, Nico Weber, Aaron Leventhal, cxx
The example given where it was not caught included code written outside the DCHECK. So, AFAICT option 2 should be fine. If anyone can show that the linker is not removing unused functions then that'd be good to fix, but sounds unrelated to DCHECKs.
Agreed. Just in case, I tested the binary size with a call to 1MB function inside a DCHECK and there was no difference: https://chromium-review.googlesource.com/c/chromium/src/+/2998706

So version 2 should be fine. However, if you have some code around a DCHECK, like

void MyClass::OnDataChanged(Data data) {
  const int checksum = ComputeCheckSum(data);
  DCHECK_EQ(cached_checksum_, checksum);
}

it's safer to wrap it in #if DCHECK_IS_ON(), as the compiler is not guaranteed to dead-code-eliminate it.

dan...@chromium.org

unread,
Jul 2, 2021, 12:01:12 PM7/2/21
to Anton Bikineev, Nektarios Paisios, Nico Weber, Aaron Leventhal, cxx
On Wed, Jun 30, 2021 at 7:35 PM Anton Bikineev <biki...@google.com> wrote:
The example given where it was not caught included code written outside the DCHECK. So, AFAICT option 2 should be fine. If anyone can show that the linker is not removing unused functions then that'd be good to fix, but sounds unrelated to DCHECKs.
Agreed. Just in case, I tested the binary size with a call to 1MB function inside a DCHECK and there was no difference: https://chromium-review.googlesource.com/c/chromium/src/+/2998706

So version 2 should be fine. However, if you have some code around a DCHECK, like

void MyClass::OnDataChanged(Data data) {
  const int checksum = ComputeCheckSum(data);
  DCHECK_EQ(cached_checksum_, checksum);
}

it's safer to wrap it in #if DCHECK_IS_ON(), as the compiler is not guaranteed to dead-code-eliminate it.

Thanks. I've written something for our Do's and Don'ts: https://chromium-review.googlesource.com/c/chromium/src/+/3003582/
Reply all
Reply to author
Forward
0 new messages