[patch] breakpoints, stepping and signals

[patch] breakpoints, stepping and signals

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 3062 bytes --]

Hi,

This patch solves a couple of issues with stepping a breakpoint while a
signal becomes pending (in particular bug #4747 and #4889). Most of the
patch is the addition of new tests based on funit-raise.S, modeled after
funit-symbols.S, that contains several ways of raising an signal from an
instruction on which a breakpoint has been put (divde by zero, illegal
address, illegal instruction, syscall raising a terminating or ignored
signal), this complements some earlier tests written (and now enabled
with this patch) that raise an signal externally or through frysk-core
(testCodeSignalInterrupt and testInstallCodeDuringCode).

When an event interrupts our breakpoint stepping we now abort and reset
the Breakpoint if it happened before the step or finish it if it
happened after the step before handling the event. The code is pretty
conservative in reverting everything related to the breakpoint step
setup. This is necessary because we are mostly using reset stepping, so
we want to set the breakpoint instruction back quickly, or other tasks
that are still running could miss it, and in the case of out of line
stepping we currently only have one out of line address available, so we
need to return it to the Proc immediately so other tasks can use it (or
a new breakpoint in this task is hit through the signal handler for
example).

frysk-core/frysk/proc/ChangeLog
2007-08-15  Mark Wielaard  <mwielaard@redhat.com>
 
    * getSetupAddress (getSetupAddress): New method.
    (stepAbort): Likewise.
    * TestTaskObserverCode.java (testCodeSignalInterrupt): Enable test.
    (testInstallCodeDuringCode): Likewise.
    (Symbol): New static helper class.
    (getGlobalLabelAddress): New helper method.
    (breakTest): New test harness for funit-raise.
    (testBreakDivZero): New test based on breakTest.
    (testBreakIllegalAddress): Likewise.
    (testBreakIllegalInstruction): Likewise.
    (testBreakSignalTerminate): Likewise.
    (testBreakSignalIgnore): Likewise.
    (AttachedObserver.task): New field.
    (updateAttached): Set task field.
    (TerminatingObserver.task): New field.
    (TerminatingObserver.signal): Likewise.
    (TerminatingObserver.value): Likewise.
    (TerminatingObserver.updateTerminating): Set new fields.

frysk-core/frysk/proc/live/ChangeLog
2007-08-15  Mark Wielaard  <mwielaard@redhat.com>

    * LinuxTaskState.java (Stepping.handleTrappedpEvent): Always check
    steppingBreakpoint.
    (checkBreakpointStepping): New helper method.
    (handleSignaledEvent): Use checkBreakpointStepping before
    continuing.
    (handleStoppedEvent): Likewise.

frysk-core/frysk/pkglibdir/ChangeLog
2007-08-15  Mark Wielaard  <mwielaard@redhat.com>

    * funit-raise.S: New test.

Tested on x86_64 kernel 2.6.20 and 2.6.22 (fc6) and x86 kernel
2.6.20-xen and 2.6.22 (f7) both dual core machines. There were no
regressions, but there are currently a lot of failures on both
architectures which make comparisons of test results somewhat hard. So
please let me know if you think this broke something for you.

Cheers,

Mark

[-- Attachment #2: breakpoint-signal.patch --]
[-- Type: text/x-patch, Size: 16915 bytes --]

Index: frysk-core/frysk/proc/Breakpoint.java
===================================================================
RCS file: /cvs/frysk/frysk-core/frysk/proc/Breakpoint.java,v
retrieving revision 1.15
diff -u -r1.15 Breakpoint.java
--- frysk-core/frysk/proc/Breakpoint.java	2 Aug 2007 11:23:28 -0000	1.15
+++ frysk-core/frysk/proc/Breakpoint.java	15 Aug 2007 11:00:33 -0000
@@ -115,6 +115,18 @@
   }
 
   /**
+   * Return the address that the original instruction has been setup
+   * if the breakpoint has been setup. This can be an out of line address
+   * or if doing reset stepping the original address.
+   */
+  public long getSetupAddress()
+  {
+    if (stepping == NOT_STEPPING)
+      throw new IllegalStateException("Not currently stepping");
+    return oo_address != 0 ? oo_address : address;
+  }
+
+  /**
    * Installs breakpoint. Caller must make sure there is no breakpoint set
    * at that address yet and that install() is not called again till remove()
    * is called on it.
@@ -253,6 +265,43 @@
 	    // is available again.
 	    origInstruction.fixupExecuteOutOfLine(task, address, oo_address);
 	    proc.doneOutOfLine(oo_address);
+	    oo_address = 0;
+	  }
+	else if (stepping == SIMULATE_STEPPING)
+	  {
+	    // FIXME: See prepareStep().
+	    System.err.println("Instruction simulation not finished! "
+			       + "Already stepped next instruction. Sorry.");
+	  }
+	else if (stepping == RESET_STEPPING)
+	  {
+	    // Put the breakpoint instruction quickly back.
+	    set(task);
+	  }
+	else
+	  throw new IllegalStateException("Impossible stepping state: "
+					  + stepping);
+      }
+
+    stepping = NOT_STEPPING;
+  }
+
+  /**
+   * Aborts a breakpoint setup (interrupted while stepping).
+   */
+  public void stepAbort(Task task)
+  {
+    if (isInstalled())
+      {
+	if (stepping == NOT_STEPPING)
+	  throw new IllegalStateException("Not stepping");
+	else if (stepping == OUT_OF_LINE_STEPPING)
+	  {
+	    // No step took place, so no fixup needed. Just but
+	    // breakpoint back and cleaer oo_address for Proc.
+	    set(task);
+	    proc.doneOutOfLine(oo_address);
+	    oo_address = 0;
 	  }
 	else if (stepping == SIMULATE_STEPPING)
 	  {
Index: frysk-core/frysk/proc/TestTaskObserverCode.java
===================================================================
RCS file: /cvs/frysk/frysk-core/frysk/proc/TestTaskObserverCode.java,v
retrieving revision 1.13
diff -u -r1.13 TestTaskObserverCode.java
--- frysk-core/frysk/proc/TestTaskObserverCode.java	10 Aug 2007 11:20:02 -0000	1.13
+++ frysk-core/frysk/proc/TestTaskObserverCode.java	15 Aug 2007 11:00:34 -0000
@@ -40,11 +40,12 @@
 package frysk.proc;
 
 import inua.eio.*;
-import frysk.testbed.LegacyOffspring;
+import frysk.testbed.*;
 import frysk.sys.*;
-import frysk.testbed.Offspring;
+import frysk.dwfl.*;
 import lib.dwfl.*;
 import java.util.*;
+
 import frysk.testbed.TestLib;
 
 public class TestTaskObserverCode extends TestLib
@@ -144,9 +145,6 @@
   // testcase for bug #4747
   public void testCodeSignalInterrupt() throws Exception
   {
-    if (unresolved(4747))
-      return;
-
     // Create a child.
     child = LegacyOffspring.createDaemon();
     task = child.findTaskUsingRefresh (true);
@@ -219,9 +217,6 @@
   // Testcase for bug #4889
   public void testInstallCodeDuringCode() throws Exception
   {
-    if (unresolved(4889))
-      return;
-
     // Create a child.
     child = LegacyOffspring.createDaemon();
     task = child.findTaskUsingRefresh (true);
@@ -291,6 +286,175 @@
     assertEquals(code.deletes, 2);
   }
 
+  // Helper class since there there isn't a get symbol method in Dwfl,
+  // so we need to wrap it all in a builder pattern.
+  static class Symbol implements SymbolBuilder
+  {
+    private String name;
+    private long address;
+    
+    private boolean found;
+    
+    private Symbol()
+    {
+      // properties get set in public static get() method.
+    }
+    
+    static Symbol get(Dwfl dwfl, String name)
+    {
+      Symbol sym = new Symbol();
+      sym.name = name;
+      DwflModule[] modules = dwfl.getModules();
+      for (int i = 0; i < modules.length && ! sym.found; i++)
+	modules[i].getSymbolByName(name, sym);
+      
+      if (sym.found)
+	return sym;
+      else
+	return null;
+    }
+    
+    String getName()
+    {
+      return name;
+    }
+    
+    long getAddress()
+    {
+      return address;
+    }
+    
+    public void symbol(String name, long value, long size,
+		       int type, int bind, int visibility)
+    {
+      if (name.equals(this.name))
+	{
+	  this.address = value;
+	  this.found = true;
+	}
+    }
+  }
+
+  /**
+   * Returns the address of a global label by quering the the Proc
+   * main Task's Dwlf.
+   */
+  long getGlobalLabelAddress(String label)
+  {
+    Dwfl dwfl = DwflCache.getDwfl(task);
+    Symbol sym = Symbol.get(dwfl, label);
+    return sym.getAddress();
+  }
+
+  private void breakTest(final int argc)
+  {
+    // Translate testname to argc (used by test to select where to jump),
+    // label name to put breakpoint on, expected signal and whether or
+    // not the exit will be clean.
+    String testName;
+    int signal;
+    boolean cleanExit;
+    switch (argc)
+      {
+      case 1:
+	testName = "div_zero";
+	signal = Sig.FPE_;
+	cleanExit = false;
+	break;
+      case 2:
+	testName = "bad_addr_segv";
+	signal = Sig.SEGV_;
+	cleanExit = false;
+	break;
+      case 3:
+	testName = "bad_inst_ill";
+	signal = Sig.ILL_;
+	cleanExit = false;
+	break;
+      case 4:
+	testName = "term_sig_hup";
+	signal = Sig.HUP_;
+	cleanExit = false;
+	break;
+      case 5:
+	testName = "ign_sig_urg";
+	signal = Sig.URG_;
+	cleanExit = true;
+	break;
+      default:
+	throw new RuntimeException("No such test: " + argc);
+      }
+    String label = testName + "_label";
+
+    String[] command = new String[argc + 1];
+    command[0] =  getExecPath("funit-raise");
+    for (int i = 1; i < argc + 1; i++)
+      command[i] = Integer.toString(i);
+
+    AttachedObserver ao = new AttachedObserver();
+    Manager.host.requestCreateAttachedProc("/dev/null",
+                                           "/dev/null",
+                                           "/dev/null", command, ao);
+    assertRunUntilStop("attach then block");
+    assertTrue("AttachedObserver got Task", ao.task != null);
+
+    task = ao.task;
+
+    long address = getGlobalLabelAddress(label);
+    CodeObserver code = new CodeObserver(task, address);
+    task.requestAddCodeObserver(code, address);
+    assertRunUntilStop("add breakpoint observer");
+
+    // Delete and unblock
+    task.requestDeleteAttachedObserver(ao);
+    assertRunUntilStop("wait for breakpoint hit");
+
+    SignaledObserver so = new SignaledObserver();
+    task.requestAddSignaledObserver(so);
+    assertRunUntilStop("add signal observer");
+
+    task.requestUnblock(code);
+    assertRunUntilStop("wait for signal observer hit");
+    assertEquals(signal, so.sig);
+
+    TerminatingObserver to = new TerminatingObserver();
+    task.requestAddTerminatingObserver(to);
+    assertRunUntilStop("add terminating observer");
+
+    task.requestUnblock(so);
+    assertRunUntilStop("wait for terminating observer hit");
+    assertEquals("killed by signal", ! cleanExit, to.signal);
+    assertEquals("exit/signal value", cleanExit ? 0 : signal, to.value);
+
+    // And let it go...
+    task.requestDeleteTerminatingObserver(to);
+  }
+
+  public void testBreakDivZero()
+  {
+    breakTest(1);
+  }
+
+  public void testBreakIllegalAddress()
+  {
+    breakTest(2);
+  }
+
+  public void testBreakIllegalInstruction()
+  {
+    breakTest(3);
+  }
+
+  public void testBreakSignalTerminate()
+  {
+    breakTest(4);
+  }
+
+  public void testBreakSignalIgnore()
+  {
+    breakTest(5);
+  }
+
   // Tests that breakpoint instructions are not visible to
   // normal users of task memory (only through raw memory view).
   public void testViewBreakpointMemory() throws Exception
@@ -651,8 +815,11 @@
   static class AttachedObserver
     implements TaskObserver.Attached
   {
+    Task task;
+
     public Action updateAttached(Task task)
     {
+      this.task = task;
       Manager.eventLoop.requestStop();
       return Action.BLOCK;
     }
@@ -702,11 +869,19 @@
     }
   }
 
-  private class TerminatingObserver
+  class TerminatingObserver
     implements TaskObserver.Terminating
   {
+    Task task;
+    boolean signal;
+    int value;
+
     public Action updateTerminating (Task task, boolean signal, int value)
     {
+      this.task = task;
+      this.signal = signal;
+      this.value = value;
+
       Manager.eventLoop.requestStop();
       return Action.BLOCK;
     }
Index: frysk-core/frysk/proc/live/LinuxTaskState.java
===================================================================
RCS file: /cvs/frysk/frysk-core/frysk/proc/live/LinuxTaskState.java,v
retrieving revision 1.3
diff -u -r1.3 LinuxTaskState.java
--- frysk-core/frysk/proc/live/LinuxTaskState.java	9 Jul 2007 16:31:30 -0000	1.3
+++ frysk-core/frysk/proc/live/LinuxTaskState.java	15 Aug 2007 11:00:34 -0000
@@ -1095,7 +1095,10 @@
       // installed a breakpoint at the address.  Otherwise it is a
       // real trap event and we should treat it like a trap
       // signal.
-      if (isa.isTaskStepped(task) || task.just_started)
+      Breakpoint steppingBreakpoint = task.steppingBreakpoint;
+      if (isa.isTaskStepped(task)
+	  || steppingBreakpoint != null
+	  || task.just_started)
 	{
 	  task.just_started = false;
 	  
@@ -1103,7 +1106,6 @@
 	  // State).  Reset/Reinstall Intruction, all logic is in the
 	  // Breakpoint and associated Instruction, which will fixup
 	  // any registers for us.
-	  Breakpoint steppingBreakpoint = task.steppingBreakpoint;
 	  if (steppingBreakpoint != null)
 	    {
 	      steppingBreakpoint.stepDone(task);
@@ -1166,6 +1168,45 @@
 	    }
 	}
     }
+
+    private void checkBreakpointStepping(Task task)
+    {
+      // Since we were stepping we expected a trap event.
+      // If we were stepping a breakpoint we have to check whether
+      // or not the step occured before or after the breakpoint was
+      // taken and make sure the breakpoint it put back in place.
+      Breakpoint steppingBreakpoint = task.steppingBreakpoint;
+      if (steppingBreakpoint != null)
+	{
+	  long pc = task.getIsa().pc(task);
+	  long setupAddress = steppingBreakpoint.getSetupAddress();
+
+	  // Check whether the breakpoint was actually stepped.
+	  // In theory there are instructions that might not change the
+	  // pc after execution, these are expected to not need fixups.
+	  // If any of them would, then we can add an explicit abort()
+	  // to Instruction so they can special case themselves.
+	  if (pc != setupAddress)
+	    steppingBreakpoint.stepDone(task);
+	  else
+	    steppingBreakpoint.stepAbort(task);
+	}
+    }
+
+    public TaskState handleSignaledEvent(Task task, int sig)
+    {
+      logger.log (Level.FINE, "{0} handleSignaledEvent, signal: {1}\n",
+		  new Object[] {task, new Integer(sig)}); 
+      checkBreakpointStepping(task);
+      return super.handleSignaledEvent(task, sig);
+    }
+
+    public TaskState handleStoppedEvent (Task task)
+    {
+      logger.log (Level.FINE, "{0} handleStoppedEvent\n", new Object[] {task}); 
+      checkBreakpointStepping(task);
+      return super.handleStoppedEvent(task);
+    }
   }
 
     /**
Index: frysk-core/frysk/pkglibdir/funit-raise.S
===================================================================
RCS file: frysk-core/frysk/pkglibdir/funit-raise.S
diff -N frysk-core/frysk/pkglibdir/funit-raise.S
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ frysk-core/frysk/pkglibdir/funit-raise.S	15 Aug 2007 11:00:34 -0000
@@ -0,0 +1,161 @@
+// This file is part of the program FRYSK.
+//
+// Copyright 2007 Red Hat Inc.
+//
+// FRYSK is free software; you can redistribute it and/or modify it
+// under the terms of the GNU General Public License as published by
+// the Free Software Foundation; version 2 of the License.
+//
+// FRYSK is distributed in the hope that it will be useful, but
+// WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+// General Public License for more details.
+// 
+// You should have received a copy of the GNU General Public License
+// along with FRYSK; if not, write to the Free Software Foundation,
+// Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+// 
+// In addition, as a special exception, Red Hat, Inc. gives You the
+// additional right to link the code of FRYSK with code not covered
+// under the GNU General Public License ("Non-GPL Code") and to
+// distribute linked combinations including the two, subject to the
+// limitations in this paragraph. Non-GPL Code permitted under this
+// exception must only link to the code of FRYSK through those well
+// defined interfaces identified in the file named EXCEPTION found in
+// the source code files (the "Approved Interfaces"). The files of
+// Non-GPL Code may instantiate templates or use macros or inline
+// functions from the Approved Interfaces without causing the
+// resulting work to be covered by the GNU General Public
+// License. Only Red Hat, Inc. may make changes or additions to the
+// list of Approved Interfaces. You must obey the GNU General Public
+// License in all respects for all o;f the FRYSK code and other code
+// used in conjunction with FRYSK except the Non-GPL Code covered by
+// this exception. If you modify this file, you may extend this
+// exception to your version of the file, but you are not obligated to
+// do so. If you do not wish to provide this exception without
+// modification, you must delete this exception statement from your
+// version and license this file solely under the GPL without
+// exception.
+
+#include "frysk-asm.h"
+#include <sys/syscall.h>
+
+// Signal that terminates and signal that gets ignored by default.
+#define SIGHUP	1
+#define SIGURG	23
+
+// Architecture dependent divide by zero and illegal instruction operator.
+#if defined(__i386__) || defined(__x86_64__)
+	#define DIV_ZERO(REG)	div REG;
+	#define ILL_INST	.word 0xffff;
+#else
+	#error unsuported architecture
+#endif
+
+// Mainly used to define the global label to beak on.
+// The raise functions are jump targets for main (based on argc).
+#define RAISE_FUNCTION_START(X)		\
+	.global X 		;	\
+	.global X_label		;	\
+	.type X, @function	;	\
+	X:		
+#define RAISE_FUNCTION_END(X) .size X, .-X
+
+RAISE_FUNCTION_START(div_zero)
+	LOAD_IMMED_BYTE(REG0, 0)
+	LOAD_IMMED_BYTE(REG1, 0)
+div_zero_label:
+	DIV_ZERO(REG1)
+	NO_OP			;  // never reached
+RAISE_FUNCTION_END(div_zero)
+
+RAISE_FUNCTION_START(bad_addr_segv)
+	LOAD_IMMED_BYTE (REG0, 0)
+bad_addr_segv_label:
+	STORE (REG0, REG0)
+	NO_OP			;  // never reached
+RAISE_FUNCTION_END(bad_addr_segv)
+
+RAISE_FUNCTION_START(bad_inst_ill)
+bad_inst_ill_label:
+	ILL_INST
+	NO_OP			;  // never reached
+RAISE_FUNCTION_END(bad_inst_ill)
+
+RAISE_FUNCTION_START(term_sig_hup)
+        LOAD_IMMED_BYTE(REG0, SYS_gettid)
+	SYSCALL
+	LOAD_IMMED_BYTE(REG1,0)
+	ADD(REG0,REG1)
+	LOAD_IMMED_BYTE(REG2,SIGHUP)
+        LOAD_IMMED_BYTE(REG0, SYS_tkill)
+term_sig_hup_label:
+	SYSCALL
+        NO_OP                   ;   // never reached
+RAISE_FUNCTION_END(term_sig_hup)
+
+RAISE_FUNCTION_START(ign_sig_urg)
+        LOAD_IMMED_BYTE(REG0, SYS_gettid)
+	SYSCALL
+	LOAD_IMMED_BYTE(REG1,0)
+	ADD(REG0,REG1)
+	LOAD_IMMED_BYTE(REG2,SIGURG)
+        LOAD_IMMED_BYTE(REG0, SYS_tkill)
+ign_sig_urg_label:	
+	SYSCALL
+        NO_OP                   ;   // Reached!
+	// Exit nicely
+	LOAD_IMMED_BYTE(REG0, 0)	
+	JUMP(exit_main)
+RAISE_FUNCTION_END(ign_sig_urg)
+	
+FUNCTION_BEGIN(main,0)
+	MAIN_PROLOGUE(0)
+
+	// Get argc (REG1) and see what the user wanted.
+	LOAD_IMMED_BYTE(REG3, 2)
+	COMPARE(REG3, REG1)
+	JUMP_EQ(div_zero)
+
+	LOAD_IMMED_BYTE(REG3, 3)
+	COMPARE(REG3, REG1)
+	JUMP_EQ(bad_addr_segv)
+
+	LOAD_IMMED_BYTE(REG3, 4)
+	COMPARE(REG3, REG1)
+	JUMP_EQ(bad_inst_ill)
+
+	LOAD_IMMED_BYTE(REG3, 5)
+	COMPARE(REG3, REG1)
+	JUMP_EQ(term_sig_hup)
+
+	LOAD_IMMED_BYTE(REG3, 6)
+	COMPARE(REG3, REG1)
+	JUMP_EQ(ign_sig_urg)
+
+	// Unknown, print usage.
+	LOAD_IMMED_BYTE(REG0, SYS_write)
+	LOAD_IMMED_BYTE(REG1, 1)
+	LOAD_IMMED_WORD(REG2, usage)
+	LOAD_IMMED_WORD(REG3, .usage-usage)
+	SYSCALL
+
+	// Non-zero return
+	LOAD_IMMED_BYTE(REG0, 1)
+	
+exit_main:	
+	MAIN_EPILOGUE(0)
+	FUNCTION_RETURN(main,0)
+FUNCTION_END(main,0)
+
+usage:	.asciz "Usage:\r\n\
+	ARG1 ...\r\n\
+The number of arguments determines the function which this program\r\n\
+calls and how it will raise a signal/trap because:\r\n\
+1: Arithmetic exception (div_zero)\r\n\
+2: Illegal address (bad_addr_segv)\r\n\
+3: Illegal instruction (bad_inst_ill)\r\n\
+4: SIGHUP terminate signal (term_sig_hup)\r\n\
+5: SIGURG ignore signal (ign_sig_urg)\r\n\
+"
+.usage: