[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Review request for: 7191786 retransformClasses() does not pass in LocalVariableTypeTable of a method
From:       serguei.spitsyn () oracle ! com (serguei ! spitsyn at oracle ! com)
Date:       2012-08-28 17:01:33
Message-ID: 503CF96D.2070606 () oracle ! com
[Download RAW message or body]



On 8/28/12 8:54 AM, Daniel D. Daugherty wrote:
> On 8/22/12 12:34 PM, serguei.spitsyn at oracle.com wrote:
>> Dmitry,
>>
>> Thank you for the review!
>>
>> Please, find new webrev version here:
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2012/7191786-JVMTI-LVTT.1/
>
> Thumbs up. No content comments.

Ok, thanks!

>
> src/share/vm/prims/jvmtiClassFileReconstituter.hpp
>     No comments.
>
> src/share/vm/prims/jvmtiClassFileReconstituter.cpp
>     line 178: ++attr_count;
>     line 421: // Write LocalVariableTable attribute
>         Bug fixes for the previous LVT fix? Just making sure.

Generally speaking, I do not think it is a real problem.
Just wanted to make it consistent with other attributes.

>
>     line 456:  // JVMSpec|     { u2 start_pc;
>         Breaking this line into the following would be more readable:
>
>             456 // JVMSpec|     {
>             457                   u2 start_pc;
>

Will do it.


Thanks!
Serguei


> Dan
>>
>> I also changed the line:
>>   196 if (elem[idx].signature_cp_index > 0) {
>> to this:
>>   196 if (elem[idx].signature_cp_index != 0) {
>>
>> Both are correct as the signature_cp_index is u2.
>> But "signature_cp_index != 0" is more clear.
>>
>>
>> Thanks,
>> Serguei
>>
>>
>> On 8/21/12 12:45 PM, serguei.spitsyn at oracle.com wrote:
>>> On 8/21/12 12:11 PM, Dmitry Samersoff wrote:
>>>> Serguei,
>>>>
>>>> On 2012-08-21 23:05, serguei.spitsyn at oracle.com wrote:
>>>>> You can  see the same pattern for all attributes (for instance,
>>>>> has_stackmap_table - L160).
>>>> OK.
>>>>
>>>>>> 202: local_variable_type_table_length
>>>>>>       is always 0 if local_variable_table_length == 0
>>>>>>
>>>>>>       so I think if should be nested.
>>>>> It does not matter.
>>>> It saves one if in some cases and makes logic better readable,
>>>> so I would like to have it changed.
>>>>
>>>> Sorry!
>>>
>>> In fact, I initially considered this option and can change it as you 
>>> prefer.
>>> But let's wait for other reviewers input.
>>>
>>> Thanks,
>>> Serguei
>>>
>>>>
>>>>>> 216: It might be better to place both attr_size changes together.
>>>>> I'm trying to follow the existing style.
>>>> OK
>>>>
>>>> -Dmitry
>>>>
>>>>
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>> -Dmitry
>>>>>>
>>>>>>
>>>>>> On 2012-08-21 22:13, serguei.spitsyn at oracle.com wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>>
>>>>>>> Please, review the fix for:
>>>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7191786
>>>>>>>
>>>>>>> Open webrev:
>>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2012/7191786-JVMTI-LVTT/ 
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Summary:
>>>>>>>
>>>>>>>   The following CR was recently fixed:
>>>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7064927
>>>>>>>
>>>>>>>   But the same issue exists for the LocalVariableTypeTable 
>>>>>>> attribute.
>>>>>>>
>>>>>>>   The fix was tested with the modified test:
>>>>>>> test/java/lang/instrument/VerifyLocalVariableTableOnRetransformTest.sh 
>>>>>>>
>>>>>>>
>>>>>>> The modification is that a local variable with generic signature 
>>>>>>> is added
>>>>>>> to the class DummyClassWithLVT.java:
>>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2012/7191786-JLI-Test-For-JVMTI-LVTT/ 
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> The test patch will be integrated into the jdk8/tl after the 
>>>>>>> HotSpot fix
>>>>>>> is promoted.
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Serguei
>>>>
>>>
>>


[prev in list] [next in list] [prev in thread] [next in thread]