Closed
Bug 900260
Opened 11 years ago
Closed 11 years ago
nsTextFragmentImpl.h:19:40: warning: large integer implicitly truncated to unsigned type [-Woverflow]
Categories
(Core :: General, defect)
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: dholbert, Assigned: dholbert)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
1.45 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Comment 1•11 years ago
|
||
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?
Assignee | ||
Comment 2•11 years ago
|
||
(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•11 years ago
|
||
(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.
Assignee | ||
Comment 4•11 years ago
|
||
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.
Assignee | ||
Comment 5•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 6•11 years ago
|
||
Sorry, forgot to qref.
Attachment #785894 -
Attachment is obsolete: true
Attachment #785894 -
Flags: review?(nfroyd)
Attachment #785895 -
Flags: review?(nfroyd)
![]() |
||
Comment 7•11 years ago
|
||
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+
Assignee | ||
Comment 9•11 years ago
|
||
Try run including this patch: https://tbpl.mozilla.org/?tree=Try&rev=a62d63fa1859
Assignee | ||
Comment 10•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #786101 -
Attachment description: fix v3 → fix v3 (now with a c-style cast)
![]() |
||
Comment 11•11 years ago
|
||
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+
Assignee | ||
Comment 12•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/263073d5b262
Flags: in-testsuite-
OS: Linux → All
Comment 13•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/263073d5b262
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in
before you can comment on or make changes to this bug.
Description
•