RFR: 7093328: JVMTI: jvmtiPrimitiveFieldCallback always report 0's for static primitives

[Coleen Phillimore]

Rickard,
This change looks good.   Is there a test for this?  Is it possible to 
add a jtreg test for this and add it to test/serviceability directory?
This code is different in the permgen elimination repository because we 
don't pass klasses as oops.   I'll start with the test in the bug but it 
would be great to have a jtreg test for it so we know if we broke it.

thanks,
Coleen

On 8/29/2012 10:25 AM, Daniel D. Daugherty wrote:
> On 8/29/12 8:16 AM, Daniel D. Daugherty wrote:
>> Logistics issue: This review request went out to OpenJDK alias, but
>> I think this webrev is not accessible outside of Oracle. The webrev
>> should be publicly accessible when the review is public. However,
>> in this case, this is a one line fix so IMHO you could get away with
>> a context diff in the suggested fix of the bug report.
>>
>> On 8/29/12 1:24 AM, Rickard Bäckman wrote:
>>> Hi all,
>>>
>>> can I get a couple of reviews for the following bug fix:
>>>
>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7093328
>>> webrev: http://rbackman.se.oracle.com/~rbackman/7093328/
>>>
>>> Thanks
>>> /R
>>
>> Thumbs up.
>>
>> src/share/vm/prims/jvmtiTagMap.cpp
>>     No content comments.
>>
>>     This is a perfect example of how casting can bite you. :-)
>>
>>     The buggy line:
>>
>>     1165     address addr = (address)k + offset;
>>
>>     dates back to the original implementation of this function:
>>
>>     src/share/vm/prims/SCCS/s.jvmtiTagMap.cpp
>>     D 1.47 05/08/22 20:30:26 ab23780 77 76  01476/01330/01852
>>     MRs:
>>     COMMENTS:
>>     6195957: further clean-up and caching of field maps
>>
>>     In the buggy code, a value is copied from the instanceKlass
>>     at the offset instead of from the Java mirror. If the value
>>     copied from the instanceKlass happened to match the expected
>>     value, then that was pure luck.
>>
>>     The bug report claims that this last "worked" in 6u26. I suspect
>>     that "working" is defined as returning a non-zero value.
>>
>> Dan
>
> Rickard,
>
> Very nice find:
>
>> I did some research (archeology), the change that introduced this:
>>
>> changeset:   2223:c7f3d0b4570f
>> user:        never
>> date:        Fri Mar 18 16:00:34 2011 -0700
>> summary:     7017732: move static fields into Class to prepare for 
>> perm ten removal
>
> I had forgotten that static fields moved from the instanceKlass
> into Class. The original code from 2005 did indeed work as expected
> way back then. In fact, there were three such uses in jvmtiTagMap.cpp
> back then and it looks like the other two uses were updated to use
> the Java mirror and one was missed.
>
> Dan
>