Re: Possible bug in StackFrameInfo#getByteCodeIndex?
Good catch! I agree that it should not be final and the constant field
should be static. I will take JDK-8193325 and follow up this issue.
-1 is ambiguous but should be initialized to a valid index when filled
by the VM. We tried to keep it compact for footprint concern. Anyway I
will propose a patch.
Mandy
On 12/12/17 11:08 AM, Martin Buchholz wrote:
Out of curiosity, I looked more at StackFrameInfo, and saw:
short bci is final and is only ever assigned to -1 in java code. What's up
with that? Ohh, it seems to be magically frobbed in hotspot:
void java_lang_StackFrameInfo::set_bci(oop element, int value) {
element->int_field_put(_bci_offset, value);
}
BUT: bci should not be final if hotspot magic is frobbing it. Can we add a
comment explaining the magic? Also set_bci seems to disagree about the
type of bci - it's trying to set an int field, but bci is a short!
Also, I expected "int value" above to be "jint value" - it's a java type!
I would prefer bci to be a java int, not short, so that it can hold all
possible bytecode index values in a natural way, together with -1 for
"invalid". Right now, -1 is ambiguous - it might mean bci == 65535.
Also, I see
private final byte RETAIN_CLASS_REF = 0x01;
Shouldn't that be static?
Re: Possible bug in StackFrameInfo#getByteCodeIndex?
Out of curiosity, I looked more at StackFrameInfo, and saw:
short bci is final and is only ever assigned to -1 in java code. What's up
with that? Ohh, it seems to be magically frobbed in hotspot:
void java_lang_StackFrameInfo::set_bci(oop element, int value) {
element->int_field_put(_bci_offset, value);
}
BUT: bci should not be final if hotspot magic is frobbing it. Can we add a
comment explaining the magic? Also set_bci seems to disagree about the
type of bci - it's trying to set an int field, but bci is a short!
Also, I expected "int value" above to be "jint value" - it's a java type!
I would prefer bci to be a java int, not short, so that it can hold all
possible bytecode index values in a natural way, together with -1 for
"invalid". Right now, -1 is ambiguous - it might mean bci == 65535.
Also, I see
private final byte RETAIN_CLASS_REF = 0x01;
Shouldn't that be static?
Re: Possible bug in StackFrameInfo#getByteCodeIndex?
On Tue, Dec 12, 2017 at 8:45 AM, David Lloyd wrote: > On Mon, Dec 11, 2017 at 9:03 PM, mandy chung > wrote: > > On 12/11/17 6:31 PM, Martin Buchholz wrote: > >> Java has an unsigned 16 bit type. Could bci be of type "char" ? > > > > Yes but I think keeping it short is fine. > > Call me strange if you like, but I never liked using char as anything > other than a UTF-16 character. Anyway one expects that the '0x' > forms should be optimized fairly easily. > It's optimized - just changes the width of the load. There's Short::toUnsignedInt (added in Java 8) if you want code reuse :). > > > -- > - DML >
Re: Possible bug in StackFrameInfo#getByteCodeIndex?
On Mon, Dec 11, 2017 at 9:03 PM, mandy chung wrote: > On 12/11/17 6:31 PM, Martin Buchholz wrote: >> Java has an unsigned 16 bit type. Could bci be of type "char" ? > > Yes but I think keeping it short is fine. Call me strange if you like, but I never liked using char as anything other than a UTF-16 character. Anyway one expects that the '0x' forms should be optimized fairly easily. -- - DML
Re: Possible bug in StackFrameInfo#getByteCodeIndex?
Thanks!
On Mon, Dec 11, 2017 at 3:53 PM, mandy chung wrote:
> I filed https://bugs.openjdk.java.net/browse/JDK-8193325. I can sponsor
> this patch for you.
>
> --- a/src/java.base/share/classes/java/lang/StackFrameInfo.java
> +++ b/src/java.base/share/classes/java/lang/StackFrameInfo.java
> @@ -93,7 +93,7 @@
> if (isNativeMethod())
> return -1;
>
> -return bci;
> +return bci & 0x;
> }
>
> Mandy
>
> On 12/7/17 8:19 PM, David Lloyd wrote:
>
> I was doing some research related to AccessController, and I noted
> this code [1] in StackFrameInfo#getByteCodeIndex():
>
> @Override
> public int getByteCodeIndex() {
> // bci not available for native methods
> if (isNativeMethod())
> return -1;
>
> return bci;
> }
>
> Now bci is of type short, and given the spec of the method, should the
> return not be:
>
> return bci & 0x;
>
> instead? Else it would return wrong values for methods with more than
> 32767 bytecodes in them.
>
> [1]
> http://hg.openjdk.java.net/jdk/jdk/file/e3b6cb90d7ce/src/java.base/share/classes/java/lang/StackFrameInfo.java#l96
>
>
--
- DML
Re: Possible bug in StackFrameInfo#getByteCodeIndex?
On 12/11/17 6:31 PM, Martin Buchholz wrote: Java has an unsigned 16 bit type. Could bci be of type "char" ? Yes but I think keeping it short is fine. Mandy
Re: Possible bug in StackFrameInfo#getByteCodeIndex?
Java has an unsigned 16 bit type. Could bci be of type "char" ?
On Mon, Dec 11, 2017 at 1:53 PM, mandy chung wrote:
> I filed https://bugs.openjdk.java.net/browse/JDK-8193325. I can sponsor
> this patch for you.
>
> --- a/src/java.base/share/classes/java/lang/StackFrameInfo.java
> +++ b/src/java.base/share/classes/java/lang/StackFrameInfo.java
> @@ -93,7 +93,7 @@
> if (isNativeMethod())
> return -1;
>
> -return bci;
> +return bci & 0x;
> }
>
> Mandy
>
>
> On 12/7/17 8:19 PM, David Lloyd wrote:
>
>> I was doing some research related to AccessController, and I noted
>> this code [1] in StackFrameInfo#getByteCodeIndex():
>>
>> @Override
>> public int getByteCodeIndex() {
>> // bci not available for native methods
>> if (isNativeMethod())
>> return -1;
>>
>> return bci;
>> }
>>
>> Now bci is of type short, and given the spec of the method, should the
>> return not be:
>>
>> return bci & 0x;
>>
>> instead? Else it would return wrong values for methods with more than
>> 32767 bytecodes in them.
>>
>> [1] http://hg.openjdk.java.net/jdk/jdk/file/e3b6cb90d7ce/src/jav
>> a.base/share/classes/java/lang/StackFrameInfo.java#l96
>>
>>
>
Re: Possible bug in StackFrameInfo#getByteCodeIndex?
I filed https://bugs.openjdk.java.net/browse/JDK-8193325. I can
sponsor this patch for you.
--- a/src/java.base/share/classes/java/lang/StackFrameInfo.java
+++ b/src/java.base/share/classes/java/lang/StackFrameInfo.java
@@ -93,7 +93,7 @@
if (isNativeMethod())
return -1;
- return bci;
+ return bci & 0x;
}
Mandy
On 12/7/17 8:19 PM, David Lloyd wrote:
I was doing some research related to AccessController, and I noted
this code [1] in StackFrameInfo#getByteCodeIndex():
@Override
public int getByteCodeIndex() {
// bci not available for native methods
if (isNativeMethod())
return -1;
return bci;
}
Now bci is of type short, and given the spec of the method, should the
return not be:
return bci & 0x;
instead? Else it would return wrong values for methods with more than
32767 bytecodes in them.
[1]
http://hg.openjdk.java.net/jdk/jdk/file/e3b6cb90d7ce/src/java.base/share/classes/java/lang/StackFrameInfo.java#l96
Possible bug in StackFrameInfo#getByteCodeIndex?
I was doing some research related to AccessController, and I noted
this code [1] in StackFrameInfo#getByteCodeIndex():
@Override
public int getByteCodeIndex() {
// bci not available for native methods
if (isNativeMethod())
return -1;
return bci;
}
Now bci is of type short, and given the spec of the method, should the
return not be:
return bci & 0x;
instead? Else it would return wrong values for methods with more than
32767 bytecodes in them.
[1]
http://hg.openjdk.java.net/jdk/jdk/file/e3b6cb90d7ce/src/java.base/share/classes/java/lang/StackFrameInfo.java#l96
--
- DML
