RFR: (S): SA: clhsdb 'where -a' throws Assertion Failure with illegal code 236 when CDS is used

[Jini George]

Thanks, Ioi.

- Jini.

On 5/11/2018 10:04 PM, Ioi Lam wrote:
> I think all code that looks at the top of stack (e.g., users of 
> BytecodeStream) operate with the original bytecode (_aload_0), not the 
> quicken one (_fast_aload_0 or _nofast_aload_0), so this bug is never 
> discovered. But I think this is still a bug and should be fixed.
> 
> I filed https://bugs.openjdk.java.net/browse/JDK-8203005
> 
> Thanks
> 
> - Ioi
> 
> 
> 
> 
> On 5/11/18 12:45 AM, Chris Plummer wrote:
>> I'm not too sure how that field ends up getting used. I'd say what's 
>> likely in this case is that it's wrong but doesn't matter. Hopefully 
>> someone with a better understanding of the use of this field will 
>> chime in.
>>
>> Chris
>>
>> On 5/11/18 12:29 AM, Jini George wrote:
>>> Thank you very much, Chris, for taking a look.
>>>
>>> The bytecodes table initialization in SA's Bytecodes.java mimics the
>>> bytecodes table initialization in hotspot's Bytecodes::initialize() from
>>> bytecodes.cpp. In bytecodes.cpp, we have the initialization of the
>>> _nofast* bytecodes thus:
>>>
>>>
>>>   def(_nofast_aload_0      , "nofast_aload_0"      , "b"    , NULL , 
>>> T_ILLEGAL,  1, true , _aload_0        );
>>>   def(_nofast_iload        , "nofast_iload"        , "bi"   , NULL , 
>>> T_ILLEGAL,  1, false, _iload          );
>>>
>>> So looks like the result type in hotspot's bytecodes.cpp also needs 
>>> to be changed ?
>>>
>>> Thanks,
>>> Jini.
>>>
>>>
>>> On 5/8/2018 11:43 PM, Chris Plummer wrote:
>>>> Hi Jini,
>>>>
>>>> Why are _nofast_aload_0 and _nofast_iload using return type 
>>>> BasicType.getTIllegal() when the return type is known?
>>>>
>>>> The test changes look good.
>>>>
>>>> thanks,
>>>>
>>>> Chris
>>>>
>>>> On 5/8/18 8:53 AM, Jini George wrote:
>>>>> Thanks, Ioi. Could I get one more reviewer to take a look at this
>>>>> ?
>>>>>
>>>>> Thanks, Jini.
>>>>>
>>>>> On 5/8/2018 8:55 PM, Ioi Lam wrote:
>>>>>> Looks good. Thanks!
>>>>>>
>>>>>> - Ioi
>>>>>>
>>>>>>
>>>>>> On 5/7/18 8:38 PM, Jini George wrote:
>>>>>>> Thank you very much, Ioi, for the review and for the
>>>>>>> clarifications and help provided offline. I have added the
>>>>>>> checks for _nofast_getfield and _nofast_putfield. SA has a bug
>>>>>>> due to which for iload, only the base bytecode (iload) gets
>>>>>>> displayed -- fast_iload and nofast_iload do not get displayed.
>>>>>>> JDK-8202693 (SA: clhsdb printall only displays the base
>>>>>>> bytecode for iload) has been filed for this. I would add the
>>>>>>> test for nofast_iload along with the fix for JDK-8202693.
>>>>>>>
>>>>>>> The modified webrev is at:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~jgeorge/8174995/webrev.01/
>>>>>>>
>>>>>>> Thanks, Jini.
>>>>>>>
>>>>>>> On 4/27/2018 1:54 AM, Ioi Lam wrote:
>>>>>>>> HI Jini,
>>>>>>>>
>>>>>>>> [1] "_nofast_aload" should be "_nofast_aload_0": aload and
>>>>>>>> aload_0 are two different bytecodes.
>>>>>>>>
>>>>>>>> [2] Only the _nofast_aload_0 bytecode is tested. For
>>>>>>>> completeness, do you think it makes sense to add test cases
>>>>>>>> for these other 3 bytecodes?
>>>>>>>>
>>>>>>>> _nofast_getfield _nofast_putfield _nofast_iload
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks - Ioi
>>>>>>>>
>>>>>>>> On 4/26/18 11:15 AM, Jini George wrote:
>>>>>>>>> Hello all,
>>>>>>>>>
>>>>>>>>> Please review the following proposed fix for the issue:
>>>>>>>>>
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8174995
>>>>>>>>>
>>>>>>>>> Webrev:
>>>>>>>>> http://cr.openjdk.java.net/~jgeorge/8174995/webrev.00/
>>>>>>>>>
>>>>>>>>> Issue: Clhsdb commands like 'where -a', 'printall' would
>>>>>>>>> throw an illegal code assertion failure when CDS is used.
>>>>>>>>>
>>>>>>>>> Root cause and proposed fix: SA has been unaware of the new
>>>>>>>>>  bytecodes introduced for rewriting at CDS dump time
>>>>>>>>> (_nofast* bytecodes). The fix is to make SA aware of these
>>>>>>>>> new _nofast* bytecodes.
>>>>>>>>>
>>>>>>>>> Tests Run and Passed: SA tests on Mach5 (including the
>>>>>>>>> tests modified to test this fix).
>>>>>>>>>
>>>>>>>>> Thank you, Jini.
>>>>>>>>
>>>>>>
>>>>
>>>>
>>
>>
>