[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR(M): 8172970: TESTBUG: need test coverage for the JVMTI functions allowed in the start phase
From: "Daniel D. Daugherty" <daniel.daugherty () oracle ! com>
Date: 2017-05-02 19:23:10
Message-ID: 6f4f5fec-6733-85c4-a741-20acb469f0e7 () oracle ! com
[Download RAW message or body]
On 5/2/17 12:40 PM, serguei.spitsyn@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 <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@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@oracle.com wrote:
>>>> Hi David,
>>>>
>>>>
>>>> On 4/28/17 10:34, serguei.spitsyn@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@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@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@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
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic