From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20053 invoked by alias); 6 Jul 2009 19:03:08 -0000 Received: (qmail 19805 invoked by uid 22791); 6 Jul 2009 19:03:06 -0000 X-SWARE-Spam-Status: No, hits=-2.2 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from web112518.mail.gq1.yahoo.com (HELO web112518.mail.gq1.yahoo.com) (98.137.26.190) by sourceware.org (qpsmtpd/0.43rc1) with SMTP; Mon, 06 Jul 2009 19:02:56 +0000 Received: (qmail 90166 invoked by uid 60001); 6 Jul 2009 19:02:54 -0000 Message-ID: <644936.88498.qm@web112518.mail.gq1.yahoo.com> Received: from [123.238.27.12] by web112518.mail.gq1.yahoo.com via HTTP; Mon, 06 Jul 2009 12:02:54 PDT Date: Mon, 06 Jul 2009 19:03:00 -0000 From: paawan oza Subject: Re: i386.record.floating.point.patch : with more testing and assurity To: Hui Zhu Cc: Michael Snyder , Mark Kettenis , "pedro@codesourcery.com" , "gdb-patches@sourceware.org" MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-07/txt/msg00155.txt.bz2 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 wrote: > From: Hui Zhu > Subject: Re: i386.record.floating.point.patch : with more testing and ass= urity > To: "paawan oza" > Cc: "Michael Snyder" , "Mark Kettenis" , "pedro@codesourcery.com" , "gdb-patche= s@sourceware.org" > Date: Sunday, July 5, 2009, 3:45 PM > Hi Paawan, >=20 > 1.=A0 gcc -g -O2=A0=A0=A0-I. -I../../src/gdb > -I../../src/gdb/common > -I../../src/gdb/config > -DLOCALEDIR=3D"\"/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=A0 > -I../../src/gdb/gnulib -Ignulib > -DMI_OUT=3D1 -DTUI=3D1=A0 -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 >=20 > paddr_nz was removed.=A0 Please update your patch follow > cvs-head. >=20 > 2. +#define I386_SAVE_FPU_REGS=A0=A0=A0 > =A0=A0=A0 0xFFFD > +#define I386_SAVE_FPU_ENV=A0=A0=A0 > =A0=A0=A0 0xFFFE > +#define I386_SAVE_FPU_ENV_REG_STACK=A0=A0=A0 > 0xFFFF >=20 > They just used in prec right?=A0 Maybe you can move them > close to record > code in i386-tedp.c. >=20 > 3. +static int i386_record_floats(struct i386_record_s *ir, > uint32_t iregnum) > +{ > +=A0 int i; > + > +=A0 /* Oza : push/pop of fpu stack is going to happen > +=A0 =A0=A0=A0currently we store st0-st7 > registers, but we need not store all > registers all the time. > +=A0 =A0=A0=A0using fstatus, we use 11-13 bits > which gives us stack top and > hence we optimize our storage. > +=A0 =A0=A0=A0alternatively we can use ftag > register too */ > +=A0 if (I386_SAVE_FPU_REGS =3D=3D iregnum) > +=A0 =A0 { > +=A0 =A0 =A0 for > (i=3DI386_ST0_REGNUM;i<=3DI386_ST0_REGNUM+7;i++) > +=A0 =A0 =A0 =A0 { > +=A0 =A0 =A0 =A0 =A0 if > (record_arch_list_add_reg (ir->regcache,i)) > +=A0 =A0 =A0 =A0 =A0 =A0 return -1; > +=A0 =A0 =A0 =A0 } > +=A0 =A0 } > About the number of fp regs.=A0 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) >=20 > #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) \ > =A0 (I387_XMM0_REGNUM (tdep) + I387_NUM_XMM_REGS > (tdep)) >=20 > They are in i387-tdep.h. >=20 > And maybe you can try function i387_supply_fsave and > i387_collect_fsave. >=20 >=20 > 4.=A0 Your code's format is not very well.=A0 Please > make it like the code in cvs. >=20 >=20 >=20 > Thanks, > Hui >=20 >=20 >=20 >=20 > On Sat, Jul 4, 2009 at 13:19, paawan oza > 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 > wrote: > > > >> From: Michael Snyder > >> Subject: Re: i386.record.floating.point.patch : > with more testing and assurity > >> To: "paawan oza" > >> Cc: "Mark Kettenis" , > "pedro@codesourcery.com" > , > "teawater@gmail.com" > , > "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,=A0 =A0 =A0=A0=A0I386_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.=A0 For now, prec is still > >> >> not support amd64 And amd64's support are > in > >> amd64-tedp.... files.=A0 >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.=A0 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. > >> > >> > > > > > > > > >=20