[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: "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date: 2017-04-29 4:33:04
Message-ID: 3eb94f3b-142c-6cd6-3d15-a0ab840880dc () oracle ! com
[Download RAW message or body]
Thanks, David!
Serguei
On 4/28/17 18:08, David Holmes wrote:
> That looks fine to me Serguei!
>
> Thanks,
> David
>
> On 29/04/2017 10:13 AM, 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