RFR(M): 8172970: TESTBUG: need test coverage for the JVMTI functions allowed in the start phase

Sat Apr 29 00:13:56 UTC 2017

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
>>>>
>>>
>