RFR(M): 8172970: TESTBUG: need test coverage for the JVMTI functions allowed in the start phase
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue May 2 18:40:48 UTC 2017
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.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