RFR: JDK-8051349: nsk/jvmti/scenarios/sampling/SP06/sp06t003 fails in nightly

[Gary Adams]

On 12/13/18, 11:51 AM, Chris Plummer wrote:
> Hi Gary,
>
> Unfortunately GetStackTrace() returns the top frame as the first frame 
> in the array. Thus if the thread is not suspended and later you call 
> GetFrameLocation() for some depth, there's no way to make sure the 
> frame at that depth is the same frame at that depth in the array 
> returned by GetStackTrace() unless you first suspend the frame before 
> calling GetFrameLocation(). You would also need to call 
> GetFrameCount() while suspended, compare that with the number of 
> frames returned by GetStackTrace(), and make any needed depth 
> adjustments. But since I think the intent is to not suspend the thread 
> here, it probably does not make sense to do that, and instead accept 
> that there might be some errors as you have done.
I'm not totally comfortable with these attempted operations and then
bypassing the test failure if the threads were not suspended.
Would it be better to only perform check  thread operations on the
suspended threads and to totally skip the operations that can not
be safely performed when the thread is not suspended.
>
> One improvement I would like to see for your fix is to only ignore 
> JVMTI_ERROR_NO_MORE_FRAMES rather than ignore all failures when 
> suspended == NSK_TRUE.
The current layering of the macros and verify routines do not lend 
themselves
to this sort of selective error checking. I'd probably file a follow up 
bug to
address specific error checking.
>
> Also, I don't think you want the suspended check here:
>
>  348         /* check if expected method frame found */
>  349         if ((suspended == NSK_TRUE) && (found <= 0)) {
>
> The check for finding the method is:
>
>  341             if (frameStack[j].method == threadsDesc[i].method) {
>
> Since frameStack[] is returned by GetStackTrace(), it is not impacted 
> when not suspended, and the expected method should always be in 
> frameStack[] somewhere. The issue is only with using 
> GetFrameLocation() to correlate what is in the result of GetStackTrace().
Agreed. The early continuation was bypassing the found method checking.

BTW, another failure has been detected in sp06t001. This time the 
threads are suspended,
but I believe there is a race between thread start and the call to 
interrupt() the thread.
I think there may be some confusion over which thread is invoking the 
interrupt() call.
It is running on the main thread from the code after a call to start the 
thread, but the
thread may not have run when the interrupt is requested.

public class sp06t001 extends DebugeeClass {
...

         // create threads list
         threads = new sp06t001Thread[] {
             new sp06t001ThreadRunning("threadRunning", log),
             new sp06t001ThreadEntering("threadEntering", log),
             new sp06t001ThreadWaiting("threadWaiting", log),
             new sp06t001ThreadSleeping("threadSleeping", log),
             new 
sp06t001ThreadRunningInterrupted("threadRunningInterrupted", log),
             new sp06t001ThreadRunningNative("threadRunningNative", log)
         };

         // run threads
         log.display("Starting tested threads");
         try {
             synchronized (endingMonitor) {
                 // start threads (except first one)
                 for (int i = 0; i < threads.length; i++) {
                     threads[i].start();
                     if (!threads[i].checkReady()) {
                         throw new Failure("Unable to prepare thread ..."
...

class sp06t001ThreadRunningInterrupted extends sp06t001Thread {
...
     public boolean checkReady() {
         // interrupt thread on wait()
         synchronized (waitingMonitor) {
             interrupt();
         }
         return true;
     }
>
> thanks,
>
> Chris
>
> On 12/13/18 5:25 AM, Gary Adams wrote:
>> While testing I ran into another of the related failures that were
>> associated with the original bug.
>> The following fake exception stacktrace is for failure analysis.
>> nsk.share.Fake_Exception_for_RULE_Creation: (sp02t003.cpp:313) jvmti->GetFrameLocation(threadsDesc[i].thread, j,&qMethod,&qLocation)
>> 	at nsk_lvcomplain(nsk_tools.cpp:172)
>> # ERROR: sp02t003.cpp, 313: jvmti->GetFrameLocation(threadsDesc[i].thread, j,&qMethod,&qLocation)
>> #   jvmti error: code=31, name=JVMTI_ERROR_NO_MORE_FRAMES
>> - sp02t003.cpp, 310:       3 frame: method: 0x7f225042b2d8, location: 16
>> # ERROR: sp02t003.cpp, 313: jvmti->GetFrameLocation(threadsDesc[i].thread, j,&qMethod,&qLocation)
>> #   jvmti error: code=31, name=JVMTI_ERROR_NO_MORE_FRAMES
>> # ERROR: sp02t003.cpp, 350: No expected method frame for not suspended thread #4 (threadRunningInterrupted)
>> In this particular failure, the GetFrameLocation call failed, because 
>> the frame was no longer
>> accessible.
>>
>> If the threads are not suspended, the GetFrameLocation may fail,
>> or if it passes, the qMethod and qLocation could belong to another
>> frame.
>>
>> I've updated the webrev to allow the call to GetFrameLocation, but to 
>> only
>> report an error if the threads are suspended. Similarly, the checks 
>> to compare
>> qMethod and qLocation will be skipped, if the threads are not suspended.
>> And the final comparison that the method was found will only be an error
>> if the threads are suspended.
>>
>>   Webrev: http://cr.openjdk.java.net/~gadams/8051349/webrev.01/
>>
>> On 12/12/18, 11:54 AM, Gary Adams wrote:
>>> After some testing with nsk verbose messages enabled,
>>> it is clear that this test is failing in checkThreads when the
>>> location did not match between the call to GetStackTrace
>>> and GetFrameLocation. For the tests that are run when the threads
>>> have not been suspended, there is no guarantee the locations
>>> will match.
>>>
>>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8051349
>>>   Webrev: http://cr.openjdk.java.net/~gadams/8051349/webrev.00/
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181214/478745bf/attachment.html>