RFR (M): 8026328: Setting a breakpoint on invokedynamic crashes the JVM

[serguei.spitsyn at oracle.com]

On 10/24/13 4:12 PM, serguei.spitsyn at oracle.com wrote:
> On 10/24/13 3:45 PM, Christian Thalinger wrote:
>>
>> On Oct 24, 2013, at 3:02 PM, Christian Thalinger 
>> <christian.thalinger at oracle.com 
>> <mailto:christian.thalinger at oracle.com>> wrote:
>>
>>>
>>> On Oct 23, 2013, at 3:46 PM,serguei.spitsyn at oracle.com 
>>> <mailto:serguei.spitsyn at oracle.com>wrote:
>>>
>>>> Chris,
>>>>
>>>> This fix is nice, thank you for doing it!
>>>> I don't see any issues except this one:
>>>>
>>>>   566 /**
>>>>   567  * Returns the return entry address for the given top-of-stack state and bytecode.
>>>>   568  */
>>>>   569 address TemplateInterpreter::return_entry(TosState state, int length, Bytecodes::Code code) {
>>>>   570   guarantee(0 <= length && length < Interpreter::number_of_return_entries, "illegal length");
>>>>   571   const int index = TosState_as_index(state);
>>>>   572   switch (code) {
>>>>   573   case Bytecodes::_invokestatic:
>>>>   574   case Bytecodes::_invokespecial:
>>>>   575   case Bytecodes::_invokevirtual:
>>>>   576     return _invoke_return_entry[index];
>>>>   577   case Bytecodes::_invokeinterface:
>>>>   578     return _invokeinterface_return_entry[index];
>>>>   579   case Bytecodes::_invokedynamic:
>>>>   580     return _invokedynamic_return_entry[index];
>>>>   581   default:
>>>>   582     return _return_entry[length].entry(state);
>>>>   583   }
>>>>   584 }
>>>> Should the above switch also include the _invokehandle case?
>>>
>>> Good point.  It should.  I’ve also added an assert to make sure we 
>>> don’t end up with an invoke instruction in the default case:
>>>
>>> +  default:
>>> + assert(!Bytecodes::is_invoke(code), err_msg("invoke instructions 
>>> should be handled separately: %s", Bytecodes::name(code)));
>>> +    return _return_entry[length].entry(state);
>>
>> Which brings up another question:  should Bytecodes::is_invoke 
>> include a check for invokehandle?  I think it should but we shouldn’t 
>> make that change so late in the 8 game.
>
> I do not see the Bytecodes::is_invoke is used widely.
>
> It is used in 4 other asserts:
>
> ops/methodData.cpp: assert(Bytecodes::is_invoke(stream->code()), 
> "should be invoke");
> oops/methodData.cpp: assert(Bytecodes::is_invoke(stream->code()), 
> "should be invoke");
> oops/methodData.cpp: assert(Bytecodes::is_invoke(stream->code()), 
> "should be invoke");
>
> interpreter/bytecodes.hpp:  static bool        has_receiver (Code 
> code)    { assert(is_invoke(code), "");  return code == _invokevirtual ||
>
>
> Fixing it in Bytecodes::is_invoke looks pretty so far but depends on 
> the usage in methodData.cpp.

Correction: "looks pretty safe so far".  :)

Thanks,
Serguei

>
>
> Thanks,
> Serguei
>
>>
>>>
>>>>
>>>>
>>>> How did you test it?
>>>> It is necessary to run the nsk.jvmti and nsk.jdi testlists.
>>>
>>> It tested various things manually and did a JPRT run.  Since I 
>>> touched every invoke return entry in the system running some random 
>>> code automatically tests this code.
>>>
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>> On 10/23/13 11:06 AM, Christian Thalinger wrote:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8026328
>>>>> http://cr.openjdk.java.net/~twisti/8026328/webrev.00/
>>>>>
>>>>> 8026328: Setting a breakpoint on invokedynamic crashes the JVM
>>>>> Reviewed-by:
>>>>>
>>>>> Well-known invoke instructions have a 2-byte index but invokedynamic has a 4-byte index. In return entries we check the bytecode instruction to decide which size the index has.
>>>>>
>>>>> The problem is that if there is a breakpoint there is no way to know what index size the instruction has because the original instruction byte was replaced with the breakpoint byte.
>>>>>
>>>>> There are a couple of ways to fix this but the proper way (in my opinion) is to have separate return entries for the different classes of invoke instructions.  This on one hand generates more return entries but on the other hand makes them smaller and simpler.
>>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20131024/e4ca1cbc/attachment.html