This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/5] Code for nds32 target
- From: Wei-cheng Wang <cole945 at gmail dot com>
- To: Yao Qi <yao at codesourcery dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Wed, 10 Jul 2013 02:23:41 +0800
- Subject: Re: [PATCH 1/5] Code for nds32 target
- References: <CAPmZyH4TFFOvyis+so2=sd0oNg3-m0Kj1H-vske=WyK57TLqVQ at mail dot gmail dot com> <51DB8D14 dot 20205 at codesourcery dot com>
Hi Yao Qi,
Thanks for you reviewing and suggestion.
I will revise the patch based on your suggestion thoroughly :)
Here are some brief answers and questions.
On 7/9/2013 12:09 PM, Yao Qi wrote:
On 07/08/2013 05:27 PM, Wei-cheng Wang wrote:
Do we really need "regnum="15"" here?
Yes, but it is unnecessary, so I will remove this.
It was here because r11-r14 does not exist in reduced
register configuration.
Since there are no comments on what nds32-remote.c is for, I assume that
it is used to talk with remote target, like jtag ice. IIUC, you invent
some new rsp packets to your own targets, but it is not recommended to
add such thing into GDB nowadays. I can't find the mail archive about
this decision, and other people can give a definitive answer here.
Yes, this file are used to talk with our remote OpenOCD and SID.
If you mean those "qPart:nds32:*" packets, I agree we should
remove them and our remote target should be changed accordingly
for conforming GDB RSP standard.
Or do you mean monitor commands ("qRcmd,command" packetss) are
also not recommended in GDB nowadays?
+}
+
+/* Implement the gdbarch_breakpoint_from_pc method. */
+
+static const gdb_byte *
+nds32_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr,
+ int *lenptr)
+{
+ static const gdb_byte NDS32_BREAK16[] = { 0xEA, 0x00 };
Do we need to worry about big-endian and little endian here?
Instructions are always big-endian.
+ if ((*pcptr) & 1)
+ error (_("Bad address %p for inserting breakpoint.\n"
+ "Address must be at least 2-byte aligned."), (void *) *pcptr);
Do you know when the last bit of address is 1? I am wondering whether
this checking is necessary or it is caused by the bug somewhere else.
Just in case users may set the breakpoint in address directly.
(gdb) break *0x4321
I think I would better stop them at the first place.
Can you define 'fucpr' in the target description? like
<flags id="fucpr" size="4">
<field name="CP0EN" start="0" end="0"/>
.....
</flags>
which is simpler, IMO.
+
+ Return the GDB type object for the "standard" data type
+ of data in register N.
+ It get pretty messy here. I need enum-types and bit-fields
+ for better representation. But they cannot be done by
+ tdesc-xml. */
target description works for enum and bitfields. For example, in
features/i386/32bit-core.xml,
Target description can not describe a enum-typed bit-field.
If bit-field struct is used, I could not specify the type for
a bit-field.
<struct id="id" size="size">
<field name="name" start="start" end="end"/>
...
</struct>
If I want to specify field type, I couldn't specify size of a field.
<struct id="id">
<field name="name" type="type"/>
...
</struct>
All I want is to display a register in such format.
(gdb) p $cr0
$1 = {CFGID = [ PERF_EXT 16_EXT PERF_EXT2 COP_EXT STR_EXT ],
REV = 16, CPUID = N13}
(cr0 consist of config flags, revision number and CPU ID.)
I agree all these type describing thing should be in
target-description, but if target-description could not describe
all the needed types, IMHO managing them in a single
place may be better then scattering them in GDB and
target-description.
Or I may help to extend target-description, so we could specify
types of bit-fields?
Any suggestion?
I have to stop reviewing here as I've run out of time of patch review
today. I may have a look at the rest of the patch tomorrow.
Thanks for your great help :)
Wei-cheng