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

[David Holmes]

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

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.

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