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

[serguei.spitsyn at oracle.com]

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