900260 - nsTextFragmentImpl.h:19:40: warning: large integer implicitly truncated to unsigned type [-Woverflow]

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Daniel Holbert [:dholbert]]

Right now, builds on 32-bit platforms spam the following warning:
{
In file included from ../../../../content/base/src/nsTextFragmentSSE2.cpp:11:0:
../../../../content/base/src/nsTextFragmentImpl.h: In static member function 'static size_t Non8BitParameters<8u>::mask()':
../../../../content/base/src/nsTextFragmentImpl.h:19:40: warning: large integer implicitly truncated to unsigned type [-Woverflow]
}

Sample log (from a Try build -- just using a recent try build because it's a clobber and hence guaranteed to compile this code):
 https://tbpl.mozilla.org/php/getParsedLog.php?id=25750195&tree=Try

This is from this line of code:
> 19   static inline size_t mask() { return 0xff00ff00ff00ff00; }

That hex value is a full 64-bit value, which is indeed way to large to fit in a size_t on a 32-bit platform.

This was added in bug 797106. Nathan / ehsan, do you know what's supposed to happen here for 32-bit platforms?

Comment 1

[Daniel Holbert [:dholbert]]

Hm. It looks like 32-bit platforms are supposed to see the Non8BitParameters<4> specialization up higher:
> 12 template<> struct Non8BitParameters<4> {
> 13   static inline size_t mask() { return 0xff00ff00; }
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsTextFragmentImpl.h#12

which uses a more reasonably-sized hex value.

nsTextFragmentSSE2.cpp invokes this code as follows:
> 28   typedef Non8BitParameters<sizeof(size_t)> p;
> 29   const size_t mask = p::mask();
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsTextFragmentSSE2.cpp#28

I'd expect that sizeof(size_t) should be 4 on a 32-bit platform, so it shouldn't be hitting line 19 at all... Unless maybe the compiler is "helpfully" precompiling the <8> specialization for us, despite the fact that it's not needed?

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> This was added in bug 797106. Nathan / ehsan, do you know what's supposed to
> happen here for 32-bit platforms?

(oops, forgot to CC them)

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Daniel Holbert [:dholbert] from comment #1)
> Hm. It looks like 32-bit platforms are supposed to see the
> Non8BitParameters<4> specialization up higher:
> > 12 template<> struct Non8BitParameters<4> {
> > 13   static inline size_t mask() { return 0xff00ff00; }
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/
> nsTextFragmentImpl.h#12
> 
> which uses a more reasonably-sized hex value.
> 
> nsTextFragmentSSE2.cpp invokes this code as follows:
> > 28   typedef Non8BitParameters<sizeof(size_t)> p;
> > 29   const size_t mask = p::mask();
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/
> nsTextFragmentSSE2.cpp#28
> 
> I'd expect that sizeof(size_t) should be 4 on a 32-bit platform, so it
> shouldn't be hitting line 19 at all... Unless maybe the compiler is
> "helpfully" precompiling the <8> specialization for us, despite the fact
> that it's not needed?

I think that's what's happening; the template specialization is being parsed eagerly, despite it never actually being used.

Does explicit truncation work, i.e. something like:

  static const size_t mask = size_t(0xff00ff00ff00ff00ULL);

?  Otherwise, we could change the type of mask to uint64_t and explicitly truncate at the point of use.

Comment 4

[Daniel Holbert [:dholbert]]

I think we should do both -- declare it as uint64_t, to make sure the value is in-range for *that* type (in case someone sticks an extra "0" in there accidentally), and then return it with a static_cast so that we won't get warnings about the uint64_t-to-size_t conversion.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: fix v2

Sorry, forgot to qref.

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 785895 [details] [diff] [review]
fix v2

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

WFM!

::: content/base/src/nsTextFragmentImpl.h
@@ +16,5 @@
>  };
>  
>  template<> struct Non8BitParameters<8> {
> +  static inline size_t mask() {
> +    static const uint64_t maskAsUint64 = 0xff00ff00ff00ff00LL;

This could be ULL instead.  Maybe doing that is a little more clear?

Comment 8

[Daniel Holbert [:dholbert]]

Yup, that makes sense. (Sorry, missed the "U" in comment 3.)

Comment 9

[Daniel Holbert [:dholbert]]

Try run including this patch: https://tbpl.mozilla.org/?tree=Try&rev=a62d63fa1859

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: fix v3 (now with a c-style cast)

Unfortunately, windows still complains about the static_cast<size_t>(maskAsUint64) line, as shown in that Comment 9's try push.

However, if I change from a static_cast to a c-style cast, it suppresses the warning. I think that's probably fine, since we're already doing the relevant bounds-checking at the uint64_t declaration & assignment.  Here's a new patch with that, with try run here:
 https://tbpl.mozilla.org/?tree=Try&rev=c7f701a91193

Nathan, is this OK with you?

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 786101 [details] [diff] [review]
fix v3 (now with a c-style cast)

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

Man, MSVC.  OK.

Comment 12

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/263073d5b262

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/263073d5b262