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

[Chris Plummer]

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.

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

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