[RFA] i386-tdep.c: fix a bug in prec i386 code

[RFA] i386-tdep.c: fix a bug in prec i386 code

[Hui Zhu]

Hi,

I found that decode code for sidt in i386_process_record has a bug.
I made a patch to fix it.

Please help me review it.

Thanks,
Hui

2010-03-04  Hui Zhu  <teawater@gmail.com>

	* i386-tdep.c (i386_process_record): Change "addr" to "tmpu64".

---
 i386-tdep.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/i386-tdep.c
+++ b/i386-tdep.c
@@ -5388,7 +5388,7 @@ i386_process_record (struct gdbarch *gdb
 		    return -1;
 		  if (record_arch_list_add_mem (tmpu64, 2))
 		    return -1;
-		  addr += 2;
+		  tmpu64 += 2;
                   if (ir.regmap[X86_RECORD_R8_REGNUM])
                     {
                       if (record_arch_list_add_mem (tmpu64, 8))

Re: [RFA] i386-tdep.c: fix a bug in prec i386 code

[Joel Brobecker]

> 2010-03-04  Hui Zhu  <teawater@gmail.com>
> 
> 	* i386-tdep.c (i386_process_record): Change "addr" to "tmpu64".

OK.

As an aside, your code needs a really good thorough cleanup. I warned
you already about the use of variables with a meaningless name, and
you'll make this sort of mistake again for as long as you keep using
them. However, my main point is that the use of a giant switch statement
makes your code very hard to read and review.  I really suggest that
you create a new file, precord-i386.c where you put your stuff there,
and instead of inlining the code inside each case, you define small
contained procedures for each instruction (or instruction group).
It'll be easier to find the code that handles such and such instruction,
easier to write a ChangeLog entry that tells us more about where the
change was made, and it'll make the switch block actually possible
to read.

-- 
Joel

Re: [RFA] i386-tdep.c: fix a bug in prec i386 code

[Hui Zhu]

On Thu, Mar 4, 2010 at 15:38, Joel Brobecker <brobecker@adacore.com> wrote:
>
> > 2010-03-04  Hui Zhu  <teawater@gmail.com>
> >
> >       * i386-tdep.c (i386_process_record): Change "addr" to "tmpu64".
>
> OK.

Checked in.

>
> As an aside, your code needs a really good thorough cleanup. I warned
> you already about the use of variables with a meaningless name, and
> you'll make this sort of mistake again for as long as you keep using
> them. However, my main point is that the use of a giant switch statement
> makes your code very hard to read and review.  I really suggest that
> you create a new file, precord-i386.c where you put your stuff there,
> and instead of inlining the code inside each case, you define small
> contained procedures for each instruction (or instruction group).
> It'll be easier to find the code that handles such and such instruction,
> easier to write a ChangeLog entry that tells us more about where the
> change was made, and it'll make the switch block actually possible
> to read.
>

Agree with you.
But prec patch have some still hang in there (see
http://sourceware.org/gdb/wiki/ProcessRecord) and this change will be
very big.
If I do it, what I will meet is:
1.  Make a patch follow the hang change, then the new patch cannot
checked in before the hang patch in.
2.  Make a patch just for cvs-head.  After this patch in, this hang
patch need update follow cvs-head.  This is not a small work.

I conflict with them.  So ...

Thanks,
Hui