RFR(S): 8151259: [TESTBUG] nsk/jvmti/RedefineClasses/redefclass030 fails with "unexpected values of outer fields of the class" when running with -Xcomp

[serguei.spitsyn at oracle.com]

Hi Chris,


On 7/23/18 11:40, Chris Plummer wrote:
> Hi Serguei,
>
> If the fix was complicated I would agree, but it really just boils 
> down to this one line change:
>
> -            fire = -1;
> +            fire = 0; // Ignore this compilation. Wait for next one.

It is not obvious that this will completely fix the problem.
Is it possible that there will not be next compilation with the -Xcomp?

If it is possible then it is better to explicitly exclude these tests 
for -Xcomp.
Otherwise, consider this reviewed.

>
> Given that, I see no reason not to increase our test coverage by 
> supporting this test during -Xcomp runs.

I'd agree if it is going to be stable.

Thanks,
Serguei

>
> thanks,
>
> Chris
>
> On 7/23/18 9:44 AM, serguei.spitsyn at oracle.com wrote:
>> Hi Chris,
>>
>> Would it be more simple to avoid running these tests with -Xcomp?
>> I guess, this would work: @requires vm.compMode != "Xcomp"
>>
>> Thanks,
>> Serguei
>>
>>
>> On 7/23/18 00:42, Chris Plummer wrote:
>>> Hello,
>>>
>>> Please review the following fix for JDK11:
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8151259
>>> http://cr.openjdk.java.net/~cjplummer/8151259/webrev.00
>>>
>>> It fixes the following 3 tests:
>>>
>>> vmTestbase/nsk/jvmti/RedefineClasses/redefclass028.java
>>> vmTestbase/nsk/jvmti/RedefineClasses/redefclass029.java
>>> vmTestbase/nsk/jvmti/RedefineClasses/redefclass030.java
>>>
>>> Any of which could fail when run with -Xcomp with (followed by a 
>>> bunch more errors):
>>>
>>>  # ERROR: Redefinition not started. Maybe running with -Xcomp. Test 
>>> ignored.
>>>
>>> Although lately we've only seen this with redefclass030.java on macosx.
>>>
>>> These 3 tests do redefinition of a "hot" method after triggering 
>>> compilation for it. After the redef some testing is done to ensure 
>>> that the redef was done correctly, but the issue these test have 
>>> actually comes before any redef is done.
>>>
>>> The test attempts to trigger compilation by calling a hot method a 
>>> lot. The agent detects compilation by receiving a CompiledMethodLoad 
>>> event. There was an issue discovered long ago that when -Xcomp is 
>>> used, the compilation happens before the "hot" method is ever 
>>> called. Then the redef would happen before compilation, and this 
>>> somehow messed up the test (I'm not exactly sure how). The fix was 
>>> to basically abandon the redef attempt when this problem is 
>>> detected, and then supposedly just let the test run to completion 
>>> (skipping the actual testing of the redef). After this change, if 
>>> you ran with -Xcomp it would pass, but if you looked in the log you 
>>> would see:
>>>
>>>  # ERROR: Redefinition not started. Maybe running with -Xcomp. Test 
>>> ignored.
>>>
>>> However, there was a bug in the logic to make the test run to 
>>> completion, and also causes the above message to not appear. Instead 
>>> the test would fail with:
>>>
>>> # ERROR: Redefinition not completed.
>>>
>>> Followed by a bunch more error message during the part of the test 
>>> that checks if the redef was done properly.
>>>
>>> If the CompiledMethodLoad event comes in before the hot method is 
>>> ever called (which it does with -Xcomp), the test sets fire = -1. If 
>>> the hot method was called, it is set to 1.  The setting of fire = -1 
>>> was added to fix the -Xcomp problem mentioned above. The jvmti agent 
>>> does the following:
>>>
>>>     do {
>>>         THREAD_sleep(1);
>>>         /* wait for compilation to happen */
>>>     } while(fire == 0);
>>>
>>>     if (fire == 1) {
>>>         /* do the redef here */
>>>         NSK_DISPLAY0("agentProc: <<<<<<<< RedefineClasses() is 
>>> successfully done\n");
>>>     } else {
>>>         // fire == -1
>>>         NSK_DISPLAY0("agentProc: \"hot\" method wasn't executed. 
>>> Don't perform redefinition\n");
>>>     }
>>>
>>> The agent then syncs with the debuggee, waiting for it finish up. 
>>> What the test expects is that waitForRedefinitionStarted() in the 
>>> debuggee will time out after two seconds while waiting for fire == 1 
>>> (which it thinks will will always happen because it was set to -1). 
>>> When it times out, the test does appear to exit properly with, but 
>>> with the following in the log, which is intended:
>>>
>>>  # ERROR: Redefinition not started. Maybe running with -Xcomp. Test 
>>> ignored.
>>>
>>> However, sometimes before waitForRedefinitionStarted() times out, 
>>> the hot method is called enough times to trigger compilation. So 
>>> another CompiledMethodLoad event arrives, and this time fire is set 
>>> to 1. Because of this, waitForRedefinitionStarted() doesn't time out 
>>> and returns with an indication that the redef has started. After 
>>> this waitForRedefinitionCompleted() is executed. It waits for the 
>>> redef to complete, but it never does since the agent decided not to 
>>> do the redef when it saw fire == -1. So 
>>> waitForRedefinitionCompleted() times out after 10 seconds and the 
>>> test fails, with:
>>>
>>> # ERROR: Redefinition not completed.
>>>
>>> Actually the above error is not really what causes the failure. When 
>>> the above error is detected, no error status is set and the test 
>>> continues as if the redef had been done. So then the logic that 
>>> detects if the redef was done properly ends up failing, and that's 
>>> where the test actually indicates a failure status. You see a whole 
>>> bunch of other errors in the log because of all the checks that fail.
>>>
>>> The fix is to not abandon the test when the first CompiledMethodLoad 
>>> event is before the hot method was called. Instead just leave 
>>> fire==0 and wait for the next CompiledMethodLoad event that is 
>>> triggered after the method is called enough times to be recompiled. 
>>> I'm not sure why it was not originally done this way. Possibly the 
>>> recompilation did not happen reliably, but I have not run into this 
>>> problem. The other changes in redefclass030.c are just cleaning up 
>>> debug tracing.
>>>
>>> Another fix was to properly set the error status when 
>>> waitForRedefinitionStarted() or waitForRedefinitionCompleted() times 
>>> out, although this is just a safety net and I didn't run into any 
>>> cases where this happened after fixing the CompiledMethodLoad event 
>>> handling. So in general the changes in redefclass030.java were not 
>>> needed, but provide better error handling.
>>>
>>> thanks,
>>>
>>> Chris
>>>
>>
>
>