Re: [SCM] master: Resequence Running and Stepping handleTrappedEvents to better flow with multiple events happening on one sigtrap.

[Phil Muldoon <pmuldoon@redhat.com>]

pmuldoon@sourceware.org wrote:

Mark and I reworked the Stepping and Running handleTrappedEvent states 
to now properly accommodate watchpoints. This required rewriting the 
flow to accumulate blockers through stepping, code software breakpoints, 
hardware data watchpoints, and unhandled sigtraps. This is code is 
pretty rich in edge cases. So there is a case building to re-architect 
it.  However doing it now, while blind the inevitable edge-cases 
watchpoints will continue to bring would be premature.  This code 
primarily alters the program flow so that  a code breakpoint, a hardware 
watchpoint, nor a step will consume a sigtrap and starve any other 
dependent events. It's passed on down to each interested event, and the 
number of blockers accumulated returned. If none of the events are 
interested in the sigtrap, the sigtrap just gets passed onto the task.

Regards

Phil
> The branch, master has been updated
>        via  cbb50856fe829fa9491ad30a89cd6ce846969693 (commit)
>       from  5f4e88713844da0810ba5d1eeaea572f7b5d9c2a (commit)
>
> Those revisions listed above that are new to this repository have
> not appeared on any other notification email.
>
> - Log -----------------------------------------------------------------
> commit cbb50856fe829fa9491ad30a89cd6ce846969693
> Author: Phil Muldoon <pmuldoon@redhat.com>
> Date:   Tue Apr 8 15:45:04 2008 +0100
>
>     Resequence Running and Stepping handleTrappedEvents to better flow with multiple events happening on one sigtrap.
>     
>     2008-04-08  Phil Muldoon  <pmuldoon@redhat.com>
>                 Mark Wielaard  <mwielaard@redhat.com>
>     
>             * LinuxPtraceTaskState.java (Running.checkWatchpoint): New.
>             (Running.handleTrappedEvent): Rewrite sequence to accumulate
>             blockers for all sigtrap events, then return.
>             (Stepping.handleTrappedEvent): Ditto.
>
> -----------------------------------------------------------------------
>
> Summary of changes:
>  frysk-core/frysk/proc/live/ChangeLog               |    8 +
>  .../frysk/proc/live/LinuxPtraceTaskState.java      |  172 +++++++++++---------
>  2 files changed, 104 insertions(+), 76 deletions(-)
>
> First 500 lines of diff:
> diff --git a/frysk-core/frysk/proc/live/ChangeLog b/frysk-core/frysk/proc/live/ChangeLog
> index edbc1ad..99e9c3a 100644
> --- a/frysk-core/frysk/proc/live/ChangeLog
> +++ b/frysk-core/frysk/proc/live/ChangeLog
> @@ -1,3 +1,11 @@
> +2008-04-08  Phil Muldoon  <pmuldoon@redhat.com>
> +	    Mark Wielaard  <mwielaard@redhat.com>
> +			
> +	* LinuxPtraceTaskState.java (Running.checkWatchpoint): New.
> +	(Running.handleTrappedEvent): Rewrite sequence to accumulate
> +	blockers for all sigtrap events, then return.
> +	(Stepping.handleTrappedEvent): Ditto.
> +	
>  2008-04-07  Stan Cox  <scox@redhat.com>
>  
>  	* LinuxPtraceHost.java (requestCreateAttachedProc): Add libs parameter.
> diff --git a/frysk-core/frysk/proc/live/LinuxPtraceTaskState.java b/frysk-core/frysk/proc/live/LinuxPtraceTaskState.java
> index 8f35740..3e590c4 100644
> --- a/frysk-core/frysk/proc/live/LinuxPtraceTaskState.java
> +++ b/frysk-core/frysk/proc/live/LinuxPtraceTaskState.java
> @@ -929,8 +929,32 @@ abstract class LinuxPtraceTaskState extends State {
>  		return blockedContinue();
>  	    return sendContinue(task, Signal.NONE);
>  	}
> +	
>  
>  	/**
> +	 * Returns the number of blockers if a watchpoint was hit, or -1 when no watchpoint 
> +	 * was hit for this Task.
> +	 */
> +	int checkWatchpoint(LinuxPtraceTask task) {
> +	    int blockers = -1;
> +	    // First test if this is a watchpoint event.
> +	    WatchpointFunctions watchpointFunction = WatchpointFunctionFactory.
> +	    	getWatchpointFunctions(task.getISA());
> +	    for (int i=0; i<watchpointFunction.getWatchpointCount();  i++) {
> +		// Test if a watchpoint has fired
> +		if (watchpointFunction.hasWatchpointTriggered(task, i)) {
> +		    if (blockers == -1)
> +			blockers = 0;
> +		    frysk.isa.watchpoints.Watchpoint trigger = watchpointFunction.readWatchpoint(task, i);
> +		    blockers += task.notifyWatchpoint(trigger.getAddress(), trigger.getRange());
> +		    watchpointFunction.resetWatchpoint(task, i);
> +		}
> +	    }
> +	    
> +	    return blockers;
> +	}
> +	
> +	/**
>  	 * Handles traps caused by breakpoints or instruction
>  	 * stepping. If there are any Code observers at the address of
>  	 * the trap they get notified. If none of the Code observers
> @@ -943,24 +967,7 @@ abstract class LinuxPtraceTaskState extends State {
>  	 */
>  	LinuxPtraceTaskState handleTrappedEvent(LinuxPtraceTask task) {
>  	    fine.log("handleTrappedEvent", task);
> -	  
> -	    // First test if this is a watchpoint event.
> -	    WatchpointFunctions watchpointFunction = WatchpointFunctionFactory.getWatchpointFunctions(task.getISA());
> -	    for (int i=0; i<watchpointFunction.getWatchpointCount();  i++) {
> -		// Test if a watchpoint has fired
> -		if (watchpointFunction.hasWatchpointTriggered(task, i)) {
> -		    frysk.isa.watchpoints.Watchpoint trigger = watchpointFunction.readWatchpoint(task, i);
> -		    watchpointFunction.resetWatchpoint(task, i);
> -		    int blockers = task.notifyWatchpoint(trigger.getAddress(), trigger.getRange());
> -		    if (blockers == 0)
> -			return sendContinue(task, Signal.NONE);
> -		    else
> -			return blockedContinue;
> -		}
> -	    }
>  
> -	    Isa isa;
> -	    isa = task.getIsaFIXME();
>  	    // First see if this was just an indication the we stepped.
>  	    // And see if we were stepping a breakpoint.  Or whether we
>  	    // installed a breakpoint at the address.  Otherwise it is a
> @@ -969,33 +976,49 @@ abstract class LinuxPtraceTaskState extends State {
>  	    // first step onto the first instruction of a just started
>  	    // task sometimes doesn't set the right task stepped flag.
>  	    // So we check and immediately clear here.
> +	    Isa isa;
> +	    isa = task.getIsaFIXME();
>  	    if (isa != null && (isa.isTaskStepped(task) || task.justStartedXXX)) {
>  		if (task.justStartedXXX) {
>  		    return stepping.handleTrappedEvent(task);
>  		} else {
> -		    System.err.println("Whoa! Wrong state for stepping: "
> -				       + this);
> -		    return sendContinue(task, Signal.NONE);
> -		}
> -	    } else {
> -		// Do we have a breakpoint installed here?
> -		long address = isa.getBreakpointAddress(task);
> -		int blockers = task.notifyCodeBreakpoint(address);
> -		if (blockers >= 0) {
> -		    // Prepare for stepping the breakpoint
> -		    setupSteppingBreakpoint(task, address);
> -		  
> -		    if (blockers == 0)
> -			return sendContinue(task, Signal.NONE);
> -		    else
> -			return blockedContinue();
> -		} else {
> -		    // This is not a trap event generated by us.  And we
> -		    // also didn't send a step request.  Deliver the
> -		    // real Trap event to the Task.
> -		    return handleSignaledEvent(task, Signal.TRAP);
> +		    throw new RuntimeException("Whoa! Wrong state for stepping: "
> +				       	       + this);
>  		}
>  	    }
> +
> +	    // Check if a watchpoint triggered.
> +	    int blockers = checkWatchpoint(task);
> +	    boolean isTrapHandled;
> +	    if (blockers != -1)
> +	        isTrapHandled = true;
> +	    else {
> +	        isTrapHandled = false;
> +	        blockers = 0;
> +	    }
> +
> +	    // Do we have a breakpoint installed here?
> +	    long address = isa.getBreakpointAddress(task);
> +	    int stepBlockers = task.notifyCodeBreakpoint(address);
> +	    if (stepBlockers >= 0) {
> +		// Prepare for stepping the breakpoint
> +		setupSteppingBreakpoint(task, address);
> +		blockers += stepBlockers;
> +		isTrapHandled = true;
> +	    } 
> +		
> +	    if (!isTrapHandled) 
> +		// This is not a trap event generated by us.  And we
> +		// also didn't send a step request.  Deliver the
> +		// real Trap event to the Task.
> +		return handleSignaledEvent(task, Signal.TRAP); 
> +	    
> +	    // Otherwise return a blocked or continue depending
> +	    // on whether something blocked.
> +	    if (blockers == 0)
> +		return sendContinue(task, Signal.NONE);
> +	    else
> +		return blockedContinue;		
>  	}
>  
>  	LinuxPtraceTaskState handleAddObservation(LinuxPtraceTask task, TaskObservation observation) {
> @@ -1080,25 +1103,19 @@ abstract class LinuxPtraceTaskState extends State {
>  	    fine.log("handleTrappedEvent", task);
>  
>  	    // First test if this is a watchpoint event.
> -	    WatchpointFunctions watchpointFunction = WatchpointFunctionFactory.getWatchpointFunctions(task.getISA());
> -	    for (int i=0; i<watchpointFunction.getWatchpointCount();  i++) {
> -		// Test if a watchpoint has fired
> -		if (watchpointFunction.hasWatchpointTriggered(task, i)) {
> -		    frysk.isa.watchpoints.Watchpoint trigger = watchpointFunction.readWatchpoint(task, i);
> -		    watchpointFunction.resetWatchpoint(task, i);
> -		    int blockers = task.notifyWatchpoint(trigger.getAddress(), trigger.getRange());
> -		    if (blockers == 0)
> -			return sendContinue(task, Signal.NONE);
> -		    else
> -			return blockedContinue;
> -		}
> +	    int blockers = checkWatchpoint(task);
> +	    boolean isTrapHandled;
> +	    if (blockers != -1)
> +	        isTrapHandled = true;
> +	    else {
> +	        isTrapHandled = false;
> +	        blockers = 0;
>  	    }
> -
> -	    
> +    
>  	    Isa isa;
>  	    isa = task.getIsaFIXME();
>        
> -	    // First see if this was just an indication the we stepped.
> +	    // Second, see if this was just an indication the we stepped.
>  	    // And see if we were stepping a breakpoint.  Or whether we
>  	    // installed a breakpoint at the address.  Otherwise it is a
>  	    // real trap event and we should treat it like a trap
> @@ -1118,15 +1135,13 @@ abstract class LinuxPtraceTaskState extends State {
>  		    task.steppingBreakpoint = null;
>  		}
>  	  
> -		if (task.notifyInstruction() > 0)
> -		    return blockedContinue();
> -		else
> -		    return sendContinue(task, Signal.NONE);
> +		blockers += task.notifyInstruction();
> +		isTrapHandled = true;
>  	    } else {
>  		// Do we have a breakpoint installed here?
>  		long address = isa.getBreakpointAddress(task);
> -		int blockers = task.notifyCodeBreakpoint(address);
> -		if (blockers >= 0) {
> +		int breakpointBlockers = task.notifyCodeBreakpoint(address);
> +		if (breakpointBlockers >= 0) {
>  		    // Sanity check
>  		    if (task.steppingBreakpoint != null)
>  			throw new RuntimeException("Already breakpoint stepping: "
> @@ -1135,11 +1150,8 @@ abstract class LinuxPtraceTaskState extends State {
>  		    // Prepare for stepping the breakpoint
>  		    setupSteppingBreakpoint(task, address);
>  	      
> -		    if (blockers == 0)
> -			return sendContinue(task, Signal.NONE);
> -		    else
> -			return blockedContinue();
> -	      
> +		    blockers += breakpointBlockers;
> +		    isTrapHandled = true;
>  		} else {
>  		    // This is not a trap event generated by us.
>  	      
> @@ -1158,20 +1170,28 @@ abstract class LinuxPtraceTaskState extends State {
>  		    // made so we should notify any instruction observers.
>  		    if ((task.sigSendXXX != Signal.NONE
>  			 || task.syscallSigretXXX
> -			 || isa.hasExecutedSpuriousTrap(task)))
> -		      if (task.notifyInstruction() > 0)
> -			return blockedContinue();
> -		      else
> -			return sendContinue(task, Signal.NONE);
> -
> -		    // Deliver the real Trap event to the Task.  This is
> -		    // somewhat weird, we are either stepping a trapping
> -		    // instruction (breakpoint) that we didn't install, or
> -		    // something is sending this process an explicit trap
> -		    // signal.
> -		    return handleSignaledEvent(task, Signal.TRAP);
> +			 || isa.hasExecutedSpuriousTrap(task))) {
> +			blockers += task.notifyInstruction();
> +			isTrapHandled = true;
> +		    }
>  		}
>  	    }
> +
> +	    if (!isTrapHandled)
> +		// Deliver the real Trap event to the Task.  This is
> +		// somewhat weird, we are either stepping a trapping
> +		// instruction (breakpoint) that we didn't install, or
> +		// something is sending this process an explicit trap
> +		// signal.
> +		return handleSignaledEvent(task, Signal.TRAP);
> +
> +	    // Otherwise return a blocked or continue depending
> +	    // on whether something blocked.
> +	    if (blockers == 0)
> +		return sendContinue(task, Signal.NONE);
> +	    else
> +		return blockedContinue;
> +
>  	}
>  
>  	private void checkBreakpointStepping(LinuxPtraceTask task) {
>
>
> hooks/post-receive
> --
> frysk system monitor/debugger
>