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

[serguei.spitsyn at oracle.com]

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.

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