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

[Daniel D. Daugherty]

Adding serviceability-dev at openjdk.java.net back into the thread...
"Reply list" seems to only pick one list...


The JCK team has opened a bug for adding a test:

     7194847 4/3 Need JCK JVM TI tests for IterateThroughHeap to
                 cover issue reported by 7093328

I don't know if the VM/SQE team will be adding a test to the VM/NSK
testbase for this. I don't think a JavaTest/JTREG test is a good
fit for this since it has to be a native test.

Dan


On 8/29/12 8:42 AM, Coleen Phillimore wrote:
>
> 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
>>