From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13689 invoked by alias); 22 Apr 2008 16:56:04 -0000 Received: (qmail 13674 invoked by uid 22791); 22 Apr 2008 16:56:02 -0000 X-Spam-Status: No, hits=0.3 required=5.0 tests=AWL,BAYES_40,J_CHICKENPOX_48,J_CHICKENPOX_54,J_CHICKENPOX_64,RCVD_IN_SORBS_DUL,RDNS_DYNAMIC X-Spam-Check-By: sourceware.org Received: from cc1341701-a.deven1.ov.home.nl (HELO gnu.wildebeest.org) (82.72.26.122) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 22 Apr 2008 16:55:45 +0000 Received: from [192.168.1.150] (helo=[192.168.1.29]) by gnu.wildebeest.org with esmtp (Exim 4.63) (envelope-from ) id 1JoLmO-0002GC-Un; Tue, 22 Apr 2008 18:55:42 +0200 Subject: Reset breakpoints on fork From: Mark Wielaard To: frysk Cc: Roland McGrath Content-Type: multipart/mixed; boundary="=-dW53poOn5DTwMbAVF8yj" Date: Fri, 25 Apr 2008 17:52:00 -0000 Message-Id: <1208883122.3426.18.camel@dijkstra.wildebeest.org> Mime-Version: 1.0 X-Mailer: Evolution 2.12.3 (2.12.3-4.fc8) X-Spam-Score: -4.4 (----) 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: 2008-q2/txt/msg00063.txt.bz2 --=-dW53poOn5DTwMbAVF8yj Content-Type: text/plain Content-Transfer-Encoding: 7bit Content-length: 2405 Hi, This fixes a long standing issue with breakpoints being left in the inferior on a fork. We used to get away with it most times since often a fork is followed by an exec() which would just clear everything. But there are actually single threaded applications that use fork as a way to have a poor mans clone(). Roland, I CCed you in the hope that you could maybe point out some nice way now or in the future to do this. The patch itself might not be very interesting to you. The way we model Tasks and events means we do this in three stages. First we get a fork event and capture all the (software) breakpoint addresses, then we already create the Proc and then the main Task based on the pid given in the event, then we wait for an (stopped) event on this now Task, then we go over all the breakpoints left and patch in the original instructions again. This of course isn't very efficient since I assume the kernel could help us out here since it know the pages we touched through ptrace poke to set the breakpoints in the first place. If there was some way to have an event from the kernel where we got the forking and forked tasks in a quiescent state (at the same time) and then could inspect both and tell the kernel whether or not to copy the modified or original code pages that would be much better. Bonus points for having advanced knowledge of a pending exec event, which would wipe away any changes we do anyway (although I don't see how this would be possible). frysk-core/frysk/proc/live/ChangeLog 2008-04-22 Mark Wielaard * Breakpoint.java (reset): Make package private. (cloneForProc): New method. * BreakpointAddresses.java (cloneForProc): New method. (clearAllBreakpoints): New method. * LinuxPtraceProc.java (LinuxPtraceProc(Task,ProcessIdentifier)): Set breakpoints through forkingProc.breakpoints.cloneForProc(). (breakpoints): Make package private. * LinuxPtraceTask.java (LinuxPtraceTask(LinuxPtraceTask, LinuxPtraceProc,TaskAttachedObserverXXX)): Set currentISA. * LinuxPtraceTaskState.java (StartMainTask.wantToDetach.handleAttach): Clear breakpoints. (StartMainTask.wantToDetach.StoppedEvent): Likewise. * TestTaskObserverCode.java (testCodeOverFork): Resolve bug #5331. No regressions on fedora 8 x86 and x86_64. The testCodeOverFork now passes. Committed and pushed, Mark --=-dW53poOn5DTwMbAVF8yj Content-Disposition: inline; filename=breakpoints-on-fork.patch Content-Type: text/x-patch; name=breakpoints-on-fork.patch; charset=UTF-8 Content-Transfer-Encoding: 7bit Content-length: 7932 diff --git a/frysk-core/frysk/proc/live/Breakpoint.java b/frysk-core/frysk/proc/live/Breakpoint.java index 0372b6d..54ea21f 100644 --- a/frysk-core/frysk/proc/live/Breakpoint.java +++ b/frysk-core/frysk/proc/live/Breakpoint.java @@ -175,8 +175,11 @@ public class Breakpoint implements Comparable /** * Actually removes the breakpoint. + * Package private for BreakpointAddresses.clearBreakpoints(), + * all other callers should use remove(). + * */ - private void reset(Task task) + void reset(Task task) { ByteBuffer buffer = ((LinuxPtraceTask)task).getRawMemory(); buffer.position(address); @@ -379,4 +382,22 @@ public class Breakpoint implements Comparable return this.getClass().getName() + "[proc=" + proc + ", address=0x" + Long.toHexString(address) + "]"; } + + /** + * Makes a clone of this Breakpoint, but for a particular Proc. This + * is used by the BreakPointAddresses class to make a clone of all + * breakpoints set on a froked proc. First makes sure to have the + * canonical (installed) breakpoint. Then makes a copy and adds the + * original instruction. See BreakPointAddresses cloneForProc() and + * clearAllAddresses for more explanation. + */ + Breakpoint cloneForProc(Proc proc) + { + // Make sure to have the canonical (installed) breakpoint. + // Then make a copy and add the original instruction. + Breakpoint breakpoint = create(this.getAddress(), this.getProc()); + Breakpoint result = new Breakpoint(breakpoint.address, proc); + result.origInstruction = breakpoint.origInstruction; + return result; + } } diff --git a/frysk-core/frysk/proc/live/BreakpointAddresses.java b/frysk-core/frysk/proc/live/BreakpointAddresses.java index 1fb127a..4564d40 100644 --- a/frysk-core/frysk/proc/live/BreakpointAddresses.java +++ b/frysk-core/frysk/proc/live/BreakpointAddresses.java @@ -236,4 +236,41 @@ class BreakpointAddresses map.clear(); breakpoints.clear(); } + + /** + * Sets up a BreakpointAddresses map for a forked proc. This sets + * the list of breakpoint addresses (plus original instructions) to + * be reset later with clearAllBreakpoints(). See comments in + * LinuxPtraceProc, LinuxPtraceTask and + * LinuxPtraceTaskState.StartMainTask.wantToDetach for the sequence + * of events. + */ + BreakpointAddresses cloneForProc(Proc proc) + { + BreakpointAddresses addresses = new BreakpointAddresses(proc); + Iterator it = breakpoints.iterator(); + while (it.hasNext()) + { + Breakpoint breakpoint = (Breakpoint) it.next(); + breakpoint = breakpoint.cloneForProc(proc); + addresses.breakpoints.add(breakpoint); + } + return addresses; + } + + /** + * Actually resets the breakpoints in the breakpoint addresses left + * by cloneForProc(). Called when the main task of the forked proc + * is ready in LinuxPtraceTaskState.StartMainTask.wantToDetach. + */ + void clearAllBreakpoints() + { + Iterator it = breakpoints.iterator(); + while (it.hasNext()) + { + Breakpoint breakpoint = (Breakpoint) it.next(); + breakpoint.reset(proc.getMainTask()); + } + removeAllCodeObservers(); + } } diff --git a/frysk-core/frysk/proc/live/LinuxPtraceProc.java b/frysk-core/frysk/proc/live/LinuxPtraceProc.java index 14e1e00..c9a22f0 100644 --- a/frysk-core/frysk/proc/live/LinuxPtraceProc.java +++ b/frysk-core/frysk/proc/live/LinuxPtraceProc.java @@ -97,7 +97,8 @@ public class LinuxPtraceProc extends LiveProc { public LinuxPtraceProc(Task task, ProcessIdentifier fork) { super(task, fork); this.newState = LinuxPtraceProcState.initial(true); - this.breakpoints = new BreakpointAddresses(this); + LinuxPtraceProc forkingProc = (LinuxPtraceProc) task.getProc(); + this.breakpoints = forkingProc.breakpoints.cloneForProc(this); this.watchpoints = new WatchpointAddresses(this); ((LinuxPtraceHost)getHost()).addProc(this); } @@ -833,9 +834,13 @@ public class LinuxPtraceProc extends LiveProc { } /** - * XXX: Should not be public. + * The addresses on which we have installed breakpoints (with + * associated Tasks and TaskObserver.Code listeners). Package + * private for access in LinuxPtraceTask constructor that creates + * a forked task and the LinuxPtraceTaskState.MainTask.wantToDetach + * class to clearthem. See the comments there. */ - public final BreakpointAddresses breakpoints; + final BreakpointAddresses breakpoints; // List of available addresses for out of line stepping. // Used a lock in getOutOfLineAddress() and doneOutOfLine(). diff --git a/frysk-core/frysk/proc/live/LinuxPtraceTask.java b/frysk-core/frysk/proc/live/LinuxPtraceTask.java index 397f69b..ede0c2b 100644 --- a/frysk-core/frysk/proc/live/LinuxPtraceTask.java +++ b/frysk-core/frysk/proc/live/LinuxPtraceTask.java @@ -110,6 +110,19 @@ public class LinuxPtraceTask extends LiveTask { ((LinuxPtraceHost)proc.getHost()).putTask(tid, this); ((LinuxPtraceProc)proc).addTask(this); newState = LinuxPtraceTaskState.mainState(); + // See the various FIXMEs below around the isa, ISA, + // currentISA, getISA(), getIsaFIXME() and clearIsa(). The + // current design is such that the isa isn't a constant of a + // proc (actually task), which means we cannot remove + // breakpoint instructions from the fork (the breakpoint + // instruction is an intrinsic of the isa). Luckily we know + // that the fork will have the same isa as the task it forked + // from, so we explicitly set it now. We cannot do the + // breakpoint resetting here (the LinuxPtraceTask is created, + // but not fully setup yet), we do that in the + // LinuxPtraceTaskState.StartMainTask.wantToDetach class + // handlers. + currentISA = forkingTask.currentISA; if (attached != null) { TaskObservation ob = new TaskObservation(this, attachedObservers, attached, true) { diff --git a/frysk-core/frysk/proc/live/LinuxPtraceTaskState.java b/frysk-core/frysk/proc/live/LinuxPtraceTaskState.java index 50ebabd..3f9b058 100644 --- a/frysk-core/frysk/proc/live/LinuxPtraceTaskState.java +++ b/frysk-core/frysk/proc/live/LinuxPtraceTaskState.java @@ -459,13 +459,21 @@ abstract class LinuxPtraceTaskState extends State { new StartMainTask("wantToDetach") { LinuxPtraceTaskState handleAttach(LinuxPtraceTask task) { fine.log("handleAttach", task); - ((LinuxPtraceProc)task.getProc()) - .performTaskAttachCompleted(task); + LinuxPtraceProc proc = (LinuxPtraceProc) task.getProc(); + // Clear left over breakpoints here. They were + // captured when the Proc was created. + proc.breakpoints.clearAllBreakpoints(); + proc.performTaskAttachCompleted(task); return StartMainTask.wantToAttach; } LinuxPtraceTaskState handleStoppedEvent(LinuxPtraceTask task, Signal signal) { if (signal == Signal.STOP || signal == Signal.TRAP) { + LinuxPtraceProc proc; + proc = (LinuxPtraceProc) task.getProc(); + // Clear left over breakpoints here. They were + // captured when the Proc was created. + proc.breakpoints.clearAllBreakpoints(); task.initializeAttachedState(); if (task.notifyForkedOffspring() > 0) { return StartMainTask.detachBlocked; diff --git a/frysk-core/frysk/proc/live/TestTaskObserverCode.java b/frysk-core/frysk/proc/live/TestTaskObserverCode.java index db209b8..f7e6f6a 100644 --- a/frysk-core/frysk/proc/live/TestTaskObserverCode.java +++ b/frysk-core/frysk/proc/live/TestTaskObserverCode.java @@ -886,9 +886,6 @@ public class TestTaskObserverCode extends TestLib // after the fork. public void testCodeOverFork() throws Exception { - if (unresolved(5331)) - return; - String[] argv = {getExecPath ("funit-fib-fork"), "2"}; child = null; DaemonBlockedAtEntry child = new DaemonBlockedAtEntry(argv); --=-dW53poOn5DTwMbAVF8yj--