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

[David Holmes]

That looks fine to me Serguei!

Thanks,
David

On 29/04/2017 10:13 AM, serguei.spitsyn at oracle.com wrote:
> 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
>>>>>
>>>>
>>
>