From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4524 invoked by alias); 5 Jul 2009 18:33:29 -0000 Received: (qmail 4512 invoked by uid 22791); 5 Jul 2009 18:33:26 -0000 X-SWARE-Spam-Status: No, hits=-1.3 required=5.0 tests=AWL,BAYES_00,J_CHICKENPOX_24,J_CHICKENPOX_25,J_CHICKENPOX_41,J_CHICKENPOX_53 X-Spam-Check-By: sourceware.org Received: from sibelius.xs4all.nl (HELO sibelius.xs4all.nl) (82.92.89.47) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 05 Jul 2009 18:33:15 +0000 Received: from brahms.sibelius.xs4all.nl (kettenis@localhost [127.0.0.1]) by brahms.sibelius.xs4all.nl (8.14.3/8.14.3) with ESMTP id n65IX812006432; Sun, 5 Jul 2009 20:33:08 +0200 (CEST) Received: (from kettenis@localhost) by brahms.sibelius.xs4all.nl (8.14.3/8.14.3/Submit) id n65IX7Kj003058; Sun, 5 Jul 2009 20:33:07 +0200 (CEST) Date: Sun, 05 Jul 2009 18:33:00 -0000 Message-Id: <200907051833.n65IX7Kj003058@brahms.sibelius.xs4all.nl> From: Mark Kettenis To: paawan1982@yahoo.com CC: gdb-patches@sourceware.org, teawater@gmail.com In-reply-to: <812757.57079.qm@web112517.mail.gq1.yahoo.com> (message from paawan oza on Wed, 1 Jul 2009 09:17:41 -0700 (PDT)) Subject: Re: i386.record.floating.point.patch : with more testing and assurity References: <812757.57079.qm@web112517.mail.gq1.yahoo.com> 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/msg00111.txt.bz2 > Date: Wed, 1 Jul 2009 09:17:41 -0700 (PDT) > From: paawan oza > > 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 > #include > #include > > /* 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.