1330397 - dom/html & dom/svg fail to build with Clang 4.0 due to -Werror=max-unsigned-zero

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jan Beich]

-Wmax-unsigned-zero is enabled by default after https://github.com/llvm-mirror/clang/commit/40adebeca0f9

$ c++ -v
FreeBSD clang version 4.0.0 (trunk 291476) (based on LLVM 4.0.0svn)
Target: x86_64-unknown-freebsd12.0
Thread model: posix
InstalledDir: /usr/bin
Found CUDA installation: /usr/local/cuda, version 7.5

$ echo ac_add_options --enable-warnings-as-errors >>.mozconfig
$ ./mach build
[...]
In file included from objdir/dom/svg/Unified_cpp_dom_svg5.cpp:74:
dom/svg/SVGPathData.cpp:210:10: error: taking the max of unsigned zero and a
      value is always equal to the other value [-Werror,-Wmax-unsigned-zero]
  return std::max(0U, segIndex - 1); // -1 because while loop takes us 1 too far
         ^~~~~~~~ ~~
dom/svg/SVGPathData.cpp:210:10: note: remove call to max function and unsigned
      zero argument
  return std::max(0U, segIndex - 1); // -1 because while loop takes us 1 too far
         ^~~~~~~~ ~~~
1 error generated.

In file included from objdir/dom/html/Unified_cpp_dom_html1.cpp:83:
dom/html/HTMLImageElement.cpp:773:14: error: taking the max of a value and
      unsigned zero is always equal to the other value [-Werror,-Wmax-unsigned-zero]
    height = std::max(height, 0u);
             ^~~~~~~~         ~~
dom/html/HTMLImageElement.cpp:773:14: note: remove call to max function and
      unsigned zero argument
    height = std::max(height, 0u);
             ^~~~~~~~       ~~~~
dom/html/HTMLImageElement.cpp:801:13: error: taking the max of a value and
      unsigned zero is always equal to the other value [-Werror,-Wmax-unsigned-zero]
    width = std::max(width, 0u);
            ^~~~~~~~        ~~
dom/html/HTMLImageElement.cpp:801:13: note: remove call to max function and
      unsigned zero argument
    width = std::max(width, 0u);
            ^~~~~~~~      ~~~~
2 errors generated.

Comment 1

[Jan Beich]

Attachment: Bug 1330397 - Fix -Wmax-unsigned-zero found by Clang 4.0.

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

Comment 2

[Jan Beich]

Caveat: I've only tested Clang 4.0 on Tier3 using empty .mozconfig, so there could be more such warnings hiding somewhere like mozjemalloc, sandbox, SPS profiler, crash reporter, etc.

Comment 3

[(no longer active)]

Comment on attachment 8825934 [details]
Bug 1330397 - Fix -Wmax-unsigned-zero found by Clang 4.0.

https://reviewboard.mozilla.org/r/103994/#review104830

r=me on the dom/html changes.

::: dom/html/HTMLImageElement.cpp
(Diff revision 1)
>  
>    if (mResponsiveSelector) {
>      double density = mResponsiveSelector->GetSelectedImageDensity();
>      MOZ_ASSERT(density >= 0.0);
>      height = NSToIntRound(double(height) / density);
> -    height = std::max(height, 0u);

We assert that density is non-negative, and height can't be negative either, so this is indeed not needed.

::: dom/html/HTMLImageElement.cpp
(Diff revision 1)
>  
>    if (mResponsiveSelector) {
>      double density = mResponsiveSelector->GetSelectedImageDensity();
>      MOZ_ASSERT(density >= 0.0);
>      width = NSToIntRound(double(width) / density);
> -    width = std::max(width, 0u);

Ditto.

::: dom/svg/SVGPathData.cpp:209
(Diff revision 1)
>      segIndex++;
>    }
>  
>    MOZ_ASSERT(i == mData.Length(), "Very, very bad - mData corrupt");
>  
> -  return std::max(0U, segIndex - 1); // -1 because while loop takes us 1 too far
> +  return (segIndex - 1); // -1 because while loop takes us 1 too far

This one I'm not sure about.  It could be that the code here expects segIndex to be 0 and it really wants to clamp up to 0, but it's using a uint32_t variable by mistake...

Please ask jwatt to take a look at this one.

Comment 4

[Jan Beich]

Comment on attachment 8825934 [details]
Bug 1330397 - Fix -Wmax-unsigned-zero found by Clang 4.0.

https://reviewboard.mozilla.org/r/103994/#review104830

> This one I'm not sure about.  It could be that the code here expects segIndex to be 0 and it really wants to clamp up to 0, but it's using a uint32_t variable by mistake...
> 
> Please ask jwatt to take a look at this one.

`mData.Length()` is restricted to `unsigned` range via `size_t` and if returns `0` then it'd hit `MOZ_ASSERT` above.

Comment 5

[Jan Beich]

Comment on attachment 8825934 [details]
Bug 1330397 - Fix -Wmax-unsigned-zero found by Clang 4.0.

https://reviewboard.mozilla.org/r/103994/#review104830

> `mData.Length()` is restricted to `unsigned` range via `size_t` and if returns `0` then it'd hit `MOZ_ASSERT` above.

Nevermind, `MOZ_ASSERT` asserts the value is true. And if `mData.Length()` returns `0` the function would return `(uint32_t)(-1)` which may be a bug.

Comment 6

[Jan Beich]

Fixed by :sylvestre in a slightly different way.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

(In reply to Jan Beich from comment #6)
> Fixed by :sylvestre in a slightly different way.
Oups, sorry, I didn't see this bug :(