RFR: 8203223: Signed integer overflow in ImageStrings::hash_code (libjimage.so)

[Severin Gehwolf]

Hi David, Aleksey,

Thanks for the reviews! Comments inline.

On Wed, 2018-05-16 at 12:01 +1000, David Holmes wrote:
> +1 on both comments.
> 
> In addition I'd prefer
> 
> +     u4 useed = (u4)seed;
> 
> for clarity, rather than just 's'.

Fixed.

> Thanks,
> David
> 
> On 16/05/2018 2:16 AM, Aleksey Shipilev wrote:
> > On 05/15/2018 06:11 PM, Severin Gehwolf wrote:
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8203223
> > > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203223/webrev.01/
> > 
> > *) Um:
> >    assert(seed > 0 && "invariant");
> > 
> > This should be this, right?
> > 
> >    assert(seed > 0, "invariant");

This was throwing me off too, but I've used the same model than other
assert() calls in this file. For example line 161 in imageFile.cpp
reads like this:
 161     assert(replaced != NULL && "allocation failed");

If I changed it to your suggestion I'm getting this compile failure:

$ bash imageFile.o.cmdline 
/disk/openjdk/upstream-sources/openjdk-hs/src/java.base/share/native/libjimage/imageFile.cpp:60:33: error: macro "assert" passed 2 arguments, but takes just 1
     assert(seed > 0, "invariant");
                                 ^
Seems to be a difference between hotspot and core-libs. So I've kept
this as is.

> > *) I would also write:
> > 
> >    return (s4)s & 0x7FFFFFFF;
> > 
> > as
> > 
> >    return (s4)(s & 0x7FFFFFFF);

Fixed.

New webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203223/webrev.02/

Thanks,
Severin