[patch] Record Task registering TaskObserver.Code with BreakpointAddresses

[patch] Record Task registering TaskObserver.Code with BreakpointAddresses

[Mark Wielaard]

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

Hi,

This solves a long standing bug (#5234) where a TaskObserver.Code was
triggered by a breakpoint being hit on a Task that it didn't originally
register with.

The problem was that the BreakpointAddresses didn't have the original
Task against which the TaskObserver.Code was registered. And originally
it was thought to not be such a big deal if a TaskObserver.Code
interface saw all breakpoint events in a Proc. It could check that it
was given the right Task in its updateHit() anyway. But this required
some ugly workarounds in some cases which are now not necessary anymore.

We now have a little static final helper class CodeObserver that records
the original Task and TaskObserver.Code so that notifyCodeObserver() can
check whether the Task (and observer) were registered for this address.
Note that it still must report zero, and not -1 if there are any
breakpoints installed, even if it wasn't for this particular Task
because the ptrace state machine depends on that to know whether or not
to go past an installed breakpoint on the current address. The patch
also marks the BreakpointAddresses class and (helper) methods as package
private.

frysk-core/frysk/proc/live/ChangeLog
2008-04-21  Mark Wielaard  <mwielaard@redhat.com>

    * BreakpointAddresses.java: Mark class and methods package private.
    (CodeObserver): New final static helper class.
    (addBreakpoint): Take CodeObserver as argument, not
    TaskObserver.Code.
    (removeBreakpoint): Likewise.
    (getCodeObservers): Return collection of CodeObservers, not
    TaskObserver.Code.
    * LinuxPtraceProc.java (BreakpointAction.run): Install or remove
    BreakpointAddresses.CodeObserver instead of bare TaskObserver.Code.
    * LinuxPtraceTask.java (notifyCodeBreakpoint): Check Task against
    BreakpointAddresses.CodeObserver.
    * TestTaskObserverCode.java (testMultiTaskUpdateCalledSeveralTimes):
    Resolved bug #5234.

The testcase testMultiTaskUpdateCalledSeveralTimes now PASSes.
No regressions.

Committed and pushed,

Mark

[-- Attachment #2: patch --]
[-- Type: text/x-patch, Size: 7097 bytes --]

diff --git a/frysk-core/frysk/proc/live/BreakpointAddresses.java b/frysk-core/frysk/proc/live/BreakpointAddresses.java
index dc59c34..1fb127a 100644
--- a/frysk-core/frysk/proc/live/BreakpointAddresses.java
+++ b/frysk-core/frysk/proc/live/BreakpointAddresses.java
@@ -42,6 +42,7 @@ package frysk.proc.live;
 
 import frysk.proc.TaskObserver;
 import frysk.proc.Proc;
+import frysk.proc.Task;
 import java.util.Collection;
 import java.util.Iterator;
 import java.util.HashMap;
@@ -62,8 +63,45 @@ import java.util.ArrayList;
  * This datastructure isn't multithread safe, it should only be called
  * from the eventloop in response to requests pending for the Proc.
  */
-public class BreakpointAddresses
+class BreakpointAddresses
 {
+
+  /**
+   * Small package private class holding Task and TaskObserver.Code
+   * interested in a particular breakpoint. Used by
+   * <code>addBreakpoint()</code>, <code>removeBreakpoint</code> and
+   * <code>getBreakpoints()</code>.
+   */
+  final static class CodeObserver
+  {
+    final Task task;
+    final TaskObserver.Code observer;
+
+    CodeObserver(Task task, TaskObserver.Code observer)
+    {
+      if (task == null || observer == null)
+	throw new NullPointerException();
+
+      this.task = task;
+      this.observer = observer;
+    }
+
+    public boolean equals(Object o)
+    {
+      if (! (o instanceof CodeObserver))
+	return false;
+
+      CodeObserver other = (CodeObserver) o;
+      return (this.task.equals(other.task)
+	      && this.observer.equals(other.observer));
+    }
+
+    public int hashCode()
+    {
+      return task.hashCode() ^ observer.hashCode();
+    }
+  }
+
   /**
    * Proc used to set breakpoints and which sents us notifications
    * when breakpoints are hit.
@@ -85,7 +123,7 @@ public class BreakpointAddresses
   /**
    * Package private constructor used by the Proc when created.
    */
-  public BreakpointAddresses(Proc proc)
+  BreakpointAddresses(Proc proc)
   {
     this.proc = proc;
     map = new HashMap();
@@ -99,7 +137,7 @@ public class BreakpointAddresses
    * added to the list of objects to notify when the breakpoint is
    * hit (and the method returns false).
    */
-  public boolean addBreakpoint(TaskObserver.Code observer, long address)
+  boolean addBreakpoint(CodeObserver observer, long address)
   {
     Breakpoint breakpoint = Breakpoint.create(address, proc);
 
@@ -129,7 +167,7 @@ public class BreakpointAddresses
    *
    * @throws IllegalArgumentException if the observer was never added.
    */
-  public boolean removeBreakpoint(TaskObserver.Code observer, long address)
+  boolean removeBreakpoint(CodeObserver observer, long address)
   {
     Breakpoint breakpoint = Breakpoint.create(address, proc);
     ArrayList list = (ArrayList) map.get(breakpoint);
@@ -149,10 +187,11 @@ public class BreakpointAddresses
 
     /**
      * Called by the Proc when it has trapped a breakpoint.  Returns a
-     * Collection of TaskObserver.Code observers interested in the given
+     * Collection of CodeObservers interested in the given
      * address or null when no Code observer was installed on this address.
+     * Package private (as is CodeObserver).
      */
-    public Collection getCodeObservers(long address) {
+    Collection getCodeObservers(long address) {
 	ArrayList observers;
 	Breakpoint breakpoint = Breakpoint.create(address, proc);
 	ArrayList list = (ArrayList) map.get(breakpoint);
@@ -165,7 +204,7 @@ public class BreakpointAddresses
 	return observers;
     }
 
-  public Breakpoint getBreakpoint(long address)
+  Breakpoint getBreakpoint(long address)
   {
     Breakpoint breakpoint = Breakpoint.create(address, proc);
     Object observer = map.get(breakpoint);
@@ -180,7 +219,7 @@ public class BreakpointAddresses
    * (possibly) installed starting at the from address up to (but not
    * including) the till address.
    */
-  public Iterator getBreakpoints(long from, long till)
+  Iterator getBreakpoints(long from, long till)
   {
     Breakpoint fromBreakpoint = Breakpoint.create(from, proc);
     Breakpoint tillBreakpoint = Breakpoint.create(till, proc);
@@ -191,9 +230,8 @@ public class BreakpointAddresses
    * Called from TaskState when the Task gets an execed event which
    * clears the whole address space.
    *
-   * XXX: Should not be public.
    */
-  public void removeAllCodeObservers()
+  void removeAllCodeObservers()
   {
     map.clear();
     breakpoints.clear();
diff --git a/frysk-core/frysk/proc/live/LinuxPtraceProc.java b/frysk-core/frysk/proc/live/LinuxPtraceProc.java
index d376fc2..14e1e00 100644
--- a/frysk-core/frysk/proc/live/LinuxPtraceProc.java
+++ b/frysk-core/frysk/proc/live/LinuxPtraceProc.java
@@ -552,14 +552,20 @@ public class LinuxPtraceProc extends LiveProc {
 
 	public void run() {
 	    if (addition) {
-		boolean mustInstall = breakpoints.addBreakpoint(code, address);
+	        BreakpointAddresses.CodeObserver observer;
+		boolean mustInstall;
+		observer = new BreakpointAddresses.CodeObserver(task, code);
+		mustInstall = breakpoints.addBreakpoint(observer, address);
 		if (mustInstall) {
 		    Breakpoint breakpoint;
 		    breakpoint = Breakpoint.create(address, LinuxPtraceProc.this);
 		    breakpoint.install(task);
 		}
 	    } else {
-		boolean mustRemove = breakpoints.removeBreakpoint(code, address);
+	        BreakpointAddresses.CodeObserver observer;
+		boolean mustRemove;
+		observer = new BreakpointAddresses.CodeObserver(task, code);
+		mustRemove = breakpoints.removeBreakpoint(observer, address);
 		if (mustRemove) {
 		    Breakpoint breakpoint;
 		    breakpoint = Breakpoint.create(address, LinuxPtraceProc.this);
diff --git a/frysk-core/frysk/proc/live/LinuxPtraceTask.java b/frysk-core/frysk/proc/live/LinuxPtraceTask.java
index 2121018..397f69b 100644
--- a/frysk-core/frysk/proc/live/LinuxPtraceTask.java
+++ b/frysk-core/frysk/proc/live/LinuxPtraceTask.java
@@ -966,10 +966,11 @@ public class LinuxPtraceTask extends LiveTask {
 
 	Iterator i = observers.iterator();
 	while (i.hasNext()) {
-	    TaskObserver.Code observer = (TaskObserver.Code) i.next();
-	    if (codeObservers.contains(observer))
-		if (observer.updateHit(this, address) == Action.BLOCK)
-		    blockers.add(observer);
+	    BreakpointAddresses.CodeObserver co;
+	    co = (BreakpointAddresses.CodeObserver) i.next();
+	    if (co.task.equals(this))
+		if (co.observer.updateHit(this, address) == Action.BLOCK)
+		    blockers.add(co.observer);
 	}
 	return blockers.size();
     }
diff --git a/frysk-core/frysk/proc/live/TestTaskObserverCode.java b/frysk-core/frysk/proc/live/TestTaskObserverCode.java
index 62108f2..db209b8 100644
--- a/frysk-core/frysk/proc/live/TestTaskObserverCode.java
+++ b/frysk-core/frysk/proc/live/TestTaskObserverCode.java
@@ -763,9 +763,6 @@ public class TestTaskObserverCode extends TestLib
   // get separate update events. bug #5234
   public void testMultiTaskUpdateCalledSeveralTimes() throws Exception
   {
-    if (unresolved(5234))
-      return;
-
     // Create a child.
     LegacyOffspring child = LegacyOffspring.createDaemon();