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: Michael Snyder <msnyder at vmware dot com>
- To: paawan oza <paawan1982 at yahoo dot com>
- Cc: Hui Zhu <teawater at gmail dot com>, Mark Kettenis <mark dot kettenis at xs4all dot nl>, "pedro at codesourcery dot com" <pedro at codesourcery dot com>, "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Sat, 11 Jul 2009 13:54:11 -0700
- Subject: Re: i386.record.floating.point.patch : with more testing and assurity
- References: <978078.82017.qm@web112508.mail.gq1.yahoo.com>
paawan oza wrote:
Hi,
please find the latest patch and ChangeLog as follows.
Hi Oza,
Thanks for all your work on this, particularly the revisions.
I think the patch has made a lot of progress.
I'm going to devote this review to text formatting and
white space. I know you've already made a lot of changes
in that area, but there are still some issues, and once
we get them taken care of we'll have an easier time.
I'll write about testing and your testcase in a separate message.
I'm afraid some of my comments might seem nit-picky. In particular,
I don't know whether English is your first language, so please don't
be offended if I correct your grammar and punctuation.
********************
ChangeLog
********************
Current: gdb-6.8.50.20090706
2009-07-06 Oza <paawan1982@yahoo.com>
* i386-tdep.c: Support for floating point recording.
***************************
i386.records.floats.patch
***************************
diff -urN ./gdb.orig/i386-tdep.c gdb.new/i386-tdep.c
--- ./gdb.orig/i386-tdep.c 2009-07-02 13:25:54.000000000 -0400
+++ gdb.new/i386-tdep.c 2009-07-06 23:44:07.000000000 -0400
@@ -3056,6 +3056,66 @@
return 0;
}
+
+/* Defines contents to record. */
+#define I386_SAVE_FPU_REGS 0xfffd
+#define I386_SAVE_FPU_ENV 0xfffe
+#define I386_SAVE_FPU_ENV_REG_STACK 0xffff
+
+/* Record the value of floating point registers which will be changed by the
+ Current instruction to "record_arch_list".
"Current" shouldn't be capitalized - it does not begin a sentence.
Also it would be nice to shorten the longest line by moving a couple
of words down to the next line.
+ Return -1 if something is wrong. */
+
+static int i386_record_floats(struct gdbarch *gdbarch,
Always one space between a function name and the parenthesis.
+ struct i386_record_s *ir,
+ uint32_t iregnum)
And then the arguments will need one more space too, to line up.
I don't know whether you use emacs, but if you do, emacs will
help you with this indenting.
+{
+ struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+ int i;
+
+ /* Oza : because of floating point insn push/pop of fpu stack
+ Is going to happen. currently we store st0-st7 registers,
Several things here.
1) Capitalization. First word in a sentence should be capitalized,
not the first word on a line. So, "Is" should not be capitalized,
but "Currently" should.
2) Periods that end a sentence should be followed by two spaces.
3) Indentation. The word "is" should be indented to align with
the word "Oza" above it. See the comment at the beginning of
i386-tdep.c for an example.
4) No space before a colon (":").
+ But we need not store all registers all the time, In future
+ We use ftag register and record only those who are not marked as an empty.
"But" and "We" should not be capitalized. Sentence ends with the
words "all the time", should be followed by a period and two spaces.
+ */
+ if (I386_SAVE_FPU_REGS == iregnum)
+ {
+ for (i = I387_ST0_REGNUM (tdep);i <= I387_ST0_REGNUM (tdep) + 7;i++)
One space after every c operator, semicolon (";") in this case.
Except at the end of a line.
+ {
+ if (record_arch_list_add_reg (ir->regcache,i))
One space after comma.
+ return -1;
+ }
+ }
+ else if (I386_SAVE_FPU_ENV == iregnum)
+ {
+ for (i = I387_FCTRL_REGNUM(tdep);i <= I387_FOP_REGNUM(tdep);i++)
One space after semicolon. One space between macro name and paren.
+ {
+ if (record_arch_list_add_reg (ir->regcache,i))
Space after comma.
+ return -1;
+ }
+ }
+ else if (I386_SAVE_FPU_ENV_REG_STACK == iregnum)
+ {
+ for (i = I387_ST0_REGNUM (tdep);i <= I387_FOP_REGNUM(tdep);i++)
+ {
+ if (record_arch_list_add_reg (ir->regcache,i))
Space after semicolons, space after comma.
+ return -1;
+ }
+ }
+ else if ((iregnum >= I387_ST0_REGNUM (tdep)) &&
+ (iregnum <= I387_FOP_REGNUM(tdep)))
+ {
+ if (record_arch_list_add_reg (ir->regcache,iregnum))
Space after comma.
+ return -1;
+ }
+ else
+ {
+ /* Parameter error. */
+ 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. */
@@ -3070,6 +3130,7 @@
uint32_t tmpu32;
uint32_t opcode;
struct i386_record_s ir;
+ struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
memset (&ir, 0, sizeof (struct i386_record_s));
ir.regcache = regcache;
@@ -4105,8 +4166,7 @@
}
break;
- /* floats */
- /* It just record the memory change of instrcution. */
+ /* Floats. */
case 0xd8:
case 0xd9:
case 0xda:
@@ -4120,46 +4180,57 @@
ir.reg |= ((opcode & 7) << 3);
if (ir.mod != 3)
{
- /* memory */
+ /* Memory. */
uint32_t addr;
if (i386_record_lea_modrm_addr (&ir, &addr))
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. */
+ break;
+ case 0x03:
+ case 0x13:
+ case 0x23:
+ case 0x33:
+ /* For fcomp, ficomp pop FPU stack, store all. */
+ if (i386_record_floats(gdbarch, &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:
Ok here, in the line for "case 0x11", the original source file
has a tab, but you've changed it to spaces. There are a lot
more cases like this below. This makes the diff greater than
it needs to be, and besides, we have an explicit rule against
gratuitous (unnecessary) white space changes mixed in with
other changes.
There is an easy way to fix this. Next time you make a diff
for submission, add -w and -b to your diff flags. That will
cause diff to ignore changes in white space.
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;
+ /* For 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. */
Sentence punctuation.
+ if (i386_record_floats(gdbarch, &ir, I387_ST0_REGNUM (tdep)))
One space between function name and paren.
+ return -1;
+ break;
case 0x08:
case 0x0a:
case 0x0b:
@@ -4167,6 +4238,7 @@
case 0x19:
case 0x1a:
case 0x1b:
+ case 0x1d:
case 0x28:
case 0x29:
case 0x2a:
@@ -4174,11 +4246,16 @@
case 0x38:
case 0x39:
case 0x3a:
- case 0x3b:
+ case 0x3b:
+ case 0x3c:
+ case 0x3d:
switch (ir.reg & 7)
{
case 0:
- break;
+ /* Handling fld, fild. */
+ if (i386_record_floats(gdbarch, &ir, I386_SAVE_FPU_REGS))
Space after function name.
+ return -1;
+ break;
case 1:
switch (ir.reg >> 4)
{
@@ -4191,6 +4268,7 @@
return -1;
break;
case 3:
+ break;
default:
if (record_arch_list_add_mem (addr, 2))
return -1;
@@ -4201,15 +4279,48 @@
switch (ir.reg >> 4)
{
case 0:
+ if (record_arch_list_add_mem (addr, 4))
+ return -1;
+ if (3 == (ir.reg & 7))
+ {
+ /* For fstp m32fp */
+ if (i386_record_floats(gdbarch, &ir, \
Space after function name.
Also I don't know whether you put that backward-slash
at the end of the line, or if the email software inserted it,
but it doesn't belong there.
+ 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)))
+ {
+ /* For fstp insn */
+ if (i386_record_floats(gdbarch, &ir, \
+ I386_SAVE_FPU_REGS))
Space after function name.
+ return -1;
+ }
break;
case 2:
if (record_arch_list_add_mem (addr, 8))
return -1;
+ if (3 == (ir.reg & 7))
+ {
+ /* For fstp m64fp */
+ if (i386_record_floats(gdbarch, &ir, \
+ I386_SAVE_FPU_REGS))
Space after function name.
+ return -1;
+ }
break;
case 3:
+ if ((3 <= (ir.reg & 7)) && (6 <= (ir.reg & 7)))
+ {
+ /* For fistp, fbld, fild, fbstp. */
+ if (i386_record_floats(gdbarch, &ir, \
+ I386_SAVE_FPU_REGS))
Space after function name.
+ return -1;
+ }
default:
if (record_arch_list_add_mem (addr, 2))
return -1;
@@ -4218,54 +4329,74 @@
break;
}
break;
- case 0x0c:
- case 0x0d:
- case 0x1d:
- case 0x2c:
- case 0x3c:
- case 0x3d:
- break;
- case 0x0e:
+ case 0x0c:
+ /* Insn fldenv. */
+ if (i386_record_floats(gdbarch, &ir, \
+ I386_SAVE_FPU_ENV_REG_STACK))
Space after function name. And no backward-slash.
+ return -1;
+ break;
+ case 0x0d:
+ /* Insn fldcw. */
+ if (i386_record_floats(gdbarch, &ir, I387_FCTRL_REGNUM (tdep)))
+ return -1;
+ break;
+ case 0x2c:
+ /* Insn frstor. */
+ if (i386_record_floats(gdbarch, &ir, \
+ I386_SAVE_FPU_ENV_REG_STACK))
Space after function name, and no backslash.
+ 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;
+ /* Insn fstp, fbstp. */
+ if (i386_record_floats(gdbarch, &ir, I386_SAVE_FPU_REGS))
Space.
+ 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;
+ /* Insn fsave. */
+ if (i386_record_floats(gdbarch, &ir, \
+ I386_SAVE_FPU_ENV_REG_STACK))
Space, and no backslash.
+ return -1;
break;
- case 0x3f:
+ case 0x3f:
if (record_arch_list_add_mem (addr, 8))
return -1;
+ /* Ins fistp. */
+ if (i386_record_floats(gdbarch, &ir, I386_SAVE_FPU_REGS))
Space.
+ return -1;
break;
default:
ir.addr -= 2;
@@ -4273,9 +4404,198 @@
goto no_support;
break;
}
- }
+ }
+ /* Opcode is an extension of modR/M byte. */
+ else
+ {
+ switch (opcode)
+ {
+ case 0xd8:
+ if (i386_record_floats(gdbarch, &ir, I387_ST0_REGNUM (tdep)))
Space.
+ return -1;
+ break;
+ case 0xd9:
+ if (0x0c == (ir.modrm >> 4))
+ {
+ if ((ir.modrm & 0x0f) <= 7)
+ {
+ if (i386_record_floats(gdbarch, &ir, I386_SAVE_FPU_REGS))
Space.
+ return -1;
+ }
+ else
+ {
+ if (i386_record_floats(gdbarch, &ir, \
+ I387_ST0_REGNUM (tdep)))
Space, and no backslash.
+ return -1;
+ /* If only st(0) is changing, then we have already recorded */
Period at end of sentence, and two spaces.
+ if ((ir.modrm & 0x0f) - 0x08)
+ {
+ if (i386_record_floats(gdbarch, &ir, \
+ I387_ST0_REGNUM (tdep) + ((ir.modrm & 0x0f) - 0x08)))
Space, no backslash, and the second line needs to be indented
so that "I387" is aligned with "gdbarch". And then of course
the line is unpleasantly long, so you can break it up into
several more lines if you wish.
+ 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(gdbarch, &ir, \
+ I387_ST0_REGNUM (tdep)))
Space, and no backslash.
+ 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(gdbarch, &ir, I386_SAVE_FPU_REGS))
Space, and no backslash.
+ return -1;
+ break;
+ case 0xfd:
+ if (i386_record_floats(gdbarch, &ir, \
+ I387_ST0_REGNUM (tdep)))
Space, and no backslash.
+ return -1;
+ if (i386_record_floats(gdbarch, &ir, \
+ I387_ST0_REGNUM (tdep) + 1))
Space, and no backslash.
+ return -1;
+ break;
+ }
+ }
+ break;
+ case 0xda:
+ if (0xe9 == ir.modrm)
+ {
+ if (i386_record_floats(gdbarch, &ir, I386_SAVE_FPU_REGS))
Space.
+ return -1;
+ }
+ else if ((0x0c == ir.modrm >> 4) || (0x0d == ir.modrm >> 4))
+ {
+ if (i386_record_floats(gdbarch, &ir, I387_ST0_REGNUM (tdep)))
Space.
+ return -1;
+ if (((ir.modrm & 0x0f) > 0) && ((ir.modrm & 0x0f) <= 7))
+ {
+ if (i386_record_floats(gdbarch, &ir, \
+ I387_ST0_REGNUM (tdep) + (ir.modrm & 0x0f)))
Space, no backslash, and indentation (see above).
+ return -1;
+ }
+ else if ((ir.modrm & 0x0f) - 0x08)
+ {
+ if (i386_record_floats(gdbarch, &ir, \
+ I387_ST0_REGNUM (tdep) + ((ir.modrm & 0x0f) - 0x08)))
Space, no backslash, and indentation (see above).
+ return -1;
+ }
+ }
+ break;
+ case 0xdb:
+ if (0xe3 == ir.modrm)
+ {
+ if (i386_record_floats(gdbarch, &ir, I386_SAVE_FPU_ENV))
Space.
+ return -1;
+ }
+ else if ((0x0c == ir.modrm >> 4) || (0x0d == ir.modrm >> 4))
+ {
+ if (i386_record_floats(gdbarch, &ir, I387_ST0_REGNUM (tdep)))
Space.
+ return -1;
+ if (((ir.modrm & 0x0f) > 0) && ((ir.modrm & 0x0f) <= 7))
+ {
+ if (i386_record_floats(gdbarch, &ir, \
+ I387_ST0_REGNUM (tdep) + (ir.modrm & 0x0f)))
Space, no backslash, and indentation (see above).
+ return -1;
+ }
+ else if ((ir.modrm & 0x0f) - 0x08)
+ {
+ if (i386_record_floats(gdbarch, &ir, \
+ I387_ST0_REGNUM (tdep) + ((ir.modrm & 0x0f) - 0x08)))
Space, no backslash, and indentation (see above).
+ 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(gdbarch, &ir, \
+ I387_ST0_REGNUM (tdep) + (ir.modrm & 0x0f)))
Space, no backslash, and indentation (see above).
+ return -1;
+ }
+ else
+ {
+ if (i386_record_floats(gdbarch, &ir, \
+ I387_ST0_REGNUM (tdep) + ((ir.modrm & 0x0f) - 0x08)))
Space, no backslash, and indentation (see above).
+ return -1;
+ }
+ }
+ break;
+ case 0xdd:
+ if (0x0c == ir.modrm >> 4)
+ {
+ if (i386_record_floats(gdbarch, &ir, \
+ I387_FTAG_REGNUM (tdep)))
Space, no backslash, and indentation (see above).
+ return -1;
+ }
+ else if ((0x0d == ir.modrm >> 4) || (0x0e == ir.modrm >> 4))
+ {
+ if ((ir.modrm & 0x0f) <= 7)
+ {
+ if (i386_record_floats(gdbarch, &ir, \
+ I387_ST0_REGNUM (tdep) + (ir.modrm & 0x0f)))
Space, no backslash, and indentation (see above).
+ return -1;
+ }
+ else
+ {
+ if (i386_record_floats(gdbarch, &ir, I386_SAVE_FPU_REGS))
Space.
+ return -1;
+ }
+ }
+ break;
+ case 0xde:
+ if ((0x0c == ir.modrm >> 4) \
+ || (0x0e == ir.modrm >> 4) \
+ || (0x0f == ir.modrm >> 4) \
+ || (0xd9 == ir.modrm))
No backslashes.
+ {
+ if (i386_record_floats(gdbarch, &ir, I386_SAVE_FPU_REGS))
Space.
+ 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(gdbarch, &ir, I386_SAVE_FPU_REGS))
Space.
+ return -1;
+ }
+ break;
+ }
+ }
break;
-
/* string ops */
/* movsS */
case 0xa4:
@@ -4694,10 +5014,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"),
+ paddress (gdbarch, ir.addr));
+ return -1;
+ }
+ opcode = (uint32_t) tmpu8;
+ ir.addr++;
+ goto reswitch;
break;
/* int3 */