This is the mail archive of the
frysk@sources.redhat.com
mailing list for the frysk project.
Re: One frysk-core patch to add breakpoint support in PPC64.
- From: Mark Wielaard <mark at klomp dot org>
- To: Yong Zheng <zhengyo at cn dot ibm dot com>
- Cc: frysk at sourceware dot org
- Date: Tue, 22 Aug 2006 14:40:08 +0200
- Subject: Re: One frysk-core patch to add breakpoint support in PPC64.
- References: <1156220482.30530.29.camel@YZ_X86_64>
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