Fixing some Code Observer issues

[Mark Wielaard <mark at klomp dot org>]

Hi,

Both Petr and me were in Toronto last week. And he pointed out that his
bug report #4895 about "updateHit gets called for each task" was still
open and that he had to work around that for fltrace. Nothing is more
motivating than meeting your users in person, so here is a fix and an
explicit test case for that issue:
    
    frysk-core/frysk/proc/ChangeLog
    2007-10-29  Mark Wielaard  <mwielaard@redhat.com>
    
           * Observable.java (observers): Now a HashMap.
           (add): Keep count.
           (delete): Likewise.
           (contains): New method.
           (iterator): Use HashMap.keySet().
           (removeAllObservers): Likewise.
           * Task.java (notifyCodeBreakpoint): Check whether observer is
           contained in the codeObservers of this Task.
           * TestTaskObserverCode.java (testMultiTaskUpdate): New test.
           (testMultiTaskUpdateUnblockDelete): Likewise.
           (requestDummyRun): New variant that takes a Task.
    
    frysk-core/frysk/proc/live/ChangeLog
    2007-10-29  Mark Wielaard  <mwielaard@redhat.com>
    
           * LinuxTaskState.java (sendContinue): Add logging.

The actual fix should have been only a simple one-liner in
Task.notifyCodeBreakpoint() to check whether the Task was actually
monitoring this particular breakpoint address. But it was a little bit
more involved than that.

Since Observers, especially Code Observers, can be added more than once
the ltrace testcase Petr added showed a latent bug where removing a Code
observer from one location would completely remove it from that task.
That is why Observable now keeps track of how many times an Observer is
added.

Writing the testMultiTaskUpdate showed another issue when you unblock
and immediately delete a Code observer (while the Task is resumed and in
the process of stepping over the breakpoint in question). I rewrote it
into its own testcase (testMultiTaskUpdateUnblockDelete) and filed a bug
report #5229. I also added some logging to LinuxTaskState.sendContinue()
to make it easier to track these issues down.

Cheers,

Mark

diff --git a/frysk-core/frysk/proc/Observable.java b/frysk-core/frysk/proc/Observable.java
index 14c98df..248378c 100644
--- a/frysk-core/frysk/proc/Observable.java
+++ b/frysk-core/frysk/proc/Observable.java
@@ -1,6 +1,6 @@
 // This file is part of the program FRYSK.
 //
-// Copyright 2005, Red Hat Inc.
+// Copyright 2005, 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
@@ -39,8 +39,7 @@
 
 package frysk.proc;
 
-import java.util.Set;
-import java.util.HashSet;
+import java.util.HashMap;
 import java.util.Iterator;
 
 /**
@@ -58,25 +57,49 @@ public class Observable
 	this.observable = observable;
     }
     /**
-     * Set of Observer's for this Observable.
+     * Set of Observer's for this Observable.  Since an observable can
+     * contain an observer multiple times we keep a count.
      */
-    private Set observers = new HashSet ();
+    private HashMap observers = new HashMap();
     /**
      * Add Observer to this Observable.
      */
     public void add (Observer observer)
     {
-	observers.add (observer);
+      Integer count = (Integer) observers.get(observer);
+	if (count == null)
+	  count = Integer.valueOf(1);
+	else
+	  count = Integer.valueOf(count.intValue() + 1);
+	observers.put(observer, count);
 	observer.addedTo (observable); // Success
     }
     /**
      * Delete Observer from this Observable.
+     * Does nothing when observer isn't part of this observable.
      */
     public void delete (Observer observer)
     {
-	observers.remove (observer);
-	observer.deletedFrom (observable); // Success.
+        Integer count = (Integer) observers.get(observer);
+        if (count == null)
+	  return;
+        int c = count.intValue();
+        if (c == 1)
+	  observers.remove(observer);
+        else
+	  observers.put(observer, Integer.valueOf(c--));
+        observer.deletedFrom (observable); // Success.
     }
+
+    /**
+     * Whether or not the given Observer is contained in this
+     * set of Observables.
+     */
+    public boolean contains(Observer observer)
+    {
+      return observers.get(observer) != null;
+    }
+
     /**
      * Fail to add the observer.
      */
@@ -89,7 +112,7 @@ public class Observable
      */
     public Iterator iterator ()
     {
-	return observers.iterator ();
+      return observers.keySet().iterator ();
     }
     /**
      * Return the current number of observers
@@ -103,7 +126,7 @@ public class Observable
      */
     public void removeAllObservers()
     {
-      Iterator iter = observers.iterator();
+      Iterator iter = observers.keySet().iterator();
       while (iter.hasNext())
         {
           Observer observer = (Observer) iter.next();
diff --git a/frysk-core/frysk/proc/Task.java b/frysk-core/frysk/proc/Task.java
index 1d63a91..bdf5274 100644
--- a/frysk-core/frysk/proc/Task.java
+++ b/frysk-core/frysk/proc/Task.java
@@ -955,8 +955,9 @@ public abstract class Task
     while (i.hasNext())
       {
 	TaskObserver.Code observer = (TaskObserver.Code) i.next();
-	if (observer.updateHit(this, address) == Action.BLOCK)
-	  blockers.add(observer);
+	if (codeObservers.contains(observer))
+	  if (observer.updateHit(this, address) == Action.BLOCK)
+	    blockers.add(observer);
       }
     return blockers.size();
   }
diff --git a/frysk-core/frysk/proc/TestTaskObserverCode.java b/frysk-core/frysk/proc/TestTaskObserverCode.java
index 20af449..4e7b116 100644
--- a/frysk-core/frysk/proc/TestTaskObserverCode.java
+++ b/frysk-core/frysk/proc/TestTaskObserverCode.java
@@ -679,6 +679,129 @@ public class TestTaskObserverCode extends TestLib
 
   }
 
+  // Tests whether two Tasks registered on different addresses
+  // get separate update events. bug #4895
+  public void testMultiTaskUpdate() throws Exception
+  {
+    // Create a child.
+    LegacyOffspring child = LegacyOffspring.createDaemon();
+
+    // Add a Task; wait for acknowledgement.
+    child.assertSendAddCloneWaitForAcks();
+
+    task = child.findTaskUsingRefresh (true);
+    proc = task.getProc();
+
+    Collection tasks = proc.getTasks();
+    Iterator it = tasks.iterator();
+    assertTrue("task one", it.hasNext());
+    Task task1 = (Task) it.next();
+    assertTrue("task two", it.hasNext());
+    Task task2 = (Task) it.next();
+    long address1 = getFunctionEntryAddress("bp1_func");
+    long address2 = getFunctionEntryAddress("bp2_func");
+    CodeObserver code1 = new CodeObserver(task1, address1);
+    CodeObserver code2 = new CodeObserver(task2, address2);
+    task1.requestAddCodeObserver(code1, address1);
+    assertRunUntilStop("add breakpoint observer at address1");
+    task2.requestAddCodeObserver(code2, address2);
+    assertRunUntilStop("add breakpoint observer at address2");
+
+    assertFalse(code1.hit);
+    assertFalse(code2.hit);
+
+    // Request a run and watch the breakpoints get hit.
+    requestDummyRun(task1);
+    assertRunUntilStop("wait for hit 1");
+    assertTrue("hit 1", code1.hit);
+    assertFalse("not hit 2", code2.hit);
+
+    code1.hit = false;
+
+    requestDummyRun(task2);
+    assertRunUntilStop("wait for hit 2");
+    assertFalse("not hit 1", code1.hit);
+    assertTrue("hit 2", code2.hit);
+
+    code2.hit = false;
+
+    task1.requestDeleteCodeObserver(code1, address1);
+    assertRunUntilStop("remove code observer 1");
+
+    task2.requestDeleteCodeObserver(code2, address2);
+    assertRunUntilStop("remove code observer 2");
+
+    assertFalse("unblocked unhit 1", code1.hit);
+    assertFalse("unblocked unhit 2", code2.hit);
+  }
+
+  // Same as the above, but resets the code observers by unblocking
+  // before deleting, which triggers bug #5229
+  public void testMultiTaskUpdateUnblockDelete() throws Exception
+  {
+    // XXX We cannot run this test at all since on FC6/x86_64 it
+    // completely hangs the frysk-core and TestRunner.
+    if (unresolved(5229))
+      return;
+
+    // Create a child.
+    LegacyOffspring child = LegacyOffspring.createDaemon();
+
+    // Add a Task; wait for acknowledgement.
+    child.assertSendAddCloneWaitForAcks();
+
+    task = child.findTaskUsingRefresh (true);
+    proc = task.getProc();
+
+    Collection tasks = proc.getTasks();
+    Iterator it = tasks.iterator();
+    assertTrue("task one", it.hasNext());
+    Task task1 = (Task) it.next();
+    assertTrue("task two", it.hasNext());
+    Task task2 = (Task) it.next();
+    long address1 = getFunctionEntryAddress("bp1_func");
+    long address2 = getFunctionEntryAddress("bp2_func");
+    CodeObserver code1 = new CodeObserver(task1, address1);
+    CodeObserver code2 = new CodeObserver(task2, address2);
+    task1.requestAddCodeObserver(code1, address1);
+    assertRunUntilStop("add breakpoint observer at address1");
+    task2.requestAddCodeObserver(code2, address2);
+    assertRunUntilStop("add breakpoint observer at address2");
+
+    assertFalse(code1.hit);
+    assertFalse(code2.hit);
+
+    // Request a run and watch the breakpoints get hit.
+    requestDummyRun(task1);
+    assertRunUntilStop("wait for hit 1");
+    assertTrue("hit 1", code1.hit);
+    assertFalse("not hit 2", code2.hit);
+
+    // Reset 1
+    code1.hit = false;
+    task1.requestUnblock(code1);
+
+    requestDummyRun(task2);
+    assertRunUntilStop("wait for hit 2");
+    assertFalse("not hit 1", code1.hit);
+    assertTrue("hit 2", code2.hit);
+
+    // Reset 2
+    code2.hit = false;
+    // XXX - FIXME - See bug #5229 why unblocking here doesn't work.
+    if (! unresolved(5229))
+      task2.requestUnblock(code2);
+
+    task1.requestDeleteCodeObserver(code1, address1);
+    assertRunUntilStop("remove code observer 1");
+
+    task2.requestDeleteCodeObserver(code2, address2);
+    assertRunUntilStop("remove code observer 2");
+
+    assertFalse("unblocked unhit 1", code1.hit);
+    assertFalse("unblocked unhit 2", code2.hit);
+  }
+
   // Tells the child to run the dummy () function
   // which calls bp1_func () and bp2_func ().
   static final Sig dummySig = Sig.PROF;
@@ -694,6 +817,11 @@ public class TestTaskObserverCode extends TestLib
     child.signal(dummySig);
   }
   
+  void requestDummyRun(Task t)
+  {
+    Signal.tkill(t.getTid(), dummySig);
+  }
+
   /**
    * Request that that given thread of the child runs its dummy
    * function which will call the pb1 and pb1 functions. Done by
diff --git a/frysk-core/frysk/proc/live/ChangeLog b/frysk-core/frysk/proc/live/ChangeLog
index f677ef6..81d75eb 100644
--- a/frysk-core/frysk/proc/live/ChangeLog
+++ b/frysk-core/frysk/proc/live/ChangeLog
@@ -1,3 +1,7 @@
+2007-10-29  Mark Wielaard  <mwielaard@redhat.com>
+
+	* LinuxTaskState.java (sendContinue): Add logging.
+	
 2007-10-20  Mark Wielaard  <mwielaard@redhat.com>
 
 	Fixes bug #5201
diff --git a/frysk-core/frysk/proc/live/LinuxTaskState.java b/frysk-core/frysk/proc/live/LinuxTaskState.java
index f28ed0c..20e3bac 100644
--- a/frysk-core/frysk/proc/live/LinuxTaskState.java
+++ b/frysk-core/frysk/proc/live/LinuxTaskState.java
@@ -736,6 +736,8 @@ class LinuxTaskState
         Running sendContinue(Task task, int sig)
         {
 	  Breakpoint bp = task.steppingBreakpoint;
+	  logger.log (Level.FINE, "{0} sendContinue, {1}\n",
+		      new Object[] { task, bp });
 	  if (bp != null)
 	    if (! bp.isInstalled())
 	      {