Re: [PATCH] Bug 20936 - provide sparc and sparcv9 target description XML files

[Ivo Raisr <ivo dot raisr at oracle dot com>]

On 12.12.2016 13:53, Yao Qi wrote:

> Hi Ivo,
> Your patch does two orthogonal things IMO,
>
>   - Pseudo register support enhancement, patch #1
>   - XML target description support and sparc*-tdep.c updates, patch #2,
>
> Can you split them to two patches?

Hi Yao,

Thank you for looking into my changes.
I think I can see your motivation behind splitting the patch into two,
for better readability and manageability.
However my intention was only to provide support for registers
supplied by target description (Valgrind shadow registers in addition
to sparc real hardware ones).
Pseudo register changes were just a necessity to get this done
because Valgrind shadow registers are considered as "real" registers
and thus they are numbered before any pseudo registers.
This also means I cannot draw a clear line between your suggested
patch #1 and #2. Can you take my whole patch as is?

> I prefer to using register names provided by target description, does
> the code below work for you?
>
>    if (tdesc_has_registers (gdbarch_target_desc (gdbarch)))
>      return tdesc_register_name (gdbarch, rawnum);
>    else if (regnum >= 0 && regnum < gdbarch_num_regs (gdbarch))
>      return sparc32_register_names[regnum];
>    else
>      return sparc32_pseudo_register_name (gdbarch, regnum);

Yes, it does. Fixed also in other functions returning register names and types.

> > -  /* Pseudo registers.  */
> > +/* Pseudo registers.  */
> > +enum sparc32_pseudo_regnum
> > +{
> >     SPARC32_D0_REGNUM,		/* %d0 */
> Explicitly set it to zero.

Fixed.

> > @@ -969,19 +1002,24 @@ sparc64_store_arguments (struct regcache
> >   
> >   	  /* If we're storing the value in a floating-point register,
> >                also store it in the corresponding %0 register(s).  */
> > -	  if (regnum >= SPARC64_D0_REGNUM && regnum <= SPARC64_D10_REGNUM)
> > -	    {
> > -	      gdb_assert (element < 6);
> > -	      regnum = SPARC_O0_REGNUM + element;
> > -	      regcache_cooked_write (regcache, regnum, valbuf);
> > -	    }
> > -	  else if (regnum >= SPARC64_Q0_REGNUM && regnum <= SPARC64_Q8_REGNUM)
> > -	    {
> > -	      gdb_assert (element < 5);
> > -	      regnum = SPARC_O0_REGNUM + element;
> > -	      regcache_cooked_write (regcache, regnum, valbuf);
> > -	      regcache_cooked_write (regcache, regnum + 1, valbuf + 8);
> > -	    }
> > +	  if (regnum >= gdbarch_num_regs (gdbarch))
> > +            {
> The indentation looks odd to me.

Indeed, just looking at the patch it does. This is because the code 
mixes spaces and tabs and I wanted to preserve as much of existing code 
as possible, to keep the patch size down. Please look at the resulting 
file - indentation is ok.

> >         /* Always store the argument in memory.  */
> > diff -Npur a/gdb/testsuite/gdb.xml/tdesc-regs.exp b/gdb/testsuite/gdb.xml/tdesc-regs.exp
> > --- a/gdb/testsuite/gdb.xml/tdesc-regs.exp	2016-02-09 19:19:39.000000000 +0000
> > +++ b/gdb/testsuite/gdb.xml/tdesc-regs.exp	2016-12-06 15:53:35.418621207 +0000
> > @@ -49,6 +49,12 @@ switch -glob -- [istarget] {
> >       "s390*-*-*" {
> >   	set core-regs {s390-core32.xml s390-acr.xml s390-fpr.xml}
> >       }
> > +    "sparc-*-*" {
> > +        set core-regs {sparc32-cpu.xml sparc32-fpu.xml sparc32-cp0.xml}
> > +    }
> > +    "sparc64-*-*" {
> > +        set core-regs {sparc64-cpu.xml sparc64-fpu.xml sparc64-cp0.xml}
> > +    }
> You need 'set regdir "sparc/"'

Fixed.

> > 	* sparc32-cp0.xml, sparc32-cpu.xml, sparc32-fpu.xml: New files for
> > 	sparc 32-bit.
> > 	* sparc64-cp0.xml, sparc64-cpu.xml, sparc64-fpu.xml: New files
> > 	for sparc 64-bit.
> > 	* sparc32-solaris.xml, sparc64-solaris.xml: New files for sparc32
> > 	and sparc64 on Solaris.
> > 	* sparc-solaris.c, sparc64-solaris.c: Generated.
> These file names should be prefixed with "feature/sparc/"

Fixed.

> > 	* sparc-tdep.h: Deal with sparc32 and sparc64 differences
> > 	in target descriptions. Separate real and pseudo registers.
> > 	* sparc-tdep.c: Validate and use registers of the target description.
> > 	Pseudo registers are numbered after all real registers from the target
> > 	description; deal with it.
> > 	* sparc64-tdep.h: Separate real and pseudo registers.
> > 	* sparc64-tdep.c: Pseudo registers are numbered after all real
> > 	registers from the target description; deal with it.
> Please describe the change on the function level, take a look at
> https://sourceware.org/gdb/wiki/ContributionChecklist there are two
> sections about ChangeLog.

Fixed.

> > 	* gdb.texinfo: New node "Sparc Features".
> This should go to a separated entry,
>
> gdb/doc:
>
> 2016-12-07  Ivo Raisr  <ivo.raisr@oracle.com>
>
> 	* gdb.texinfo (Standard Target Features): Document SPARC features.
> 	(Sparc Features): New node.

Fixed.

> > 	* tdesc-regs.exp: Provide sparc core registers for the tests.
> Likewise,
>
> gdb/testsuite:
>
> 2016-12-07  Ivo Raisr  <ivo.raisr@oracle.com>
>
> 	* gdb.xml/tdesc-regs.exp: Provide sparc core registers for
> 	the tests.

Fixed.

Please find a new patch and 3 new ChangeLog entries. Let me know your 
comments.

I.

Attachment: sparc_tdesc.patch-002
Description: Text document

Attachment: ChangeLog.doc.entry
Description: Text document

Attachment: ChangeLog.entry-002
Description: Text document

Attachment: ChangeLog.testsuite.entry
Description: Text document