Re: i386.record.floating.point.patch : with more testing and assurity

[paawan oza <paawan1982@yahoo.com>]

Hi Hui,

following comments are taken care.

1) paddr_nz is removed, now patch follows latest cvs head.
2) constants are moved close to prec code as you suggested.
3) for floating point register numbers, now I am using
I387_ST0_REGNUM(tdep) [gdbarch] struct.
4) I have tried my level best to make formatting better.

please find the latest patch in next mail.

Regards,
Oza.


--- On Sun, 7/5/09, Hui Zhu <teawater@gmail.com> wrote:

> From: Hui Zhu <teawater@gmail.com>
> Subject: Re: i386.record.floating.point.patch : with more testing and assurity
> To: "paawan oza" <paawan1982@yahoo.com>
> Cc: "Michael Snyder" <msnyder@vmware.com>, "Mark Kettenis" <mark.kettenis@xs4all.nl>, "pedro@codesourcery.com" <pedro@codesourcery.com>, "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
> Date: Sunday, July 5, 2009, 3:45 PM
> Hi Paawan,
> 
> 1.  gcc -g -O2   -I. -I../../src/gdb
> -I../../src/gdb/common
> -I../../src/gdb/config
> -DLOCALEDIR="\"/usr/local/share/locale\""
> -DHAVE_CONFIG_H -I../../src/gdb/../include/opcode
> -I../../src/gdb/../readline/.. -I../bfd
> -I../../src/gdb/../bfd
> -I../../src/gdb/../include -I../libdecnumber
> -I../../src/gdb/../libdecnumber 
> -I../../src/gdb/gnulib -Ignulib
> -DMI_OUT=1 -DTUI=1  -Wall
> -Wdeclaration-after-statement
> -Wpointer-arith -Wformat-nonliteral -Wno-pointer-sign
> -Wno-unused
> -Wno-switch -Wno-char-subscripts -Werror -c -o i386-tdep.o
> -MT
> i386-tdep.o -MMD -MP -MF .deps/i386-tdep.Tpo
> ../../src/gdb/i386-tdep.c
> cc1: warnings being treated as errors
> ../../src/gdb/i386-tdep.c: In function
> 'i386_process_record':
> ../../src/gdb/i386-tdep.c:4985: warning: implicit
> declaration of
> function 'paddr_nz'
> ../../src/gdb/i386-tdep.c:4985: warning: format '%s'
> expects type
> 'char *', but argument 2 has type 'int'
> make[2]: *** [i386-tdep.o] Error 1
> make[2]: Leaving directory `/media/disk/gdb/bgdb/gdb'
> make[1]: *** [all-gdb] Error 2
> make[1]: Leaving directory `/media/disk/gdb/bgdb'
> make: *** [all] Error 2
> 
> paddr_nz was removed.  Please update your patch follow
> cvs-head.
> 
> 2. +#define I386_SAVE_FPU_REGS   
>     0xFFFD
> +#define I386_SAVE_FPU_ENV   
>     0xFFFE
> +#define I386_SAVE_FPU_ENV_REG_STACK   
> 0xFFFF
> 
> They just used in prec right?  Maybe you can move them
> close to record
> code in i386-tedp.c.
> 
> 3. +static int i386_record_floats(struct i386_record_s *ir,
> uint32_t iregnum)
> +{
> +  int i;
> +
> +  /* Oza : push/pop of fpu stack is going to happen
> +     currently we store st0-st7
> registers, but we need not store all
> registers all the time.
> +     using fstatus, we use 11-13 bits
> which gives us stack top and
> hence we optimize our storage.
> +     alternatively we can use ftag
> register too */
> +  if (I386_SAVE_FPU_REGS == iregnum)
> +    {
> +      for
> (i=I386_ST0_REGNUM;i<=I386_ST0_REGNUM+7;i++)
> +        {
> +          if
> (record_arch_list_add_reg (ir->regcache,i))
> +            return -1;
> +        }
> +    }
> About the number of fp regs.  Please use the code:
> #define I387_ST0_REGNUM(tdep) ((tdep)->st0_regnum)
> #define I387_NUM_XMM_REGS(tdep) ((tdep)->num_xmm_regs)
> #define I387_MM0_REGNUM(tdep) ((tdep)->mm0_regnum)
> 
> #define I387_FCTRL_REGNUM(tdep) (I387_ST0_REGNUM (tdep) +
> 8)
> #define I387_FSTAT_REGNUM(tdep) (I387_FCTRL_REGNUM (tdep) +
> 1)
> #define I387_FTAG_REGNUM(tdep) (I387_FCTRL_REGNUM (tdep) +
> 2)
> #define I387_FISEG_REGNUM(tdep) (I387_FCTRL_REGNUM (tdep) +
> 3)
> #define I387_FIOFF_REGNUM(tdep) (I387_FCTRL_REGNUM (tdep) +
> 4)
> #define I387_FOSEG_REGNUM(tdep) (I387_FCTRL_REGNUM (tdep) +
> 5)
> #define I387_FOOFF_REGNUM(tdep) (I387_FCTRL_REGNUM (tdep) +
> 6)
> #define I387_FOP_REGNUM(tdep) (I387_FCTRL_REGNUM (tdep) +
> 7)
> #define I387_XMM0_REGNUM(tdep) (I387_ST0_REGNUM (tdep) +
> 16)
> #define I387_MXCSR_REGNUM(tdep) \
>   (I387_XMM0_REGNUM (tdep) + I387_NUM_XMM_REGS
> (tdep))
> 
> They are in i387-tdep.h.
> 
> And maybe you can try function i387_supply_fsave and
> i387_collect_fsave.
> 
> 
> 4.  Your code's format is not very well.  Please
> make it like the code in cvs.
> 
> 
> 
> Thanks,
> Hui
> 
> 
> 
> 
> On Sat, Jul 4, 2009 at 13:19, paawan oza<paawan1982@yahoo.com>
> wrote:
> >
> > Hi,
> >
> > Actually, the initial patch which I submitted were
> using them.
> > but as I have incorporated Hui's comments I have
> removed those constants completely.
> > in the sense I have no longer extended the
> enumration.
> >
> > but of course, those registers are recorded as and
> when required.
> > e.g. (ffree insn changes FTAG register, so we record
> it)
> >
> > Regards,
> > Oza.
> >
> > --- On Sat, 7/4/09, Michael Snyder <msnyder@vmware.com>
> wrote:
> >
> >> From: Michael Snyder <msnyder@vmware.com>
> >> Subject: Re: i386.record.floating.point.patch :
> with more testing and assurity
> >> To: "paawan oza" <paawan1982@yahoo.com>
> >> Cc: "Mark Kettenis" <mark.kettenis@xs4all.nl>,
> "pedro@codesourcery.com"
> <pedro@codesourcery.com>,
> "teawater@gmail.com"
> <teawater@gmail.com>,
> "gdb-patches@sourceware.org"
> <gdb-patches@sourceware.org>
> >> Date: Saturday, July 4, 2009, 3:19 AM
> >> paawan oza wrote:
> >> > Hi,
> >> >
> >> > In My understanding the point was like
> below.
> >> > in the patch there were following register
> extended in
> >> enumeration in i386-tdep.h
> >> >
> >> > I386_FSTAT,
> >> > I386_FTAG,       I386_FISEG,
> >> > I386_FIOFF,
> >> > I386_FOSEG,
> >> > I386_FOOFF,
> >> > I386_FOP
> >> >
> >> >
> >> > According to Hui in some of his previous
> mails...his
> >> idea was
> >> >> FCTRL, FOP and so on are the fp reg of
> >> amd64.  For now, prec is still
> >> >> not support amd64 And amd64's support are
> in
> >> amd64-tedp.... files.  >Change i386_regnum is
> not a
> >> good idea. I suggest you divide fp patch to 2
> >parts. One
> >> is for i386, the other for amd64. For now, just
> send i386
> >> patch >for review.  And send amd64 patch when
> prec
> >> support amd64"
> >> >
> >> >
> >> > while, my idea/understanding is:
> >> > FCTRL, FOP registers are not only a part of
> amd64, but
> >> also part of i386 (x87 FPU unit) also.
> >> > so according to me these registers are part
> of i386
> >> also and it needed to be also in i386-tdep.h.
> >> >
> >> > Regards,
> >> > Oza.
> >>
> >> I'm not sure why you want to add those constants
> to
> >> i386-tdep.h,
> >> when the rest of your patch does not seem to use
> them.
> >>
> >>
> >
> >
> >
> >
>