From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4854 invoked by alias); 15 Aug 2007 11:05:21 -0000 Received: (qmail 4512 invoked by uid 22791); 15 Aug 2007 11:05:16 -0000 X-Spam-Status: No, hits=-0.0 required=5.0 tests=AWL,BAYES_00,DK_POLICY_SIGNSOME,FORGED_RCVD_HELO,TW_DW,URIBL_BLACK X-Spam-Check-By: sourceware.org Received: from wildebeest.demon.nl (HELO gnu.wildebeest.org) (83.160.170.119) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 15 Aug 2007 11:05:11 +0000 Received: from dijkstra.wildebeest.org ([192.168.1.29]) by gnu.wildebeest.org with esmtp (Exim 4.63) (envelope-from ) id 1ILGgT-00006I-UM for frysk@sourceware.org; Wed, 15 Aug 2007 13:05:08 +0200 Subject: [patch] breakpoints, stepping and signals From: Mark Wielaard To: frysk@sourceware.org Content-Type: multipart/mixed; boundary="=-pQVW+/ZMt/aR8UFN5gJx" Date: Wed, 15 Aug 2007 11:05:00 -0000 Message-Id: <1187175905.3791.17.camel@dijkstra.wildebeest.org> Mime-Version: 1.0 X-Mailer: Evolution 2.8.3 (2.8.3-2.fc6) X-Spam-Score: -4.4 (----) X-Virus-Checked: Checked by ClamAV on sourceware.org X-IsSubscribed: yes Mailing-List: contact frysk-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Post: List-Help: , Sender: frysk-owner@sourceware.org X-SW-Source: 2007-q3/txt/msg00301.txt.bz2 --=-pQVW+/ZMt/aR8UFN5gJx Content-Type: text/plain Content-Transfer-Encoding: 7bit Content-length: 3062 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 * 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 * 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 * 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 --=-pQVW+/ZMt/aR8UFN5gJx Content-Disposition: inline; filename=breakpoint-signal.patch Content-Type: text/x-patch; name=breakpoint-signal.patch; charset=UTF-8 Content-Transfer-Encoding: 7bit Content-length: 16915 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 + +// 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: --=-pQVW+/ZMt/aR8UFN5gJx--