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

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

Re: RFR(M) : 8202392: [TESTBUG] open source vm testbase heapdump tests

[serguei.spit...@oracle.com]

+1

Thanks,
Serguei

On 5/11/18 06:52, Mikhailo Seledtsov wrote:

Looks good to me,

Misha

On 5/9/18, 5:14 PM, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8202392/webrev.00/index.html

1396 lines changed: 1396 ins; 0 del; 0 mod;

Hi all,

could you please review his patch which open sources heapdump tests 
from so-called VM testbase? as it's obvious from test names, they 
test heap dumping using jmap and a number of JVM flags.


As usually w/ VM testbase code, these tests are old, they have been 
run in hotspot testing for a long period of time. Originally, these 
tests were run by a test harness different from jtreg and had 
different build and execution schemes, some parts couldn't be easily 
translated to jtreg, so tests might have actions or pieces of code 
which look weird. In a long term, we are planning to rework them.


webrev: 
http://cr.openjdk.java.net/~iignatyev//8202392/webrev.00/index.html

JBS: https://bugs.openjdk.java.net/browse/JDK-8202392
testing: :vmTestbase_vm_heapdump test group

Thanks,
-- Igor

Re: RFR: (S): JDK-8195613: [SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int

[Chris Plummer]

Hi Jini,

Why do you have the following:

  98 if (arch.equals("amd64") || arch.equals("i386") || 
arch.equals("x86")) {


Is it because VM_Version::CPU_SHA is only expected on these platforms? 
If so, is there a reason you didn't choose a long constant that exists 
on all platforms?


And just one minor nit. I think the new code warrants some comments 
indicating what it is trying to test for.


thanks,

Chris

On 5/11/18 5:11 AM, Jini George wrote:

Thank you, David. Could I get another pair of eyes to take a look at:

http://cr.openjdk.java.net/~jgeorge/8195613/webrev.02/index.html

Thanks,
Jini.

On 5/11/2018 11:58 AM, David Holmes wrote:

Forgot to add - no need to see an updated webrev.

Thanks,
David

On 11/05/2018 4:22 PM, David Holmes wrote:

Hi Jini,

On 11/05/2018 5:34 AM, Jini George wrote:

Hi David,

Thank you very much for the review and for the clarifications 
offline! I have addressed your comments and have a new webrev at:


http://cr.openjdk.java.net/~jgeorge/8195613/webrev.01/index.html


Looks fine.

A couple of minor comments on the test:

You don't need:

   79 LingeredApp.stopApp(theApp);

as it is done in the finally block.

Can you add comments in checkForTruncation stating where the 
expected values were obtained from.


Thanks,
David


Thanks,
Jini.


On 5/4/2018 12:23 PM, David Holmes wrote:

Hi Jini,

On 4/05/2018 2:17 AM, Jini George wrote:

Hello!

Requesting reviews for:

https://bugs.openjdk.java.net/browse/JDK-8195613

([SA] HotSpotTypeDataBase.readVMLongConstants truncates values to 
int)


Webrev: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.00/


Actual fix seems fine.

I have a few comments on the test ...

- Why are you only testing on 64-bit? The truncation would happen 
on 32-bit as well.


- CheckForTruncation seems rather complicated, not sure why we 
need to look for two different things with one being amd64 specific??


- We've had discussions in the past about splitting strings on \n 
or \r\n depending on whether it's Windows or not. Better to use 
String.split("\R") regex function.


- Exception message strings should not contain newline characters. 
Also you can simplify them:


Reading XXX: got value NNN but expected MMM

And you don't need to define a local variable "String message" you 
can just:


throw new Exception("Reading XXX: got value " + value + " but 
expected " + expected);


Thanks,
David
-


Testing: The SA tests passed on Mach5.

Thanks,
Jini.

Re: RFR: (S): JDK-8195613: [SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int

[serguei.spit...@oracle.com]

  
  
Hi Jini,
  
  It looks good to me.
  A minor comment on the ClhsdbLongConstant.java.frames.html:
  
  Could you add a comment with an example of the expected longConstantOutput?
I do not need another webrev if you add it.

Thanks,
Serguei
  
  
  On 5/11/18 05:11, Jini George wrote:

Thank
  you, David. Could I get another pair of eyes to take a look at:
  
  
  http://cr.openjdk.java.net/~jgeorge/8195613/webrev.02/index.html
  
  
  Thanks,
  
  Jini.
  
  
  On 5/11/2018 11:58 AM, David Holmes wrote:
  
  Forgot to add - no need to see an updated
webrev.


Thanks,

David


On 11/05/2018 4:22 PM, David Holmes wrote:

Hi Jini,
  
  
  On 11/05/2018 5:34 AM, Jini George wrote:
  
  Hi David,


Thank you very much for the review and for the
clarifications offline! I have addressed your comments and
have a new webrev at:


http://cr.openjdk.java.net/~jgeorge/8195613/webrev.01/index.html

  
  
  Looks fine.
  
  
  A couple of minor comments on the test:
  
  
  You don't need:
  
  
     79 LingeredApp.stopApp(theApp);
  
  
  as it is done in the finally block.
  
  
  Can you add comments in checkForTruncation stating where the
  expected values were obtained from.
  
  
  Thanks,
  
  David
  
  
  Thanks,

Jini.



On 5/4/2018 12:23 PM, David Holmes wrote:

Hi Jini,
  
  
  On 4/05/2018 2:17 AM, Jini George wrote:
  
  Hello!


Requesting reviews for:


https://bugs.openjdk.java.net/browse/JDK-8195613


([SA] HotSpotTypeDataBase.readVMLongConstants truncates
values to int)


Webrev:
http://cr.openjdk.java.net/~jgeorge/8195613/webrev.00/

  
  
  Actual fix seems fine.
  
  
  I have a few comments on the test ...
  
  
  - Why are you only testing on 64-bit? The truncation would
  happen on 32-bit as well.
  
  
  - CheckForTruncation seems rather complicated, not sure
  why we need to look for two different things with one
  being amd64 specific??
  
  
  - We've had discussions in the past about splitting
  strings on \n or \r\n depending on whether it's Windows or
  not. Better to use String.split("\R") regex function.
  
  
  - Exception message strings should not contain newline
  characters. Also you can simplify them:
  
  
  Reading XXX: got value NNN but expected MMM
  
  
  And you don't need to define a local variable "String
  message" you can just:
  
  
  throw new Exception("Reading XXX: got value " + value + "
  but expected " + expected);
  
  
  Thanks,
  
  David
  
  -
  
  
  Testing: The SA tests passed on
Mach5.


Thanks,

Jini.



  

  

  


  

Re: RFR(S): 8185803: JdbExprTest.sh fails in JDK10-hs nightly due to "Name unknown: java.lang.Long.MAX_VALUE "

[Chris Plummer]

Actually I did notice that and had a new webrev ready to go out and en 
email drafted, but then noticed the solaris-sparc timeout when I tested 
again.


Chris

On 5/10/18 9:50 PM, David Holmes wrote:

I was going to say that you also need to remove:

  29 #  @requires os.family != "windows"
  30 #  @key intermittent

:)

But something seems fishy about this bug to me anyway - comments added 
to bug report.


Cheers,
David

On 11/05/2018 12:59 PM, Chris Plummer wrote:

Hello,

I'm withdrawing this RFR. I noticed with repeated runs it was 
sometimes failing on Solaris. It looks like for the most part the 
test ran ok, but then at the end of the log you see:


--Finish execution with sending "quit" command to JDB
--Sending cmd: quit
--Quit cmd was sent
--waitForFinish: Waiting for mydojdbCmds() to finish

And it never returns from this. It looks to be the same issue as 
JDK-8171483 , but 
with a different test. Since the shell script stability issues will 
be resolved when the scripts are converted to pure java tests by 
JDK-8179318 , I 
think it's best to just let this bug be fixed at the same time as 
JDK-8179318 . I was 
hoping to get the test stable and off the problemlist with this fix, 
but since that's not 100% attainable, it's not really worth pushing 
the fix at this time.


thanks,

Chris

On 5/10/18 5:01 PM, Chris Plummer wrote:

Hello,

Please review the following simple fix for 8185803:

https://bugs.openjdk.java.net/browse/JDK-8185803
http://cr.openjdk.java.net/~cjplummer/8185803/webrev.00/

Although this bug has been around for nearly a year, it used to only 
fail on Windows. After the push for JDK-8198426 last month, it 
started to fail on every run. The test runs jdb, stops at a 
breakpoint in the debugee, and the issues the following command:


   print java.lang.Long.MAX_VALUE

And the response back is:

com.sun.tools.example.debug.expr.ParseException: Name unknown: 
java.lang.Long.MAX_VALUE

   java.lang.Long.MAX_VALUE = null

The issue is that Long has not been initialized (it has been 
loaded), so the java.lang.Long.MAX_VALUE symbol is not valid. This 
became the case on every run after JDK-8198426 because it removed a 
bunch of java code that executed at startup, and this code must have 
been causing Long to get initialized. I'm not sure why it used to 
only fail on Windows before JDK-8198426, but it passes now on all 
platforms with my fix, which is to add a reference to java.lang.Long 
in the debugee so the Long will always be initialized.


thanks,

Chris

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

[Chris Plummer]

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.

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

Re: RFR(M) : 8202392: [TESTBUG] open source vm testbase heapdump tests

[Mikhailo Seledtsov]

Looks good to me,

Misha

On 5/9/18, 5:14 PM, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8202392/webrev.00/index.html

1396 lines changed: 1396 ins; 0 del; 0 mod;

Hi all,

could you please review his patch which open sources heapdump tests from 
so-called VM testbase? as it's obvious from test names, they test heap dumping 
using jmap and a number of JVM flags.

As usually w/ VM testbase code, these tests are old, they have been run in 
hotspot testing for a long period of time. Originally, these tests were run by 
a test harness different from jtreg and had different build and execution 
schemes, some parts couldn't be easily translated to jtreg, so tests might have 
actions or pieces of code which look weird. In a long term, we are planning to 
rework them.

webrev: http://cr.openjdk.java.net/~iignatyev//8202392/webrev.00/index.html
JBS: https://bugs.openjdk.java.net/browse/JDK-8202392
testing: :vmTestbase_vm_heapdump test group

Thanks,
-- Igor

Re: RFR: (S): JDK-8195613: [SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int

[Jini George]

Thank you, David. Could I get another pair of eyes to take a look at:

http://cr.openjdk.java.net/~jgeorge/8195613/webrev.02/index.html

Thanks,
Jini.

On 5/11/2018 11:58 AM, David Holmes wrote:

Forgot to add - no need to see an updated webrev.

Thanks,
David

On 11/05/2018 4:22 PM, David Holmes wrote:

Hi Jini,

On 11/05/2018 5:34 AM, Jini George wrote:

Hi David,

Thank you very much for the review and for the clarifications 
offline! I have addressed your comments and have a new webrev at:


http://cr.openjdk.java.net/~jgeorge/8195613/webrev.01/index.html


Looks fine.

A couple of minor comments on the test:

You don't need:

   79 LingeredApp.stopApp(theApp);

as it is done in the finally block.

Can you add comments in checkForTruncation stating where the expected 
values were obtained from.


Thanks,
David


Thanks,
Jini.


On 5/4/2018 12:23 PM, David Holmes wrote:

Hi Jini,

On 4/05/2018 2:17 AM, Jini George wrote:

Hello!

Requesting reviews for:

https://bugs.openjdk.java.net/browse/JDK-8195613

([SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int)

Webrev: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.00/


Actual fix seems fine.

I have a few comments on the test ...

- Why are you only testing on 64-bit? The truncation would happen on 
32-bit as well.


- CheckForTruncation seems rather complicated, not sure why we need 
to look for two different things with one being amd64 specific??


- We've had discussions in the past about splitting strings on \n or 
\r\n depending on whether it's Windows or not. Better to use 
String.split("\R") regex function.


- Exception message strings should not contain newline characters. 
Also you can simplify them:


Reading XXX: got value NNN but expected MMM

And you don't need to define a local variable "String message" you 
can just:


throw new Exception("Reading XXX: got value " + value + " but 
expected " + expected);


Thanks,
David
-


Testing: The SA tests passed on Mach5.

Thanks,
Jini.

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

[Chris Plummer]

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.

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

[Jini George]

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.