991234 - TypeDescrSet.cpp:328:15: warning: implicit conversion from 'unsigned long' to 'int32_t' (aka 'int') changes value from 18446744073709551615 to -1 [-Wconstant-conversion]

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Daniel Holbert [:dholbert]]

Build warning with clang 3.4:
{
js/src/jit/TypeDescrSet.cpp:328:15: warning: implicit conversion from 'unsigned long' to 'int32_t' (aka 'int') changes value from 18446744073709551615 to -1 [-Wconstant-conversion]
     *offset = SIZE_MAX;
             ~ ^~~~~~~~
 /usr/include/stdint.h:261:22: note: expanded from macro 'SIZE_MAX'
 #  define SIZE_MAX              (18446744073709551615UL)
                                  ^~~~~~~~~~~~~~~~~~~~~~
}

The code in question is:
>317 bool
> 318 TypeDescrSet::fieldNamed(IonBuilder &builder,
> 319                          jsid id,
> 320                          int32_t *offset,
> 321                          TypeDescrSet *out,
> 322                          size_t *index)
> 323 {
[...]
> 328     *offset = SIZE_MAX;
> 329     *index = SIZE_MAX;

aOffset used to be a size_t*, but it changed to int32_t* in bug 966575:
 http://hg.mozilla.org/mozilla-central/rev/2f97d3cb75e9#l5.58
which just landed on m-c yesterday.

Presumably we shouldn't set it to SIZE_MAX anymore, since that's not the appropriate maximum value for its type?

Comment 1

[Niko Matsakis [:nmatsakis]]

Yes, that looks wrong, I wonder why I didn't see this warning.

Comment 2

[Daniel Holbert [:dholbert]]

Cool. Mind fixing? :)

(I'm not sure what we should be using instead of SIZE_MAX here (INT32_MAX?) since I don't know why we're setting this value in the first place, but I'm guessing you know what the appropriate value should be.)

Comment 3

[Niko Matsakis [:nmatsakis]]

Attachment: Bug991234.diff

This variable changes from a size_t to an int32_t, so use -1 as the error value. Actually it doesn't matter what value we use, really, because (as indicated in the comment in the .h file) the caller ought not to read `*size` unless `*out` is a non-empty-set.

Also, just FYI, bug 973238 replaces this TypeDescrSet type altogether (patches not yet uploaded).

Comment 4

[Hannes Verschore [:h4writer]]

Comment on attachment 8401183 [details] [diff] [review]
Bug991234.diff

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

Can you remove the unneeded cast to int32_t in:
http://mxr.mozilla.org/mozilla-central/source/js/src/jit/IonBuilder.cpp#10143

And also add an assert that offset isn't -1?

Comment 5

[Niko Matsakis [:nmatsakis]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/533e61eb95d3

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/6519732bc27d (along with bug 977126 from the same push) for Windows mochitest-2 bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=37235091&tree=Mozilla-Inbound

Comment 7

[Niko Matsakis [:nmatsakis]]

I suspect this patch (nor bug 977126) was not the cause of the mochitest-2 bustage. In any case, I have done another try run with the same four patches against the current mozilla-centra and everything looks clean:

https://tbpl.mozilla.org/?tree=Try&rev=82fd9e987cae

Comment 8

[Niko Matsakis [:nmatsakis]]

https://tbpl.mozilla.org/?tree=Try&rev=82fd9e987cae (this time isolated from bug 977126, since if anything will cause failures, it's more likely to be that)

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/62e5a22aeec5