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

RESOLVED FIXED in mozilla26

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks 1 bug)

Trunk
mozilla26
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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?
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?
(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)
Depends on: 900274
(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.
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.
Posted patch fix v1 (obsolete) — Splinter Review
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #785894 - Flags: review?(nfroyd)
Blocks: 900274
No longer depends on: 900274
Posted patch fix v2 (obsolete) — Splinter Review
Sorry, forgot to qref.
Attachment #785894 - Attachment is obsolete: true
Attachment #785894 - Flags: review?(nfroyd)
Attachment #785895 - Flags: review?(nfroyd)
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?
Attachment #785895 - Flags: review?(nfroyd) → review+
Yup, that makes sense. (Sorry, missed the "U" in comment 3.)
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?
Attachment #785895 - Attachment is obsolete: true
Attachment #786101 - Flags: review?(nfroyd)
Attachment #786101 - Attachment description: fix v3 → fix v3 (now with a c-style cast)
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.
Attachment #786101 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/263073d5b262
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.