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

[Chris Plummer]

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