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.
