RFR(M): 8172970: TESTBUG: need test coverage for the JVMTI functions allowed in the start phase
Daniel D. Daugherty
daniel.daugherty at oracle.com
Tue May 2 18:09:10 UTC 2017
> 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.spitsyn at 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.spitsyn at oracle.com wrote:
>> Hi David,
>>
>>
>> On 4/28/17 10:34, serguei.spitsyn at oracle.com wrote:
>>> Hi David,
>>>
>>>
>>> On 4/28/17 04:42, David Holmes wrote:
>>>> Hi Serguei,
>>>>
>>>> On 28/04/2017 6:07 PM, serguei.spitsyn at 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.spitsyn at 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.spitsyn at 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
>>>>>>
>>>>>
>>>
>>
>
More information about the hotspot-dev
mailing list