This is the mail archive of the frysk@sources.redhat.com mailing list for the frysk project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: One frysk-core patch to add breakpoint support in PPC64.


Hi Yong Zheng,

On Tue, 2006-08-22 at 12:21 +0800, Yong Zheng wrote:
> Based on the codes submitted by Mark, we do one patch to add the
> breakpoint support in PPC64. At the meanwhile, we find some attributes
> in Breakpoint.java are dependent on ISA, so we do some modifications on
> Breakpoint.java and TaskState.java.

Great to see you were able to adapt the code to ppc64 and improve the
design at the same time. I have only a few small comments.

> Index: frysk-core/frysk/proc/Isa.java
> ===================================================================
> RCS file: /cvs/frysk/frysk-core/frysk/proc/Isa.java,v
> retrieving revision 1.10
> diff -u -r1.10 Isa.java
> --- frysk-core/frysk/proc/Isa.java      8 Aug 2006 08:27:11 -0000       1.10
> +++ frysk-core/frysk/proc/Isa.java      22 Aug 2006 03:19:06 -0000
> @@ -57,7 +57,26 @@
>  
>    int getWordSize();
>    ByteOrder getByteOrder();
> -
> +  
> +  /**
> +   * Get the breakpoint instruction.
> +   * 
> +   * @return bytes[] the instruction of the ISA.
> +   */
> +  byte[] getBpInstruction();

It might be nice to actually name this getBreakpointInstruction(). It is
a bit long, but much clearer.

> +   * The function will take different actions according to task's ISA.
> +   * 
> +   */
> +  long getBreakpointAddress(Task task);
> +  
>    // int addressSize;
>    // InstructionSet;
>    // FloatingPointFormat;
> Index: frysk-core/frysk/proc/TaskState.java
> ===================================================================
> RCS file: /cvs/frysk/frysk-core/frysk/proc/TaskState.java,v
> retrieving revision 1.102
> diff -u -r1.102 TaskState.java
> --- frysk-core/frysk/proc/TaskState.java        15 Aug 2006 17:16:58 -0000      1.102
> +++ frysk-core/frysk/proc/TaskState.java        22 Aug 2006 03:19:56 -0000
> @@ -1031,7 +1031,7 @@
>         long address;
>         try
>           {
> -           address = task.getIsa().pc(task) - 1;
> +           address = task.getIsa().getBreakpointAddress(task);
>           }

There is a FIXME comment just above this section of code. You fixed
it! :) So please also remove that comment.

>    /**
>     * Actually sets the breakpoint.
>     */
> -  private void set(Task task)
> +  private void set(Task task) throws TaskException
>    {
> -    ByteBuffer buffer = task.memory;
> +    int len = 0;
> +    ByteBuffer buffer = null;
> +    byte[] bpInstruction = null;
> +    
> +    if (null == task)
> +      return;

This method should never be called with a null task. If it is that is an
bug somewhere. Just let the NullPointerException happen so the developer
gets a fair warning and can figure out where and why the task argument
was null.

> +    buffer = task.memory;
>      buffer.position(address);
> -    orig = buffer.getByte();
> +    
> +    try
> +      {
> +        bpInstruction = task.getIsa().getBpInstruction();
> +      }
> +    catch (TaskException e)
> +      {
> +        throw e;
> +      }

Don't catch and rethrow TaskException here. The caller of set(Task) will
have to deal with it (and you already do in install()).
    
>    private void reset(Task task)
>    {
> -    ByteBuffer buffer = task.memory;
> +    int len = 0;
> +    ByteBuffer buffer = null;
> +    
> +    if (null == task)
> +      return;

Again, this should be an error, don't silently return please.

> Index: frysk-core/frysk/proc/IsaIA32.java
> ===================================================================
> RCS file: /cvs/frysk/frysk-core/frysk/proc/IsaIA32.java,v
> retrieving revision 1.2
> diff -u -r1.2 IsaIA32.java
> --- frysk-core/frysk/proc/IsaIA32.java  8 Aug 2006 08:27:11 -0000       1.2
> +++ frysk-core/frysk/proc/IsaIA32.java  22 Aug 2006 03:21:42 -0000
> @@ -48,6 +48,8 @@
>    static final int I387_OFFSET = 18*4;
>    static final int DBG_OFFSET = 63 * 4;
>  
> +  private static final byte[] BREAKPOINT_INSTRUCTION = { (byte)0xcc };
> +  
>    static class IA32Register extends Register
>    {
>      IA32Register(String name, int wordOffset)
> @@ -121,6 +123,46 @@
>    {
>      return ByteOrder.LITTLE_ENDIAN;
>    }
> +  
> +  /**
> +   * Get the breakpoint instruction.
> +   * 
> +   * @return bytes[] the instruction of the ISA or null if TRAP is not 
> +   *         initialized.
> +   */
> +  public final byte[] getBpInstruction()
> +  {
> +    byte[] instruction = null;
> +    
> +    if (null == IsaIA32.BREAKPOINT_INSTRUCTION)
> +      return null;

That is an unnecessary check. It cannot be null, since you declare it
above. And if it were null we are in big trouble, so we want a
NullPointerException not a silent return.

> +  /**
> +   * Get the true breakpoint address according to PC register after hitting 
> +   * one breakpoint set in task. In X86, the length of breakpoint instruction
> +   * will be added to the PC register's value. So the true breakpoint address
> +   * is the PC register's value minus the length of breakpoint. 
> +   */
> +  public long getBreakpointAddress(Task task)
> +  {
> +    long pcValue = 0;
> +    
> +    if (null == task)
> +      return pcValue;

Again, don't silently return when the task argument is null, that should
be a real error.

> --- frysk-core/frysk/proc/IsaEMT64.java 8 Aug 2006 08:27:11 -0000       1.2
> +++ frysk-core/frysk/proc/IsaEMT64.java 22 Aug 2006 03:22:15 -0000
and
> --- frysk-core/frysk/proc/IsaPPC.java   4 Aug 2006 09:07:42 -0000       1.1
> +++ frysk-core/frysk/proc/IsaPPC.java   22 Aug 2006 03:24:39 -0000

Same comments as for the IsaIA32.java code.

> ===================================================================
> RCS file: /cvs/frysk/frysk-core/frysk/proc/IsaPPC.java,v
> retrieving revision 1.1
> diff -u -r1.1 IsaPPC.java
> @@ -91,4 +91,22 @@
>    {
>      return ByteOrder.BIG_ENDIAN;
>    }
> +  
> +  /**
> +   * Not support now.
> +   * 
> +   * @return bytes[] the instruction of the ISA.
> +   */
> +  public byte[] getBpInstruction()
> +  {
> +    return null;
> +  }
>
> +  /**
> +   * Not support now.
> +   */
> +  public long getBreakpointAddress(Task task)
> +  {
> +   return 0; 
> +  }

It might be better to throw a RuntimeException here to make clear that
any usage of breakpoints isn't supported for PPC yet.

> -  // XXX add ppc case
> +  // There's great difference to get the addresses of one function between 
> +  // PPC64 and other platform(such as X86/X86_64). What we get through the
> +  // the form "&function_name" is the address of function descriptor but 
> +  // not the true entry address of the function.
> +#ifndef __powerpc64__
>    printf("%p\n", &first_breakpoint_function);
>    printf("%p\n", &second_breakpoint_function);
> +#else
> +  printf("%p\n", (void *)(*(long *)&first_breakpoint_function));
> +  printf("%p\n", (void *) (*(long *)&second_breakpoint_function));
> +#endif

Note that there is another usage of &function_name in
frysk-sys/frysk/sys/cni/TestLib.cxx for testing PtraceByteBuffer. It
needs a similar fix like this for ppc64.

Cheers,

Mark


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]