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

[Jini George]

Thanks, Chris.

- Jini.

On 5/11/2018 11:26 PM, Chris Plummer wrote:
> Thanks Ioi.
> 
> Jini, you can count me as a reviewer for your changes.
> 
> thanks,
> 
> Chris
> 
> On 5/11/18 9:34 AM, 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.
>>>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>
>>>
>>
> 
>