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

[Severin Gehwolf]

On Wed, 2018-05-16 at 12:39 +0100, Alan Bateman wrote:
> On 16/05/2018 09:02, Severin Gehwolf wrote:
> > :
> > New webrev:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203223/webrev.02/
> > 
> 
> This version looks okay to me.
> 
> -Alan

Thanks for the review!

Cheers,
Severin

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

[Alan Bateman]

On 16/05/2018 09:02, Severin Gehwolf wrote:

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


This version looks okay to me.

-Alan

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

[Andrew Haley]

On 05/16/2018 09:10 AM, Aleksey Shipilev wrote:
> On 05/16/2018 10:02 AM, Severin Gehwolf wrote:
>> $ 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");
> 
> Oh, WTF! So it is the trick to print the assert message with C asserts.

Yikes!  WTF indeed.  :-)

-- 
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. 
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

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

[Aleksey Shipilev]

On 05/16/2018 10:02 AM, Severin Gehwolf wrote:
> $ 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");

Oh, WTF! So it is the trick to print the assert message with C asserts.

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

Looks good!

-Aleksey

Re: 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 & 0x7FFF;
> > 
> > as
> > 
> >return (s4)(s & 0x7FFF);

Fixed.

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

Thanks,
Severin

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

[David Holmes]

+1 on both comments.

In addition I'd prefer

+ u4 useed = (u4)seed;

for clarity, rather than just 's'.

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");

*) I would also write:

   return (s4)s & 0x7FFF;

as

   return (s4)(s & 0x7FFF);

-Aleksey

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

[Aleksey Shipilev]

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");

*) I would also write:

  return (s4)s & 0x7FFF;

as

  return (s4)(s & 0x7FFF);

-Aleksey