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

[serguei.spitsyn at oracle.com]

On 10/24/13 4:21 PM, Christian Thalinger wrote:
> I’ve added another small change to make Zero build again:
>
> diff -r 97d400662426 src/cpu/zero/vm/globals_zero.hpp
> --- a/src/cpu/zero/vm/globals_zero.hppWed Oct 23 19:22:28 2013 +0000
> +++ b/src/cpu/zero/vm/globals_zero.hppThu Oct 24 16:20:54 2013 -0700
> @@ -57,6 +57,8 @@ define_pd_global(bool,  UseMembar,
>  // GC Ergo Flags
>  define_pd_global(uintx, CMSYoungGenPerWorker, 16*M);  // default max 
> size of CMS young gen, per GC worker thread
>
>
> +define_pd_global(uintx, TypeProfileLevel, 0);
> +
>  #define ARCH_FLAGS(develop, product, diagnostic, experimental, 
> notproduct)
>
>
>  #endif // CPU_ZERO_VM_GLOBALS_ZERO_HPP

Looks good.

Thanks,
Serguei

>
> 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);
>>
>>>
>>>
>>> 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/40733b3d/attachment.html