RFR(XS): 4965252: JvmtiExport::post_raw_field_modification jni ref handling is odd

[Rickard Bäckman]

David,

On May 16, 2013, at 7:12 AM, David Holmes wrote:

> Hi Rickard,
> 
> On 15/05/2013 7:37 PM, Rickard Bäckman wrote:
>> can I please have this change reviewed?
>> 
>> My interpretation is that this isn't really a bug, since the parameter sig_type is never set to [.
> 
> Right - it looks like a bug visually but turns out to be dead code. FYI the  "sig_type == '['" was added to the if under 4639363 - no idea why. I don't see [ ever being used for sigtype.

+1

> 
>> The suggested change is to remove the check for [ in the if and add an assert. I also created a boolean
>> to track handle creation to simplify cleanup.
> 
> Not sure the boolean was worth the effort - the existing check for L suffices. :)

Sure, but I like not repeating checks :)

> 
> Even the assert seems overly conservative given the single caller :)

At least it is safe.

> 
> Still trying to decide what was meant in the bug report about the lack of an "enclosing handle block" ??

My guess was that we have tried to do 

{
  make_local()
  destroy_local()
}

I don't think this makes things clearer in this case.

> 
> Good to go if it hasn't already :)

Thank you for the review. (Too late to make it to the list though :) )

/R

> 
> Cheers,
> David
> 
> 
>> 
>> The caller of this method is InterpreterRuntime::post_field_modification
>> which sets the sig_type to:
>> 
>>   switch(cp_entry->flag_state()) {
>>     case btos: sig_type = 'Z'; break;
>>     case ctos: sig_type = 'C'; break;
>>     case stos: sig_type = 'S'; break;
>>     case itos: sig_type = 'I'; break;
>>     case ftos: sig_type = 'F'; break;
>>     case atos: sig_type = 'L'; break;
>>     case ltos: sig_type = 'J'; break;
>>     case dtos: sig_type = 'D'; break;
>>     default:  ShouldNotReachHere(); return;
>>   }
>> 
>> Testing done: nsk.jvmti.testlist with fastdebug build.
>> 
>> Webrev: http://cr.openjdk.java.net/~rbackman/4965252/
>> 
>> Thanks
>> /R
>> 
>> 
>>