Re: [PATCH 1/5] Code for nds32 target

[Wei-cheng Wang <cole945@gmail.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