This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: i386.record.floating.point.patch : with more testing and assurity
- From: Mark Kettenis <mark dot kettenis at xs4all dot nl>
- To: paawan1982 at yahoo dot com
- Cc: gdb-patches at sourceware dot org, teawater at gmail dot com
- Date: Sun, 5 Jul 2009 20:33:07 +0200 (CEST)
- Subject: Re: i386.record.floating.point.patch : with more testing and assurity
- References: <812757.57079.qm@web112517.mail.gq1.yahoo.com>
> Date: Wed, 1 Jul 2009 09:17:41 -0700 (PDT)
> From: paawan oza <paawan1982@yahoo.com>
>
> Hi,
>
> For your review convenience, I am sending the patch in plain text as a part of email body.
> please also refer to the previous mails, where a point is raised by Hui.
> I provided justification for the same.
> please help with the point to throw more light.
Please review the GNU coding standards. Your new code has some
formatting problems.
> TEST_CASE
> ****************************************************
> test_float.c
> ****************************************************
> #include <stdio.h>
> #include <math.h>
> #include <stdlib.h>
>
> /* the test intends to test following insns.
> flds faddp fstps fstpl fldl fxch fabs fdivrp fmulp fsubrp fucomp fnstsw fsqrt
> fchs f2xm1 fyl2x fxtract fprem1 fld fdecstp fld1 fldl2t fldl2e FLDPI FLDLG2 FLDLN2
> FLDZ fincstp ffree fptan fpatan fincstp fsincos frndint fscale fsin fcos fcmovb
> fcmovbe fcmove fcmovu fcmovnb fcmovnbe fsave frstor fstsw
> */
>
> float no1,no2,no3,no4,no5,no6,no7;
> double x = 100.345, y = 25.7789;
> long double ldx = 88888888888888888888.88, ldy = 9999999999999999999.99;
> float result,resultd,resultld;
> float *float_memory;
>
> /* initialization of floats */
> void init_floats()
> {
> no1 = 10.45;
> no2 = 20.77;
> no3 = 156.89874646;
> no4 = 14.56;
> no5 = 11.11;
> no6 = 66.77;
> no7 = 88.88;
> float_memory = malloc(sizeof(float) * 4);
> *float_memory = 256.256;
> *(float_memory + 1) = 356.356;
> *(float_memory + 2) = 456.456;
> *(float_memory + 3) = 556.556;
> }
>
> /* marks FPU stack as empty */
> void empty_fpu_stack()
> {
> asm ("ffree %st(1) \n\t"
> "ffree %st(2) \n\t"
> "ffree %st(3) \n\t"
> "ffree %st(4) \n\t"
> "ffree %st(5) \n\t"
> "ffree %st(6) \n\t"
> "ffree %st(7)");
> }
>
> /* tests floating point arithmatic */
> void test_arith_floats()
> {
> result = no1 + no2 + no3 + no4 + no5 + no6 + no7;
> printf("result is %f\n",result);
>
> result = fmodf(no2,no1);
> printf("result is %f\n",result);
>
> resultd = fmod(x,y);
> printf("result is %f\n",resultd);
>
> resultld = fmodl(ldy,ldy);
> printf("result is %f\n",resultld);
>
> result = fabsf(no1);
> printf("result is %f\n",result);
>
> result = no3 / no4;
> printf("result is %f\n",result);
>
> result = no1 * no2 * no3 * no4;
> printf("result is %f\n",result);
>
> result = no1 - no2 - no3 - no4;
> printf("result is %f\n",result);
>
>
> asm ("fld %0" : :"m"(*float_memory));
> asm ("fchs");
>
> /* test for f2xm1 */
> asm ("fld %0" : :"m"(*float_memory));
> asm ("f2xm1");
>
> asm ("fyl2x");
>
> asm ("fld %0" : :"m"(*float_memory));
> asm ("fxtract");
>
> asm ("fld %0" : :"m"(*float_memory));
> asm ("fprem1");
>
> /* decrement fpu stack pointer only status register should get affected */
> asm ("fld %0" : :"m"(*float_memory));
>
> empty_fpu_stack();
>
> asm ("fld1");
> asm ("fldl2t");
> asm ("fldl2e");
> asm ("fldpi");
> asm ("fldlg2");
> asm ("fldln2");
> asm ("fldz");
>
> empty_fpu_stack();
> /* finishing emptying the stack */
>
> result = sqrt(no3);
> printf("result is %f\n",result);
> }
>
> void test_log_exp_floats()
> {
> result = log10(no3);
> printf("result is %f\n",result);
>
> result = log(no3);
> printf("result is %f\n",result);
>
> result = exp10(no3);
> printf("result is %f\n",result);
>
> result = exp(no3);
> printf("result is %f\n",result);
> }
>
> void test_trigo_floats()
> {
> result = sin(30);
> printf("result is %f\n",result);
>
> result = cos(30);
> printf("result is %f\n",result);
>
> result = tan(30);
> printf("result is %f\n",result);
>
> result = atan(30);
> printf("result is %f\n",result);
>
> asm ("fld %0" : :"m"(*float_memory));
> asm ("fptan");
>
> /* changes st1 and popping register stack */
> asm ("fpatan");
>
> asm("fincstp");
> asm ("fld %0" : :"m"(float_memory));
> asm ("fsincos");
>
> asm ("fld %0" : :"m"(*float_memory));
> asm ("frndint");
>
> asm ("fld %0" : :"m"(*float_memory));
> asm ("fld %0" : :"m"(*(float_memory+1)));
> asm ("fscale");
>
> empty_fpu_stack();
>
> asm ("fld %0" : :"m"(*float_memory));
> asm ("fsin");
> asm ("fcos");
>
> /* currently we assume condition likely and always record the registers
> code could be optimized only if the flag is set then record */
> asm ("fld %0" : :"m"(*float_memory));
> asm ("fld %0" : :"m"(*(float_memory+1)));
> asm ("fcmovb %st(1), %st");
> asm ("fcmovbe %st(1), %st");
> asm ("fcmove %st(1), %st");
> asm ("fcmovu %st(1), %st");
> asm ("fcmovnb %st(1), %st");
> asm ("fcmovnbe %st(1), %st");
>
> empty_fpu_stack();
> /* finished emtyping the stack */
> }
>
> void test_compare_floats()
> {
> ldy = 88888888888888888888.88;
> if (ldx == ldy)
> ldy = 7777777777777777777777777777.777;
> else
> ldy = 666666666666666666666666666.666;
> }
>
> /* test loading and saving of FPU environment */
> void test_fpu_env()
> {
> asm ("fsave %0" : "=m"(*float_memory) : );
> asm ("frstor %0" : : "m"(*float_memory));
> asm ("fstsw %ax");
> }
>
> int main()
> {
> init_floats();
> test_arith_floats();
> test_log_exp_floats();
> test_trigo_floats();
> test_compare_floats();
> test_fpu_env();
> }
As other already indicated, you should really turn this into something
that is integrated in the GDB testsuite.
> diff -urN gdb.orig/i386-tdep.c gdb.new/i386-tdep.c
> --- gdb.orig/i386-tdep.c 2009-05-29 17:08:40.000000000 -0400
> +++ gdb.new/i386-tdep.c 2009-06-01 20:02:23.000000000 -0400
> @@ -543,6 +543,9 @@
> /* The maximum number of saved registers. This should include all
> registers mentioned above, and %eip. */
> #define I386_NUM_SAVED_REGS I386_NUM_GREGS
> +#define I386_SAVE_FPU_REGS 0xFFFD
> +#define I386_SAVE_FPU_ENV 0xFFFE
> +#define I386_SAVE_FPU_ENV_REG_STACK 0xFFFF
What are these constants used for? Do they have anything to do with
the number of saved registers? Also, please use lowercase for
hexadecimal constants.
> struct i386_frame_cache
> {
> @@ -2985,6 +2988,54 @@
> return 0;
> }
>
> +/* Record the value of floating point registers which will be changed by the current instruction
> + to "record_arch_list".
> + return -1 if something is wrong. */
Everey new sentence should start with a capital; everey full stop (.)
should be followed by two spaces. Also please wrap your comments such
that fit in a 80-column wide screen.
> +
> +static int i386_record_floats(struct i386_record_s *ir, uint32_t iregnum)
static int
> +{
> + 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. */
Sorry, but I don't understand what you're saying here.
> + if (I386_SAVE_FPU_REGS == iregnum)
> + {
> + for (i=I386_ST0_REGNUM;i<=I386_ST7_REGNUM;i++)
Spacing problems.
> + {
> + if (record_arch_list_add_reg (ir->regcache,i))
> + return -1;
> + }
> + }
> + else if (I386_SAVE_FPU_ENV == iregnum)
> + {
> + for (i=I386_FCTRL;i<=I386_FOP;i++)
> + {
> + if (record_arch_list_add_reg (ir->regcache,i))
> + return -1;
> + }
> + }
> + else if (I386_SAVE_FPU_ENV_REG_STACK == iregnum)
> + {
> + for (i=I386_ST0_REGNUM;i<=I386_FOP;i++)
> + {
> + if (record_arch_list_add_reg (ir->regcache,i))
> + return -1;
> + }
> + }
> + else if (iregnum >= I386_ST0_REGNUM && iregnum <= I386_FOP)
> + {
> + if (record_arch_list_add_reg (ir->regcache,iregnum))
> + return -1;
> + }
> + else
> + {
> + /* param Error */
Capitalization; missing full stop.
> + return -1;
> + }
> + return 0;
> +}
> +
> /* Parse the current instruction and record the values of the registers and
> memory that will be changed in current instruction to "record_arch_list".
> Return -1 if something wrong. */
> @@ -4035,7 +4086,6 @@
> break;
>
> /* floats */
> - /* It just record the memory change of instrcution. */
> case 0xd8:
> case 0xd9:
> case 0xda:
> @@ -4056,39 +4106,49 @@
> return -1;
> switch (ir.reg)
> {
> - case 0x00:
> - case 0x01:
> case 0x02:
> - case 0x03:
> + case 0x12:
> + case 0x22:
> + case 0x32:
> + /* for FCOM, FICOM nothing to do */
Please use lowercase for assembler mnemonics.
> + break;
> + case 0x03:
> + case 0x13:
> + case 0x23:
> + case 0x33:
> + /* FCOMP, FICOMP pop FPU stack, store all */
> + if (i386_record_floats(&ir, I386_SAVE_FPU_REGS))
> + return -1;
> + break;
> + case 0x00:
> + case 0x01:
> case 0x04:
> case 0x05:
> case 0x06:
> case 0x07:
> case 0x10:
> - case 0x11:
> - case 0x12:
> - case 0x13:
> + case 0x11:
> case 0x14:
> case 0x15:
> case 0x16:
> case 0x17:
> case 0x20:
> case 0x21:
> - case 0x22:
> - case 0x23:
> case 0x24:
> case 0x25:
> case 0x26:
> case 0x27:
> case 0x30:
> case 0x31:
> - case 0x32:
> - case 0x33:
> case 0x34:
> case 0x35:
> case 0x36:
> case 0x37:
> - break;
> + /* FADD, FMUL, FSUB, FSUBR, FDIV, FDIVR, FIADD, FIMUL, FISUB, FISUBR, FIDIV, FIDIVR
> + ModR/M.reg is an extension of code, always affects st(0) register */
> + if (i386_record_floats(&ir, I386_ST0_REGNUM))
> + return -1;
> + break;
> case 0x08:
> case 0x0a:
> case 0x0b:
> @@ -4096,6 +4156,7 @@
> case 0x19:
> case 0x1a:
> case 0x1b:
> + case 0x1d:
> case 0x28:
> case 0x29:
> case 0x2a:
> @@ -4103,11 +4164,16 @@
> case 0x38:
> case 0x39:
> case 0x3a:
> - case 0x3b:
> + case 0x3b:
> + case 0x3c:
> + case 0x3d:
> switch (ir.reg & 7)
> {
> case 0:
> - break;
> + /* FLD, FILD */
> + if (i386_record_floats(&ir, I386_SAVE_FPU_REGS))
> + return -1;
> + break;
> case 1:
> switch (ir.reg >> 4)
> {
> @@ -4120,6 +4186,7 @@
> return -1;
> break;
> case 3:
> + break;
> default:
> if (record_arch_list_add_mem (addr, 2))
> return -1;
> @@ -4130,15 +4197,42 @@
> switch (ir.reg >> 4)
> {
> case 0:
> + if (record_arch_list_add_mem (addr, 4))
> + return -1;
> + if (3 == (ir.reg & 7))
> + {
> + /* FSTP m32fp */
> + if (i386_record_floats(&ir, I386_SAVE_FPU_REGS))
> + return -1;
> + }
> + break;
> case 1:
> if (record_arch_list_add_mem (addr, 4))
> return -1;
> + if ((3 == (ir.reg & 7)) || (5 == (ir.reg & 7)) || (7 == (ir.reg & 7)))
> + {
> + /* FSTP */
> + if (i386_record_floats(&ir, I386_SAVE_FPU_REGS))
> + return -1;
> + }
> break;
> case 2:
> if (record_arch_list_add_mem (addr, 8))
> return -1;
> + if (3 == (ir.reg & 7))
> + {
> + /* FSTP m64fp */
> + if (i386_record_floats(&ir, I386_SAVE_FPU_REGS))
> + return -1;
> + }
> break;
> case 3:
> + if ((3 <= (ir.reg & 7)) && (6 <= (ir.reg & 7)))
> + {
> + /* FISTP, FBLD, FILD, FBSTP */
> + if (i386_record_floats(&ir, I386_SAVE_FPU_REGS))
> + return -1;
> + }
> default:
> if (record_arch_list_add_mem (addr, 2))
> return -1;
> @@ -4147,54 +4241,71 @@
> break;
> }
> break;
> - case 0x0c:
> - case 0x0d:
> - case 0x1d:
> - case 0x2c:
> - case 0x3c:
> - case 0x3d:
> - break;
> - case 0x0e:
> + case 0x0c:
> + /* FLDENV */
> + if (i386_record_floats(&ir, I386_SAVE_FPU_ENV_REG_STACK))
> + return -1;
> + break;
> + case 0x0d:
> + /* FLDCW */
> + if (i386_record_floats(&ir, I386_FCTRL))
> + return -1;
> + break;
> + case 0x2c:
> + /* FRTSTOR */
> + if (i386_record_floats(&ir, I386_SAVE_FPU_ENV_REG_STACK))
> + return -1;
> + break;
> + case 0x0e:
> if (ir.dflag)
> {
> - if (record_arch_list_add_mem (addr, 28))
> - return -1;
> + if (record_arch_list_add_mem (addr, 28))
> + return -1;
> }
> else
> {
> - if (record_arch_list_add_mem (addr, 14))
> - return -1;
> + if (record_arch_list_add_mem (addr, 14))
> + return -1;
> }
> break;
> - case 0x0f:
> - case 0x2f:
> + case 0x0f:
> + case 0x2f:
> if (record_arch_list_add_mem (addr, 2))
> return -1;
> break;
> - case 0x1f:
> - case 0x3e:
> + case 0x1f:
> + case 0x3e:
> if (record_arch_list_add_mem (addr, 10))
> return -1;
> + /* FSTP, FBSTP */
> + if (i386_record_floats(&ir, I386_SAVE_FPU_REGS))
> + return -1;
> break;
> - case 0x2e:
> + case 0x2e:
> if (ir.dflag)
> {
> - if (record_arch_list_add_mem (addr, 28))
> - return -1;
> - addr += 28;
> + if (record_arch_list_add_mem (addr, 28))
> + return -1;
> + addr += 28;
> }
> else
> {
> - if (record_arch_list_add_mem (addr, 14))
> - return -1;
> - addr += 14;
> + if (record_arch_list_add_mem (addr, 14))
> + return -1;
> + addr += 14;
> }
> if (record_arch_list_add_mem (addr, 80))
> return -1;
> + /* FSAVE */
> + if (i386_record_floats(&ir, I386_SAVE_FPU_ENV_REG_STACK))
> + return -1;
> break;
> - case 0x3f:
> + case 0x3f:
> if (record_arch_list_add_mem (addr, 8))
> return -1;
> + /* FISTP */
> + if (i386_record_floats(&ir, I386_SAVE_FPU_REGS))
> + return -1;
> break;
> default:
> ir.addr -= 2;
> @@ -4202,9 +4313,180 @@
> goto no_support;
> break;
> }
> - }
> + }
> + /* opcode is an extension of modR/M byte */
> + else
> + {
> + switch (opcode)
> + {
> + case 0xd8:
> + if (i386_record_floats(&ir, I386_ST0_REGNUM))
> + return -1;
> + break;
> + case 0xd9:
> + if (0x0c == (ir.modrm >> 4))
> + {
> + if ((ir.modrm & 0x0f) <= 7)
> + {
> + if (i386_record_floats(&ir, I386_SAVE_FPU_REGS))
> + return -1;
> + }
> + else
> + {
> + if (i386_record_floats(&ir, I386_ST0_REGNUM))
> + return -1;
> + /* if only st(0) is changing, then we have already recorded */
> + if ((ir.modrm & 0x0f) - 0x08)
> + {
> + if (i386_record_floats(&ir, I386_ST0_REGNUM + ((ir.modrm & 0x0f) - 0x08)))
> + return -1;
> + }
> + }
> + }
> + else
> + {
> + switch(ir.modrm)
> + {
> + case 0xe0:
> + case 0xe1:
> + case 0xf0:
> + case 0xf5:
> + case 0xf8:
> + case 0xfa:
> + case 0xfc:
> + case 0xfe:
> + case 0xff:
> + if (i386_record_floats(&ir, I386_ST0_REGNUM))
> + return -1;
> + break;
> + case 0xf1:
> + case 0xf2:
> + case 0xf3:
> + case 0xf4:
> + case 0xf6:
> + case 0xf7:
> + case 0xe8:
> + case 0xe9:
> + case 0xea:
> + case 0xeb:
> + case 0xec:
> + case 0xed:
> + case 0xee:
> + case 0xf9:
> + case 0xfb:
> + if (i386_record_floats(&ir, I386_SAVE_FPU_REGS))
> + return -1;
> + break;
> + case 0xfd:
> + if (i386_record_floats(&ir, I386_ST0_REGNUM))
> + return -1;
> + if (i386_record_floats(&ir, I386_ST1_REGNUM))
> + return -1;
> + break;
> + }
> + }
> + break;
> + case 0xda:
> + if (0xe9 == ir.modrm)
> + {
> + if (i386_record_floats(&ir, I386_SAVE_FPU_REGS))
> + return -1;
> + }
> + else if ((0x0c == ir.modrm >> 4) || (0x0d == ir.modrm >> 4))
> + {
> + if (i386_record_floats(&ir, I386_ST0_REGNUM))
> + return -1;
> + if (((ir.modrm & 0x0f) > 0) && ((ir.modrm & 0x0f) <= 7))
> + {
> + if (i386_record_floats(&ir, I386_ST0_REGNUM + (ir.modrm & 0x0f)))
> + return -1;
> + }
> + else if ((ir.modrm & 0x0f) - 0x08)
> + {
> + if (i386_record_floats(&ir, I386_ST0_REGNUM + ((ir.modrm & 0x0f) - 0x08)))
> + return -1;
> + }
> + }
> + break;
> + case 0xdb:
> + if (0xe3 == ir.modrm)
> + {
> + if (i386_record_floats(&ir, I386_SAVE_FPU_ENV))
> + return -1;
> + }
> + else if ((0x0c == ir.modrm >> 4) || (0x0d == ir.modrm >> 4))
> + {
> + if (i386_record_floats(&ir, I386_ST0_REGNUM))
> + return -1;
> + if (((ir.modrm & 0x0f) > 0) && ((ir.modrm & 0x0f) <= 7))
> + {
> + if (i386_record_floats(&ir, I386_ST0_REGNUM + (ir.modrm & 0x0f)))
> + return -1;
> + }
> + else if ((ir.modrm & 0x0f) - 0x08)
> + {
> + if (i386_record_floats(&ir, I386_ST0_REGNUM + ((ir.modrm & 0x0f) - 0x08)))
> + return -1;
> + }
> + }
> + break;
> + case 0xdc:
> + if ((0x0c == ir.modrm >> 4) || (0x0d == ir.modrm >> 4) || (0x0f == ir.modrm >> 4))
> + {
> + if ((ir.modrm & 0x0f) <= 7)
> + {
> + if (i386_record_floats(&ir, I386_ST0_REGNUM + (ir.modrm & 0x0f)))
> + return -1;
> + }
> + else
> + {
> + if (i386_record_floats(&ir, I386_ST0_REGNUM + ((ir.modrm & 0x0f) - 0x08)))
> + return -1;
> + }
> + }
> + break;
> + case 0xdd:
> + if (0x0c == ir.modrm >> 4)
> + {
> + if (i386_record_floats(&ir,I386_FTAG))
> + return -1;
> + }
> + else if ((0x0d == ir.modrm >> 4) || (0x0e == ir.modrm >> 4))
> + {
> + if ((ir.modrm & 0x0f) <= 7)
> + {
> + if (i386_record_floats(&ir, I386_ST0_REGNUM + (ir.modrm & 0x0f)))
> + return -1;
> + }
> + else
> + {
> + if (i386_record_floats(&ir, I386_SAVE_FPU_REGS))
> + return -1;
> + }
> + }
> + break;
> + case 0xde:
> + if ((0x0c == ir.modrm >> 4) || (0x0e == ir.modrm >> 4) || (0x0f == ir.modrm >> 4) || (0xd9 == ir.modrm))
> + {
> + if (i386_record_floats(&ir, I386_SAVE_FPU_REGS))
> + return -1;
> + }
> + break;
> + case 0xdf:
> + if (0xe0 == ir.modrm)
> + {
> + if (record_arch_list_add_reg (ir.regcache, I386_EAX_REGNUM))
> + return -1;
> + }
> + else if ((0x0f == ir.modrm >> 4) || (0x0e == ir.modrm >> 4))
> + {
> + if (i386_record_floats(&ir, I386_SAVE_FPU_REGS))
> + return -1;
> + }
> + break;
> + }
> + }
> break;
> -
> /* string ops */
> /* movsS */
> case 0xa4:
> @@ -4623,10 +4905,17 @@
> /* fwait */
> /* XXX */
> case 0x9b:
> - printf_unfiltered (_("Process record doesn't support instruction "
> - "fwait.\n"));
> - ir.addr -= 1;
> - goto no_support;
> + if (target_read_memory (ir.addr, &tmpu8, 1))
> + {
> + if (record_debug)
> + printf_unfiltered (_("Process record: error reading memory at "
> + "addr 0x%s len = 1.\n"),
> + paddr_nz (ir.addr));
> + return -1;
> + }
> + opcode = (uint32_t) tmpu8;
> + ir.addr++;
> + goto reswitch;
> break;
>
> /* int3 */
> diff -urN gdb.orig/i386-tdep.h gdb.new/i386-tdep.h
> --- gdb.orig/i386-tdep.h 2009-05-17 17:56:44.000000000 -0400
> +++ gdb.new/i386-tdep.h 2009-05-31 16:33:14.000000000 -0400
> @@ -145,7 +145,22 @@
> I386_ES_REGNUM, /* %es */
> I386_FS_REGNUM, /* %fs */
> I386_GS_REGNUM, /* %gs */
> - I386_ST0_REGNUM /* %st(0) */
> + I386_ST0_REGNUM, /* %st(0) */
> + I386_ST1_REGNUM, /* %st(1) */
> + I386_ST2_REGNUM, /* %st(2) */
> + I386_ST3_REGNUM, /* %st(3) */
> + I386_ST4_REGNUM, /* %st(4) */
> + I386_ST5_REGNUM, /* %st(5) */
> + I386_ST6_REGNUM, /* %st(6) */
> + I386_ST7_REGNUM, /* %st(7) */
> + I386_FCTRL, /* floating point env regs : FCTRL-FOP */
> + I386_FSTAT,
> + I386_FTAG,
> + I386_FISEG,
> + I386_FIOFF,
> + I386_FOSEG,
> + I386_FOOFF,
> + I386_FOP
> };
If you no longer need this, please remove this from your diff.