901529 - content/base/src/nsTextFragmentSSE2.cpp(45) : warning C4309: 'argument' : truncation of constant value

Status: RESOLVED FIXED

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

Description

[Daniel Holbert [:dholbert]]

Windows build warning:
{
content/base/src/nsTextFragmentSSE2.cpp(45) : warning C4309: 'argument' : truncation of constant value
}

Here's a recent try build (with -Werror turned on in this directory), which shows this warning:
 https://tbpl.mozilla.org/php/getParsedLog.php?id=26013956&tree=Try

It points to this line of code:
> 45   __m128i vectmask = _mm_set1_epi16(0xff00);

...and the issue is that _mm_set1_epi16 takes a *signed* short[1], whereas 0xff00 is a large unsigned short (larger than the largest signed short).

One way we could fix this is by declaring _mm_set1_epi16 as an unsigned short, and then static_casting it to short for the _mm_set1_epi16 call.

[1] http://msdn.microsoft.com/en-us/library/k0ya3x0e%28v=vs.80%29.aspx

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix v1

(In reply to Daniel Holbert [:dholbert] (lowered responsiveness through 8/6) from comment #0)
> One way we could fix this is by declaring _mm_set1_epi16 as an unsigned
> short, and then static_casting it to short for the _mm_set1_epi16 call.

Here's that option, basically. Hopefully this works?  Giving it a try run now; I'll request review later if it works.

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 785766 [details] [diff] [review]
fix v1

Yup, this seems to fix it.

Log of a build with the fix:
 https://tbpl.mozilla.org/php/getParsedLog.php?id=26167150&tree=Try#error0
(It's still busted because of a FAIL_ON_WARNINGS in the patch-stack plus another build warning (bug 900260), but no warnings reported for nsTextFragmentSSE2.cpp itself.)

Comment 3

[Justin Lebar (not reading bugmail)]

Comment on attachment 785766 [details] [diff] [review]
fix v1

Wow, I hope that wasn't affecting correctness!

Comment 4

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f803e142efc4

Comment 5

[Daniel Holbert [:dholbert]]

(Pretty sure this didn't affect correctness - I think the old code ended up doing what we expect. Evidence for that: we hit a bunch of these in bug 552805, in a C++ test, a while back -- the test passed (and behavior didn't change when we fixed the warnings), even though it was hitting a ton of these warnings for all the test data.)

Comment 6

[Daniel Holbert [:dholbert]]

(er, technically the MSVC C4309 versions of those C++-test-warnings from comment 5 were reported in bug 449291, and then I filed & fixed bug 552805 for the equivalent GCC warnings, and that took care of the MSVC warnings as well. Anyway: comment 5's point stands, but just see bug 449291 for where the MSVC warnings were reported, for anyone who cares.)

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f803e142efc4