1369837 - security/sandbox/chromium/base/time/time.h:690:37: error: possible misuse of comma operator here [-Werror,-Wcomma]

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Daniel Holbert [:dholbert]]

Filing this bug for a build warning that I'm getting (many times over) from a chromium header that gets included in various places.

I'm using clang 5.0 Trunk (built from svn 304217), and the build warning output looks like this (for every file that indirectly includes this "time.h" header:

==========
 2:57.37 In file included from /scratch/work/builds/mozilla-inbound/mozilla/security/sandbox/chromium-shim/base/threading/platform_thread_linux.cpp:11:
 2:57.37 In file included from /scratch/work/builds/mozilla-inbound/mozilla/security/sandbox/chromium/base/threading/platform_thread.h:16:
 2:57.37 /scratch/work/builds/mozilla-inbound/mozilla/security/sandbox/chromium/base/time/time.h:690:37: error: possible misuse of comma operator here [-Werror,-Wcomma]
 2:57.37           DCHECK(positive_value > 0),
 2:57.37                                     ^
 2:57.37 /scratch/work/builds/mozilla-inbound/mozilla/security/sandbox/chromium/base/time/time.h:690:11: note: cast expression to void to silence warning
 2:57.37           DCHECK(positive_value > 0),
 2:57.37           ^~~~~~~~~~~~~~~~~~~~~~~~~~
 2:57.37           static_cast<void>(        )
 2:57.37 /scratch/work/builds/mozilla-inbound/mozilla/security/sandbox/chromium/base/logging.h:702:3: note: expanded from macro 'DCHECK'
 2:57.37   LAZY_STREAM(LOG_STREAM(DCHECK), DCHECK_IS_ON() ? !(condition) : false) \
 2:57.38   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 2:57.38 /scratch/work/builds/mozilla-inbound/mozilla/security/sandbox/chromium/base/logging.h:364:3: note: expanded from macro 'LAZY_STREAM'
 2:57.38   !(condition) ? (void) 0 : ::logging::LogMessageVoidify() & (stream)
 2:57.38   ^
 2:57.43 1 error generated.
==========

We can work around this pretty easily by taking clang's suggestion and adding a (void) cast to suppress the warning.

I've filed a bug upstream:
  https://bugs.chromium.org/p/chromium/issues/detail?id=729123
but we might as well also fix this locally too. It looks like we've taken some other spot-fixes to address issues in this chromium subtree checkout, at least.

Comment 1

[Chris Peterson [:cpeterson]]

Looks like this Chromium code is using the comma so DCHECK is part of the return expression instead of a statement so that the TimeDelta::FromProduct function will be constexpr.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Bug 1369837: Add a void cast to silence clang Wcomma build warning, in sandbox's snapshot of chromium header.

The build warning is for "possible misuse of comma operator".

The comma operator is a bit of a footgun becasue its first operand's result
just gets dropped on the floor (in this case, the result of the DCHECK
expression).  It appears that Chromium's use of the comma operator here is
intentional, though -- so we might as well accept clang's suggestion and "cast
expression to void to silence warning".

This is also filed upstream as:
 https://bugs.chromium.org/p/chromium/issues/detail?id=729123

Review commit: https://reviewboard.mozilla.org/r/145360/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145360/

Comment 3

[Daniel Holbert [:dholbert]]

(Bob, would you be the right person to review this?  I'm guessing so, given bug 1366701 / bug 1337331, but feel free to redirect the review request if needed.)

Comment 4

[Bob Owen (:bobowen)]

Comment on attachment 8873965 [details]
Bug 1369837: Add a void cast to silence clang Wcomma build warning, in sandbox's snapshot of chromium header.

https://reviewboard.mozilla.org/r/145360/#review149384

Given that this is filed upstream let's go with this (thanks for doing that :-) ).

If for some reason that gets rejected then I might look to ignore this error in this code instead.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Bob Owen (:bobowen) from comment #4)
> If for some reason that gets rejected then I might look to ignore this error
> in this code instead.

FWIW, I started out by trying to make us ignore this warning (suppressing it in moz.build files), but I gave up after I'd fixed two different moz.build files and was still getting new instances of this failure for other #includers of the header.

(This header is included (directly or indirectly) in multiple source directories with their own moz.build files, so we would need to suppress the warning in each of those client moz.build files. Not sure precisely how many, but "more than two" made this feel like the better solution for the time being).

Thanks for the review!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/85f41e26f8b6
Add a void cast to silence clang Wcomma build warning, in sandbox's snapshot of chromium header. r=bobowen

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/85f41e26f8b6

Comment 8

[Daniel Holbert [:dholbert]]

For the record: the Chromium bug report was updated today...
 https://bugs.chromium.org/p/chromium/issues/detail?id=729123#c5
...to note that they landed a patch for this back in September:
 https://chromium.googlesource.com/chromium/src/+/fb3a33751c190240b64a1b22feee6704c2a822bc%5E%21/#F0


Their patch pulled the "DCHECK(positive_value > 0)" statement out of the return statement, and removed the #ifdefs around it (which were just to special case a particular ~old MSVC version).

I don't think we need to bother updating our hackaround to match theirs at this point; we can just let our local hackaround get overwritten when we next synchronize like Bug 1337331.