Re: RFR 8205654: serviceability/dcmd/framework/HelpTest.java timed out

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

Daniil and David,

Just wanted to let you know that I'm reviewing this.


On 1/20/19 21:18, David Holmes wrote:
Thanks for the update Daniil. I still remain concerned about the 
robustness of the command-line parsing - this seems like a feature 
that needs its own set of tests.



I guess, the main David's concern is new file
  src/jdk.jcmd/linux/classes/sun/tools/ProcessHelper.java

which includes introduced by Daniil command-line processing.

Thanks,
Serguei




I'll leave it up to Serguei and others as to how to proceed.

David
-

On 19/01/2019 9:08 am, Daniil Titov wrote:

Hi David and Serguei,

Please review a new version of the fix that now covers the case when 
Java executes a module with the main class name explicitly specified 
in the command line.


Webrev: http://cr.openjdk.java.net/~dtitov/8205654/webrev.03
Bug: : https://bugs.openjdk.java.net/browse/JDK-8205654

Thanks!
--Daniil

On 1/8/19, 6:05 PM, "David Holmes"  wrote:

 Hi Daniil,
  Sorry this slipped through the Xmas break cracks :)
  On 22/12/2018 12:04 pm, Daniil Titov wrote:
 > Hi David and Serguei,
 >
 > Please review a new version of the fix that for Linux platform 
uses the proc filesystem to retrieve the main class name for the 
running Java process.

 >
 > Webrev: http://cr.openjdk.java.net/~dtitov/8205654/webrev.02/
 > Bug: https://bugs.openjdk.java.net/browse/JDK-8205654
  It's more complex than I had envisaged but seems to be 
doing the job.
 I'm not sure how robust the command-line parsing is, in 
particular it

 doesn't handle these forms:
  or  java [options] -m [/] [args...]
 java [options] --module [/] [args...]
 (to execute the main class in a module)
  I can't really comment on all the details.
  Thanks,
 David
 -
  > Thanks,
 > Daniil
 >
 > On 11/29/18, 4:52 PM, "David Holmes" 
 wrote:

 >
 >  Hi Daniil,
 >
 >  On 30/11/2018 7:30 am, Daniil Titov wrote:
 >  > Thank you, David!
 >  >
 >  > The proposed fix didn't help. It still hangs at some 
occasions.  Additional tracing showed that when jcmd is invoked with 
the main class name it iterates over all running Java processes and 
temporary attaches to them to retrieve the main class name. It hangs 
while trying to attach to one of the running Java processes. There 
are numerous Java processes running at the host machine some 
associated with the test framework itself and another with the tests 
running in parallel. It is not clear what exact is this particular 
process since the jcmd hangs before retrieving the process' main 
class name, but after all tests terminated the process with this id 
is no longer running.  I have to revoke this review since more 
investigation is required.

 >
 >  That sounds like an unsolvable problem for the test. You 
can't control
 >  other Java processes on the machine, and searching by 
name requires

 >  asking each of them in turn.
 >
 >  How do we get the list of Java processes in the first 
place? Perhaps we

 >  need to do some /proc//cmdline peeking?
 >
 >  Cheers,
 >  David
 >
 >  >
 >  > Best regards,
 >  > Daniil
 >  >
 >  >
 >  >
 >  > On 11/11/18, 1:35 PM, "David Holmes" 
 wrote:

 >  >
 >  >  Hi Daniil,
 >  >
 >  >  I took a quick look at this one ... two minor 
comments

 >  >
 >  >  The static class names could just be "Process" as 
they will acquire the
 >  >  enclosing class name as part of their own name 
anyway. As it is this

 >  >  gets repeated eg:
 >  >
 >  >  HelpTest$HelpTestProcess
 >  > InvalidCommandTest$InvalidCommandTestProcess
 >  >
 >  >  TestJavaProcess.java:
 >  >
 >  >  39 public static void main(String argv[]) {
 >  >
 >  >  Nit: Should be "String[] argv" in Java style
 >  >
 >  >  Thanks,
 >  >  David
 >  >
 >  >  On 10/11/2018 3:18 PM, Daniil Titov wrote:
 >  >  > Please review the change that fixes 
serviceability/dcmd/framework/* tests from a time out. The fix for 
JDK-8166642 made serviceability/dcmd/framework/* tests non-concurrent 
to ensure that they don't interact with each other and there are no 
multiple tests running simultaneously since all they do share the 
common main class name com.sun.javatest.regtest.agent.MainWrapper. 
However, it looks like the  tests from other directories still might 
run in parallel with these tests and they also have 
com.sun.javatest.regtest.agent.MainWrapper as a main class.

 >  >  >
 >  >  > The fix  ensures that each 
serviceability/dcmd/framework/* test uses a Java process 

Re: [RFR]8215622: Add dump to file support for jmap histo

[臧琳]

Hi David, Paul,
I have uploaded the refined webrev at
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.06/
And submitted a CSR at https://bugs.openjdk.java.net/browse/JDK-8218131
May I ask your help to review?
Thanks!

BRs,
Lin

From: David Holmes 
Sent: Thursday, January 31, 2019 9:19:31 AM
To: Hohensee, Paul; 臧琳; JC Beyler
Cc: serviceability-dev@openjdk.java.net
Subject: Re: [RFR]8215622: Add dump to file support for jmap histo

On 31/01/2019 10:41 am, Hohensee, Paul wrote:
> Also, I suspect that this change needs a CSR, since we're adding 
> functionality to jmap's histo switch. Opinions?

Yes. Changes to command-line flags need a CSR.

Thanks,
David

> Thanks,
>
> Paul
>
> On 1/30/19, 9:33 AM, "serviceability-dev on behalf of Hohensee, Paul" 
>  hohen...@amazon.com> wrote:
>
>  A nit in TestLoggerWeakRefLeak.java, separate the 2 arguments to 
> heapHisto() by a space.
>
>  vm.heapHisto("", "-live")
>
>  Also, the header comment line for getInstanceCountFromHeapHisto() should 
> be changed to
>
>   * 'vm.heapHisto("", "-live")' will request a full GC
>
>  In attachListener.cpp, the line
>
>out->print_cr("Invalid argument to dumpheap operation: %s", arg1);
>
>  should be
>
>out->print_cr("Invalid argument to inspectheap operation: %s", 
> arg1);
>
>  Otherwise looks fine. The two failing tests (the other one was 
> test/jdk/java/util/concurrent/locks/Lock/TimedAcquireLeak.java) now pass on 
> my mac laptop.
>
>  Paul
>
>  On 1/30/19, 12:18 AM, "臧琳"  wrote:
>
>  Dear All,
>  This issue mentioned is that test case 
> "java/util/logging/TestLoggerWeakRefLeak.java" Failed after applied the 
> webrev.
>  I have identified the root cause of the issue. it is caused by 2 
> problems.
>   1. The path processing in heap_inspection() at 
> attachListener.cpp.
>   The problem is that when use jmap -histo:live , the path is 
> actually set to "" when it is transfer to socket. so it cause JNI_ERR.
>   I need to modify the logic here that if path[0] == '\0' , 
> fall through to the next argument parsing, rather than return JNI_ERR.
>
>   2. Another problem is HotSpotVirtualMachine.java, it has a 
> heapHisto() method, and the testcase use vm.heapHisto("-live"), And after the 
> patch applied, it should be vm.heapHisto(""/*filepath*/, "-live"), seems we 
> need to change the API for handling it.
>
>   I have upload a webrev05:
>   http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.05/
>
>  May I ask your help for review?
>
>  Thanks,
>  Lin
>  
>  From: 臧琳
>  Sent: Monday, January 28, 2019 5:49:42 PM
>  To: Hohensee, Paul; JC Beyler
>  Cc: serviceability-dev@openjdk.java.net
>  Subject: Re: [RFR]8215622: Add dump to file support for jmap histo
>
>  Dear all,
>   I have found there is problem for handling the "filepath" and 
> it cause test "java/util/logging/TestLoggerWeakRefLeak.java" fail, I have 
> identified the root cause, please wait for the new update.
>  Thanks!
>
>  BRs,
>  Lin
>  
>  From: 臧琳
>  Sent: Friday, January 25, 2019 9:00:19 AM
>  To: Hohensee, Paul; JC Beyler
>  Cc: serviceability-dev@openjdk.java.net
>  Subject: Re: [RFR]8215622: Add dump to file support for jmap histo
>
>  Dear All,
>  May I get more review about this webrev?
>  http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.04/
>
>  Thanks!
>  BRs,
>  Lin
>  
>  From: 臧琳
>  Sent: Tuesday, January 22, 2019 2:18:06 PM
>  To: Hohensee, Paul; JC Beyler
>  Cc: serviceability-dev@openjdk.java.net
>  Subject: Re: [RFR]8215622: Add dump to file support for jmap histo
>
>  Hi Paul,
>  Thanks a lot for your review. I have refined it based on your 
> comments.
>  http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.04/
>  Would you like to help review it again? Thanks
>
>  BRs,
>  Lin
>  
>  From: Hohensee, Paul 
>  Sent: Friday, January 18, 2019 6:11:14 AM
>  To: 臧琳; JC Beyler
>  Cc: serviceability-dev@openjdk.java.net
>  Subject: Re: [RFR]8215622: Add dump to file support for jmap histo
>
>  Just a few small things.
>
>  In attachListener.cpp, line 278, is the static_cast needed? 
> fileStream is a subclass of outputStream, so it should be ok to pass to the 
> VM_GC_Heap_Inspection constructor, but maybe there's some C++ arcana I don't 
> know about.
>
> 

Re: RFR (12) JDK-8218025: disable pop_frame and force_early_return caps for Graal

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

  
  
Yes, David suggested the same.
  Your emails crossed.
  
  Thanks,
  Serguei
  
  On 1/30/19 21:22, Igor Ignatyev wrote:

from my
  point of view, having the following code at the end
  of JvmtiManageCapabilities::init_onload_capabilities is much clear
  and easier to understand:
  
  
  

  // Workaround for 8195635: disable pop_frame and
force_early_return capabilities


  #if INCLUDE_JVMCI
    if (UseJVMCICompiler) {
      jc.can_pop_frame = 0;
      jc.can_force_early_return = 0;
    }
  #endif // INCLUDE_JVMCI

  
  
  
  Thanks,
  -- Igor
  

  
On Jan 30, 2019, at 9:18 PM, dean.l...@oracle.com wrote:


   On 1/30/19 8:59 PM, serguei.spit...@oracle.com
wrote:
 So, the fix needs to be more like this:
  +  // Workaround for 8195635:
+  // disable pop_frame and force_early_return capabilities with Graal
+ #if INCLUDE_JVMCI
+  if (!(EnableJVMCI && UseJVMCICompiler)) {
   jc.can_pop_frame = 1;
   jc.can_force_early_return = 1;
+  }
+ #endif

Not sure, if the check for EnableJVMCI can be removed above.


We still need it to work when INCLUDE_JVMCI is not
defined.
How about

JVMCI_ONLY(if (UseJVMCICompiler)) {
...
}

or

if (JVMCI_ONLY(UseJVMCICompiler) NOT_JVMCI(true)) {
...
}

dl

  

  


  


  

Re: RFR (12) JDK-8218025: disable pop_frame and force_early_return caps for Graal

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

On 1/30/19 21:24, David Holmes wrote:

On 31/01/2019 3:18 pm, dean.l...@oracle.com wrote:

On 1/30/19 8:59 PM, serguei.spit...@oracle.com wrote:

So, the fix needs to be more like this:
+ // Workaround for 8195635:
+ // disable pop_frame and force_early_return capabilities with Graal
+ #if INCLUDE_JVMCI
+ if (!(EnableJVMCI && UseJVMCICompiler)) {
    jc.can_pop_frame = 1;
    jc.can_force_early_return = 1;
+ } + #endif Not sure, if the check for EnableJVMCI can be removed 
above.


We still need it to work when INCLUDE_JVMCI is not defined.
How about

JVMCI_ONLY(if (UseJVMCICompiler)) {
...
}

or

if (JVMCI_ONLY(UseJVMCICompiler) NOT_JVMCI(true)) {
...
}


Or just turn them on unconditionally first and turn off explicitly for 
JVMCI:


 jc.can_pop_frame = 1;
 jc.can_force_early_return = 1;
+ #if INCLUDE_JVMCI
+  // Workaround for 8195635:
+  // disable pop_frame and force_early_return capabilities with Graal
+ if (EnableJVMCI && UseJVMCICompiler) {
+ jc.can_pop_frame = 0;
+ jc.can_force_early_return = 0;
+ }
+ #endif


Oh, Dean is right.
We need these caps initialized even if the macro INCLUDE_JVMCI is undefined.
Then I like variant from David above.

Thanks,
Serguei



David


dl

Re: RFR (12) JDK-8218025: disable pop_frame and force_early_return caps for Graal

[David Holmes]

On 31/01/2019 3:18 pm, dean.l...@oracle.com wrote:

On 1/30/19 8:59 PM, serguei.spit...@oracle.com wrote:

So, the fix needs to be more like this:
+ // Workaround for 8195635:
+ // disable pop_frame and force_early_return capabilities with Graal
+ #if INCLUDE_JVMCI
+ if (!(EnableJVMCI && UseJVMCICompiler)) {
jc.can_pop_frame = 1;
jc.can_force_early_return = 1;
+ } + #endif Not sure, if the check for EnableJVMCI can be removed above.


We still need it to work when INCLUDE_JVMCI is not defined.
How about

JVMCI_ONLY(if (UseJVMCICompiler)) {
...
}

or

if (JVMCI_ONLY(UseJVMCICompiler) NOT_JVMCI(true)) {
...
}


Or just turn them on unconditionally first and turn off explicitly for 
JVMCI:


 jc.can_pop_frame = 1;
 jc.can_force_early_return = 1;
+ #if INCLUDE_JVMCI
+  // Workaround for 8195635:
+  // disable pop_frame and force_early_return capabilities with Graal
+ if (EnableJVMCI && UseJVMCICompiler) {
+ jc.can_pop_frame = 0;
+ jc.can_force_early_return = 0;
+ }
+ #endif

David


dl

Re: RFR (12) JDK-8218025: disable pop_frame and force_early_return caps for Graal

[Igor Ignatyev]

from my point of view, having the following code at the end of 
JvmtiManageCapabilities::init_onload_capabilities is much clear and easier to 
understand:

> // Workaround for 8195635: disable pop_frame and force_early_return 
> capabilities
> #if INCLUDE_JVMCI
>   if (UseJVMCICompiler) {
> jc.can_pop_frame = 0;
> jc.can_force_early_return = 0;
>   }
> #endif // INCLUDE_JVMCI


Thanks,
-- Igor

> On Jan 30, 2019, at 9:18 PM, dean.l...@oracle.com wrote:
> 
> On 1/30/19 8:59 PM, serguei.spit...@oracle.com 
>  wrote:
>> So, the fix needs to be more like this:
>> +  // Workaround for 8195635:
>> +  // disable pop_frame and force_early_return capabilities with Graal
>> + #if INCLUDE_JVMCI
>> +  if (!(EnableJVMCI && UseJVMCICompiler)) {
>>jc.can_pop_frame = 1;
>>jc.can_force_early_return = 1;
>> +  }
>> + #endif
>> 
>> Not sure, if the check for EnableJVMCI can be removed above.
> 
> We still need it to work when INCLUDE_JVMCI is not defined.
> How about
> 
> JVMCI_ONLY(if (UseJVMCICompiler)) {
> ...
> }
> 
> or
> 
> if (JVMCI_ONLY(UseJVMCICompiler) NOT_JVMCI(true)) {
> ...
> }
> 
> dl
> 

Re: RFR (12) JDK-8218025: disable pop_frame and force_early_return caps for Graal

[dean . long]

On 1/30/19 8:59 PM, serguei.spit...@oracle.com wrote:

So, the fix needs to be more like this:
+ // Workaround for 8195635:
+ // disable pop_frame and force_early_return capabilities with Graal
+ #if INCLUDE_JVMCI
+ if (!(EnableJVMCI && UseJVMCICompiler)) {
jc.can_pop_frame = 1;
jc.can_force_early_return = 1;
+ } + #endif Not sure, if the check for EnableJVMCI can be removed above.


We still need it to work when INCLUDE_JVMCI is not defined.
How about

JVMCI_ONLY(if (UseJVMCICompiler)) {
...
}

or

if (JVMCI_ONLY(UseJVMCICompiler) NOT_JVMCI(true)) {
...
}

dl

Re: RFR (12) JDK-8218025: disable pop_frame and force_early_return caps for Graal

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

  
  
On 1/30/19 20:46,
  serguei.spit...@oracle.com wrote:

Hi
  Alex,
  
  
  Vladimir I. also mentioned the same as Igor that it is safe to use
  UseJVMCICompiler under "ifdef INCLUDE_JVMCI".
  
  
  One example from the runtime/thread.cpp:
  
  #if INCLUDE_JVMCI
  
    bool force_JVMCI_intialization = false;
  
    if (EnableJVMCI) {
  
      // Initialize JVMCI eagerly when it is explicitly requested.
  
      // Or when JVMCIPrintProperties is enabled.
  
      // The JVMCI Java initialization code will read this flag and
  
      // do the printing if it's set.
  
      force_JVMCI_intialization = EagerJVMCI ||
  JVMCIPrintProperties;
  
  
      if (!force_JVMCI_intialization) {
  
    // 8145270: Force initialization of JVMCI runtime otherwise
  requests for blocking
  
    // compilations via JVMCI will not actually block until
  JVMCI is initialized.
  
    force_JVMCI_intialization = UseJVMCICompiler &&
  (!UseInterpreter || !BackgroundCompilation);
  
      }
  
    }
  
  #endif
  
  
  One more example from prims/jni.cpp:
  
  
  #if INCLUDE_JVMCI
  
      if (EnableJVMCI) {
  
    if (UseJVMCICompiler) {
  
      . . .
  
    }
  
      }
  
  #endif
  


Sorry, I've accidentally pressed 'Send' button.

Continuing ...

So, the fix needs to be more like this:
+  // Workaround for 8195635:
+  // disable pop_frame and force_early_return capabilities with Graal
+ #if INCLUDE_JVMCI
+  if (!(EnableJVMCI && UseJVMCICompiler)) {
   jc.can_pop_frame = 1;
   jc.can_force_early_return = 1;
+  }
+ #endif

Not sure, if the check for EnableJVMCI can be removed above.


Otherwise, it looks good to me.
Also, I think, it is good to have updates for the
ProblemList-graal.txt files.

Thanks,
Serguei




  
  On 1/30/19 17:34, Igor Ignatyev wrote:
  
  Hi Alex,


UseJVMCICompiler is declared only if INCLUDE_JVMCI is defined,
so your current patch will break builds where it's undefined.
the rest (esp. problem list ;) ) looks good to me.


adding hotspot-compiler alias.


Thanks,

-- Igor


On Jan 30, 2019, at 5:27 PM, Alex Menkov
   wrote:
  
  
  Hi all,
  
  
  Please review a fix for tck-red bug:
  
  https://bugs.openjdk.java.net/browse/JDK-8218025
  
  webrev:
  
http://cr.openjdk.java.net/~amenkov/tck_red_disable_caps/webrev/
  
  
  ForceEarlyReturn and PopFrame JCK tests intermittently fail
  with Graal.
  
  Real fix for the issue is too risky for jdk12, so we have to
  disable pop_frame and force_early_return capabilities running
  with Graal (the capabilities are optional).
  
  Currently Graal is the only compiler, so the fix checks if
  JVMCI compiler is enabled.
  
  JCK test passes with disabled capabilities.
  
  A number of hotspot tests do not check if the capabilities are
  enabled (as hotspot is expected to support all capabilities)
  and fail with Graal - they are problem-listed until the real
  problem (JDK-8195635) is resolved.
  
  
  --alex
  

  
  


  

Re: RFR (12) JDK-8218025: disable pop_frame and force_early_return caps for Graal

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

Hi Alex,

Vladimir I. also mentioned the same as Igor.

One example from the runtime/thread.cpp:
#if INCLUDE_JVMCI
  bool force_JVMCI_intialization = false;
  if (EnableJVMCI) {
    // Initialize JVMCI eagerly when it is explicitly requested.
    // Or when JVMCIPrintProperties is enabled.
    // The JVMCI Java initialization code will read this flag and
    // do the printing if it's set.
    force_JVMCI_intialization = EagerJVMCI || JVMCIPrintProperties;

    if (!force_JVMCI_intialization) {
  // 8145270: Force initialization of JVMCI runtime otherwise 
requests for blocking
  // compilations via JVMCI will not actually block until JVMCI is 
initialized.
  force_JVMCI_intialization = UseJVMCICompiler && (!UseInterpreter 
|| !BackgroundCompilation);

    }
  }
#endif

One more example from prims/jni.cpp:

#if INCLUDE_JVMCI
    if (EnableJVMCI) {
  if (UseJVMCICompiler) {
    . . .
  }
    }
#endif






On 1/30/19 17:34, Igor Ignatyev wrote:

Hi Alex,

UseJVMCICompiler is declared only if INCLUDE_JVMCI is defined, so your current 
patch will break builds where it's undefined. the rest (esp. problem list ;) ) 
looks good to me.

adding hotspot-compiler alias.

Thanks,
-- Igor


On Jan 30, 2019, at 5:27 PM, Alex Menkov  wrote:

Hi all,

Please review a fix for tck-red bug:
https://bugs.openjdk.java.net/browse/JDK-8218025
webrev:
http://cr.openjdk.java.net/~amenkov/tck_red_disable_caps/webrev/

ForceEarlyReturn and PopFrame JCK tests intermittently fail with Graal.
Real fix for the issue is too risky for jdk12, so we have to disable pop_frame 
and force_early_return capabilities running with Graal (the capabilities are 
optional).
Currently Graal is the only compiler, so the fix checks if JVMCI compiler is 
enabled.
JCK test passes with disabled capabilities.
A number of hotspot tests do not check if the capabilities are enabled (as 
hotspot is expected to support all capabilities) and fail with Graal - they are 
problem-listed until the real problem (JDK-8195635) is resolved.

--alex

Re: RFR: JDK-8217473: SA: Tests using ClhsdbLauncher fail on SAP docker containers

[Jini George]

Thank you, David!

- Jini.

On 1/31/2019 7:45 AM, David Holmes wrote:

Hi Jini,

This looks reasonable to me.

Thanks,
David

On 30/01/2019 9:59 pm, Jini George wrote:

Requesting reviews for:

BugID: https://bugs.openjdk.java.net/browse/JDK-8217473
Webrev:http://cr.openjdk.java.net/~jgeorge/8217473/webrev.01/index.html

The patch includes changes to use SkippedException to skip the tests 
when canPtraceAttachLinux() in Platform.java returns false.


Thanks,
Jini.

Re: RFR: JDK-8217473: SA: Tests using ClhsdbLauncher fail on SAP docker containers

[David Holmes]

Hi Jini,

This looks reasonable to me.

Thanks,
David

On 30/01/2019 9:59 pm, Jini George wrote:

Requesting reviews for:

BugID: https://bugs.openjdk.java.net/browse/JDK-8217473
Webrev:http://cr.openjdk.java.net/~jgeorge/8217473/webrev.01/index.html

The patch includes changes to use SkippedException to skip the tests 
when canPtraceAttachLinux() in Platform.java returns false.


Thanks,
Jini.

Re: RFR (12) JDK-8218025: disable pop_frame and force_early_return caps for Graal

[Igor Ignatyev]

Hi Alex,

UseJVMCICompiler is declared only if INCLUDE_JVMCI is defined, so your current 
patch will break builds where it's undefined. the rest (esp. problem list ;) ) 
looks good to me.

adding hotspot-compiler alias.

Thanks,
-- Igor

> On Jan 30, 2019, at 5:27 PM, Alex Menkov  wrote:
> 
> Hi all,
> 
> Please review a fix for tck-red bug:
> https://bugs.openjdk.java.net/browse/JDK-8218025
> webrev:
> http://cr.openjdk.java.net/~amenkov/tck_red_disable_caps/webrev/
> 
> ForceEarlyReturn and PopFrame JCK tests intermittently fail with Graal.
> Real fix for the issue is too risky for jdk12, so we have to disable 
> pop_frame and force_early_return capabilities running with Graal (the 
> capabilities are optional).
> Currently Graal is the only compiler, so the fix checks if JVMCI compiler is 
> enabled.
> JCK test passes with disabled capabilities.
> A number of hotspot tests do not check if the capabilities are enabled (as 
> hotspot is expected to support all capabilities) and fail with Graal - they 
> are problem-listed until the real problem (JDK-8195635) is resolved.
> 
> --alex

RFR (12) JDK-8218025: disable pop_frame and force_early_return caps for Graal

[Alex Menkov]

Hi all,

Please review a fix for tck-red bug:
https://bugs.openjdk.java.net/browse/JDK-8218025
webrev:
http://cr.openjdk.java.net/~amenkov/tck_red_disable_caps/webrev/

ForceEarlyReturn and PopFrame JCK tests intermittently fail with Graal.
Real fix for the issue is too risky for jdk12, so we have to disable 
pop_frame and force_early_return capabilities running with Graal (the 
capabilities are optional).
Currently Graal is the only compiler, so the fix checks if JVMCI 
compiler is enabled.

JCK test passes with disabled capabilities.
A number of hotspot tests do not check if the capabilities are enabled 
(as hotspot is expected to support all capabilities) and fail with Graal 
- they are problem-listed until the real problem (JDK-8195635) is resolved.


--alex

Re: [RFR]8215622: Add dump to file support for jmap histo

[David Holmes]

On 31/01/2019 10:41 am, Hohensee, Paul wrote:

Also, I suspect that this change needs a CSR, since we're adding functionality 
to jmap's histo switch. Opinions?


Yes. Changes to command-line flags need a CSR.

Thanks,
David


Thanks,

Paul

On 1/30/19, 9:33 AM, "serviceability-dev on behalf of Hohensee, Paul" 
 wrote:

 A nit in TestLoggerWeakRefLeak.java, separate the 2 arguments to 
heapHisto() by a space.
 
 vm.heapHisto("", "-live")
 
 Also, the header comment line for getInstanceCountFromHeapHisto() should be changed to
 
  * 'vm.heapHisto("", "-live")' will request a full GC
 
 In attachListener.cpp, the line
 
   out->print_cr("Invalid argument to dumpheap operation: %s", arg1);
 
 should be
 
   out->print_cr("Invalid argument to inspectheap operation: %s", arg1);
 
 Otherwise looks fine. The two failing tests (the other one was test/jdk/java/util/concurrent/locks/Lock/TimedAcquireLeak.java) now pass on my mac laptop.
 
 Paul
 
 On 1/30/19, 12:18 AM, "臧琳"  wrote:
 
 Dear All,

 This issue mentioned is that test case 
"java/util/logging/TestLoggerWeakRefLeak.java" Failed after applied the webrev.
 I have identified the root cause of the issue. it is caused by 2 
problems.
  1. The path processing in heap_inspection() at attachListener.cpp.
  The problem is that when use jmap -histo:live , the path is actually 
set to "" when it is transfer to socket. so it cause JNI_ERR.
  I need to modify the logic here that if path[0] == '\0' , 
fall through to the next argument parsing, rather than return JNI_ERR.
 
  2. Another problem is HotSpotVirtualMachine.java, it has a heapHisto() method, and the testcase use vm.heapHisto("-live"), And after the patch applied, it should be vm.heapHisto(""/*filepath*/, "-live"), seems we need to change the API for handling it.
 
  I have upload a webrev05:

  http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.05/
 
 May I ask your help for review?
 
 Thanks,

 Lin
 
 From: 臧琳
 Sent: Monday, January 28, 2019 5:49:42 PM
 To: Hohensee, Paul; JC Beyler
 Cc: serviceability-dev@openjdk.java.net
 Subject: Re: [RFR]8215622: Add dump to file support for jmap histo
 
 Dear all,

  I have found there is problem for handling the "filepath" and it cause test 
"java/util/logging/TestLoggerWeakRefLeak.java" fail, I have identified the root cause, 
please wait for the new update.
 Thanks!
 
 BRs,

 Lin
 
 From: 臧琳
 Sent: Friday, January 25, 2019 9:00:19 AM
 To: Hohensee, Paul; JC Beyler
 Cc: serviceability-dev@openjdk.java.net
 Subject: Re: [RFR]8215622: Add dump to file support for jmap histo
 
 Dear All,

 May I get more review about this webrev?
 http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.04/
 
 Thanks!

 BRs,
 Lin
 
 From: 臧琳
 Sent: Tuesday, January 22, 2019 2:18:06 PM
 To: Hohensee, Paul; JC Beyler
 Cc: serviceability-dev@openjdk.java.net
 Subject: Re: [RFR]8215622: Add dump to file support for jmap histo
 
 Hi Paul,

 Thanks a lot for your review. I have refined it based on your 
comments.
 http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.04/
 Would you like to help review it again? Thanks
 
 BRs,

 Lin
 
 From: Hohensee, Paul 
 Sent: Friday, January 18, 2019 6:11:14 AM
 To: 臧琳; JC Beyler
 Cc: serviceability-dev@openjdk.java.net
 Subject: Re: [RFR]8215622: Add dump to file support for jmap histo
 
 Just a few small things.
 
 In attachListener.cpp, line 278, is the static_cast needed? fileStream is a subclass of outputStream, so it should be ok to pass to the VM_GC_Heap_Inspection constructor, but maybe there's some C++ arcana I don't know about.
 
 In attachListener.cpp, line 275, change "Fail to" to "Failed to".
 
 In JMap.java, line 286, change  use \"all\""touse \"all\"."to match line 277.
 
 Thanks,
 
 Paul
 
 On 1/11/19, 1:39 AM, "臧琳"  wrote:
 
 Hi Jc, Paul and All,

  Here is webrev03 
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.03/
  would you like to help review?
 
 Thanks,

 Lin
 

Re: [RFR]8215622: Add dump to file support for jmap histo

[Hohensee, Paul]

Also, I suspect that this change needs a CSR, since we're adding functionality 
to jmap's histo switch. Opinions?

Thanks,

Paul

On 1/30/19, 9:33 AM, "serviceability-dev on behalf of Hohensee, Paul" 
 
wrote:

A nit in TestLoggerWeakRefLeak.java, separate the 2 arguments to 
heapHisto() by a space.

vm.heapHisto("", "-live")

Also, the header comment line for getInstanceCountFromHeapHisto() should be 
changed to

 * 'vm.heapHisto("", "-live")' will request a full GC

In attachListener.cpp, the line

  out->print_cr("Invalid argument to dumpheap operation: %s", arg1);

should be

  out->print_cr("Invalid argument to inspectheap operation: %s", arg1);

Otherwise looks fine. The two failing tests (the other one was 
test/jdk/java/util/concurrent/locks/Lock/TimedAcquireLeak.java) now pass on my 
mac laptop.

Paul

On 1/30/19, 12:18 AM, "臧琳"  wrote:

Dear All,
This issue mentioned is that test case 
"java/util/logging/TestLoggerWeakRefLeak.java" Failed after applied the webrev.
I have identified the root cause of the issue. it is caused by 2 
problems.
 1. The path processing in heap_inspection() at attachListener.cpp.
 The problem is that when use jmap -histo:live , the path is 
actually set to "" when it is transfer to socket. so it cause JNI_ERR.
 I need to modify the logic here that if path[0] == '\0' , fall 
through to the next argument parsing, rather than return JNI_ERR.

 2. Another problem is HotSpotVirtualMachine.java, it has a 
heapHisto() method, and the testcase use vm.heapHisto("-live"), And after the 
patch applied, it should be vm.heapHisto(""/*filepath*/, "-live"), seems we 
need to change the API for handling it.

 I have upload a webrev05:
 http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.05/

May I ask your help for review?

Thanks,
Lin

From: 臧琳
Sent: Monday, January 28, 2019 5:49:42 PM
To: Hohensee, Paul; JC Beyler
Cc: serviceability-dev@openjdk.java.net
Subject: Re: [RFR]8215622: Add dump to file support for jmap histo

Dear all,
 I have found there is problem for handling the "filepath" and it 
cause test "java/util/logging/TestLoggerWeakRefLeak.java" fail, I have 
identified the root cause, please wait for the new update.
Thanks!

BRs,
Lin

From: 臧琳
Sent: Friday, January 25, 2019 9:00:19 AM
To: Hohensee, Paul; JC Beyler
Cc: serviceability-dev@openjdk.java.net
Subject: Re: [RFR]8215622: Add dump to file support for jmap histo

Dear All,
May I get more review about this webrev?
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.04/

Thanks!
BRs,
Lin

From: 臧琳
Sent: Tuesday, January 22, 2019 2:18:06 PM
To: Hohensee, Paul; JC Beyler
Cc: serviceability-dev@openjdk.java.net
Subject: Re: [RFR]8215622: Add dump to file support for jmap histo

Hi Paul,
Thanks a lot for your review. I have refined it based on your 
comments.
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.04/
Would you like to help review it again? Thanks

BRs,
Lin

From: Hohensee, Paul 
Sent: Friday, January 18, 2019 6:11:14 AM
To: 臧琳; JC Beyler
Cc: serviceability-dev@openjdk.java.net
Subject: Re: [RFR]8215622: Add dump to file support for jmap histo

Just a few small things.

In attachListener.cpp, line 278, is the static_cast needed? fileStream 
is a subclass of outputStream, so it should be ok to pass to the 
VM_GC_Heap_Inspection constructor, but maybe there's some C++ arcana I don't 
know about.

In attachListener.cpp, line 275, change "Fail to" to "Failed to".

In JMap.java, line 286, change  use \"all\""touse \"all\"." 
   to match line 277.

Thanks,

Paul

On 1/11/19, 1:39 AM, "臧琳"  wrote:

Hi Jc, Paul and All,
 Here is webrev03 
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.03/
 would you like to help review?

Thanks,
Lin

From: 臧琳
Sent: Friday, January 11, 2019 10:25:27 AM
To: JC Beyler
Cc: Hohensee, Paul; serviceability-dev@openjdk.java.net
Subject: 答复: 

Re: RFR: JDK-8215550: Debugger does not work after reattach

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

Hi Gary,

It looks good to me.
Nice discovery!

Thanks,
Serguei


On 1/30/19 15:03, gary.ad...@oracle.com wrote:

Second reviewer or is it trivial enough for one?

On 1/30/19 1:57 PM, Chris Plummer wrote:

This looks good.

thanks,

Chris

On 1/30/19 8:24 AM, Gary Adams wrote:

Following the trail from debugLoop_run, at the bottom of the loop
there is a path through debugInit_reset that involves 
eventHandler_reset

and eventually eventHelper_reset. This seems like a better place to
clear the flag back to original state.

  Revised webrev: http://cr.openjdk.java.net/~gadams/8215550/webrev.01/

On 1/29/19, 6:11 PM, Chris Plummer wrote:
Ok, so you can't do a "cont" because threads are not suspended. 
That means someone resumed them. When/where was this done?


Regarding threadIDs changing, my guess is that debugLoop_run() is 
re-entered when the new connection is established. This will result 
in commonRef_reset() being called, which invalidates all reference 
IDs, including threadIDs. So the first time the agent needs to send 
a threadID to the debugger, it needs to create a new one.


Chris

On 1/29/19 1:52 PM, gary.ad...@oracle.com wrote:

Issuing a "cont" in the second session does not work, because
the threads are not suspended.

It's interesting that the thread ids have all changed in
the reconnected session.

...

main[1] threads
Group system:
  (java.lang.ref.Reference$ReferenceHandler)0x374 Reference 
Handler running
  (java.lang.ref.Finalizer$FinalizerThread)0x375 Finalizer cond. 
waiting
  (java.lang.Thread)0x376 Signal 
Dispatcher running

Group main:
  (java.lang.Thread)0x1 main  running (at breakpoint)
Group InnocuousThreadGroup:
  (jdk.internal.misc.InnocuousThread)0x377 Common-Cleaner cond. 
waiting

main[1] exit
...
> threads
Group system:
  (java.lang.ref.Reference$ReferenceHandler)0x3b2 Reference 
Handler running
  (java.lang.ref.Finalizer$FinalizerThread)0x3b3 Finalizer cond. 
waiting
  (java.lang.Thread)0x3b4 Signal 
Dispatcher running

Group main:
  (java.lang.Thread)0x3b7 main  running
Group InnocuousThreadGroup:
  (jdk.internal.misc.InnocuousThread)0x3b8 Common-Cleaner cond. 
waiting

> cont
> Nothing suspended.






On 1/29/19 2:27 PM, Chris Plummer wrote:
What is the state of the threads after the detach? If they are 
all automatically resumed by the agent, then I think the 
unblocking should be done by the same code that resumes the 
threads. If they are still suspended, then why would we want to 
unblock when the next connection comes in? It should be up to the 
debugger to detect that all threads are suspended and act 
accordingly.


Also, what happens if after attaching again you issue a "cont" 
command?


Chris

On 1/29/19 9:55 AM, Gary Adams wrote:
As far as I can tell, the quit and exit commands are only 
handled locally
on the debugger side of the connection. There is no packet sent 
to the

libjdwp agentlib.

On 1/29/19, 12:17 PM, Chris Plummer wrote:

Hi Gary,

When the "exit" or "quit" is done, aren't all threads resumed 
at that point, and shouldn't that result in the command loop 
being unblocked?


thanks,

Chris

On 1/29/19 8:09 AM, Gary Adams wrote:
Significant protections are put in place to protect the 
commandLoop

from multiple events that that have a suspend-all policy. The
commandLoop uses a special block variable to ensure only
a VirtualMachine or ThreadReference call to resume() will unblock
the commandLoop. See

src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c

In this particular bug report, the user was stopped at a 
breakpoint
when they sent the "exit" command. The same effect can be 
produced

with a "quit" command or any killing of the debugger process.

When the second session is started the commandLoop is still 
blocked,

so a new breakpoint will never be dequeued from the commandQueue.

The proposed workaround will ensure the commandLoop is unblocked
when a new session is started.

  Issue: https://bugs.openjdk.java.net/browse/JDK-8215550
  Webrev: http://cr.openjdk.java.net/~gadams/8215550/webrev.00/

All testing has been done by manually replicating the reported
command sequences. I'll see if an existing breakpoint test can be
enhanced to cover this scenario.

Re: RFR: JDK-8215550: Debugger does not work after reattach

[gary.ad...@oracle.com]

Second reviewer or is it trivial enough for one?

On 1/30/19 1:57 PM, Chris Plummer wrote:

This looks good.

thanks,

Chris

On 1/30/19 8:24 AM, Gary Adams wrote:

Following the trail from debugLoop_run, at the bottom of the loop
there is a path through debugInit_reset that involves eventHandler_reset
and eventually eventHelper_reset. This seems like a better place to
clear the flag back to original state.

  Revised webrev: http://cr.openjdk.java.net/~gadams/8215550/webrev.01/

On 1/29/19, 6:11 PM, Chris Plummer wrote:
Ok, so you can't do a "cont" because threads are not suspended. That 
means someone resumed them. When/where was this done?


Regarding threadIDs changing, my guess is that debugLoop_run() is 
re-entered when the new connection is established. This will result 
in commonRef_reset() being called, which invalidates all reference 
IDs, including threadIDs. So the first time the agent needs to send 
a threadID to the debugger, it needs to create a new one.


Chris

On 1/29/19 1:52 PM, gary.ad...@oracle.com wrote:

Issuing a "cont" in the second session does not work, because
the threads are not suspended.

It's interesting that the thread ids have all changed in
the reconnected session.

...

main[1] threads
Group system:
  (java.lang.ref.Reference$ReferenceHandler)0x374 Reference Handler 
running
  (java.lang.ref.Finalizer$FinalizerThread)0x375 Finalizer cond. 
waiting
  (java.lang.Thread)0x376 Signal Dispatcher 
running

Group main:
  (java.lang.Thread)0x1 main  running (at breakpoint)
Group InnocuousThreadGroup:
  (jdk.internal.misc.InnocuousThread)0x377 Common-Cleaner cond. 
waiting

main[1] exit
...
> threads
Group system:
  (java.lang.ref.Reference$ReferenceHandler)0x3b2 Reference Handler 
running
  (java.lang.ref.Finalizer$FinalizerThread)0x3b3 Finalizer cond. 
waiting
  (java.lang.Thread)0x3b4 Signal Dispatcher 
running

Group main:
  (java.lang.Thread)0x3b7 main  running
Group InnocuousThreadGroup:
  (jdk.internal.misc.InnocuousThread)0x3b8 Common-Cleaner cond. 
waiting

> cont
> Nothing suspended.






On 1/29/19 2:27 PM, Chris Plummer wrote:
What is the state of the threads after the detach? If they are all 
automatically resumed by the agent, then I think the unblocking 
should be done by the same code that resumes the threads. If they 
are still suspended, then why would we want to unblock when the 
next connection comes in? It should be up to the debugger to 
detect that all threads are suspended and act accordingly.


Also, what happens if after attaching again you issue a "cont" 
command?


Chris

On 1/29/19 9:55 AM, Gary Adams wrote:
As far as I can tell, the quit and exit commands are only handled 
locally
on the debugger side of the connection. There is no packet sent 
to the

libjdwp agentlib.

On 1/29/19, 12:17 PM, Chris Plummer wrote:

Hi Gary,

When the "exit" or "quit" is done, aren't all threads resumed at 
that point, and shouldn't that result in the command loop being 
unblocked?


thanks,

Chris

On 1/29/19 8:09 AM, Gary Adams wrote:
Significant protections are put in place to protect the 
commandLoop

from multiple events that that have a suspend-all policy. The
commandLoop uses a special block variable to ensure only
a VirtualMachine or ThreadReference call to resume() will unblock
the commandLoop. See

src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c

In this particular bug report, the user was stopped at a 
breakpoint

when they sent the "exit" command. The same effect can be produced
with a "quit" command or any killing of the debugger process.

When the second session is started the commandLoop is still 
blocked,

so a new breakpoint will never be dequeued from the commandQueue.

The proposed workaround will ensure the commandLoop is unblocked
when a new session is started.

  Issue: https://bugs.openjdk.java.net/browse/JDK-8215550
  Webrev: http://cr.openjdk.java.net/~gadams/8215550/webrev.00/

All testing has been done by manually replicating the reported
command sequences. I'll see if an existing breakpoint test can be
enhanced to cover this scenario.

Re: RFR: JDK-8215550: Debugger does not work after reattach

[Chris Plummer]

This looks good.

thanks,

Chris

On 1/30/19 8:24 AM, Gary Adams wrote:

Following the trail from debugLoop_run, at the bottom of the loop
there is a path through debugInit_reset that involves eventHandler_reset
and eventually eventHelper_reset. This seems like a better place to
clear the flag back to original state.

  Revised webrev: http://cr.openjdk.java.net/~gadams/8215550/webrev.01/

On 1/29/19, 6:11 PM, Chris Plummer wrote:
Ok, so you can't do a "cont" because threads are not suspended. That 
means someone resumed them. When/where was this done?


Regarding threadIDs changing, my guess is that debugLoop_run() is 
re-entered when the new connection is established. This will result 
in commonRef_reset() being called, which invalidates all reference 
IDs, including threadIDs. So the first time the agent needs to send a 
threadID to the debugger, it needs to create a new one.


Chris

On 1/29/19 1:52 PM, gary.ad...@oracle.com wrote:

Issuing a "cont" in the second session does not work, because
the threads are not suspended.

It's interesting that the thread ids have all changed in
the reconnected session.

...

main[1] threads
Group system:
  (java.lang.ref.Reference$ReferenceHandler)0x374 Reference Handler 
running
  (java.lang.ref.Finalizer$FinalizerThread)0x375 Finalizer cond. 
waiting
  (java.lang.Thread)0x376 Signal Dispatcher 
running

Group main:
  (java.lang.Thread)0x1 main  running (at breakpoint)
Group InnocuousThreadGroup:
  (jdk.internal.misc.InnocuousThread)0x377 Common-Cleaner cond. waiting
main[1] exit
...
> threads
Group system:
  (java.lang.ref.Reference$ReferenceHandler)0x3b2 Reference Handler 
running
  (java.lang.ref.Finalizer$FinalizerThread)0x3b3 Finalizer cond. 
waiting
  (java.lang.Thread)0x3b4 Signal Dispatcher 
running

Group main:
  (java.lang.Thread)0x3b7 main  running
Group InnocuousThreadGroup:
  (jdk.internal.misc.InnocuousThread)0x3b8 Common-Cleaner cond. waiting
> cont
> Nothing suspended.






On 1/29/19 2:27 PM, Chris Plummer wrote:
What is the state of the threads after the detach? If they are all 
automatically resumed by the agent, then I think the unblocking 
should be done by the same code that resumes the threads. If they 
are still suspended, then why would we want to unblock when the 
next connection comes in? It should be up to the debugger to detect 
that all threads are suspended and act accordingly.


Also, what happens if after attaching again you issue a "cont" 
command?


Chris

On 1/29/19 9:55 AM, Gary Adams wrote:
As far as I can tell, the quit and exit commands are only handled 
locally
on the debugger side of the connection. There is no packet sent to 
the

libjdwp agentlib.

On 1/29/19, 12:17 PM, Chris Plummer wrote:

Hi Gary,

When the "exit" or "quit" is done, aren't all threads resumed at 
that point, and shouldn't that result in the command loop being 
unblocked?


thanks,

Chris

On 1/29/19 8:09 AM, Gary Adams wrote:

Significant protections are put in place to protect the commandLoop
from multiple events that that have a suspend-all policy. The
commandLoop uses a special block variable to ensure only
a VirtualMachine or ThreadReference call to resume() will unblock
the commandLoop. See

src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c

In this particular bug report, the user was stopped at a breakpoint
when they sent the "exit" command. The same effect can be produced
with a "quit" command or any killing of the debugger process.

When the second session is started the commandLoop is still 
blocked,

so a new breakpoint will never be dequeued from the commandQueue.

The proposed workaround will ensure the commandLoop is unblocked
when a new session is started.

  Issue: https://bugs.openjdk.java.net/browse/JDK-8215550
  Webrev: http://cr.openjdk.java.net/~gadams/8215550/webrev.00/

All testing has been done by manually replicating the reported
command sequences. I'll see if an existing breakpoint test can be
enhanced to cover this scenario.

Re: [RFR]8215622: Add dump to file support for jmap histo

[Hohensee, Paul]

A nit in TestLoggerWeakRefLeak.java, separate the 2 arguments to heapHisto() by 
a space.

vm.heapHisto("", "-live")

Also, the header comment line for getInstanceCountFromHeapHisto() should be 
changed to

 * 'vm.heapHisto("", "-live")' will request a full GC

In attachListener.cpp, the line

  out->print_cr("Invalid argument to dumpheap operation: %s", arg1);

should be

  out->print_cr("Invalid argument to inspectheap operation: %s", arg1);

Otherwise looks fine. The two failing tests (the other one was 
test/jdk/java/util/concurrent/locks/Lock/TimedAcquireLeak.java) now pass on my 
mac laptop.

Paul

On 1/30/19, 12:18 AM, "臧琳"  wrote:

Dear All,
This issue mentioned is that test case 
"java/util/logging/TestLoggerWeakRefLeak.java" Failed after applied the webrev.
I have identified the root cause of the issue. it is caused by 2 
problems.
 1. The path processing in heap_inspection() at attachListener.cpp.
 The problem is that when use jmap -histo:live , the path is 
actually set to "" when it is transfer to socket. so it cause JNI_ERR.
 I need to modify the logic here that if path[0] == '\0' , fall 
through to the next argument parsing, rather than return JNI_ERR.

 2. Another problem is HotSpotVirtualMachine.java, it has a heapHisto() 
method, and the testcase use vm.heapHisto("-live"), And after the patch 
applied, it should be vm.heapHisto(""/*filepath*/, "-live"), seems we need to 
change the API for handling it.

 I have upload a webrev05:
 http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.05/

May I ask your help for review?

Thanks,
Lin

From: 臧琳
Sent: Monday, January 28, 2019 5:49:42 PM
To: Hohensee, Paul; JC Beyler
Cc: serviceability-dev@openjdk.java.net
Subject: Re: [RFR]8215622: Add dump to file support for jmap histo

Dear all,
 I have found there is problem for handling the "filepath" and it cause 
test "java/util/logging/TestLoggerWeakRefLeak.java" fail, I have identified the 
root cause, please wait for the new update.
Thanks!

BRs,
Lin

From: 臧琳
Sent: Friday, January 25, 2019 9:00:19 AM
To: Hohensee, Paul; JC Beyler
Cc: serviceability-dev@openjdk.java.net
Subject: Re: [RFR]8215622: Add dump to file support for jmap histo

Dear All,
May I get more review about this webrev?
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.04/

Thanks!
BRs,
Lin

From: 臧琳
Sent: Tuesday, January 22, 2019 2:18:06 PM
To: Hohensee, Paul; JC Beyler
Cc: serviceability-dev@openjdk.java.net
Subject: Re: [RFR]8215622: Add dump to file support for jmap histo

Hi Paul,
Thanks a lot for your review. I have refined it based on your comments.
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.04/
Would you like to help review it again? Thanks

BRs,
Lin

From: Hohensee, Paul 
Sent: Friday, January 18, 2019 6:11:14 AM
To: 臧琳; JC Beyler
Cc: serviceability-dev@openjdk.java.net
Subject: Re: [RFR]8215622: Add dump to file support for jmap histo

Just a few small things.

In attachListener.cpp, line 278, is the static_cast needed? fileStream is a 
subclass of outputStream, so it should be ok to pass to the 
VM_GC_Heap_Inspection constructor, but maybe there's some C++ arcana I don't 
know about.

In attachListener.cpp, line 275, change "Fail to" to "Failed to".

In JMap.java, line 286, change  use \"all\""touse \"all\"."
to match line 277.

Thanks,

Paul

On 1/11/19, 1:39 AM, "臧琳"  wrote:

Hi Jc, Paul and All,
 Here is webrev03 
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.03/
 would you like to help review?

Thanks,
Lin

From: 臧琳
Sent: Friday, January 11, 2019 10:25:27 AM
To: JC Beyler
Cc: Hohensee, Paul; serviceability-dev@openjdk.java.net
Subject: 答复: [RFR]8215622: Add dump to file support for jmap histo

Hi Jc,
 Thanks a lot. it is not a necessary change, I forget to omit it:)

BRs,
Lin

发件人: JC Beyler 
发送时间: 2019年1月11日 0:58:22
收件人: 臧琳
抄送: Hohensee, Paul; serviceability-dev@openjdk.java.net
主题: Re: [RFR]8215622: Add dump to file support for jmap histo

Hi Lin,

Small nit:

http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.02/src/java.base/share/native/libjli/java.c.udiff.html

needs a copyright update,
Jc


On Wed, 

Re: RFR: JDK-8215550: Debugger does not work after reattach

[Gary Adams]

Following the trail from debugLoop_run, at the bottom of the loop
there is a path through debugInit_reset that involves eventHandler_reset
and eventually eventHelper_reset. This seems like a better place to
clear the flag back to original state.

  Revised webrev: http://cr.openjdk.java.net/~gadams/8215550/webrev.01/

On 1/29/19, 6:11 PM, Chris Plummer wrote:
Ok, so you can't do a "cont" because threads are not suspended. That 
means someone resumed them. When/where was this done?


Regarding threadIDs changing, my guess is that debugLoop_run() is 
re-entered when the new connection is established. This will result in 
commonRef_reset() being called, which invalidates all reference IDs, 
including threadIDs. So the first time the agent needs to send a 
threadID to the debugger, it needs to create a new one.


Chris

On 1/29/19 1:52 PM, gary.ad...@oracle.com wrote:

Issuing a "cont" in the second session does not work, because
the threads are not suspended.

It's interesting that the thread ids have all changed in
the reconnected session.

...

main[1] threads
Group system:
  (java.lang.ref.Reference$ReferenceHandler)0x374 Reference Handler 
running

  (java.lang.ref.Finalizer$FinalizerThread)0x375 Finalizer cond. waiting
  (java.lang.Thread)0x376 Signal Dispatcher 
running

Group main:
  (java.lang.Thread)0x1 main  running (at breakpoint)
Group InnocuousThreadGroup:
  (jdk.internal.misc.InnocuousThread)0x377 Common-Cleanercond. 
waiting

main[1] exit
...
> threads
Group system:
  (java.lang.ref.Reference$ReferenceHandler)0x3b2 Reference Handler 
running

  (java.lang.ref.Finalizer$FinalizerThread)0x3b3 Finalizer cond. waiting
  (java.lang.Thread)0x3b4 Signal Dispatcher 
running

Group main:
  (java.lang.Thread)0x3b7 main  running
Group InnocuousThreadGroup:
  (jdk.internal.misc.InnocuousThread)0x3b8 Common-Cleanercond. 
waiting

> cont
> Nothing suspended.






On 1/29/19 2:27 PM, Chris Plummer wrote:
What is the state of the threads after the detach? If they are all 
automatically resumed by the agent, then I think the unblocking 
should be done by the same code that resumes the threads. If they 
are still suspended, then why would we want to unblock when the next 
connection comes in? It should be up to the debugger to detect that 
all threads are suspended and act accordingly.


Also, what happens if after attaching again you issue a "cont" command?

Chris

On 1/29/19 9:55 AM, Gary Adams wrote:
As far as I can tell, the quit and exit commands are only handled 
locally

on the debugger side of the connection. There is no packet sent to the
libjdwp agentlib.

On 1/29/19, 12:17 PM, Chris Plummer wrote:

Hi Gary,

When the "exit" or "quit" is done, aren't all threads resumed at 
that point, and shouldn't that result in the command loop being 
unblocked?


thanks,

Chris

On 1/29/19 8:09 AM, Gary Adams wrote:

Significant protections are put in place to protect the commandLoop
from multiple events that that have a suspend-all policy. The
commandLoop uses a special block variable to ensure only
a VirtualMachine or ThreadReference call to resume() will unblock
the commandLoop. See

  src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c

In this particular bug report, the user was stopped at a breakpoint
when they sent the "exit" command. The same effect can be produced
with a "quit" command or any killing of the debugger process.

When the second session is started the commandLoop is still blocked,
so a new breakpoint will never be dequeued from the commandQueue.

The proposed workaround will ensure the commandLoop is unblocked
when a new session is started.

  Issue: https://bugs.openjdk.java.net/browse/JDK-8215550
  Webrev: http://cr.openjdk.java.net/~gadams/8215550/webrev.00/

All testing has been done by manually replicating the reported
command sequences. I'll see if an existing breakpoint test can be
enhanced to cover this scenario.

RFR: JDK-8217473: SA: Tests using ClhsdbLauncher fail on SAP docker containers

[Jini George]

Requesting reviews for:

BugID: https://bugs.openjdk.java.net/browse/JDK-8217473
Webrev:http://cr.openjdk.java.net/~jgeorge/8217473/webrev.01/index.html

The patch includes changes to use SkippedException to skip the tests 
when canPtraceAttachLinux() in Platform.java returns false.


Thanks,
Jini.

Re: Is RegisterMap updated?

[Andrew Haley]

On 1/30/19 10:00 AM, David Griffiths wrote:
> I'm not sure this works though for the frame at the top of the stack.
> The pushing of the registers that you mention appears to take place in
> the updateRegisterMap method which is called when getting the sender.
> At the top of the stack the register will be live and not saved
> anywhere?

No the pushing of the register happens in generated code.

> Whatever, I'm seeing a NPE caused by a location that has
> WHERE_IN_REGISTER and a register number of 16 that doesn't have
> locationValid set in the map. If I modify createStackValue to check
> valueAddr being null then it avoids the NPE and getLocals then returns
> all the locals except for the one held in a register (the other ones
> are on the stack). Not sure what a register number of 16 equates to on
> x86, is that supposed to correspond to "not set" or something?

You're not helping.  Are you sure you've got an up-to-date HotSpot
checkout? You're not falling over bug 8217407?

-- 
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. 
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: Is RegisterMap updated?

[David Griffiths]

I'm not sure this works though for the frame at the top of the stack.
The pushing of the registers that you mention appears to take place in
the updateRegisterMap method which is called when getting the sender.
At the top of the stack the register will be live and not saved
anywhere?

Whatever, I'm seeing a NPE caused by a location that has
WHERE_IN_REGISTER and a register number of 16 that doesn't have
locationValid set in the map. If I modify createStackValue to check
valueAddr being null then it avoids the NPE and getLocals then returns
all the locals except for the one held in a register (the other ones
are on the stack). Not sure what a register number of 16 equates to on
x86, is that supposed to correspond to "not set" or something?

Cheers,

David

On Tue, 29 Jan 2019 at 13:41, Andrew Haley  wrote:
>
> On 1/29/19 11:24 AM, David Griffiths wrote:
> > Hi, in CompiledVFrame.createStackValue there is the following code:
> >
> >   // First find address of value
> >   Address valueAddr = loc.isRegister()
> > // Value was in a callee-save register
> > ? getRegisterMap().getLocation(new VMReg(loc.getRegisterNumber()))
> > // Else value was directly saved on the stack. The frame's
> > original stack pointer,
> > // before any extension by its callee (due to Compiler1
> > linkage on SPARC), must be used.
> > : ((Address)fr.getUnextendedSP()).addOffsetTo(loc.getStackOffset());
> >
> > It appears from what I can make out that for the register case the map
> > is not updated and so valueAddr is null. The only thing I can see that
> > calls setLocation is updateMapWithSavedLink. Seems like it should be
> > possible to return the register value for the frame at the top of the
> > stack though?
>
> It could be, but we don't do that. The registers in the map are those
> pushed onto the stack by generated Java code.
>
> --
> Andrew Haley
> Java Platform Lead Engineer
> Red Hat UK Ltd. 
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: [RFR]8215622: Add dump to file support for jmap histo

[臧琳]

Dear All,
This issue mentioned is that test case 
"java/util/logging/TestLoggerWeakRefLeak.java" Failed after applied the webrev.
I have identified the root cause of the issue. it is caused by 2 problems.
 1. The path processing in heap_inspection() at attachListener.cpp.
 The problem is that when use jmap -histo:live , the path is actually 
set to "" when it is transfer to socket. so it cause JNI_ERR.
 I need to modify the logic here that if path[0] == '\0' , fall through 
to the next argument parsing, rather than return JNI_ERR.

 2. Another problem is HotSpotVirtualMachine.java, it has a heapHisto() 
method, and the testcase use vm.heapHisto("-live"), And after the patch 
applied, it should be vm.heapHisto(""/*filepath*/, "-live"), seems we need to 
change the API for handling it.

 I have upload a webrev05:
 http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.05/

May I ask your help for review?

Thanks,
Lin

From: 臧琳
Sent: Monday, January 28, 2019 5:49:42 PM
To: Hohensee, Paul; JC Beyler
Cc: serviceability-dev@openjdk.java.net
Subject: Re: [RFR]8215622: Add dump to file support for jmap histo

Dear all,
 I have found there is problem for handling the "filepath" and it cause 
test "java/util/logging/TestLoggerWeakRefLeak.java" fail, I have identified the 
root cause, please wait for the new update.
Thanks!

BRs,
Lin

From: 臧琳
Sent: Friday, January 25, 2019 9:00:19 AM
To: Hohensee, Paul; JC Beyler
Cc: serviceability-dev@openjdk.java.net
Subject: Re: [RFR]8215622: Add dump to file support for jmap histo

Dear All,
May I get more review about this webrev?
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.04/

Thanks!
BRs,
Lin

From: 臧琳
Sent: Tuesday, January 22, 2019 2:18:06 PM
To: Hohensee, Paul; JC Beyler
Cc: serviceability-dev@openjdk.java.net
Subject: Re: [RFR]8215622: Add dump to file support for jmap histo

Hi Paul,
Thanks a lot for your review. I have refined it based on your comments.
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.04/
Would you like to help review it again? Thanks

BRs,
Lin

From: Hohensee, Paul 
Sent: Friday, January 18, 2019 6:11:14 AM
To: 臧琳; JC Beyler
Cc: serviceability-dev@openjdk.java.net
Subject: Re: [RFR]8215622: Add dump to file support for jmap histo

Just a few small things.

In attachListener.cpp, line 278, is the static_cast needed? fileStream is a 
subclass of outputStream, so it should be ok to pass to the 
VM_GC_Heap_Inspection constructor, but maybe there's some C++ arcana I don't 
know about.

In attachListener.cpp, line 275, change "Fail to" to "Failed to".

In JMap.java, line 286, change  use \"all\""touse \"all\"."to 
match line 277.

Thanks,

Paul

On 1/11/19, 1:39 AM, "臧琳"  wrote:

Hi Jc, Paul and All,
 Here is webrev03 
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.03/
 would you like to help review?

Thanks,
Lin

From: 臧琳
Sent: Friday, January 11, 2019 10:25:27 AM
To: JC Beyler
Cc: Hohensee, Paul; serviceability-dev@openjdk.java.net
Subject: 答复: [RFR]8215622: Add dump to file support for jmap histo

Hi Jc,
 Thanks a lot. it is not a necessary change, I forget to omit it:)

BRs,
Lin

发件人: JC Beyler 
发送时间: 2019年1月11日 0:58:22
收件人: 臧琳
抄送: Hohensee, Paul; serviceability-dev@openjdk.java.net
主题: Re: [RFR]8215622: Add dump to file support for jmap histo

Hi Lin,

Small nit:

http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.02/src/java.base/share/native/libjli/java.c.udiff.html

needs a copyright update,
Jc


On Wed, Jan 9, 2019 at 7:48 PM 臧琳 mailto:zangl...@jd.com>> 
wrote:
Dear All,
   I have updated the refined webrev at 
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.02/
   Would you like to help review? Thanks!

BRs,
Lin
From: 臧琳
Sent: Wednesday, January 9, 2019 11:00 AM
To: 'JC Beyler' mailto:jcbey...@google.com>>
Cc: Hohensee, Paul mailto:hohen...@amazon.com>>; 
serviceability-dev@openjdk.java.net
Subject: RE: [RFR]8215622: Add dump to file support for jmap histo

Dear JC,
   Thanks to point it out, I processed the “-file=” case in JMap.java 
but forgot to do it in attachListener.cpp. I will do it in next webrev.

Cheers,
Lin

From: JC Beyler mailto:jcbey...@google.com>>
Sent: Wednesday, January 9, 2019 10:51 AM
To: 臧琳 mailto:zangl...@jd.com>>
Cc: Hohensee, Paul mailto:hohen...@amazon.com>>; 
serviceability-dev@openjdk.java.net
Subject: Re: [RFR]8215622: Add dump to file support for jmap histo

Hi Lin,

Inlined as well