Review request (M): 7118863: Move sizeof(klassOopDesc) into the *Klass::*_offset_in_bytes() functions

[Tom Rodriguez]

On Dec 7, 2011, at 5:35 AM, Stefan Karlsson wrote:

> On 2011-12-07 14:25, Bertrand Delsart wrote:
>> Would it be possible to change the name of the offset functions ?
>> 
>> By using the same name, but with a different semantic, you silently break all the ports for which the change has not yet been applied. These ports will compile correctly but will fail at execution time because they will be adding sizeof(klassOopDesc) twice. Fixing them will require to double check the changeset and identity all the functions for which the semantic has changed. Similar issues might happens if someone backports some code which includes your changes to a previous hotspot version which uses the old semantic.
> 
> Good suggestion. Thanks.

The current names are simple and clear.  What can you replace them with?  The Klass:: ones could be moved into klassOopDesc but that would still create a delete relative to the perm repo since klassOopDesc is going away.

tom

> 
> StefanK
> 
>> 
>> By changing the names, it will be trivial to detect at build time which calls have not been yet been correctly fixed. This also simplifies the review since we will be sure all calls have been dealt with :-)
>> 
>> Bertrand.
>> 
>> On 12/ 7/11 01:52 PM, Stefan Karlsson wrote:
>>> Please review this REF.
>>> 
>>> http://cr.openjdk.java.net/~stefank/7118863/webrev/
>>> 
>>> The main motivation for this patch is to make it easier to merge with
>>> the permgen project. But, IMHO, they also help the code by not having
>>> different ways to go from a klass field offset to an offset against the
>>> klassOopDesc.
>>> 
>>> Two things to note about this patch.
>>> 1) There's one place in the code where we don't add
>>> sizeof(klassOopDesc). I maintain this oddity and will leave the correct
>>> fix for a later change.
>>> src/share/vm/opto/compile.cpp
>>> 
>>> 2) I have no way to verify the shark changes.
>>> src/share/vm/shark/sharkIntrinsics.cpp
>>> src/share/vm/shark/sharkTopLevelBlock.cpp
>>> 
>>> From the RFE:
>>> The *Klass::*_offset_in_bytes() functions are used to get the offset to
>>> a field in a Klass. All places where they are used, we add something
>>> like sizeof(klassOopDesc) to make the offset against the klassOopDesc
>>> instead.
>>> 
>>> For example in opto/memnode.cpp:
>>> if (tkls->offset() == Klass::access_flags_offset_in_bytes() +
>>> (int)sizeof(oopDesc)) {
>>> 
>>> There's a couple of variants to this:
>>> instanceKlass::init_thread_offset_in_bytes() + sizeof(klassOopDesc)
>>> Klass::access_flags_offset_in_bytes() + (int)sizeof(oopDesc)
>>> Klass::secondary_supers_offset_in_bytes() + klassOopDesc::header_size()
>>> * HeapWordSize
>>> Klass::java_mirror_offset_in_bytes()
>>> klassOopDesc::klass_part_offset_in_bytes()
>>> 
>>> The proposal is to move all these size adjustments into the
>>> *Klass:*_offset_in_bytes() functions.
>> 
>> 
>