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

[serguei.spitsyn at oracle.com]

The updated webrev is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8172970-start-phase.2/

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
>