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

[Ioi Lam]

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.
>>>>>>>
>>>>>
>>>
>>>
>
>