Review request for: 7191786 retransformClasses() does not pass in LocalVariableTypeTable of a method

[Daniel D. Daugherty]

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.

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.

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

             456 // JVMSpec|     {
             457                   u2 start_pc;

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
>>>
>>
>