RFR (M) 8058259: compute_offset() is confusing for static fields

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

RFR (M) 8058259: compute_offset() is confusing for static fields

coleen.phillimore
Summary: remove most hard-coded offsets, have compute_offset function
that takes a string and creates a TempNewSymbol, have
static_field_addr() not add in
InstanceMirrorKlass::offset_of_static_fields, ie use offset from find_field

The jvmci code uses find_field to get the offset of static fields, then
used static_field_addr() and then subtracted
InstanceMirrorKlass::offset_of_static fields because the function
expected the hardcoded offsets.  Removed hardcoded static offsets and
non-static offsets where possible.

This change also removes the nonproduct flag
CheckAssertionStatusDirectives (default false).

Tested with tier1-5 tests on Oracle platforms, and temporary code to get
failures in known class layouts, and see error logging.

open webrev at http://cr.openjdk.java.net/~coleenp/8058259.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8058259

thanks,
Coleen
Reply | Threaded
Open this post in threaded view
|

Re: RFR (M) 8058259: compute_offset() is confusing for static fields

Kim Barrett
> On Jan 4, 2018, at 9:29 PM, [hidden email] wrote:
>
> Summary: remove most hard-coded offsets, have compute_offset function that takes a string and creates a TempNewSymbol, have static_field_addr() not add in InstanceMirrorKlass::offset_of_static_fields, ie use offset from find_field
>
> The jvmci code uses find_field to get the offset of static fields, then used static_field_addr() and then subtracted InstanceMirrorKlass::offset_of_static fields because the function expected the hardcoded offsets.  Removed hardcoded static offsets and non-static offsets where possible.
>
> This change also removes the nonproduct flag CheckAssertionStatusDirectives (default false).
>
> Tested with tier1-5 tests on Oracle platforms, and temporary code to get failures in known class layouts, and see error logging.
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8058259.01/webrev
> bug link https://bugs.openjdk.java.net/browse/JDK-8058259
>
> thanks,
> Coleen

Looks good.  Just a couple minor things.  I don't need another webrev
for any of these.

------------------------------------------------------------------------------
src/hotspot/share/classfile/javaClasses.cpp
1229 void java_lang_Thread::compute_offsets() {

Wondering why some of the field names are referred to via
vmSymbols::XXX_name(), while others use string constants?

------------------------------------------------------------------------------
src/hotspot/share/runtime/arguments.cpp
 526   { "CheckAssertionStatusDirectives",JDK_Version::undefined(), JDK_Version::jdk(10), JDK_Version::jdk(11) },

Expired version should be jdk(12) rather than jdk(11).

Obsolete version should be jdk(11) rather than jdk(10), but that may
not be possible until we've updated to be building 11.

------------------------------------------------------------------------------

Reply | Threaded
Open this post in threaded view
|

Re: RFR (M) 8058259: compute_offset() is confusing for static fields

coleen.phillimore
Hi Kim,  Thanks for the review (and pre-review).

On 1/5/18 1:51 AM, Kim Barrett wrote:

>> On Jan 4, 2018, at 9:29 PM, [hidden email] wrote:
>>
>> Summary: remove most hard-coded offsets, have compute_offset function that takes a string and creates a TempNewSymbol, have static_field_addr() not add in InstanceMirrorKlass::offset_of_static_fields, ie use offset from find_field
>>
>> The jvmci code uses find_field to get the offset of static fields, then used static_field_addr() and then subtracted InstanceMirrorKlass::offset_of_static fields because the function expected the hardcoded offsets.  Removed hardcoded static offsets and non-static offsets where possible.
>>
>> This change also removes the nonproduct flag CheckAssertionStatusDirectives (default false).
>>
>> Tested with tier1-5 tests on Oracle platforms, and temporary code to get failures in known class layouts, and see error logging.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8058259.01/webrev
>> bug link https://bugs.openjdk.java.net/browse/JDK-8058259
>>
>> thanks,
>> Coleen
> Looks good.  Just a couple minor things.  I don't need another webrev
> for any of these.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/classfile/javaClasses.cpp
> 1229 void java_lang_Thread::compute_offsets() {
>
> Wondering why some of the field names are referred to via
> vmSymbols::XXX_name(), while others use string constants?

I left some string constants because they were used more than once, so
it seemed slightly better and not worth changing for consistency.  My
main purpose of doing this was so when new fields are added for classes,
they don't have to be added to vmSymbols.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/runtime/arguments.cpp
>   526   { "CheckAssertionStatusDirectives",JDK_Version::undefined(), JDK_Version::jdk(10), JDK_Version::jdk(11) },
>
> Expired version should be jdk(12) rather than jdk(11).
>
> Obsolete version should be jdk(11) rather than jdk(10), but that may
> not be possible until we've updated to be building 11.

Oh I forgot to change that back.  I'm waiting for the version to change,
then I'll change it to 11, 12.

Thanks,
Coleen
>
> ------------------------------------------------------------------------------
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (M) 8058259: compute_offset() is confusing for static fields

Kim Barrett
> On Jan 5, 2018, at 8:25 AM, [hidden email] wrote:
>
> Hi Kim,  Thanks for the review (and pre-review).
>
> On 1/5/18 1:51 AM, Kim Barrett wrote:
>>> On Jan 4, 2018, at 9:29 PM, [hidden email] wrote:
>>>
>>> Summary: remove most hard-coded offsets, have compute_offset function that takes a string and creates a TempNewSymbol, have static_field_addr() not add in InstanceMirrorKlass::offset_of_static_fields, ie use offset from find_field
>>>
>>> The jvmci code uses find_field to get the offset of static fields, then used static_field_addr() and then subtracted InstanceMirrorKlass::offset_of_static fields because the function expected the hardcoded offsets.  Removed hardcoded static offsets and non-static offsets where possible.
>>>
>>> This change also removes the nonproduct flag CheckAssertionStatusDirectives (default false).
>>>
>>> Tested with tier1-5 tests on Oracle platforms, and temporary code to get failures in known class layouts, and see error logging.
>>>
>>> open webrev at http://cr.openjdk.java.net/~coleenp/8058259.01/webrev
>>> bug link https://bugs.openjdk.java.net/browse/JDK-8058259
>>>
>>> thanks,
>>> Coleen
>> Looks good.  Just a couple minor things.  I don't need another webrev
>> for any of these.
>>
>> ------------------------------------------------------------------------------
>> src/hotspot/share/classfile/javaClasses.cpp
>> 1229 void java_lang_Thread::compute_offsets() {
>>
>> Wondering why some of the field names are referred to via
>> vmSymbols::XXX_name(), while others use string constants?
>
> I left some string constants because they were used more than once, so it seemed slightly better and not worth changing for consistency.  My main purpose of doing this was so when new fields are added for classes, they don't have to be added to vmSymbols.

Okay.

>>
>> ------------------------------------------------------------------------------
>> src/hotspot/share/runtime/arguments.cpp
>>  526   { "CheckAssertionStatusDirectives",JDK_Version::undefined(), JDK_Version::jdk(10), JDK_Version::jdk(11) },
>>
>> Expired version should be jdk(12) rather than jdk(11).
>>
>> Obsolete version should be jdk(11) rather than jdk(10), but that may
>> not be possible until we've updated to be building 11.
>
> Oh I forgot to change that back.  I'm waiting for the version to change, then I'll change it to 11, 12.

Okay.

>
> Thanks,
> Coleen
>>
>> ------------------------------------------------------------------------------


Reply | Threaded
Open this post in threaded view
|

Re: RFR (M) 8058259: compute_offset() is confusing for static fields

serguei.spitsyn@oracle.com
In reply to this post by coleen.phillimore
Hi Coleen,

The fix looks good to me.
Looks like a nice cleanup.

Thanks,
Serguei


On 1/4/18 18:29, [hidden email] wrote:

> Summary: remove most hard-coded offsets, have compute_offset function
> that takes a string and creates a TempNewSymbol, have
> static_field_addr() not add in
> InstanceMirrorKlass::offset_of_static_fields, ie use offset from
> find_field
>
> The jvmci code uses find_field to get the offset of static fields,
> then used static_field_addr() and then subtracted
> InstanceMirrorKlass::offset_of_static fields because the function
> expected the hardcoded offsets.  Removed hardcoded static offsets and
> non-static offsets where possible.
>
> This change also removes the nonproduct flag
> CheckAssertionStatusDirectives (default false).
>
> Tested with tier1-5 tests on Oracle platforms, and temporary code to
> get failures in known class layouts, and see error logging.
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8058259.01/webrev
> bug link https://bugs.openjdk.java.net/browse/JDK-8058259
>
> thanks,
> Coleen

Reply | Threaded
Open this post in threaded view
|

Re: RFR (M) 8058259: compute_offset() is confusing for static fields

coleen.phillimore
Thank you, Serguei!
Coleen

On 1/5/18 2:46 PM, [hidden email] wrote:

> Hi Coleen,
>
> The fix looks good to me.
> Looks like a nice cleanup.
>
> Thanks,
> Serguei
>
>
> On 1/4/18 18:29, [hidden email] wrote:
>> Summary: remove most hard-coded offsets, have compute_offset function
>> that takes a string and creates a TempNewSymbol, have
>> static_field_addr() not add in
>> InstanceMirrorKlass::offset_of_static_fields, ie use offset from
>> find_field
>>
>> The jvmci code uses find_field to get the offset of static fields,
>> then used static_field_addr() and then subtracted
>> InstanceMirrorKlass::offset_of_static fields because the function
>> expected the hardcoded offsets.  Removed hardcoded static offsets and
>> non-static offsets where possible.
>>
>> This change also removes the nonproduct flag
>> CheckAssertionStatusDirectives (default false).
>>
>> Tested with tier1-5 tests on Oracle platforms, and temporary code to
>> get failures in known class layouts, and see error logging.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8058259.01/webrev
>> bug link https://bugs.openjdk.java.net/browse/JDK-8058259
>>
>> thanks,
>> Coleen
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (M) 8058259: compute_offset() is confusing for static fields

coleen.phillimore
In reply to this post by Kim Barrett
Thank you, Kim.
Coleen

On 1/5/18 1:59 PM, Kim Barrett wrote:

>> On Jan 5, 2018, at 8:25 AM, [hidden email] wrote:
>>
>> Hi Kim,  Thanks for the review (and pre-review).
>>
>> On 1/5/18 1:51 AM, Kim Barrett wrote:
>>>> On Jan 4, 2018, at 9:29 PM, [hidden email] wrote:
>>>>
>>>> Summary: remove most hard-coded offsets, have compute_offset function that takes a string and creates a TempNewSymbol, have static_field_addr() not add in InstanceMirrorKlass::offset_of_static_fields, ie use offset from find_field
>>>>
>>>> The jvmci code uses find_field to get the offset of static fields, then used static_field_addr() and then subtracted InstanceMirrorKlass::offset_of_static fields because the function expected the hardcoded offsets.  Removed hardcoded static offsets and non-static offsets where possible.
>>>>
>>>> This change also removes the nonproduct flag CheckAssertionStatusDirectives (default false).
>>>>
>>>> Tested with tier1-5 tests on Oracle platforms, and temporary code to get failures in known class layouts, and see error logging.
>>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8058259.01/webrev
>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8058259
>>>>
>>>> thanks,
>>>> Coleen
>>> Looks good.  Just a couple minor things.  I don't need another webrev
>>> for any of these.
>>>
>>> ------------------------------------------------------------------------------
>>> src/hotspot/share/classfile/javaClasses.cpp
>>> 1229 void java_lang_Thread::compute_offsets() {
>>>
>>> Wondering why some of the field names are referred to via
>>> vmSymbols::XXX_name(), while others use string constants?
>> I left some string constants because they were used more than once, so it seemed slightly better and not worth changing for consistency.  My main purpose of doing this was so when new fields are added for classes, they don't have to be added to vmSymbols.
> Okay.
>
>>> ------------------------------------------------------------------------------
>>> src/hotspot/share/runtime/arguments.cpp
>>>   526   { "CheckAssertionStatusDirectives",JDK_Version::undefined(), JDK_Version::jdk(10), JDK_Version::jdk(11) },
>>>
>>> Expired version should be jdk(12) rather than jdk(11).
>>>
>>> Obsolete version should be jdk(11) rather than jdk(10), but that may
>>> not be possible until we've updated to be building 11.
>> Oh I forgot to change that back.  I'm waiting for the version to change, then I'll change it to 11, 12.
> Okay.
>
>> Thanks,
>> Coleen
>>> ------------------------------------------------------------------------------
>