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,

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
>