Review Request JDK-8193325: StackFrameInfo::getByteCodeIndex returns wrong value if bci > 32767

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Review Request JDK-8193325: StackFrameInfo::getByteCodeIndex returns wrong value if bci > 32767

Mandy Chung
http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8193325/webrev.00/

This fixes the bug in hotspot that sets the bci field as int but it's of
short type and also StackFrameInfo to return a proper bci value.  Coleen
suggests not to change set_bci to take jushort but instead do the cast
as in the proposed fix.  The VM is in favor of C++ types and stop
propagation of Java types.  bci is of int in VM implementation and so be
consistent.

Mandy
[1] mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050581.html
Reply | Threaded
Open this post in threaded view
|

Re: Review Request JDK-8193325: StackFrameInfo::getByteCodeIndex returns wrong value if bci > 32767

David Holmes
Hi Mandy,

On 13/12/2017 10:42 AM, mandy chung wrote:
> http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8193325/webrev.00/
>
> This fixes the bug in hotspot that sets the bci field as int but it's of
> short type and also StackFrameInfo to return a proper bci value.  Coleen
> suggests not to change set_bci to take jushort but instead do the cast
> as in the proposed fix.  The VM is in favor of C++ types and stop
> propagation of Java types.  bci is of int in VM implementation and so be
> consistent.

I must admit I'm unclear why the VM needs the BCI to be an int when
logically it is a jushort, but perhaps, as per getByteCodeIndex() it
needs to be able to store "-1" to indicate no-bci?

The fix itself seems fine, though if we're going to chop off the VM
value at 16-bits perhaps we should assert that it isn't a value that
requires > 16 bits?

Thanks,
David

>
> Mandy
> [1] mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050581.html
Reply | Threaded
Open this post in threaded view
|

Re: Review Request JDK-8193325: StackFrameInfo::getByteCodeIndex returns wrong value if bci > 32767

John Rose-3
On Dec 12, 2017, at 5:24 PM, David Holmes <[hidden email]> wrote:
>
> as per getByteCodeIndex() it needs to be able to store "-1" to indicate no-bci?

Yes, that's the essential requirement, to encode a "none" value.

FWIW, one natural way to do that here would be to make the int field
@Stable and have the accessor return one less than the stored field value.

Then, by the rules of @Stable, the field starts out at the logical value
of -1, and can transition to a valid BCI if patched to a non-zero value
in the range 1..(2^16).

Making it "final" and initialized to -1 is not too terrible either, although
that creates some tech. debt we need to discharge when upgrading
finals to be trustable.

— John
Reply | Threaded
Open this post in threaded view
|

Re: Review Request JDK-8193325: StackFrameInfo::getByteCodeIndex returns wrong value if bci > 32767

Mandy Chung


On 12/12/17 5:29 PM, John Rose wrote:

> On Dec 12, 2017, at 5:24 PM, David Holmes <[hidden email]
> <mailto:[hidden email]>> wrote:
>>
>> as per getByteCodeIndex() it needs to be able to store "-1" to
>> indicate no-bci?
>
> Yes, that's the essential requirement, to encode a "none" value.
>
> FWIW, one natural way to do that here would be to make the int field
> @Stable and have the accessor return one less than the stored field value.
>
> Then, by the rules of @Stable, the field starts out at the logical value
> of -1, and can transition to a valid BCI if patched to a non-zero value
> in the range 1..(2^16).
>

Thanks for suggesting @Stable that would help this case.  I can't
understand why the field has to be patched with non-zero value.  Is this
something special requirement for @Stable?

> Making it "final" and initialized to -1 is not too terrible either,
> although
> that creates some tech. debt we need to discharge when upgrading
> finals to be trustable.

Mandy
[1]
http://cr.openjdk.java.net/~bchristi/8185925/StackFrameInfo.jol.rmDeclClass.txt
Reply | Threaded
Open this post in threaded view
|

Re: Review Request JDK-8193325: StackFrameInfo::getByteCodeIndex returns wrong value if bci > 32767

coleen.phillimore
In reply to this post by Mandy Chung

Hi, The hotspot part looks good.
Thanks!
Coleen

On 12/12/17 7:42 PM, mandy chung wrote:

> http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8193325/webrev.00/
>
> This fixes the bug in hotspot that sets the bci field as int but it's
> of short type and also StackFrameInfo to return a proper bci value. 
> Coleen suggests not to change set_bci to take jushort but instead do
> the cast as in the proposed fix.  The VM is in favor of C++ types and
> stop propagation of Java types.  bci is of int in VM implementation
> and so be consistent.
>
> Mandy
> [1]
> mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050581.html