1247301 - [Brotli] Warning in bit_reader.h: use of logical '||' with constant operand

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb}]

 5:22.18 /home/ckerschb/moz/mc-obj-dbg/dist/include/./bit_reader.h:41:22: error: use of logical '||' with constant operand [-Werror,-Wconstant-logical-operand]
 5:22.18   if (IS_CONSTANT(n) || BROTLI_HAS_UBFX) {
 5:22.18                      ^  ~~~~~~~~~~~~~~~
 5:22.18 /home/ckerschb/moz/mc-obj-dbg/dist/include/./bit_reader.h:41:22: note: use '|' for a bitwise operation
 5:22.18   if (IS_CONSTANT(n) || BROTLI_HAS_UBFX) {
 5:22.18                      ^~
 5:22.18                      |
 5:22.18 1 error generated.

Comment 2

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1247301_fix_build_error.patch

Comment 3

[Frédéric Wang (:fredw)]

Comment on attachment 8717932 [details] [diff] [review]
bug_1247301_fix_build_error.patch

mmh, I saw this build warning too, but the name of the defines really suggests that the Brotli authors wanted standard boolean OR not bitwise OR ; so I didn't patch it.

Here are the relevant definitions from port.h:

#if defined(BROTLI_TARGET_ARM)
#define BROTLI_HAS_UBFX 1
#else
#define BROTLI_HAS_UBFX 0
#endif

#if BROTLI_MODERN_COMPILER || __has_builtin(__builtin_constant_p)
#define IS_CONSTANT(x) __builtin_constant_p(x)
#else
#define IS_CONSTANT(x) 0
#endif

and __builtin_constant_p returns 0 or 1 per the GCC documentation.

So I think the proposed patch will work, but there might be a way to rewrite this while preserving the semantics implied by ||. Could you please report this upstream to see how they prefer to fix that?

Comment 4

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Frédéric Wang (:fredw) from comment #3)
> Comment on attachment 8717932 [details] [diff] [review]
> bug_1247301_fix_build_error.patch
> 
> mmh, I saw this build warning too, but the name of the defines really
> suggests that the Brotli authors wanted standard boolean OR not bitwise OR ;
> so I didn't patch it.
> 
> Here are the relevant definitions from port.h:
> 
> #if defined(BROTLI_TARGET_ARM)
> #define BROTLI_HAS_UBFX 1
> #else
> #define BROTLI_HAS_UBFX 0
> #endif
> 
> #if BROTLI_MODERN_COMPILER || __has_builtin(__builtin_constant_p)
> #define IS_CONSTANT(x) __builtin_constant_p(x)
> #else
> #define IS_CONSTANT(x) 0
> #endif
> 
> and __builtin_constant_p returns 0 or 1 per the GCC documentation.
> 
> So I think the proposed patch will work, but there might be a way to rewrite
> this while preserving the semantics implied by ||. Could you please report
> this upstream to see how they prefer to fix that?

Fred, I would prefer you can take care of that - I just realized the build error.

Comment 5

[Frédéric Wang (:fredw)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> Fred, I would prefer you can take care of that - I just realized the build
> error.

OK, I will.

Comment 6

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Frédéric Wang (:fredw) from comment #5)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> > Fred, I would prefer you can take care of that - I just realized the build
> > error.
> 
> OK, I will.

Thanks!

Comment 7

[Frédéric Wang (:fredw)]

Comment on attachment 8717932 [details] [diff] [review]
bug_1247301_fix_build_error.patch

I'm clearing review flag. Just to repeat, I believe the propose changed will fix the compilation warning and will provide the expected execution behavior. However, as I understand, -Wconstant-logical-operand is supposed to prevent people from doing logical operations with constant bit mask, instead of bitwise operations. That does not apply here since the constants in question are really 0 and 1 used to represent boolean values false and true.

Comment 8

[Frédéric Wang (:fredw)]

Attachment: Patch

So the upstream fix will be to use double exclamation marks to force these values to be interpreted as booleans. This is more appropriate than changing the semantic of the operator.

Comment 9

[Frédéric Wang (:fredw)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e72f400b328&selectedJob=17223458
http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-0e72f400b3289c1a070de82abf0e148699c10a7f/try-macosx64/try-macosx64-bm87-try1-build13620.txt.gz

Comment 10

[Patrick McManus [:mcmanus]]

Comment on attachment 8723524 [details] [diff] [review]
Patch

Review of attachment 8723524 [details] [diff] [review]:
-----------------------------------------------------------------

please note in the readme.mozilla (or whatever its called) that this upstream patch has been imported and we're not quite in sync with the cset mentioned in that file

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6016ec162137

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6016ec162137