RE: [8u-backport] RFR: JDK-8164383 : jhsdb dumps core on Solaris 12 when loading dumped core

2018-09-27 Thread Fairoz Matte 
Hi Jini, thanks for the review. May I get one more review for this?

Thanks,
Fairoz

> -Original Message-
> From: Jini George
> Sent: Friday, September 28, 2018 10:37 AM
> To: Fairoz Matte ; serviceability-
> d...@openjdk.java.net
> Subject: Re: [8u-backport] RFR: JDK-8164383 : jhsdb dumps core on Solaris 12
> when loading dumped core
> 
> This looks good to me, Fairoz.
> 
> Thanks,
> Jini.
> 
> On 9/26/2018 1:37 PM, Fairoz Matte wrote:
> > Hi Jini,
> >
> > Thanks for pointing out that, yes we cannot make that cleanup for JDK8.
> > Keeping it very simple and taking only changes required to fix
> > JDK-8164383 http://cr.openjdk.java.net/~fmatte/8164383/webrev.02/
> >
> > I have verified running "test/serviceability/sa/jmap-
> hashcode/Test8028623.java" test case (found from one of the duplicate issue
> of JDK-8164383).
> > Results are as expected before and after the patch on Solaris 12 and Solaris
> 10.
> >
> > Along with that, I have verified with Internal testing and found no
> > issues
> >
> > Thanks,
> > Fairoz
> >
> >> -Original Message-
> >> From: Jini George
> >> Sent: Tuesday, September 25, 2018 3:48 PM
> >> To: Fairoz Matte ; serviceability-
> >> d...@openjdk.java.net
> >> Subject: Re: [8u-backport] RFR: JDK-8164383 : jhsdb dumps core on
> >> Solaris 12 when loading dumped core
> >>
> >> Hi Fairoz,
> >>
> >> I took a better look at the changes and I realized that the cleanup
> >> related to SOLARIS_11_B159_OR_LATER would be valid for only JDK9 and
> >> later. Since
> >> JDK8 is supported for Solaris 10 too, I believe that the cleanup
> >> related changes done as a part of JDK-8164383 should not be done for
> JDK-8.
> >>
> >> Thanks!
> >> Jini.
> >>
> >> On 9/24/2018 7:21 PM, Fairoz Matte wrote:
> >>> Hi Jini,
> >>>
>  -Original Message-
>  From: Jini George
>  Sent: Friday, September 21, 2018 4:07 PM
>  To: Fairoz Matte ; serviceability-
>  d...@openjdk.java.net
>  Subject: Re: [8u-backport] RFR: JDK-8164383 : jhsdb dumps core on
>  Solaris 12 when loading dumped core
> 
>  Hi Fairoz,
> 
>  This looks good to me. One nit which got missed out in the original
>  change also is that in saproc.cpp, the following comments
> 
>  452
>  453 // Pstack_iter() proc_stack_f callback prior to Nevada-B159
> 
>  476 // Pstack_iter() proc_stack_f callback in Nevada-B159 or later
>  477 /*ARGSUSED*/
> 
> >>>
> >>> I have incorporated above changes
> >>>
>  would not be required anymore. And we would not need the wrapper
> to
>  the callback routine fill_cframe_list() -- as in, we would need
>  only one routine with the appropriate arguments passed. But you are
>  free to ignore this since this was not done as a part of the original
> change.
> >>>
> >>> Removed wrapper_fill_cframe_list function and fill_cframe_list
> >>> function
> >> has been used directly.
> >>>
> >>> Please find the updated webrev
> >>> http://cr.openjdk.java.net/~fmatte/8164383/webrev.01/
> >>>
> >>> Thanks,
> >>> Fairoz
> >>>
> 
>  Thanks,
>  Jini (Not a Reviewer).
> 
> 
> 
>  On 9/20/2018 7:06 PM, Fairoz Matte wrote:
> > Hi,
> >
> > Kindly review the backport of "JDK-8164383 : jhsdb dumps core on
> > Solaris 12 when loading dumped core" to 8u
> >
> > Webrev - http://cr.openjdk.java.net/~fmatte/8164383/webrev.00/
> >
> > JBS bug - https://bugs.openjdk.java.net/browse/JDK-8164383
> >
> > JDK9 changeset -
> > http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/ce3eaa22b582
> >
> > JDK9 review thread -
> > http://mail.openjdk.java.net/pipermail/serviceability-dev/2016-Oct
> > ob
> > er
> > /020543.html
> >
> > Thanks,
> > Fairoz
> >

Re: [8u-backport] RFR: JDK-8164383 : jhsdb dumps core on Solaris 12 when loading dumped core

2018-09-27 Thread Jini George 

This looks good to me, Fairoz.

Thanks,
Jini.

On 9/26/2018 1:37 PM, Fairoz Matte wrote:

Hi Jini,

Thanks for pointing out that, yes we cannot make that cleanup for JDK8.
Keeping it very simple and taking only changes required to fix JDK-8164383
http://cr.openjdk.java.net/~fmatte/8164383/webrev.02/

I have verified running "test/serviceability/sa/jmap-hashcode/Test8028623.java" 
test case (found from one of the duplicate issue of JDK-8164383).
Results are as expected before and after the patch on Solaris 12 and Solaris 10.

Along with that, I have verified with Internal testing and found no issues

Thanks,
Fairoz


-Original Message-
From: Jini George
Sent: Tuesday, September 25, 2018 3:48 PM
To: Fairoz Matte ; serviceability-
d...@openjdk.java.net
Subject: Re: [8u-backport] RFR: JDK-8164383 : jhsdb dumps core on Solaris 12
when loading dumped core

Hi Fairoz,

I took a better look at the changes and I realized that the cleanup related to
SOLARIS_11_B159_OR_LATER would be valid for only JDK9 and later. Since
JDK8 is supported for Solaris 10 too, I believe that the cleanup related
changes done as a part of JDK-8164383 should not be done for JDK-8.

Thanks!
Jini.

On 9/24/2018 7:21 PM, Fairoz Matte wrote:

Hi Jini,


-Original Message-
From: Jini George
Sent: Friday, September 21, 2018 4:07 PM
To: Fairoz Matte ; serviceability-
d...@openjdk.java.net
Subject: Re: [8u-backport] RFR: JDK-8164383 : jhsdb dumps core on
Solaris 12 when loading dumped core

Hi Fairoz,

This looks good to me. One nit which got missed out in the original
change also is that in saproc.cpp, the following comments

452
453 // Pstack_iter() proc_stack_f callback prior to Nevada-B159

476 // Pstack_iter() proc_stack_f callback in Nevada-B159 or later
477 /*ARGSUSED*/



I have incorporated above changes


would not be required anymore. And we would not need the wrapper to
the callback routine fill_cframe_list() -- as in, we would need only
one routine with the appropriate arguments passed. But you are free
to ignore this since this was not done as a part of the original change.


Removed wrapper_fill_cframe_list function and fill_cframe_list function

has been used directly.


Please find the updated webrev
http://cr.openjdk.java.net/~fmatte/8164383/webrev.01/

Thanks,
Fairoz



Thanks,
Jini (Not a Reviewer).



On 9/20/2018 7:06 PM, Fairoz Matte wrote:

Hi,

Kindly review the backport of "JDK-8164383 : jhsdb dumps core on
Solaris 12 when loading dumped core" to 8u

Webrev - http://cr.openjdk.java.net/~fmatte/8164383/webrev.00/

JBS bug - https://bugs.openjdk.java.net/browse/JDK-8164383

JDK9 changeset -
http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/ce3eaa22b582

JDK9 review thread -
http://mail.openjdk.java.net/pipermail/serviceability-dev/2016-Octob
er
/020543.html

Thanks,
Fairoz

Re: RFR (S) 8210842: Handle JNIGlobalRefLocker.cpp

2018-09-27 Thread Martin Buchholz 
Some of us have lobbied to make openjdk source C++11, but it's not yet.

If you're brave, you can try to change that flag to -std=gnu++11

On Thu, Sep 27, 2018 at 2:33 PM, JC Beyler  wrote:
> Hi all,
>
> Sorry to come back to this so late in the game, but somehow when I synced my
> hg clone (or the issue was always there and it is a flaky build), it seems
> that something in the build might have changed? Basically now it seems that
> the build is adding flags that makes my usage of unique_ptr fail.
>
> I "think" it is due to the build adding the gnu++98 standard (But this has
> been there for a while it seems so most likely a side-effect is it is being
> now used):
>
> CXXSTD_CXXFLAG="-std=gnu++98"
> FLAGS_CXX_COMPILER_CHECK_ARGUMENTS(ARGUMENT: [$CXXSTD_CXXFLAG -Werror],
> IF_FALSE: [CXXSTD_CXXFLAG=""])
>
> (from:
> https://hg.openjdk.java.net/jdk/jdk/file/dade6dd87bb4/make/autoconf/flags-cflags.m4)
> but I'm not sure.
>
> When I remove that flag, my g++ goes to a more recent standard and
> unique_ptr works.
>
> So I now have to ask you all:
>   1) Should we try to fix the build system to at least have C++11 for the
> C++ tests, then my webrev.04 can stay as is but has to wait for that to
> happen
>   2) Should we push a new version that does not use unique_ptr? That
> solution would look like the following webrev:
>  http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.05/
>
> Sorry for the last minute rug pull,
> Jc
>
> On Thu, Sep 27, 2018 at 11:32 AM Mikael Vidstedt
>  wrote:
>>
>>
>> Very, very nice! Thanks for adding the comment and renaming the class!
>> Ship it!
>>
>> Cheers,
>> Mikael
>>
>>
>> On Sep 27, 2018, at 10:45 AM, JC Beyler  wrote:
>>
>> Hi Mikael and David,
>>
>> @David: I thought it was implicit but did not want to presume on this one
>> because my goal is to start propagating this new class in the test base and
>> get the checks to be done implicitly so I was probably being over-cautious
>> @Mikael: done and done, what do you think of the comment here :
>> http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.04/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.hpp.html
>>
>> For all, the new webrev is here:
>> Webrev: http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.04/
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8210842
>>
>> Thanks,
>> Jc
>>
>> On Thu, Sep 27, 2018 at 6:03 AM David Holmes 
>> wrote:
>>>
>>> Sorry Jc, I thought my LGTM was implicit. :)
>>>
>>> Thanks,
>>> David
>>>
>>> On 26/09/2018 11:52 PM, JC Beyler wrote:
>>> > Ping on this, anybody have time to do a review and give a LGTM? Or
>>> > David
>>> > if you have time and will to provide an explicit LGTM :)
>>> >
>>> > Thanks,
>>> > Jc
>>> >
>>> > On Mon, Sep 24, 2018 at 9:18 AM JC Beyler >> > > wrote:
>>> >
>>> > Hi David,
>>> >
>>> > Sounds good to me, Alex gave me one LGTM, so it seems I'm waiting
>>> > for an explicit LGTM from you or from someone else on this list to
>>> > do a review.
>>> >
>>> > Thanks again for your help,
>>> > Jc
>>> >
>>> > On Sun, Sep 23, 2018 at 9:22 AM David Holmes
>>> > mailto:david.hol...@oracle.com>> wrote:
>>> >
>>> > Hi Jc,
>>> >
>>> > I don't think it is an issue for this to go ahead. If the
>>> > static
>>> > analysis tool has an issue then we can either find out how to
>>> > teach it
>>> > about this code structure, or else flag the issues as false
>>> > positives.
>>> > I'd be relying on one of the serviceability team here to handle
>>> > that if
>>> > the problem arises.
>>> >
>>> > Thanks,
>>> > David
>>> >
>>> > On 23/09/2018 12:17 PM, JC Beyler wrote:
>>> >  > Hi David,
>>> >  >
>>> >  > No worries with the time to answer; I appreciate the review!
>>> >  >
>>> >  > That's a fair point. Is it possible to "describe" to the
>>> > static analysis
>>> >  > tool to "trust" calls made in the SafeJNIEnv class?
>>> >  >
>>> >  > Otherwise, do you have any idea on how to go forward? For
>>> > what it's
>>> >  > worth, this current webrev does not try to replace exception
>>> > checking
>>> >  > with the SafeJNIEnv (it actually adds it), so for now the
>>> >  > question/solution could be delayed. I could continue working
>>> > on this in
>>> >  > the scope of both the nsk/gc/lock/jniref code base and the
>>> > NSK_VERIFIER
>>> >  > macro removal and we can look at how to tackle the cases
>>> > where the tests
>>> >  > are actually calling exception checking (I know my
>>> > heapmonitor does for
>>> >  > example).
>>> >  >
>>> >  > Let me know what you think and thanks for the review,
>>> >  > Jc
>>> >  >
>>> >  >
>>> >  > On Sun, Sep 23, 2018 at 6:48 AM David Holmes

RFR JDK-8203928: [Test] Convert non-JDB scaffolding serviceability shell script tests to java

2018-09-27 Thread Alex Menkov 

Hi all,

please review a fix for
https://bugs.openjdk.java.net/browse/JDK-8203928
webrev:
http://cr.openjdk.java.net/~amenkov/sh2java/non-jdb/webrev.01/

Some details:

ImmutableResourceTest.java
  - required compile/run args are specified by using jtreg tag options;

JITDebug.java
  - was not able to reproduce failures described (looks like that it's 
something ancient). Actually we use dt_socket transport on Windows for a 
long time without any issues;
  - replaced Runtime.exec with ProcessBuilder, used some /test/lib 
stuff, fixed some minor issues;


connect/spi/JdiLoadedByCustomLoader.java
redefine/RedefineTest.java
redefineMethod/RedefineTest.java
  - implemented compilation tasks in java, removed shell files;

PrivateTransportTest.java (was test/jdk/com/sun/jdi/PrivateTransportTest.sh)
  - just converted the test from shell to java.

--alex

Re: RFR: JDK-8208473: [TESTBUG] nsk/jdb/exclude/exclude001/exclude001.java is timing out on solaris-sparc again

2018-09-27 Thread Chris Plummer 
The extra check after timing out doesn't seem like it should help. 
You've already called findMessage() 2100 times at 200ms intervals. Why 
would one more call after that help? I think it might be the 
receiveReply() call that is fixing it. It does a waitForPrompt(), so 
this probably gives us another 42 ms for the prompt to come in. This 
call to receiveReply() is actually a bug itself since we are doing it 
just to print the current buffer, not the buffer after waiting for a 
prompt to come in.


In any case, looks like this prompt is taking more than 420200 
milliseconds to come in, but does eventually come in, and extra waiting 
in receiveReply() is what is causing you to eventually see the prompt. I 
think bumping up the timeout to 600 and the waittime to 10 is the proper 
fix here.


And to address the receiveReply() issue, I'd suggest calling it using 
receiveReply(startPos, false, 0), where 0 is the prompt count, and have 
receiveReply() not wait for a prompt when the count is 0.


Chris

On 9/27/18 11:44 AM, Gary Adams wrote:

Speaking of not being bullet proof, during testing of the fix to
wait for a specific prompt an intermittent failure was observed.
...

Sending command: trace methods 0x2a9
reply[0]: MyThread-0[1]
Sending command: cont
WARNING: message not recieved: MyThread-0[1]
Remaining debugger output follows:
reply[0]:>
reply[1]: Method exited: return value =, 
"thread=MyThread-0", nsk.jdb.exclude.exclude001.MyThread.run(), 
line=93 bci=14

reply[2]: 93    }
reply[3]:
reply[4]: MyThread-0[1]
# ERROR: Caught unexpected exception while executing the test: 
nsk.share.Failure: Expected message not received during 420200 
milliseconds:

...

The wait for message times out looking for "MyThread-0[1]".
A WARNING is printed and the "remaining debugger output"
shows that "MyThread-0[1]" is in the buffer.

I'm still investigating why the message match is not found.

Adding a final check before failing the wait for message
seems to workaround the problem.

diff --git a/test/hotspot/jtreg/vmTestbase/nsk/share/jdb/Jdb.java 
b/test/hotspot/jtreg/vmTestbase/nsk/share/jdb/Jdb.java

--- a/test/hotspot/jtreg/vmTestbase/nsk/share/jdb/Jdb.java
+++ b/test/hotspot/jtreg/vmTestbase/nsk/share/jdb/Jdb.java
@@ -515,10 +515,11 @@
 long delta = 200; // time in milliseconds to wait at every 
iteration.

 long total = 0;    // total time has waited.
 long max = 
getLauncher().getJdbArgumentHandler().getWaitTime() * 60 * 1000; // 
maximum time to wait.

+    int found = 0;

 Object dummy = new Object();
 while ((total += delta) <= max) {
-    int found = 0;
+    found = 0;

 // search for message
 {
@@ -553,6 +554,12 @@
 log.display("WARNING: message not recieved: " + message);
 log.display("Remaining debugger output follows:");
 receiveReply(startPos);
+
+    // One last chance
+    found = findMessage(startPos, message);
+    if (found > 0) {
+    return found;
+    }
 throw new Failure("Expected message not received during " + 
total + " milliseconds:"

 + "\n\t" + message);
 }


On 9/20/18, 5:47 PM, Chris Plummer wrote:
Looks good. Still not bullet proof, but I'm not sure it's possible to 
write tests like this in a way that will work no matter what output 
is produced by the method enter/exit events.


Chris

On 9/20/18 10:59 AM, Gary Adams wrote:

The test failure has been identified due to the "int[2]"
being misrecognized as a compound prompt.  This caused a cont
command to be sent prematurely.

The proposed fix waits for the correct prompt before
advancing to the next command.

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

Testing is in progress.

Re: RFR 8163083: SocketListeningConnector does not allow invocations with port 0

2018-09-27 Thread serguei . spitsyn 

It looks good to me.

Thanks!
Serguei

On 9/27/18 11:50 AM, Daniil Titov wrote:


Hi Serguei,

The webrev was updated to address all these comments.

Webrev: http://cr.openjdk.java.net/~dtitov/8163083/webrev.08 



Bug: https://bugs.openjdk.java.net/browse/JDK-8163083

Thanks!

--Daniil

*From: *
*Organization: *Oracle Corporation
*Date: *Thursday, September 27, 2018 at 11:33 AM
*To: *Daniil Titov , JC Beyler 

*Cc: *, , 

*Subject: *Re: RFR 8163083: SocketListeningConnector does not allow 
invocations with port 0


Just noticed one more minor issue:

+import java.util.Collection;
+import java.util.Iterator;
+import java.util.LinkedHashMap;


 It seems the above imports are not really used and can be removed.

Thanks,
Serguei

On 9/27/18 11:22 AM, serguei.spit...@oracle.com 
 wrote:


Hi Daniil,

It looks great, thank you for the update.
I have a couple of more minor comments on the test.

   56 testWithDefaultArgs(connector);

   57 testWithDefaultArgs2(connector);

   58 testWithWildcardPort1(connector);

   59 testWithWildcardPort2(connector);


  Please, rename testWithDefaultArgs to testWithDefaultArgs1
  to make naming consistent.

  111 throw new RuntimeException("Test testWithDefaultArgsNegative 
failed. The argument map was not updated with" +

  112 " the bound port number.");

  115 // This call should fail since the previous the argument 
map is already updated with the port

  116 // number of the started listener


  Could you, please, re-balance the above line pairs to make the
first line shorter?

No need in new webrev if you fix the above.

Thanks,
Serguei

On 9/27/18 11:00 AM, Daniil Titov wrote:

Hi Serguei,

Thank you for reviewing this change. Initially I understood
the whole fragment below  (from the Javadoc for
com.sun.jdi.connect.ListeningConnector.startListening()
method) as a direction for the user how to obtain and prepare
the argument map before passing it in startListening() method.

   61  * The argument map associates argument name strings to 
instances

   62  * of {@link Connector.Argument}. The default argument map 
for a

   63  * connector can be obtained through {@link 
Connector#defaultArguments}.

   64  * Argument map values can be changed, but map entries should 
not be

   65  * added or deleted.

But I agree that the last sentence could also mean that the
argument map values could be changes as a result of the method
invocation and in this case the new fragment in the Javadoc is
not needed.

Please review the updated webrev that does not add new Javadoc
fragment and includes other changes you suggested.


Webrev: http://cr.openjdk.java.net/~dtitov/8163083/webrev.07/


Bug: https://bugs.openjdk.java.net/browse/JDK-8163083

Thanks!

--Daniil

*From: *

*Organization: *Oracle Corporation
*Date: *Wednesday, September 26, 2018 at 8:12 PM
*To: *Daniil Titov 
, JC Beyler
 
*Cc: *
, 
,


*Subject: *Re: RFR 8163083: SocketListeningConnector does not
allow invocations with port 0

Hi Daniil,

It is great that you came up the fix for this issue.
Thanks to Gary for the help too.

I wonder if we could get away without updating the javadoc
of com/sun/jdi/connect/ListeningConnector.java.
Filing a CSR would not be needed then (simple javadoc
reformatting should not require a CSR).

So, my question is if this new fragment is really needed:

  76  * 

  77  * If the addressing information provided in {@code
arguments} implies

  78  * the auto detection this information might be
updated with the address

  79  * of the started listener.

The javadoc for startListening already has this fragment:

   61  * The argument map associates argument name strings to 
instances

   62  * of {@link Connector.Argument}. The default argument map 
for a

   63  * connector can be obtained through {@link 
Connector#defaultArguments}.

   64  * Argument map values can be changed, but map entries should 
not be

   65  * added or deleted.

  


that allows to change the argument

Re: RFR 8163083: SocketListeningConnector does not allow invocations with port 0

2018-09-27 Thread Daniil Titov 
Hi Serguei,

 

The webrev was updated to address all these comments.

 

Webrev: http://cr.openjdk.java.net/~dtitov/8163083/webrev.08

Bug: https://bugs.openjdk.java.net/browse/JDK-8163083 

 

Thanks!

--Daniil

 

 

 

From: 
Organization: Oracle Corporation
Date: Thursday, September 27, 2018 at 11:33 AM
To: Daniil Titov , JC Beyler 
Cc: , , 

Subject: Re: RFR 8163083: SocketListeningConnector does not allow invocations 
with port 0

 

Just noticed one more minor issue:
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.LinkedHashMap;

 It seems the above imports are not really used and can be removed.

Thanks,
Serguei

On 9/27/18 11:22 AM, serguei.spit...@oracle.com wrote:

Hi Daniil,

It looks great, thank you for the update.
I have a couple of more minor comments on the test.
  56 testWithDefaultArgs(connector);
  57 testWithDefaultArgs2(connector);
  58 testWithWildcardPort1(connector);
  59 testWithWildcardPort2(connector);

  Please, rename testWithDefaultArgs to testWithDefaultArgs1
  to make naming consistent.
 111 throw new RuntimeException("Test testWithDefaultArgsNegative 
failed. The argument map was not updated with" +
 112 " the bound port number.");
 
 115 // This call should fail since the previous the argument map 
is already updated with the port
 116 // number of the started listener

  Could you, please, re-balance the above line pairs to make the first line 
shorter?

No need in new webrev if you fix the above.

Thanks,
Serguei

On 9/27/18 11:00 AM, Daniil Titov wrote:

Hi Serguei,

 

Thank you for reviewing this change. Initially I understood the whole fragment 
below  (from the Javadoc for 
com.sun.jdi.connect.ListeningConnector.startListening() method) as a direction 
for the user how to obtain and prepare the argument map before passing it in 
startListening() method.

 
  61  * The argument map associates argument name strings to instances
  62  * of {@link Connector.Argument}. The default argument map for a
  63  * connector can be obtained through {@link 
Connector#defaultArguments}.
  64  * Argument map values can be changed, but map entries should not be
  65  * added or deleted.
 

But I agree that the last sentence could also mean that the argument map values 
could be changes as a result of the method invocation and in this case the new 
fragment in the Javadoc is not needed.

 

Please review the updated webrev that does not add new Javadoc fragment and 
includes other changes you suggested.


Webrev: http://cr.openjdk.java.net/~dtitov/8163083/webrev.07/

Bug: https://bugs.openjdk.java.net/browse/JDK-8163083 

 

Thanks!

--Daniil

 

From: 
Organization: Oracle Corporation
Date: Wednesday, September 26, 2018 at 8:12 PM
To: Daniil Titov , JC Beyler 
Cc: , , 

Subject: Re: RFR 8163083: SocketListeningConnector does not allow invocations 
with port 0

 

Hi Daniil,

It is great that you came up the fix for this issue.
Thanks to Gary for the help too.

I wonder if we could get away without updating the javadoc
of com/sun/jdi/connect/ListeningConnector.java.
Filing a CSR would not be needed then (simple javadoc reformatting should not 
require a CSR).

So, my question is if this new fragment is really needed:
  76  * 
  77  * If the addressing information provided in {@code arguments} implies
  78  * the auto detection this information might be updated with the 
address
  79  * of the started listener.
The javadoc for startListening already has this fragment:
  61  * The argument map associates argument name strings to instances
  62  * of {@link Connector.Argument}. The default argument map for a
  63  * connector can be obtained through {@link 
Connector#defaultArguments}.
  64  * Argument map values can be changed, but map entries should not be
  65  * added or deleted.
 
that allows to change the argument map values.
It looks like, it has to be Okay to add the bound port number there.
 
Please, let me know what you think.
Some formatting comments to addition to Jc's comments:
  77  * If the addressing information provided in {@code arguments} implies
  78  * the auto detection this information might be updated with the 
address
  79  * of the started listener.
 
  This sentence needs to be split in two:
 
  77  * If the addressing information provided in {@code arguments} implies
  78  * the auto detection. This information might be updated with the 
address
  79  * of the started listener.
+
+protected void updateArgumentMapIfRequired(
+Map 
args,TransportService.ListenKey listener) {
+}
+
  The indent has to be 4, not 8.
 
+if(isWildcardPort(args)) {
+String[] address = listener.address().split(":");
+if (address.length > 1) {
+args.get(ARG_PORT).setValue(address[1]);
+}
+}
 
  A space is

Re: RFR: JDK-8208473: [TESTBUG] nsk/jdb/exclude/exclude001/exclude001.java is timing out on solaris-sparc again

2018-09-27 Thread Gary Adams 

Speaking of not being bullet proof, during testing of the fix to
wait for a specific prompt an intermittent failure was observed.
...

Sending command: trace methods 0x2a9
reply[0]: MyThread-0[1]
Sending command: cont
WARNING: message not recieved: MyThread-0[1]
Remaining debugger output follows:
reply[0]:>
reply[1]: Method exited: return value =, "thread=MyThread-0", 
nsk.jdb.exclude.exclude001.MyThread.run(), line=93 bci=14
reply[2]: 93}
reply[3]:
reply[4]: MyThread-0[1]
# ERROR: Caught unexpected exception while executing the test: 
nsk.share.Failure: Expected message not received during 420200 milliseconds:
...

The wait for message times out looking for "MyThread-0[1]".
A WARNING is printed and the "remaining debugger output"
shows that "MyThread-0[1]" is in the buffer.

I'm still investigating why the message match is not found.

Adding a final check before failing the wait for message
seems to workaround the problem.

diff --git a/test/hotspot/jtreg/vmTestbase/nsk/share/jdb/Jdb.java 
b/test/hotspot/jtreg/vmTestbase/nsk/share/jdb/Jdb.java

--- a/test/hotspot/jtreg/vmTestbase/nsk/share/jdb/Jdb.java
+++ b/test/hotspot/jtreg/vmTestbase/nsk/share/jdb/Jdb.java
@@ -515,10 +515,11 @@
 long delta = 200; // time in milliseconds to wait at every 
iteration.

 long total = 0;// total time has waited.
 long max = getLauncher().getJdbArgumentHandler().getWaitTime() 
* 60 * 1000;  // maximum time to wait.

+int found = 0;

 Object dummy = new Object();
 while ((total += delta) <= max) {
-int found = 0;
+found = 0;

 // search for message
 {
@@ -553,6 +554,12 @@
 log.display("WARNING: message not recieved: " + message);
 log.display("Remaining debugger output follows:");
 receiveReply(startPos);
+
+// One last chance
+found = findMessage(startPos, message);
+if (found > 0) {
+return found;
+}
 throw new Failure("Expected message not received during " + 
total + " milliseconds:"

 + "\n\t" + message);
 }


On 9/20/18, 5:47 PM, Chris Plummer wrote:
Looks good. Still not bullet proof, but I'm not sure it's possible to 
write tests like this in a way that will work no matter what output is 
produced by the method enter/exit events.


Chris

On 9/20/18 10:59 AM, Gary Adams wrote:

The test failure has been identified due to the "int[2]"
being misrecognized as a compound prompt.  This caused a cont
command to be sent prematurely.

The proposed fix waits for the correct prompt before
advancing to the next command.

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

Testing is in progress.

Re: JDK-8203350: Crash in vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/TestDescription.java

2018-09-27 Thread Chris Plummer 

Hi Gary,

Aren't you just hiding a potential jvmti bug with this change? If you 
think this is a test bug and this is a proper fix, I'd like to see an 
explanation of how the test is causing the crash. The explanation would 
need to involve native code, since pure java should never crash.


thanks,

Chris

On 9/27/18 5:18 AM, Gary Adams wrote:

I've been unsuccessful trying to reproduce the failure in hs201t002.

  Issue: https://bugs.openjdk.java.net/browse/JDK-8203350

Colleen made a comment on the bug that the reference
from hs201t002a to class hs201t002 might be an issue
for the redefined class.

I found in test hs201t001 that an intentional reference
before entering hs201t001a.doInit() is made to avoid
that classloader operation.

It's not clear to me why that was done, but the same workaround
could be used in hs201t002a, if it would make the test more robust.


diff --git 
a/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/newclass/hs201t002a.java 
b/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/newclass/hs201t002a.java 

--- 
a/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/newclass/hs201t002a.java
+++ 
b/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/newclass/hs201t002a.java

@@ -26,6 +26,7 @@
 public class hs201t002a extends Exception {

 public hs201t002a () {
+    System.out.println("Current step: " + hs201t002.currentStep); 
// Avoid calling classloader to find hs201t002 in doInit()

 doInit();
 }

Re: RFR 8163083: SocketListeningConnector does not allow invocations with port 0

2018-09-27 Thread serguei . spitsyn 

Just noticed one more minor issue:

+import java.util.Collection;
+import java.util.Iterator;
+import java.util.LinkedHashMap;


 It seems the above imports are not really used and can be removed.

Thanks,
Serguei

On 9/27/18 11:22 AM, serguei.spit...@oracle.com wrote:

Hi Daniil,

It looks great, thank you for the update.
I have a couple of more minor comments on the test.

   56 testWithDefaultArgs(connector);
   57 testWithDefaultArgs2(connector);
   58 testWithWildcardPort1(connector);
   59 testWithWildcardPort2(connector);

  Please, rename testWithDefaultArgs to testWithDefaultArgs1
  to make naming consistent.

  111 throw new RuntimeException("Test testWithDefaultArgsNegative 
failed. The argument map was not updated with" +
  112 " the bound port number.");

  115 // This call should fail since the previous the argument map 
is already updated with the port
  116 // number of the started listener

  Could you, please, re-balance the above line pairs to make the first 
line shorter?


No need in new webrev if you fix the above.

Thanks,
Serguei

On 9/27/18 11:00 AM, Daniil Titov wrote:


Hi Serguei,

Thank you for reviewing this change. Initially I understood the whole 
fragment below  (from the Javadoc for 
com.sun.jdi.connect.ListeningConnector.startListening() method) as a 
direction for the user how to obtain and prepare the argument map 
before passing it in startListening() method.


   61  * The argument map associates argument name strings to instances
   62  * of {@link Connector.Argument}. The default argument map for a
   63  * connector can be obtained through {@link 
Connector#defaultArguments}.
   64  * Argument map values can be changed, but map entries should not be
   65  * added or deleted.

But I agree that the last sentence could also mean that the argument 
map values could be changes as a result of the method invocation and 
in this case the new fragment in the Javadoc is not needed.


Please review the updated webrev that does not add new Javadoc 
fragment and includes other changes you suggested.



Webrev: http://cr.openjdk.java.net/~dtitov/8163083/webrev.07/ 



Bug: https://bugs.openjdk.java.net/browse/JDK-8163083

Thanks!

--Daniil

*From: *
*Organization: *Oracle Corporation
*Date: *Wednesday, September 26, 2018 at 8:12 PM
*To: *Daniil Titov , JC Beyler 

*Cc: *, , 

*Subject: *Re: RFR 8163083: SocketListeningConnector does not allow 
invocations with port 0


Hi Daniil,

It is great that you came up the fix for this issue.
Thanks to Gary for the help too.

I wonder if we could get away without updating the javadoc
of com/sun/jdi/connect/ListeningConnector.java.
Filing a CSR would not be needed then (simple javadoc reformatting 
should not require a CSR).


So, my question is if this new fragment is really needed:

  76  * 
  77  * If the addressing information provided in {@code 
arguments} implies
  78  * the auto detection this information might be updated with 
the address

  79  * of the started listener.

The javadoc for startListening already has this fragment:

   61  * The argument map associates argument name strings to instances
   62  * of {@link Connector.Argument}. The default argument map for a
   63  * connector can be obtained through {@link 
Connector#defaultArguments}.
   64  * Argument map values can be changed, but map entries should not be
   65  * added or deleted.
that allows to change the argument map values.
It looks like, it has to be Okay to add the bound port number there.
Please, let me know what you think.

Some formatting comments to addition to Jc's comments:

  77  * If the addressing information provided in {@code 
arguments} implies
  78  * the auto detection this information might be updated with 
the address

  79  * of the started listener.
   This sentence needs to be split in two:
  77  * If the addressing information provided in {@code 
arguments} implies
  78  * the auto detection. This information might be updated 
with the address

  79  * of the started listener.
+
+    protected void updateArgumentMapIfRequired(
+    Map 
args,TransportService.ListenKey listener) {

+    }
+
  The indent has to be 4, not 8.
+    if(isWildcardPort(args)) {
+    String[] address = listener.address().split(":");
+    if (address.length > 1) {
+    args.get(ARG_PORT).setValue(address[1]);
+    }
+    }
  A space is missed after the first 'if'.
   50 filter(c ->
   51     
c.name().equals("com.sun.jdi.SocketListen")).findFirst().get();

  This is better to be one liner.

   57 testWithDefaultArgs(connector);
   58 testWithDefaultArgs2(connector);
   59 testWithWildcardPort(connector);
   60   

Re: RFR 8163083: SocketListeningConnector does not allow invocations with port 0

2018-09-27 Thread serguei . spitsyn 

Hi Daniil,

It looks great, thank you for the update.
I have a couple of more minor comments on the test.

  56 testWithDefaultArgs(connector);
  57 testWithDefaultArgs2(connector);
  58 testWithWildcardPort1(connector);
  59 testWithWildcardPort2(connector);


  Please, rename testWithDefaultArgs to testWithDefaultArgs1
  to make naming consistent.

 111 throw new RuntimeException("Test testWithDefaultArgsNegative 
failed. The argument map was not updated with" +
 112 " the bound port number.");

 115 // This call should fail since the previous the argument map 
is already updated with the port
 116 // number of the started listener


  Could you, please, re-balance the above line pairs to make the first 
line shorter?


No need in new webrev if you fix the above.

Thanks,
Serguei

On 9/27/18 11:00 AM, Daniil Titov wrote:


Hi Serguei,

Thank you for reviewing this change. Initially I understood the whole 
fragment below  (from the Javadoc for 
com.sun.jdi.connect.ListeningConnector.startListening() method) as a 
direction for the user how to obtain and prepare the argument map 
before passing it in startListening() method.


   61  * The argument map associates argument name strings to instances
   62  * of {@link Connector.Argument}. The default argument map for a
   63  * connector can be obtained through {@link 
Connector#defaultArguments}.
   64  * Argument map values can be changed, but map entries should not be
   65  * added or deleted.

But I agree that the last sentence could also mean that the argument 
map values could be changes as a result of the method invocation and 
in this case the new fragment in the Javadoc is not needed.


Please review the updated webrev that does not add new Javadoc 
fragment and includes other changes you suggested.



Webrev: http://cr.openjdk.java.net/~dtitov/8163083/webrev.07/ 



Bug: https://bugs.openjdk.java.net/browse/JDK-8163083

Thanks!

--Daniil

*From: *
*Organization: *Oracle Corporation
*Date: *Wednesday, September 26, 2018 at 8:12 PM
*To: *Daniil Titov , JC Beyler 

*Cc: *, , 

*Subject: *Re: RFR 8163083: SocketListeningConnector does not allow 
invocations with port 0


Hi Daniil,

It is great that you came up the fix for this issue.
Thanks to Gary for the help too.

I wonder if we could get away without updating the javadoc
of com/sun/jdi/connect/ListeningConnector.java.
Filing a CSR would not be needed then (simple javadoc reformatting 
should not require a CSR).


So, my question is if this new fragment is really needed:

  76  * 
  77  * If the addressing information provided in {@code 
arguments} implies
  78  * the auto detection this information might be updated with 
the address

  79  * of the started listener.

The javadoc for startListening already has this fragment:

   61  * The argument map associates argument name strings to instances
   62  * of {@link Connector.Argument}. The default argument map for a
   63  * connector can be obtained through {@link 
Connector#defaultArguments}.
   64  * Argument map values can be changed, but map entries should not be
   65  * added or deleted.
that allows to change the argument map values.
It looks like, it has to be Okay to add the bound port number there.
Please, let me know what you think.

Some formatting comments to addition to Jc's comments:

  77  * If the addressing information provided in {@code 
arguments} implies
  78  * the auto detection this information might be updated with 
the address

  79  * of the started listener.
   This sentence needs to be split in two:
  77  * If the addressing information provided in {@code 
arguments} implies
  78  * the auto detection. This information might be updated with 
the address

  79  * of the started listener.
+
+    protected void updateArgumentMapIfRequired(
+    Map 
args,TransportService.ListenKey listener) {

+    }
+
  The indent has to be 4, not 8.
+    if(isWildcardPort(args)) {
+    String[] address = listener.address().split(":");
+    if (address.length > 1) {
+    args.get(ARG_PORT).setValue(address[1]);
+    }
+    }
  A space is missed after the first 'if'.
   50 filter(c ->
   51     
c.name().equals("com.sun.jdi.SocketListen")).findFirst().get();

  This is better to be one liner.

   57 testWithDefaultArgs(connector);
   58 testWithDefaultArgs2(connector);
   59 testWithWildcardPort(connector);
   60 testWithWildcardPort2(connector);
   A suggestion is to rename above as below to have the names more unified:
   57 testWithDefaultArgs1(connector);
   58 testWithDefaultArgs2(connector);
   59 testWithWildcardPort1(connector);
   60

Re: RFR (S) 8210842: Handle JNIGlobalRefLocker.cpp

2018-09-27 Thread David Holmes 

Hi Jc,

This seems fine to me. I'll leave it to you and Mikael to wrestle out 
the naming.


Thanks,
David

On 27/09/2018 1:45 PM, JC Beyler wrote:

Hi Mikael and David,

@David: I thought it was implicit but did not want to presume on this 
one because my goal is to start propagating this new class in the test 
base and get the checks to be done implicitly so I was probably being 
over-cautious
@Mikael: done and done, what do you think of the comment here : 
http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.04/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.hpp.html 



For all, the new webrev is here:
Webrev: http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.04/ 


Bug: https://bugs.openjdk.java.net/browse/JDK-8210842

Thanks,
Jc

On Thu, Sep 27, 2018 at 6:03 AM David Holmes > wrote:


Sorry Jc, I thought my LGTM was implicit. :)

Thanks,
David

On 26/09/2018 11:52 PM, JC Beyler wrote:
 > Ping on this, anybody have time to do a review and give a LGTM?
Or David
 > if you have time and will to provide an explicit LGTM :)
 >
 > Thanks,
 > Jc
 >
 > On Mon, Sep 24, 2018 at 9:18 AM JC Beyler mailto:jcbey...@google.com>
 > >> wrote:
 >
 >     Hi David,
 >
 >     Sounds good to me, Alex gave me one LGTM, so it seems I'm waiting
 >     for an explicit LGTM from you or from someone else on this
list to
 >     do a review.
 >
 >     Thanks again for your help,
 >     Jc
 >
 >     On Sun, Sep 23, 2018 at 9:22 AM David Holmes
 >     mailto:david.hol...@oracle.com>
>>
wrote:
 >
 >         Hi Jc,
 >
 >         I don't think it is an issue for this to go ahead. If the
static
 >         analysis tool has an issue then we can either find out how to
 >         teach it
 >         about this code structure, or else flag the issues as false
 >         positives.
 >         I'd be relying on one of the serviceability team here to
handle
 >         that if
 >         the problem arises.
 >
 >         Thanks,
 >         David
 >
 >         On 23/09/2018 12:17 PM, JC Beyler wrote:
 >          > Hi David,
 >          >
 >          > No worries with the time to answer; I appreciate the
review!
 >          >
 >          > That's a fair point. Is it possible to "describe" to the
 >         static analysis
 >          > tool to "trust" calls made in the SafeJNIEnv class?
 >          >
 >          > Otherwise, do you have any idea on how to go forward? For
 >         what it's
 >          > worth, this current webrev does not try to replace
exception
 >         checking
 >          > with the SafeJNIEnv (it actually adds it), so for now the
 >          > question/solution could be delayed. I could continue
working
 >         on this in
 >          > the scope of both the nsk/gc/lock/jniref code base and the
 >         NSK_VERIFIER
 >          > macro removal and we can look at how to tackle the cases
 >         where the tests
 >          > are actually calling exception checking (I know my
 >         heapmonitor does for
 >          > example).
 >          >
 >          > Let me know what you think and thanks for the review,
 >          > Jc
 >          >
 >          >
 >          > On Sun, Sep 23, 2018 at 6:48 AM David Holmes
 >         mailto:david.hol...@oracle.com>
>
 >          > 
 >                   >
 >          >     Hi Jc,
 >          >
 >          >     Sorry for the delay on getting back to this but I'm
 >         travelling at the
 >          >     moment.
 >          >
 >          >     This looks quite neat. Thanks also to Alex for all the
 >         suggestions.
 >          >
 >          >     My only remaining concern is that static analysis
tools
 >         may not like
 >          >     this because they may not be able to determine that we
 >         won't make
 >          >     subsequent JNI calls when an exception happens in the
 >         first. That's not
 >          >     a reason not to do this of course, just flagging
that we
 >         may have to do
 >          >     something to deal with that problem.
 >          >
 >   

Re: RFR 8163083: SocketListeningConnector does not allow invocations with port 0

2018-09-27 Thread Daniil Titov 
Hi Serguei,

 

Thank you for reviewing this change. Initially I understood the whole fragment 
below  (from the Javadoc for 
com.sun.jdi.connect.ListeningConnector.startListening() method) as a direction 
for the user how to obtain and prepare the argument map before passing it in 
startListening() method.

 
  61  * The argument map associates argument name strings to instances
  62  * of {@link Connector.Argument}. The default argument map for a
  63  * connector can be obtained through {@link 
Connector#defaultArguments}.
  64  * Argument map values can be changed, but map entries should not be
  65  * added or deleted.
 

But I agree that the last sentence could also mean that the argument map values 
could be changes as a result of the method invocation and in this case the new 
fragment in the Javadoc is not needed.

 

Please review the updated webrev that does not add new Javadoc fragment and 
includes other changes you suggested.


Webrev: http://cr.openjdk.java.net/~dtitov/8163083/webrev.07/

Bug: https://bugs.openjdk.java.net/browse/JDK-8163083 

 

Thanks!

--Daniil

 

 

From: 
Organization: Oracle Corporation
Date: Wednesday, September 26, 2018 at 8:12 PM
To: Daniil Titov , JC Beyler 
Cc: , , 

Subject: Re: RFR 8163083: SocketListeningConnector does not allow invocations 
with port 0

 

Hi Daniil,

It is great that you came up the fix for this issue.
Thanks to Gary for the help too.

I wonder if we could get away without updating the javadoc
of com/sun/jdi/connect/ListeningConnector.java.
Filing a CSR would not be needed then (simple javadoc reformatting should not 
require a CSR).

So, my question is if this new fragment is really needed:
  76  * 
  77  * If the addressing information provided in {@code arguments} implies
  78  * the auto detection this information might be updated with the 
address
  79  * of the started listener.
The javadoc for startListening already has this fragment:
  61  * The argument map associates argument name strings to instances
  62  * of {@link Connector.Argument}. The default argument map for a
  63  * connector can be obtained through {@link 
Connector#defaultArguments}.
  64  * Argument map values can be changed, but map entries should not be
  65  * added or deleted.
 
that allows to change the argument map values.
It looks like, it has to be Okay to add the bound port number there.
 
Please, let me know what you think.
Some formatting comments to addition to Jc's comments:
  77  * If the addressing information provided in {@code arguments} implies
  78  * the auto detection this information might be updated with the 
address
  79  * of the started listener.
 
  This sentence needs to be split in two:
 
  77  * If the addressing information provided in {@code arguments} implies
  78  * the auto detection. This information might be updated with the 
address
  79  * of the started listener.
+
+    protected void updateArgumentMapIfRequired(
+    Map 
args,TransportService.ListenKey listener) {
+    }
+
  The indent has to be 4, not 8.
 
+    if(isWildcardPort(args)) {
+    String[] address = listener.address().split(":");
+    if (address.length > 1) {
+    args.get(ARG_PORT).setValue(address[1]);
+    }
+    }
 
  A space is missed after the first 'if'.
 
 
  50 filter(c ->
  51     
c.name().equals("com.sun.jdi.SocketListen")).findFirst().get();
  This is better to be one liner.
  57 testWithDefaultArgs(connector);
  58 testWithDefaultArgs2(connector);
  59 testWithWildcardPort(connector);
  60 testWithWildcardPort2(connector);
 
  A suggestion is to rename above as below to have the names more unified:
 
  57 testWithDefaultArgs1(connector);
  58 testWithDefaultArgs2(connector);
  59 testWithWildcardPort1(connector);
  60 testWithWildcardPort2(connector);
 
Thanks,
Serguei

On 9/25/18 10:32 AM, Daniil Titov wrote:

HI JC,

 

The webrev is updated to address this. 

 

Webrev: http://cr.openjdk.java.net/~dtitov/8163083/webrev.06

Bug: https://bugs.openjdk.java.net/browse/JDK-8163083

Thanks!
--Daniil

 

From: JC Beyler 
Date: Monday, September 24, 2018 at 8:47 PM
To: 
Cc: , , 

Subject: Re: RFR 8163083: SocketListeningConnector does not allow invocations 
with port 0

 

Hi Daniil,

 

Still looks good to me :)

 

Nit: empty line 83 of the new test is not required!

Jc

 

On Mon, Sep 24, 2018 at 5:54 PM Daniil Titov  wrote:

Hi Alex,

Please review the updated webrev that has new test moved to 
test/jdk/comsun/jdi/connect folder.

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

Thanks!
--Daniil


On 9/24/18, 2:56 PM, "Alex Menkov"  wrote:

Daniil,

Just remembered that SQE requested to not add new tests to vmTestbase 
(see

Re: RFR (XS) JDK-8207745: serviceability/sa/TestJmapCore.java times out parsing a 4GB hprof file

2018-09-27 Thread David Holmes 

LGTM.

Thanks,
David

On 27/09/2018 1:16 AM, Sharath Ballal wrote:

Can I get one more review from a (R)eviewer please ?

Thanks,
Sharath


-Original Message-
From: Sharath Ballal
Sent: Wednesday, September 26, 2018 9:57 AM
To: Jini Susan George; serviceability-dev
Subject: RE: RFR (XS) JDK-8207745: serviceability/sa/TestJmapCore.java times 
out parsing a 4GB hprof file

Thanks for the review Jini.


Thanks,
Sharath


-Original Message-
From: Jini George
Sent: Wednesday, September 26, 2018 9:42 AM
To: Sharath Ballal; serviceability-dev
Subject: Re: RFR (XS) JDK-8207745: serviceability/sa/TestJmapCore.java times 
out parsing a 4GB hprof file

Looks good to me.

Thanks,
Jini.

On 9/24/2018 9:19 AM, Sharath Ballal wrote:

Hi,

Pls review this test fix to add "-Xmx512m" to SA tests which create
either core file or heap dump, to avoid timeout due to large heap sizes.

Bug id: https://bugs.openjdk.java.net/browse/JDK-8207745

Webrev: http://cr.openjdk.java.net/~sballal/8207745/webrev.00/


SA tests passed on Mach5 run.

Thanks,

Sharath

Re: RFR (S) 8210842: Handle JNIGlobalRefLocker.cpp

2018-09-27 Thread David Holmes 

Sorry Jc, I thought my LGTM was implicit. :)

Thanks,
David

On 26/09/2018 11:52 PM, JC Beyler wrote:
Ping on this, anybody have time to do a review and give a LGTM? Or David 
if you have time and will to provide an explicit LGTM :)


Thanks,
Jc

On Mon, Sep 24, 2018 at 9:18 AM JC Beyler > wrote:


Hi David,

Sounds good to me, Alex gave me one LGTM, so it seems I'm waiting
for an explicit LGTM from you or from someone else on this list to
do a review.

Thanks again for your help,
Jc

On Sun, Sep 23, 2018 at 9:22 AM David Holmes
mailto:david.hol...@oracle.com>> wrote:

Hi Jc,

I don't think it is an issue for this to go ahead. If the static
analysis tool has an issue then we can either find out how to
teach it
about this code structure, or else flag the issues as false
positives.
I'd be relying on one of the serviceability team here to handle
that if
the problem arises.

Thanks,
David

On 23/09/2018 12:17 PM, JC Beyler wrote:
 > Hi David,
 >
 > No worries with the time to answer; I appreciate the review!
 >
 > That's a fair point. Is it possible to "describe" to the
static analysis
 > tool to "trust" calls made in the SafeJNIEnv class?
 >
 > Otherwise, do you have any idea on how to go forward? For
what it's
 > worth, this current webrev does not try to replace exception
checking
 > with the SafeJNIEnv (it actually adds it), so for now the
 > question/solution could be delayed. I could continue working
on this in
 > the scope of both the nsk/gc/lock/jniref code base and the
NSK_VERIFIER
 > macro removal and we can look at how to tackle the cases
where the tests
 > are actually calling exception checking (I know my
heapmonitor does for
 > example).
 >
 > Let me know what you think and thanks for the review,
 > Jc
 >
 >
 > On Sun, Sep 23, 2018 at 6:48 AM David Holmes
mailto:david.hol...@oracle.com>
 > >> wrote:
 >
 >     Hi Jc,
 >
 >     Sorry for the delay on getting back to this but I'm
travelling at the
 >     moment.
 >
 >     This looks quite neat. Thanks also to Alex for all the
suggestions.
 >
 >     My only remaining concern is that static analysis tools
may not like
 >     this because they may not be able to determine that we
won't make
 >     subsequent JNI calls when an exception happens in the
first. That's not
 >     a reason not to do this of course, just flagging that we
may have to do
 >     something to deal with that problem.
 >
 >     Thanks,
 >     David
 >
 >     On 20/09/2018 11:56 AM, JC Beyler wrote:
 >      > Hi Alex,
 >      >
 >      > Done here, thanks for the review:
 >      >
 >      > Webrev:
http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.03/

 >     
 >      >

 >      > Bug: https://bugs.openjdk.java.net/browse/JDK-8210842
 >      >
 >      > Thanks again!
 >      > Jc
 >      >
 >      >
 >      > On Wed, Sep 19, 2018 at 5:13 PM Alex Menkov
 >     mailto:alexey.men...@oracle.com>
>
 >      > 
 >           >
 >      >     Hi Jc,
 >      >
 >      >     Looks good to me.
 >      >     A minor note:
 >      >     - I'd move ErrorHandler typedef to SafeJNIEnv
class to avoid
 >     global
 >      >     namespece pollution (ErrorHandler is too generic
name).
 >      >
 >      >     --alex
 >      >
 >      >     On 09/19/2018 15:56, JC Beyler wrote:
 >      >      > Hi Alex,
 >      >      >
 >      >      > I've updated the webrev to:
 >      >      > Webrev:
 > http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.02/

 >     
 >      >   
  

Re: RFR (M) 8211036 : Remove the NSK_STUB macros from vmTestbase for non jvmti

2018-09-27 Thread David Holmes 

LGTM :)

Thanks,
David

On 26/09/2018 11:51 PM, JC Beyler wrote:

Hi all,

Anybody on the hotspot-dev list have any comments or a LGTM?

Thanks!
Jc

On Tue, Sep 25, 2018 at 10:56 AM Alex Menkov 
wrote:


+1

--alex

On 09/24/2018 11:40, Igor Ignatyev wrote:

(cc-ing hotspot-dev alias)

Hi Jc,

the fix looks good to me. don't forget to clean up nsk/share/jni/README

at the end.


Thanks,
-- Igor


On Sep 24, 2018, at 9:28 AM, JC Beyler  wrote:

Hi all,

As the tests have become C++ tests, the NSK_CPP_STUBS are no longer

needed. I did two awk scripts to remove them and will be rolling them out
in 50 file max reviews to streamline the reviews for the reviewers.


So here is the first which handles all the cases outside of the jvmti

subfolder:


Webrev: http://cr.openjdk.java.net/~jcbeyler/8211036/webrev.00/ <

http://cr.openjdk.java.net/~jcbeyler/8211036/webrev.00/>

Bug: https://bugs.openjdk.java.net/browse/JDK-8211036 <

https://bugs.openjdk.java.net/browse/JDK-8211036>


The bug contains information on the two scripts I used.

Thanks,
Jc

JDK-8203350: Crash in vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/TestDescription.java

2018-09-27 Thread Gary Adams 

I've been unsuccessful trying to reproduce the failure in hs201t002.

  Issue: https://bugs.openjdk.java.net/browse/JDK-8203350

Colleen made a comment on the bug that the reference
from hs201t002a to class hs201t002 might be an issue
for the redefined class.

I found in test hs201t001 that an intentional reference
before entering hs201t001a.doInit() is made to avoid
that classloader operation.

It's not clear to me why that was done, but the same workaround
could be used in hs201t002a, if it would make the test more robust.


diff --git 
a/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/newclass/hs201t002a.java 
b/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/newclass/hs201t002a.java
--- 
a/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/newclass/hs201t002a.java
+++ 
b/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/newclass/hs201t002a.java

@@ -26,6 +26,7 @@
 public class hs201t002a extends Exception {

 public hs201t002a () {
+System.out.println("Current step: " + hs201t002.currentStep); 
// Avoid calling classloader to find hs201t002 in doInit()

 doInit();
 }

nsk/jvmti/scenarios/hotswap bugs on the ProblemList

2018-09-27 Thread Gary Adams 
While looking at some other hotswap tests, I noticed 2 tests on the 
ProblemList.

I'm attempting some runs with the tests removed from the list to try
and reproduce the errors locally.

  Issues:
 https://bugs.openjdk.java.net/browse/JDK-6813266 hs204t001
 https://bugs.openjdk.java.net/browse/JDK-8204506 hs102t002


diff --git a/test/hotspot/jtreg/ProblemList.txt 
b/test/hotspot/jtreg/ProblemList.txt

--- a/test/hotspot/jtreg/ProblemList.txt
+++ b/test/hotspot/jtreg/ProblemList.txt
@@ -185,8 +185,6 @@
 vmTestbase/nsk/jvmti/ResourceExhausted/resexhausted004/TestDescription.java 
7013634,6606767 generic-all
 vmTestbase/nsk/jvmti/ThreadStart/threadstart001/TestDescription.java 
8016181 generic-all

 vmTestbase/nsk/jvmti/scenarios/extension/EX03/ex03t001/TestDescription.java 
8173658 generic-all
-vmTestbase/nsk/jvmti/scenarios/hotswap/HS102/hs102t002/TestDescription.java 
8204506,8203350 generic-all
-vmTestbase/nsk/jvmti/scenarios/hotswap/HS204/hs204t001/hs204t001.java 
6813266 generic-all

 vmTestbase/nsk/jvmti/scenarios/sampling/SP06/sp06t003/TestDescription.java 
8051349 generic-all
 vmTestbase/nsk/jvmti/AttachOnDemand/attach034/TestDescription.java 
8042145 generic-all
 vmTestbase/nsk/jvmti/AttachOnDemand/attach045/TestDescription.java 
8202971 generic-all



It's odd that  the JDK-6813266 is in the ProblemList, when the bug was 
closed
as not reproducible. I'm wondering if it was quarantined when the bug 
was closed.


For JDK-8204506, it's not clear if this is a graal only bug and only 
needs to be

listed in ProblemList-graal.txt.

Re: RFR: JDK-8210984: [TESTBUG] hs203t003 fails with "# ERROR: hs203t003.cpp, 218: NSK_CPP_STUB2 ( ResumeThread, jvmti, thread)"

2018-09-27 Thread Gary Adams 

Patch attached.

Ran another 1000 clean testruns with the sleep(1)
pauses. Restored the sleep(100) for the final patch.

On 9/26/18, 2:12 PM, Chris Plummer wrote:

On 9/26/18 11:07 AM, Gary Adams wrote:

On 9/26/18, 1:40 PM, Chris Plummer wrote:

Hi Gary,

Should the following comment come first, not after the join() call:

 115 mt.join();
 116 // Wait till the other thread completes its execution.

I'll move the comment up.


Rather than using JVMTI to detect if the field is suspended, 
couldn't you have just set a static variable in 
callbackFieldAccess() and check it from isSuspended()?

All of the other native calls take a thread and operate on it.
It seemed reasonable to use the same check that popThreadFrame used.



Before doing the fix, did you first check if the bug is easily 
reproduced by making is sleep for 1ms instead of 100ms?

That's how I got the problem to reproduce.
Switching to sleep(1) got 5 failures in 300 testruns.
Ok, and I assume you then tested the fix with the 1ms sleep? If so, 
then ship it. Otherwise do this testing first.


thanks,

Chris


thanks,

Chris

On 9/26/18 7:55 AM, Gary Adams wrote:

A race condition exists in hs203t003 between the main test thread and
the processing in callbackFieldAccess processing. The main thread
already includes a polling loop to wait for the redefine class 
operation

to be performed, but it does not wait for the following suspend thread
operation to be completed.

This changeset adds an additional wait for the suspend thread to 
complete.

It also checks the error returns from popThreadFrame and resumeThread
to issue an additional error message if these native routines 
returned an error.


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











# HG changeset patch
# User gadams
# Date 1538047993 14400
#  Thu Sep 27 07:33:13 2018 -0400
# Node ID 2dc330dc1343a36ae38209da78f96d23c4e8f8d2
# Parent  eb3e72f181af1945573507d98854479c748e34e5
8210984: [TESTBUG] hs203t003 fails with "# ERROR: hs203t003.cpp, 218: 
NSK_CPP_STUB2 ( ResumeThread, jvmti, thread)"
Reviewed-by: cjplummer, jcbeyler

diff --git 
a/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS203/hs203t003/hs203t003.cpp
 
b/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS203/hs203t003/hs203t003.cpp
--- 
a/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS203/hs203t003/hs203t003.cpp
+++ 
b/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS203/hs203t003/hs203t003.cpp
@@ -174,6 +174,23 @@
 return JNI_OK;
 }
 
+JNIEXPORT jboolean JNICALL
+Java_nsk_jvmti_scenarios_hotswap_HS203_hs203t003_hs203t003_isSuspended(JNIEnv 
* jni,
+jclass clas,
+jthread thread) {
+jboolean retvalue;
+jint state;
+retvalue = JNI_FALSE;
+if ( ! NSK_JVMTI_VERIFY( NSK_CPP_STUB3(GetThreadState, jvmti, thread, 
) )  ) {
+nsk_printf(" Agent :: Error while getting thread state.\n");
+nsk_jvmti_agentFailed();
+} else {
+if ( state & JVMTI_THREAD_STATE_SUSPENDED) {
+  retvalue = JNI_TRUE;
+}
+}
+return retvalue;
+}
 
 JNIEXPORT jboolean JNICALL
 
Java_nsk_jvmti_scenarios_hotswap_HS203_hs203t003_hs203t003_popThreadFrame(JNIEnv
 * jni,
diff --git 
a/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS203/hs203t003/hs203t003.java
 
b/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS203/hs203t003/hs203t003.java
--- 
a/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS203/hs203t003/hs203t003.java
+++ 
b/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS203/hs203t003/hs203t003.java
@@ -62,7 +62,8 @@
 public class hs203t003 extends RedefineAgent {
 
 public native boolean popThreadFrame(Thread thread);
-public native boolean resumeThread(Thread thread);
+public native boolean isSuspended(Thread thread);
+public native boolean resumeThread(Thread thread);
 
 
 public hs203t003(String[] arg) {
@@ -82,10 +83,10 @@
 MyThread mt = new MyThread();
 try {
 mt.start();
-// check if we can can pop the thread.
-// we can not do redefine/pop frame on run method.
+// Check if we can can pop the thread.
+// We can not do redefine/pop frame on run method.
 while (!MyThread.resume.get());
-// sleep for some few secs to get redefined.
+// Sleep for some few secs to get redefined.
 while (!isRedefined()) {
 if (!agentStatus()) {
 System.out.println("Failed to redefine class");
@@ -93,10 +94,26 @@
 }
 Thread.sleep(100);
 }
-popThreadFrame(mt); // pop the frame.
-resumeThread(mt);   // resume the thread.
+// Wait for the thread to be suspended.
+while (!isSuspended(mt)) {
+if (!agentStatus()) {