RFR: JDK-7107013: sun.jvm.hotspot.runtime.Bytes.swapLong conversion to long mishandled

[Sharath Ballal]

Thanks Bernd.

 

 

-Sharath Ballal

 

 

From: Bernd Eckenfels [mailto:ecki at zusammenkunft.net] 
Sent: Monday, November 14, 2016 3:13 PM
To: serviceability-dev at openjdk.java.net; Sharath Ballal
Subject: Re: RFR: JDK-7107013: sun.jvm.hotspot.runtime.Bytes.swapLong conversion to long mishandled

 

Hello,

 

Don't worry about by comment, I am not sure that I have seen an change update or understand the explanation, but as my observation was only a minor nit it is fine with me in all cases. (I am not an official reviewer)

Gruss
Bernd
-- 
http://bernd.eckenfels.net

 





On Mon, Nov 14, 2016 at 9:29 AM +0100, "Sharath Ballal" <HYPERLINK "mailto:sharath.ballal at oracle.com" \nsharath.ballal at oracle.com> wrote:

Bernd,

Are you ok with the explanation and existing changes ?

 

-Sharath Ballal

 

 

From: Sharath Ballal 
Sent: Friday, November 11, 2016 12:17 PM
To: Bernd Eckenfels; HYPERLINK "mailto:serviceability-dev at openjdk.java.net"serviceability-dev at openjdk.java.net
Subject: RE: RFR: JDK-7107013: sun.jvm.hotspot.runtime.Bytes.swapLong conversion to long mishandled

 

Hello Bernd,

 

45 return (short) (((x >> 8) & 0xFF) | (x << 8));

Or

return (short)((x << 8) | ((x >> 8) & 0xFF));

 

In the above two lines of code, whichever you choose to write, they both will give the same result.  Short will be implicitly converted to int and then the operation will take place.

 

If you write the code as below (to make it consistent with int and long), you will get an error saying “possible lossy conversion from int to short” 

 

return ((short) (x << 8) | ((x >> 8) & 0xFF));

or

return ((short) (x << 8) | (short)((x >> 8) & 0xFF));

 

So an explicit cast will be required in both the above cases.

 

Let me know if you would want to see the code changed to “return (short)((x << 8) | ((x >> 8) & 0xFF));” to make it look consistent with the rest of the code.

 

(Since I was not in to/cc, it took me longer to see the mail)

 

 

-Sharath Ballal

 

 

From: Bernd Eckenfels [mailto:ecki at zusammenkunft.net] 
Sent: Thursday, November 10, 2016 2:12 PM
To: HYPERLINK "mailto:serviceability-dev at openjdk.java.net"serviceability-dev at openjdk.java.net
Subject: Re: RFR: JDK-7107013: sun.jvm.hotspot.runtime.Bytes.swapLong conversion to long mishandled

 

Hello, I was talking about the code order not the semantics:

 

  45 return (short) (((x >> 8) & 0xFF) | (x << 8));

  54 return ((int)swapShort((short) x) << 16) | (swapShort((short) (x >> 16)) & 0xFFFF);

  63 return ((long)swapInt((int) x) << 32) | (swapInt((int) (x >> 32)) & 0xFFFFFFFF);

 

In 45 the Low half is expressed first (and no masking) in 54 and 63 the MSB half is expressed first.

 

I guess casting and masking and order should be the same for all 3?

 

Gruss
Bernd
-- 
http://bernd.eckenfels.net

 

 

On Thu, Nov 10, 2016 at 9:30 AM +0100, "Sharath Ballal" <HYPERLINK "mailto:sharath.ballal at oracle.com" \nsharath.ballal at oracle.com> wrote:

Hello Bernd,

 

For int also its “low 2 bytes | high 2 bytes” and again these two byte pairs are swapped by the swapshort as “lowbyte | highbyte”

Similarly for long its “ low 4 bytes | high 4 bytes” and again these pairs are swapped recursively.

 

So for int if the little endian byte order was 3 2 1 0 it is now converted to 0 1 2 3 and similarly for long.

 

-Sharath Ballal

 

 

From: Bernd Eckenfels [mailto:ecki at zusammenkunft.net] 
Sent: Thursday, November 10, 2016 1:20 PM
To: HYPERLINK "mailto:serviceability-dev at openjdk.java.net"serviceability-dev at openjdk.java.net; Dmitry Samersoff; Sharath Ballal
Subject: Re: RFR: JDK-7107013: sun.jvm.hotspot.runtime.Bytes.swapLong conversion to long mishandled

 

Hello,

 

Is the a reason why swapShort has a " lowbyte | highbyyte" and the other two methods the other way around? I would all write in the natural order of. Igendianess (I.e. Change the first).

Gruss
Bernd
-- 
http://bernd.eckenfels.net

 






On Thu, Nov 10, 2016 at 8:32 AM +0100, "Sharath Ballal" <HYPERLINK "mailto:sharath.ballal at oracle.com" \nsharath.ballal at oracle.com> wrote:

Thanks Dmitry.  I have made the changes in line 54.
http://cr.openjdk.java.net/~sballal/7107013/webrev.01/ 
 
I didn't change the recursive calls to swap functions because that looks more readable.
 
-Sharath Ballal
 
 
-----Original Message-----
From: Dmitry Samersoff 
Sent: Thursday, November 10, 2016 12:46 AM
To: Sharath Ballal; HYPERLINK "mailto:serviceability-dev at openjdk.java.net"serviceability-dev at openjdk.java.net
Subject: Re: RFR: JDK-7107013: sun.jvm.hotspot.runtime.Bytes.swapLong conversion to long mishandled
 
Sharath,
 
Please, add (int) to ll. 54 for better readability.
 
PS:
 
Despite the fact that C2 does a great job eliminating useless code (multiple calls to if (!swap) in this case) it would be nice to use simple, well known arithmetic directly instead of subsequent calls to other swap functions.
 
-Dmitry
 
On 2016-11-09 19:30, Sharath Ballal wrote:
> Hello,
> 
> Pls review this small fix
> 
>  
> 
> Issue: https://bugs.openjdk.java.net/browse/JDK-7107013
> 
> Webrev: http://cr.openjdk.java.net/~sballal/7107013/webrev.00/
> 
>  
> 
>  
> 
> -Sharath Ballal
> 
>  
> 
>  
> 
 
 
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20161114/b71cb7c9/attachment-0001.html>