[patch] Re: Instruction breakpoint-stepping testsuite (Was: Breakpoint stepping)

[Mark Wielaard <mark@klomp.org>]

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

Hi,

On Tue, 2007-07-10 at 12:50 +0200, Mark Wielaard wrote:
> I think I know how this can be made way easier. The current setup
> inserts labels at every instruction. This makes it really hard to
> combine with the generic funit-asm.h assembly instructions since it
> introduces 2 layers of macros to write a simple test. And the test has
> to be marked up fully with the labels and a path through the program
> that can be checked. But the current setup already shows an easier way
> to do this. In TestInstructions we already do a full step through the
> assembly and double check that all the labels are hit in the right
> order. But this can be turned around. We can first do this full step
> through the assembly instructions and record all addresses, then we put
> breakpoints at every instruction that we just past and do another run
> with the breakpoints inserted that should then run exactly the same as
> before (except for hitting a breakpoint at every step of course).
> 
> If we do it that way then we just need the start and end address of the
> instruction stream and can hopefully completely reuse funit-asm.h as
> long as the test writer makes sure the program always ends at the same
> address.

The above is what the following patch implements. The test is replaced
by a full .S assembly file that only uses instructions as defined in
frysk-asm.h. It creates 2 global labels istart and iend inside the test
function (instructions) that are used by the rewitten TestInstructions
harness which now assumes setting a breakpoint, stopping and removing it
(to detect istart) and instruction stepping (till iend is detected) work
(which is a good assumption to make since we have other unit tests that
explicitly test this) in setup(). Then each different test then sets
breakpoints on the addresses collected between istart and iend and the
test function is called again by funit-instructions and each test checks
that the same instruction sequence is hit independent from which Code
breakpoint (and/or other) observers are inserted.

frysk-core/frysk/proc/ChangeLog
2007-07-13  Mark Wielaard  <mwielaard@redhat.com>

   * TestInstructions.java (pid, proc, in, out, labels): Removed fields.
   (addresses, io, start_address, end_address): New fields.
   (getGlobalLabelAddress): New method.
   (Symbol): New static class.
   (setup): Rewritten to no use ForkLib and to use Code and Instruction
   observer to find code instruction path.
   (tearDown): Clear addresses.
   (testBreakAndStepInstructions): Renamed to...
   (testBreakOnStartThenStepAllInstructions): Rewriten to use addresses,
   breakpoint on first, delete, then step all.
   (testAllBreakpoints): Rewritten to use addresses.
   (testInsertAllBreakpointsAndStepAll): New test.
   (InstructionObserver.block): New field.
   (InstructionObserver.deletedFrom): Don't stop event loop.
   (InstructionObserver.updateExecuted): Check block field.
   (InstructionObserver.setBlock): New method.

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

   * funit-instructions.c: Removed.
   * funit-instructions.S: New file.

Tested on x86_64 and x86.

The instruction sequences have been extended a bit to exercise many of
the generic instructions found in frysk-asm.h. The next thing would be
extending these sequences with full architecture specific instructions.
Which has to be added when we have a full ssol instruction parser.

When cross 32-64 bit tests are enabled funit-instructions should now
just compile and run as is in either 32 or 64 bit mode.

Cheers,

Mark

[-- Attachment #2: instructions-test.patch --]
[-- Type: text/x-patch, Size: 20225 bytes --]

Index: frysk-core/frysk/pkglibdir/funit-instructions.S
===================================================================
RCS file: frysk-core/frysk/pkglibdir/funit-instructions.S
diff -N frysk-core/frysk/pkglibdir/funit-instructions.S
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ frysk-core/frysk/pkglibdir/funit-instructions.S	16 Jul 2007 09:15:37 -0000
@@ -0,0 +1,100 @@
+// 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 of 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"
+
+.data	// Start of data segment
+	
+.text	// Start of code segment
+
+	// Calls the function instruction twice so the test
+	// harness (TestInstructions) can step through it once
+	// recording all releavant information and then can run
+	// it another time to check that it runs exactly the same
+	// in the presence of Code breakpoint (and/or other) observers.	
+	.global main
+	.type main,@function
+main:
+	ENTER_MAIN
+	CALL(instructions)
+	CALL(instructions)	
+	EXIT
+
+	// Make this function global so the test harness can find
+	// the begin and end of it while stepping and setting
+	// breakpoints. The test harness will use the istart and iend
+	// markers and assume that those two addresses are used only
+	// once per run of this function and that when this function is
+	// called multiple times (see main) the exact same instruction
+	// sequence is executed.
+	.globl instructions,istart,iend
+	.type instructions,@function
+instructions:
+	ENTER
+istart:	NO_OP
+	NO_OP
+	JUMP(label1)
+label2:	JUMP(label3)
+label1: NO_OP
+	JUMP(label2)
+label3:	NO_OP
+	CALL(subfunc)
+	CALL(subfunc)
+iend:	NO_OP
+	EXIT
+
+	// Non-global helper function called from instructions to
+	// check enter/exit of functions.
+	.type subfunc,@function
+subfunc:
+	ENTER
+	LOAD_IMMED(REG2,1)
+sublabel:	
+	COMPARE_IMMED(REG2,1)
+	JUMP_NE(sublabel)
+	LOAD_IMMED(REG2,2)
+	COMPARE_IMMED(REG2,1)
+	JUMP_NE(subexit)
+	JUMP(sublabel)
+subexit:	
+	EXIT
+	
+// make the stack non-executable
+.section        .note.GNU-stack,"",@progbits
+	
Index: frysk-core/frysk/pkglibdir/funit-instructions.c
===================================================================
RCS file: frysk-core/frysk/pkglibdir/funit-instructions.c
diff -N frysk-core/frysk/pkglibdir/funit-instructions.c
--- frysk-core/frysk/pkglibdir/funit-instructions.c	3 Jul 2007 16:35:19 -0000	1.1
+++ /dev/null	1 Jan 1970 00:00:00 -0000
@@ -1,101 +0,0 @@
-// 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 of 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 <stdio.h>
-#include <stdlib.h>
-
-int
-main (int argc, char *argv[], char *envp[])
-{
-  __label__ nop1_instruction;
-  __label__ nop2_instruction;
-  __label__ nop3_instruction;
-
-  __label__ jump1_instruction;
-  __label__ jump2_instruction;
-  __label__ jump3_instruction;
-
-  __label__ done;
-
-  // Tell the tester the addresses of the assembly instruction labels
-  // in order that they should appear.
-  printf("%p\n", &&nop1_instruction);
-  printf("%p\n", &&nop2_instruction);
-  printf("%p\n", &&jump1_instruction);
-  printf("%p\n", &&nop3_instruction);
-  printf("%p\n", &&jump3_instruction);
-  printf("%p\n", &&jump2_instruction);
-  printf("%p\n", &&nop4_instruction);
-  printf("%p\n", &&done);
-  printf("%p\n", (void *) 0);
-  fflush(stdout);
-
-  // Wait till we are OK to go!
-  char c = getchar();
-  if (c == EOF)
-    exit (-1);
-
-// This works for both x86 and x86_64
-#if defined __i386__  || defined __x86_64__
- nop1_instruction:
-  asm volatile ("nop1_instruction: nop");
-  
- nop2_instruction:
-  asm volatile ("nop2_instruction: nop");
-
- jump1_instruction:
-  asm volatile ("jump1_instruction: jmp nop3_instruction");
-
- jump2_instruction:
-  asm volatile ("jump2_instruction: jmp nop4_instruction");
-
- nop3_instruction:
-  asm volatile ("nop3_instruction: nop");
-
- jump3_instruction:
-  asm volatile ("jump3_instruction: jmp jump2_instruction");
-
- nop4_instruction:
-  asm volatile ("nop4_instruction: nop");
-#else
-#error Not ported for this arch.
-#endif
- done:
-  return 0;
-}
Index: frysk-core/frysk/proc/TestInstructions.java
===================================================================
RCS file: /cvs/frysk/frysk-core/frysk/proc/TestInstructions.java,v
retrieving revision 1.1
diff -u -u -r1.1 TestInstructions.java
--- frysk-core/frysk/proc/TestInstructions.java	3 Jul 2007 16:35:19 -0000	1.1
+++ frysk-core/frysk/proc/TestInstructions.java	16 Jul 2007 09:15:37 -0000
@@ -39,29 +39,87 @@
 
 package frysk.proc;
 
-import frysk.testbed.ForkTestLib;
-
-import java.io.BufferedReader;
-import java.io.DataOutputStream;
-import java.io.InputStreamReader;
-import java.io.IOException;
-
 import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.Iterator;
 
+import lib.dw.*;
+import frysk.dwfl.*;
+
 public class TestInstructions
   extends TestLib
 {
-  // Process id, Proc, and Task representation of our test program.
-  private int pid;
-  private Proc proc;
   private Task task;
 
-  // How we communicate with the test program.
-  private BufferedReader in;
-  private DataOutputStream out;
+  private long start_address;
+  private long end_address;
+
+  private ArrayList addresses;
+
+  // Created and added by setup() in blocked state.
+  // Tests will need to delete (or unblock) it as soon as they are ready
+  // setting up their own observer.
+  private InstructionObserver io;
+
+  /**
+   * 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();
+  }
+  
+  // 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;
+    }
 
-  private ArrayList labels;
+    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;
+	}
+    }
+  }
 
   /**
    * Launch our test program and setup clean environment with the test
@@ -74,47 +132,50 @@
     // and destroyed in tearDown().
     super.setUp();
 
-    // Create a process that we will communicate with through stdin/out.
-    String command = getExecPath ("funit-instructions");
-    ForkTestLib.ForkedProcess process;
-    process = ForkTestLib.fork(new String[] { command });
-    pid = process.pid;
-    in = new BufferedReader(new InputStreamReader(process.in));
-    out = new DataOutputStream(process.out);
-    
-    // Make sure the core knows about it.
-    Manager.host.requestFindProc(new ProcId(pid), new Host.FindProc()
-      {
-	public void procFound (ProcId procId)
-	{
-	  proc = Manager.host.getProc(procId);
-	  Manager.eventLoop.requestStop();
-	}
-	public void procNotFound (ProcId procId, Exception e)
-	{
-	  fail(procId + " not found: " + e);
-	}
-      });
-    assertRunUntilStop("finding proc");
-    
-    task = proc.getMainTask();
+    // Create a process that we want to observe.
+    AttachedObserver ao = new AttachedObserver();
+    String[] cmd = new String[] { getExecPath("funit-instructions") };
+    Manager.host.requestCreateAttachedProc("/dev/null",
+                                           "/dev/null",
+                                           "/dev/null", cmd, ao);
+    assertRunUntilStop("attach then block");
+    assertTrue("AttachedObserver got Task", ao.task != null);
+
+    task = ao.task;
+
+    start_address = getGlobalLabelAddress("istart");
+    end_address = getGlobalLabelAddress("iend");
+
+    CodeObserver code = new CodeObserver(start_address);
+    task.requestAddCodeObserver(code, start_address);
+    assertRunUntilStop("inserting setup code observer");
+    task.requestDeleteAttachedObserver(ao);
+    assertRunUntilStop("setup start address code observer");
+
+    // Read the addresses of all the instructions by stepping from
+    // istart to iend.
+    addresses = new ArrayList();
+    addresses.add(Long.valueOf(start_address));
+    io = new InstructionObserver(task);
+    task.requestAddInstructionObserver(io);
+    assertRunUntilStop("setup instruction observer");
+    task.requestDeleteCodeObserver(code, start_address);
+    assertRunUntilStop("setup remove start code observer");
 
-    // Read the addresses of the labels
-    labels = new ArrayList();
-    try
+    boolean iend_seen = false;
+    while (! iend_seen)
       {
-	String line = in.readLine();
-	while (! line.equals("(nil)"))
-	  {
-	    Long label = Long.decode(line);
-	    labels.add(label);
-	    line = in.readLine();
-	  }
-      }
-    catch (IOException e)
-      {
-	fail("reading breakpoint addresses: " + e);
+	task.requestUnblock(io);
+	assertRunUntilStop("Step till iend");
+	long addr = io.getAddr();
+	Long value = Long.valueOf(addr);
+	addresses.add(value);
+	iend_seen = addr == end_address;
       }
+
+    // And make one final step out of the istart-iend range
+    task.requestUnblock(io);
+    assertRunUntilStop("Step out of range");
   }
 
   /**
@@ -124,12 +185,8 @@
    */
   public void tearDown()
   {
-    pid = -1;
-    proc = null;
     task = null;
-    in = null;
-    out = null;
-    labels = null;
+    addresses = null;
 
     Manager.eventLoop.requestStop();
 
@@ -137,65 +194,140 @@
     super.tearDown();
   }
 
-  public void testBreakAndStepInstructions() throws IOException
+  public void testBreakOnStartThenStepAllInstructions()
   {
-    long first = ((Long) labels.remove(0)).longValue();
+    Long value = (Long) addresses.remove(0);
+    long first = value.longValue();
     CodeObserver code = new CodeObserver(first);
     task.requestAddCodeObserver(code, first);
     assertRunUntilStop("first code observer added");
 
-    out.writeByte(1);
-    assertRunUntilStop("go!");
+    // Go!
+    task.requestDeleteInstructionObserver(io);
+    assertRunUntilStop("Remove setup instruction observer");
 
     assertEquals("stopped at first breakpoint",
 		 task.getIsa().pc(task), first);
 
-    InstructionObserver io = new InstructionObserver(task);
+    // Reinsert instruction observer now that we are at the start.
     task.requestAddInstructionObserver(io);
     assertRunUntilStop("add instruction observer");
 
     task.requestUnblock(code);
-    Iterator it = labels.iterator();
+    Iterator it = addresses.iterator();
     while (it.hasNext())
       {
-	long nextLabel = ((Long) it.next()).longValue();
+	long addr = ((Long) it.next()).longValue();
 	task.requestUnblock(io);
-	assertRunUntilStop("unblock for " + nextLabel);
-	assertEquals("step observer hit: " + nextLabel,
-		     io.getAddr(), nextLabel);
+	assertRunUntilStop("unblock for " + addr);
+	assertEquals("step observer hit: " + addr, io.getAddr(), addr);
       }
 
       task.requestUnblock(io);
   }
 
-  public void testAllBreakpoints() throws IOException
+  public void testAllBreakpoints()
   {
+    // Map to make sure that even if we loop only one code observer is
+    // inserted at each address.
+    HashMap codeObserver = new HashMap();
     ArrayList codeObservers = new ArrayList();
-    Iterator it = labels.iterator();
+    Iterator it = addresses.iterator();
     while (it.hasNext())
       {
-	long label = ((Long) it.next()).longValue();
-	CodeObserver code = new CodeObserver(label);
-	task.requestAddCodeObserver(code, label);
+	Long value = (Long) it.next();
+	CodeObserver code = (CodeObserver) codeObserver.get(value);
+	if (code == null)
+	  {
+	    long addr = value.longValue();
+	    code = new CodeObserver(addr);
+	    codeObserver.put(value, code);
+	    task.requestAddCodeObserver(code, addr);
+	    assertRunUntilStop("add code observer" + addr);
+	  }
 	codeObservers.add(code);
-	assertRunUntilStop("add code observer for " + label);
       }
 
-    out.writeByte(1);
-    assertRunUntilStop("go!");
+    // Go!
+    task.requestDeleteInstructionObserver(io);
+    assertRunUntilStop("Remove setup instruction observer");
 
-    it = labels.iterator();
+    it = addresses.iterator();
     while (it.hasNext())
       {
-	long label = ((Long) it.next()).longValue();
+	long addr = ((Long) it.next()).longValue();
 	CodeObserver code = (CodeObserver) codeObservers.remove(0);
-	assertEquals("code observer hit: " + label,
-		     task.getIsa().pc(task), label);
+	assertEquals("code observer hit: " + addr,
+		     task.getIsa().pc(task), addr);
 	task.requestUnblock(code);
 	if (it.hasNext())
 	  assertRunUntilStop("wait for next code observer hit after "
-			     + Long.toHexString(label));
+			     + Long.toHexString(addr));
+      }
+  }
+
+  public void testInsertAllBreakpointsAndStepAll()
+  {
+    // Map to make sure that even if we loop only one code observer is
+    // inserted at each address.
+    HashMap codeObserver = new HashMap();
+    ArrayList codeObservers = new ArrayList();
+    Iterator it = addresses.iterator();
+    while (it.hasNext())
+      {
+	Long value = (Long) it.next();
+	CodeObserver code = (CodeObserver) codeObserver.get(value);
+	if (code == null)
+	  {
+	    long addr = value.longValue();
+	    code = new CodeObserver(addr);
+	    codeObserver.put(value, code);
+	    task.requestAddCodeObserver(code, addr);
+	    assertRunUntilStop("add code observer" + addr);
+	  }
+	codeObservers.add(code);
       }
+
+    // Go!
+    io.setBlock(false);
+    task.requestUnblock(io);
+    assertRunUntilStop("Unblock setup instruction observer");
+    
+    it = addresses.iterator();
+    while (it.hasNext())
+      {
+	long addr = ((Long) it.next()).longValue();
+	CodeObserver code = (CodeObserver) codeObservers.remove(0);
+	assertEquals("code observer hit: " + addr,
+		     task.getIsa().pc(task), addr);
+	assertEquals("step observer hit: " + addr, io.getAddr(), addr);
+	task.requestUnblock(io);
+	task.requestUnblock(code);
+	if (it.hasNext())
+	  assertRunUntilStop("wait for next code observer hit after "
+			     + Long.toHexString(addr));
+      }
+  }
+  
+  class AttachedObserver implements TaskObserver.Attached
+  {
+    Task task;
+    
+    public void addedTo (Object o){ }
+    
+    public Action updateAttached (Task task)
+    {
+      this.task = task;
+      Manager.eventLoop.requestStop();
+      return Action.BLOCK;
+    }
+    
+    public void addFailed  (Object observable, Throwable w)
+    {
+      System.err.println("addFailed: " + observable + " cause: " + w);
+    }
+    
+    public void deletedFrom (Object o) { }
   }
 
   private class CodeObserver
@@ -216,7 +348,7 @@
       Manager.eventLoop.requestStop();
       return Action.BLOCK;
     }
-
+    
     public void addFailed(Object o, Throwable w)
     {
       fail("add to " + o + " failed, because " + w);
@@ -239,6 +371,8 @@
     private final Task task; 
     private long addr;
 
+    private boolean block = true;
+
     public void addedTo(Object o)
     {
       Manager.eventLoop.requestStop();
@@ -246,7 +380,6 @@
 
     public void deletedFrom(Object o)
     {
-      Manager.eventLoop.requestStop();
     }
 
     public void addFailed (Object o, Throwable w)
@@ -267,8 +400,17 @@
                                         + this.task);
       
       addr = task.getIsa().pc(task);
-      Manager.eventLoop.requestStop();
-      return Action.BLOCK;
+      if (block)
+	{
+	  Manager.eventLoop.requestStop();
+	  return Action.BLOCK;
+	}
+      return Action.CONTINUE;
+    }
+
+    public void setBlock(boolean block)
+    {
+      this.block = block;
     }
 
     public long getAddr()