date:20170502

Re: VisualVM CPU sampling not working

2017-05-02 Thread Mandy Chung

serviceability-dev (cc’ed) is the proper mailing list for this issue
and so bcc jdk9-dev.

https://bugs.openjdk.java.net/browse/JDK-8167121 has been pushed
to jdk8u-dev.  We will need to get this fix in a 8u release.

Mandy

> On May 2, 2017, at 2:15 PM, Alan Snyder  wrote:
> 
> Is this a known problem? It resembles JDK-8165005. Makes it hard to 
> investigate performance problems if the tools don't work. Are there other 
> tools that work?
> 
> This is VisualVM 1.39 on an application running under jdk9-ea+166.
> 
> It reports:
> 
> Cannot access threads in target application.
> 
> The log file shows:
> 
> java.lang.IllegalArgumentException: Unexpected composite type for ThreadInfo
>  at 
> sun.management.ThreadInfoCompositeData.validateCompositeData(ThreadInfoCompositeData.java:372)
>  at 
> sun.management.ThreadInfoCompositeData.getInstance(ThreadInfoCompositeData.java:68)
>  at java.lang.management.ThreadInfo.(ThreadInfo.java:263)
>  at java.lang.management.ThreadInfo.from(ThreadInfo.java:794)
> 
> Caused: java.lang.reflect.InvocationTargetException
>  at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
>  at 
> sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
>  at 
> sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
>  at java.lang.reflect.Method.invoke(Method.java:498)
>  at sun.reflect.misc.Trampoline.invoke(MethodUtil.java:71)
>  at sun.reflect.GeneratedMethodAccessor6.invoke(Unknown Source)
>  at 
> sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
>  at java.lang.reflect.Method.invoke(Method.java:498)
>  at sun.reflect.misc.MethodUtil.invoke(MethodUtil.java:275)
>  at 
> com.sun.jmx.mbeanserver.DefaultMXBeanMappingFactory$CompositeBuilderViaFrom.fromCompositeData(DefaultMXBeanMappingFactory.java:1018)
> 
> Caused: java.io.InvalidObjectException: Failed to invoke from(CompositeData)
>  at 
> com.sun.jmx.mbeanserver.DefaultMXBeanMappingFactory.invalidObjectException(DefaultMXBeanMappingFactory.java:1457)
>  at 
> com.sun.jmx.mbeanserver.DefaultMXBeanMappingFactory$CompositeBuilderViaFrom.fromCompositeData(DefaultMXBeanMappingFactory.java:1021)
>  at 
> com.sun.jmx.mbeanserver.DefaultMXBeanMappingFactory$CompositeMapping.fromNonNullOpenValue(DefaultMXBeanMappingFactory.java:919)
>  at 
> com.sun.jmx.mbeanserver.DefaultMXBeanMappingFactory$NonNullMXBeanMapping.fromOpenValue(DefaultMXBeanMappingFactory.java:133)
>  at 
> com.sun.jmx.mbeanserver.DefaultMXBeanMappingFactory$ArrayMapping.fromNonNullOpenValue(DefaultMXBeanMappingFactory.java:584)
>  at 
> com.sun.jmx.mbeanserver.DefaultMXBeanMappingFactory$NonNullMXBeanMapping.fromOpenValue(DefaultMXBeanMappingFactory.java:133)
>  at 
> com.sun.jmx.mbeanserver.ConvertingMethod.fromOpenReturnValue(ConvertingMethod.java:131)
>  at com.sun.jmx.mbeanserver.MXBeanProxy.invoke(MXBeanProxy.java:168)
>  at 
> javax.management.MBeanServerInvocationHandler.invoke(MBeanServerInvocationHandler.java:258)
> 
> Caused: java.lang.reflect.UndeclaredThrowableException
>  at com.sun.proxy.$Proxy16.dumpAllThreads(Unknown Source)
>  at 
> com.sun.tools.visualvm.sampler.cpu.ThreadInfoProvider.dumpAllThreads(ThreadInfoProvider.java:103)
> 
> [catch] at 
> com.sun.tools.visualvm.sampler.cpu.ThreadInfoProvider.initialize(ThreadInfoProvider.java:88)
>  at 
> com.sun.tools.visualvm.sampler.cpu.ThreadInfoProvider.(ThreadInfoProvider.java:54)
>  at com.sun.tools.visualvm.sampler.SamplerImpl$11.run(SamplerImpl.java:485)
>  at org.openide.util.RequestProcessor$Task.run(RequestProcessor.java:1423)
>  at 
> org.openide.util.RequestProcessor$Processor.run(RequestProcessor.java:2033)
>

Re: [PATCH] attach in linux should be relative to /proc/pid/root and namespace aware

2017-05-02 Thread Erik Gahlin

I am not a (R)eviewer, so I can't give it my blessings, but I can 
sponsor it.


I will create a webrev so it easier to review.

Erik


I have attached a patch that allows jcmd to work against a java process running
inside a Docker container. Apologies if this is not in the correct format. It 
was
built against jdk8u. I couldn’t seem to find an existing JIRA for it.

Diagnostic commands (i.e. jcmd, jstack, etc) fail to attach to a target JVM
that is inside a container (e.g. Docker).

A Linux container often isolates a process in a PID and Mount namespace that is
separate from the "root container" (analogous to the hypervisor/dom0 in
hardware virtualization environments, or the global zone on Solaris). A target
JVM that is isolated in either a PID namespace, or a Mount namespace will fail
the attach sequence.

When the target JVM is in its own PID namespace the pid of the process is
distinct from what the real pid of the process as it relates to the root
container. For example, in the root container you can observe a JVM with a pid
of 17734, however if that JVM is running inside a Docker container the pid
inside its PID namespace is likely 1. So when the target JVM receives the
SIGQUIT it looks in /proc/self/cwd/ for .attach_pid1 however the external
attaching JVM has created the file /proc/17734/cwd/.attach_pid17734. Given this
discrepancy the target JVM will output to stderr thread status, since
/proc/self/cwd/.attach_pid1 doesn't exist and won't continue with the attach
sequence.

The solution is to parse /proc/pid/status for the field NSpid (available since
Linux 4.1) which contains a list of pids, where the last entry is the "inner
most" PID namespace value. (Namespaces can be stacked, unlike Solaris Zones
which have a virtualization depth of 1)

The rest of the Linux attach sequence assumes a shared mount namespace by
waiting for /tmp/.java_pid17734 to appear. But if the attaching process is in a
separate namespace because the target JVM is in a mount namepsace (or in a
chroot as well) the unix domain socket for attaching won't appear.

Instead the attach sequence should resolve file names relative to
/proc/17734/root which has a materialized view of the rootfs for the target.

Thanks!

TJ

Re: RFR: JDK-8179538: Update jdk.jdi to be HTML-5 friendly

2017-05-02 Thread Mandy Chung

Looks fine to me.

Mandy

> On May 2, 2017, at 12:40 PM, Kumar Srinivasan  
> wrote:
> 
> Hello,
> 
> Please review changes to make jdk.jdi HTML5 friendly,
> table cell padding has not been addressed and will be fixed
> separately, this takes care of others.
> 
> Note: Some of the errors were due to incorrect@throws but
> in all cases there is the correct one further down, please use
> sdiffs in these cases.
> 
> @throws {@link InvalidTypeException} if any
> 
> before:jdk.jdi {error=42, warning=1}
> after: jdk.jdi {error=1}
> 
> http://cr.openjdk.java.net/~ksrini/8179538/webrev.00/index.html
> https://bugs.openjdk.java.net/browse/JDK-8179538
> 
> Thanks
> Kumar
>

RFR: JDK-8179538: Update jdk.jdi to be HTML-5 friendly

2017-05-02 Thread Kumar Srinivasan


Hello,

Please review changes to make jdk.jdi HTML5 friendly,
table cell padding has not been addressed and will be fixed
separately, this takes care of others.

Note: Some of the errors were due to incorrect@throws but
in all cases there is the correct one further down, please use
sdiffs in these cases.

@throws {@link InvalidTypeException} if any

before:jdk.jdi {error=42, warning=1}
after: jdk.jdi {error=1}

http://cr.openjdk.java.net/~ksrini/8179538/webrev.00/index.html
https://bugs.openjdk.java.net/browse/JDK-8179538

Thanks
Kumar

Re: RFR(M): 8172970: TESTBUG: need test coverage for the JVMTI functions allowed in the start phase

2017-05-02 Thread Daniel D. Daugherty

On 5/2/17 12:40 PM, serguei.spit...@oracle.com wrote:

Hi Dan,

Thank you a lot for the comments!
All are nice catches.
I have to admit I've done too many typos in new test.
Some of them is a result of the 'last minute' changes.

The updated webrev is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8172970-start-phase.4/ 

make/test/JtregNative.gmk
No comments.

test/serviceability/jvmti/StartPhase/AllowedFunctions/AllowedFunctions.java
No comments.

test/serviceability/jvmti/StartPhase/AllowedFunctions/libAllowedFunctions.c
L126: printf(" class: \'%s\'\n", name);
L137: printf("Class source file name: \'%s\'\n", name);
You don't need to escape the single-quotes with backslash here.

Thumbs up!

Dan

Thanks,
Serguei

On 5/2/17 11:09, Daniel D. Daugherty wrote:

> New webrev version is:
> 
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8172970-start-phase.3/

make/test/JtregNative.gmk
No comments.

test/serviceability/jvmti/StartPhase/AllowedFunctions/AllowedFunctions.java 

L27:  * @summary Verify the functions that allowed to operate in 
the start phase

Typo: 'that allowed' -> 'that are allowed'

L28:  * with and without can_generate_early_vmstart capability
Please add '.' to the end.

test/serviceability/jvmti/StartPhase/AllowedFunctions/libAllowedFunctions.c 

L27: #include 
Should this include be in "alpha" order?

L115: printf("  ## Error: unexpected class status: 
0x%02x\n", status);

L117: printf("Class status: 0x%08x\n", status);
Why the different format specifications? "02x" versus "08x"?

L126: printf(" class: %s\n", name);
L137: printf("Class source file name: %s\n", name);
Please consider adding single-quotes around the %s.

L175: check_jvmti_error(jvmti, "GetClassMethods", err);
Typo: "GetClassMethods" -> "GetClassFields"

L229: err = (*jvmti)->IsMethodObsolete(jvmti, method, & 
is_obsolete);

Please delete space after '&'.

L265: check_jvmti_error(jvmti, "GetMethodModifiers", err);
Typo: "GetMethodModifiers" -> "GetFieldModifiers"

L301: if (methods != NULL) {
Typo: 'methods' -> 'fields'

This one can result in a memory leak.

L308: jvmtiError err;
L322: jvmtiError err;
'err' is unused. Please delete it.

L396: check_jvmti_error(jvmti, "AddCapabilites", err);
 Other errors in here include "## Agent_Initialize: "; why not
 this one?

L398: size = (jint)sizeof(callbacks);
L399: memset(, 0, sizeof(callbacks));
Perhaps use 'size' instead of 'sizeof(callbacks)' since you 
have it.

You have a nice list of functions in the bug report.
You might want to include the list of functions that
are NOT covered by this test along with a brief comment
about why that is okay.

Dan

On 5/2/17 3:02 AM, serguei.spit...@oracle.com wrote:

PING:
 I've got a thumbs up from David Holmes.
 One more review is needed for this jdk 10 test enhancement.

Thanks!
Serguei

On 4/28/17 17:13, serguei.spit...@oracle.com wrote:

Hi David,

On 4/28/17 10:34, serguei.spit...@oracle.com wrote:

Hi David,

On 4/28/17 04:42, David Holmes wrote:

Hi Serguei,

On 28/04/2017 6:07 PM, serguei.spit...@oracle.com wrote:

The updated webrev is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8172970-start-phase.2/ 

Thanks for the updates (the issue with long is that on win64 it 
is only 32-bit while void* is 64-bit).

Ok, thanks.
Than you are right, using long on win64 is not compatible.

I prefer to see fast-fail rather than potentially triggering 
cascading failures (check_jvmti_error could even call exit() I 
think). But let's see what others think - it's only a preference 
not a necessity.

Ok, I'll consider call exit() as it would keep it simple.

New webrev version is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8172970-start-phase.3/ 

Thanks,
Serguei

Thanks,
Serguei

Thanks,
David

I've re-arranged a little bit code in the ClassPrepare callback 
and the

function test_class_functions().

Thanks,
Serguei

On 4/28/17 00:47, serguei.spit...@oracle.com wrote:

Hi David,

Thank you for looking at the test!

On 4/27/17 23:11, David Holmes wrote:

Hi Serguei,

On 28/04/2017 3:14 PM, serguei.spit...@oracle.com wrote:

Please, review the jdk 10 fix for the test enhancement:
https://bugs.openjdk.java.net/browse/JDK-8172970

Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8172970-start-phase.1/ 

Sorry but I can't quite figure out exactly what this test is 
doing.

What is the overall call structure here?

This is to make sure the functions allowed in the start and live
phases work Ok.
As the list of functions is pretty big the test does sanity checks
that the functions do not crash nor return errors.

I was expecting to see a difference

Re: RFR(M): 8172970: TESTBUG: need test coverage for the JVMTI functions allowed in the start phase

2017-05-02 Thread serguei.spit...@oracle.com

Hi Dan,

Thank you a lot for the comments!
All are nice catches.
I have to admit I've done too many typos in new test.
Some of them is a result of the 'last minute' changes.

The updated webrev is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8172970-start-phase.4/

Thanks,
Serguei

On 5/2/17 11:09, Daniel D. Daugherty wrote:

> New webrev version is:
> 
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8172970-start-phase.3/

make/test/JtregNative.gmk
No comments.

test/serviceability/jvmti/StartPhase/AllowedFunctions/AllowedFunctions.java 

L27:  * @summary Verify the functions that allowed to operate in 
the start phase

Typo: 'that allowed' -> 'that are allowed'

L28:  * with and without can_generate_early_vmstart capability
Please add '.' to the end.

test/serviceability/jvmti/StartPhase/AllowedFunctions/libAllowedFunctions.c 

L27: #include 
Should this include be in "alpha" order?

L115: printf("  ## Error: unexpected class status: 
0x%02x\n", status);

L117: printf("Class status: 0x%08x\n", status);
Why the different format specifications? "02x" versus "08x"?

L126: printf(" class: %s\n", name);
L137: printf("Class source file name: %s\n", name);
Please consider adding single-quotes around the %s.

L175: check_jvmti_error(jvmti, "GetClassMethods", err);
Typo: "GetClassMethods" -> "GetClassFields"

L229: err = (*jvmti)->IsMethodObsolete(jvmti, method, & 
is_obsolete);

Please delete space after '&'.

L265: check_jvmti_error(jvmti, "GetMethodModifiers", err);
Typo: "GetMethodModifiers" -> "GetFieldModifiers"

L301: if (methods != NULL) {
Typo: 'methods' -> 'fields'

This one can result in a memory leak.

L308: jvmtiError err;
L322: jvmtiError err;
'err' is unused. Please delete it.

L396: check_jvmti_error(jvmti, "AddCapabilites", err);
 Other errors in here include "## Agent_Initialize: "; why not
 this one?

L398: size = (jint)sizeof(callbacks);
L399: memset(, 0, sizeof(callbacks));
Perhaps use 'size' instead of 'sizeof(callbacks)' since you 
have it.

You have a nice list of functions in the bug report.
You might want to include the list of functions that
are NOT covered by this test along with a brief comment
about why that is okay.

Dan

On 5/2/17 3:02 AM, serguei.spit...@oracle.com wrote:

PING:
 I've got a thumbs up from David Holmes.
 One more review is needed for this jdk 10 test enhancement.

Thanks!
Serguei

On 4/28/17 17:13, serguei.spit...@oracle.com wrote:

Hi David,

On 4/28/17 10:34, serguei.spit...@oracle.com wrote:

Hi David,

On 4/28/17 04:42, David Holmes wrote:

Hi Serguei,

On 28/04/2017 6:07 PM, serguei.spit...@oracle.com wrote:

The updated webrev is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8172970-start-phase.2/ 

Thanks for the updates (the issue with long is that on win64 it is 
only 32-bit while void* is 64-bit).

Ok, thanks.
Than you are right, using long on win64 is not compatible.

I prefer to see fast-fail rather than potentially triggering 
cascading failures (check_jvmti_error could even call exit() I 
think). But let's see what others think - it's only a preference 
not a necessity.

Ok, I'll consider call exit() as it would keep it simple.

New webrev version is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8172970-start-phase.3/ 

Thanks,
Serguei

Thanks,
Serguei

Thanks,
David

I've re-arranged a little bit code in the ClassPrepare callback 
and the

function test_class_functions().

Thanks,
Serguei

On 4/28/17 00:47, serguei.spit...@oracle.com wrote:

Hi David,

Thank you for looking at the test!

On 4/27/17 23:11, David Holmes wrote:

Hi Serguei,

On 28/04/2017 3:14 PM, serguei.spit...@oracle.com wrote:

Please, review the jdk 10 fix for the test enhancement:
  https://bugs.openjdk.java.net/browse/JDK-8172970

Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8172970-start-phase.1/ 

Sorry but I can't quite figure out exactly what this test is 
doing.

What is the overall call structure here?

This is to make sure the functions allowed in the start and live
phases work Ok.
As the list of functions is pretty big the test does sanity checks
that the functions do not crash nor return errors.

I was expecting to see a difference between things that can be 
called
at early-start and those that can not - or are these all 
expected to

work okay in either case?

All these functions are expected to work okay in both cases.
Of course, the main concern is the early start.
But we have never had such coverage in the past so that the normal
start phase needs to be covered too.

A few comments:

 44 #define TranslateError(err) "JVMTI error"

I don't see the point of the above.

Good catch - removed.
It is a left over

Re: RFR(M): 8172970: TESTBUG: need test coverage for the JVMTI functions allowed in the start phase

2017-05-02 Thread Daniel D. Daugherty

> New webrev version is:
> 
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8172970-start-phase.3/

make/test/JtregNative.gmk
No comments.

test/serviceability/jvmti/StartPhase/AllowedFunctions/AllowedFunctions.java
L27:  * @summary Verify the functions that allowed to operate in 
the start phase

Typo: 'that allowed' -> 'that are allowed'

L28:  * with and without can_generate_early_vmstart capability
Please add '.' to the end.

test/serviceability/jvmti/StartPhase/AllowedFunctions/libAllowedFunctions.c
L27: #include 
Should this include be in "alpha" order?

L115: printf("  ## Error: unexpected class status: 
0x%02x\n", status);

L117: printf("Class status: 0x%08x\n", status);
Why the different format specifications? "02x" versus "08x"?

L126: printf(" class: %s\n", name);
L137: printf("Class source file name: %s\n", name);
Please consider adding single-quotes around the %s.

L175: check_jvmti_error(jvmti, "GetClassMethods", err);
Typo: "GetClassMethods" -> "GetClassFields"

L229: err = (*jvmti)->IsMethodObsolete(jvmti, method, & 
is_obsolete);

Please delete space after '&'.

L265: check_jvmti_error(jvmti, "GetMethodModifiers", err);
Typo: "GetMethodModifiers" -> "GetFieldModifiers"

L301: if (methods != NULL) {
Typo: 'methods' -> 'fields'

This one can result in a memory leak.

L308: jvmtiError err;
L322: jvmtiError err;
'err' is unused. Please delete it.

L396: check_jvmti_error(jvmti, "AddCapabilites", err);
 Other errors in here include "## Agent_Initialize: "; why not
 this one?

L398: size = (jint)sizeof(callbacks);
L399: memset(, 0, sizeof(callbacks));
Perhaps use 'size' instead of 'sizeof(callbacks)' since you 
have it.

You have a nice list of functions in the bug report.
You might want to include the list of functions that
are NOT covered by this test along with a brief comment
about why that is okay.

Dan

On 5/2/17 3:02 AM, serguei.spit...@oracle.com wrote:

PING:
 I've got a thumbs up from David Holmes.
 One more review is needed for this jdk 10 test enhancement.

Thanks!
Serguei

On 4/28/17 17:13, serguei.spit...@oracle.com wrote:

Hi David,

On 4/28/17 10:34, serguei.spit...@oracle.com wrote:

Hi David,

On 4/28/17 04:42, David Holmes wrote:

Hi Serguei,

On 28/04/2017 6:07 PM, serguei.spit...@oracle.com wrote:

The updated webrev is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8172970-start-phase.2/ 

Thanks for the updates (the issue with long is that on win64 it is 
only 32-bit while void* is 64-bit).

Ok, thanks.
Than you are right, using long on win64 is not compatible.

I prefer to see fast-fail rather than potentially triggering 
cascading failures (check_jvmti_error could even call exit() I 
think). But let's see what others think - it's only a preference 
not a necessity.

Ok, I'll consider call exit() as it would keep it simple.

New webrev version is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8172970-start-phase.3/ 

Thanks,
Serguei

Thanks,
Serguei

Thanks,
David

I've re-arranged a little bit code in the ClassPrepare callback 
and the

function test_class_functions().

Thanks,
Serguei

On 4/28/17 00:47, serguei.spit...@oracle.com wrote:

Hi David,

Thank you for looking at the test!

On 4/27/17 23:11, David Holmes wrote:

Hi Serguei,

On 28/04/2017 3:14 PM, serguei.spit...@oracle.com wrote:

Please, review the jdk 10 fix for the test enhancement:
  https://bugs.openjdk.java.net/browse/JDK-8172970

Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8172970-start-phase.1/ 

Sorry but I can't quite figure out exactly what this test is doing.
What is the overall call structure here?

This is to make sure the functions allowed in the start and live
phases work Ok.
As the list of functions is pretty big the test does sanity checks
that the functions do not crash nor return errors.

I was expecting to see a difference between things that can be 
called
at early-start and those that can not - or are these all 
expected to

work okay in either case?

All these functions are expected to work okay in both cases.
Of course, the main concern is the early start.
But we have never had such coverage in the past so that the normal
start phase needs to be covered too.

A few comments:

 44 #define TranslateError(err) "JVMTI error"

I don't see the point of the above.

Good catch - removed.
It is a left over from another test that I used as initial template.

---

 99 static long get_thread_local(jvmtiEnv *jvmti, jthread thread) {

The thread local functions use "long" as the datatype but that will
only be 32-bit on 64-bit Windows. I think you need to use intptr_t
for complete portability.

The type long has the same format as the type void* which

Re: RFR(M): 8172970: TESTBUG: need test coverage for the JVMTI functions allowed in the start phase

2017-05-02 Thread serguei.spit...@oracle.com


PING:
 I've got a thumbs up from David Holmes.
 One more review is needed for this jdk 10 test enhancement.

Thanks!
Serguei



On 4/28/17 17:13, serguei.spit...@oracle.com wrote:

Hi David,


On 4/28/17 10:34, serguei.spit...@oracle.com wrote:

Hi David,


On 4/28/17 04:42, David Holmes wrote:

Hi Serguei,

On 28/04/2017 6:07 PM, serguei.spit...@oracle.com wrote:

The updated webrev is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8172970-start-phase.2/ 



Thanks for the updates (the issue with long is that on win64 it is 
only 32-bit while void* is 64-bit).


Ok, thanks.
Than you are right, using long on win64 is not compatible.



I prefer to see fast-fail rather than potentially triggering 
cascading failures (check_jvmti_error could even call exit() I 
think). But let's see what others think - it's only a preference not 
a necessity.


Ok, I'll consider call exit() as it would keep it simple.



New webrev version is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8172970-start-phase.3/ 




Thanks,
Serguei



Thanks,
Serguei



Thanks,
David



I've re-arranged a little bit code in the ClassPrepare callback and 
the

function test_class_functions().

Thanks,
Serguei


On 4/28/17 00:47, serguei.spit...@oracle.com wrote:

Hi David,

Thank you for looking at the test!


On 4/27/17 23:11, David Holmes wrote:

Hi Serguei,

On 28/04/2017 3:14 PM, serguei.spit...@oracle.com wrote:

Please, review the jdk 10 fix for the test enhancement:
  https://bugs.openjdk.java.net/browse/JDK-8172970


Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8172970-start-phase.1/ 





Sorry but I can't quite figure out exactly what this test is doing.
What is the overall call structure here?


This is to make sure the functions allowed in the start and live
phases work Ok.
As the list of functions is pretty big the test does sanity checks
that the functions do not crash nor return errors.


I was expecting to see a difference between things that can be 
called

at early-start and those that can not - or are these all expected to
work okay in either case?


All these functions are expected to work okay in both cases.
Of course, the main concern is the early start.
But we have never had such coverage in the past so that the normal
start phase needs to be covered too.




A few comments:

 44 #define TranslateError(err) "JVMTI error"

I don't see the point of the above.


Good catch - removed.
It is a left over from another test that I used as initial template.



---

 99 static long get_thread_local(jvmtiEnv *jvmti, jthread thread) {

The thread local functions use "long" as the datatype but that will
only be 32-bit on 64-bit Windows. I think you need to use intptr_t
for complete portability.


The type long has the same format as the type void* which has to be
portable even on win-32.
But maybe I'm missing something.
Anyway, I've replaced it with the intptr_t.




---

 277 printf("Filed declaring");

typo: filed -> field



Good catch - fixed.



---

All your little wrapper functions make the JVMTI call and then call
check_jvmti_error - but all that does is record if it passed or
failed. If it failed you still continue with the next operation even
if it relies on the success of the first one eg:

 378 set_thread_local(jvmti, thread, exp_val);
 379 act_val = get_thread_local(jvmti, cur_thread);

and the sequences in print_method_info:

 228 err = (*jvmti)->IsMethodNative(jvmti, method, _native);
 229 check_jvmti_error(jvmti, "IsMethodNative", err);
 230 printf("Method is native: %d\n", is_native);
 231
 232 if (is_native == JNI_FALSE) {
 233 err = (*jvmti)->GetMaxLocals(jvmti, method, 
_max);


The call at #233 may not be valid because the method actually is
native but the IsMethodNative call failed for some reason.



It is intentional. I have done it as a last cleanup.
The point is to simplify code by skipping all the extra checks if it
does not lead to any fatal errors.
The most important in such a case is that the static variable result
is set to FAILED.
It will cause the test to fail.
Then there is no point to analyze the printed results if a JVMTI 
error

reported before.

If you insist, I will add back all the extra check to make sure all
printed output is valid.


Thanks,
Serguei


Thanks,
David
-




Summary:
  The task was to provide a test coverage for the JVMTI functions
allowed during the start phase.
  It includes both enabling and disabling the
can_generate_early_vmstart
capability.
  Testing the JVMTI functions allowed in any case has not been 
targeted

by this fix.

Testing:
  New test is passed.


Thanks,
Serguei

Re: VisualVM CPU sampling not working

Re: [PATCH] attach in linux should be relative to /proc/pid/root and namespace aware

Re: RFR: JDK-8179538: Update jdk.jdi to be HTML-5 friendly

RFR: JDK-8179538: Update jdk.jdi to be HTML-5 friendly

Re: RFR(M): 8172970: TESTBUG: need test coverage for the JVMTI functions allowed in the start phase

Re: RFR(M): 8172970: TESTBUG: need test coverage for the JVMTI functions allowed in the start phase

Re: RFR(M): 8172970: TESTBUG: need test coverage for the JVMTI functions allowed in the start phase

Re: RFR(M): 8172970: TESTBUG: need test coverage for the JVMTI functions allowed in the start phase

8 matches

Site Navigation

Mail list logo

Footer information