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

[Hui Zhu <teawater at gmail dot com>]

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.
>>
>>
>
>
>
>