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 <stdint.h>
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(&callbacks, 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 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,
&is_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,
&locals_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