[PATCH] Don't clobber info->mach in gdb_print_insn_mips

[PATCH] Don't clobber info->mach in gdb_print_insn_mips

[Fred Fish]

I updated to the latest CVS gdb over the weekend and afterwards my
mipsisa32-intrinsity-elf target would no longer disassemble
coprocessor 2 instructions correctly.  I tracked it down to the
info->mach field being clobbered by gdb_print_insn_mips.  The attached
patch fixes it, though I'm not sure if this is the correct solution.

Note that mipsisa32-intrinsity-elf is not yet a supported target
configuration as it is a work in progress and the patches have not
been submitted yet.  But mipsisa32-unknown-elf has a similar problem.

-Fred


2003-06-23  Fred Fish  <fnf@intrinsity.com>

	* mips-tdep.c (gdb_print_insn_mips): Don't clobber a previously
	set value of info->mach.


Index: mips-tdep.c
===================================================================
RCS file: /mips/newtools/fsf/gdb/gdb/mips-tdep.c,v
retrieving revision 1.20
diff -c -r1.20 mips-tdep.c
*** mips-tdep.c	2003/06/23 02:13:09	1.20
--- mips-tdep.c	2003/06/23 13:33:02
***************
*** 5279,5288 ****
       guess that if the address passed in is odd, it's 16-bits.  */
    if (proc_desc)
      info->mach = pc_is_mips16 (PROC_LOW_ADDR (proc_desc)) ?
!       bfd_mach_mips16 : 0;
    else
      info->mach = pc_is_mips16 (memaddr) ?
!       bfd_mach_mips16 : 0;
  
    /* Round down the instruction address to the appropriate boundary.  */
    memaddr &= (info->mach == bfd_mach_mips16 ? ~1 : ~3);
--- 5279,5288 ----
       guess that if the address passed in is odd, it's 16-bits.  */
    if (proc_desc)
      info->mach = pc_is_mips16 (PROC_LOW_ADDR (proc_desc)) ?
!       bfd_mach_mips16 : info->mach;
    else
      info->mach = pc_is_mips16 (memaddr) ?
!       bfd_mach_mips16 : info->mach;
  
    /* Round down the instruction address to the appropriate boundary.  */
    memaddr &= (info->mach == bfd_mach_mips16 ? ~1 : ~3);

Re: [PATCH] Don't clobber info->mach in gdb_print_insn_mips

[Andrew Cagney]

> I updated to the latest CVS gdb over the weekend and afterwards my
> mipsisa32-intrinsity-elf target would no longer disassemble
> coprocessor 2 instructions correctly.  I tracked it down to the
> info->mach field being clobbered by gdb_print_insn_mips.  The attached
> patch fixes it, though I'm not sure if this is the correct solution.

What happens if info->mach is never set?  gdb_disassemble_info should 
have already set it correctly.

Andrew


> Note that mipsisa32-intrinsity-elf is not yet a supported target
> configuration as it is a work in progress and the patches have not
> been submitted yet.  But mipsisa32-unknown-elf has a similar problem.
> 

Re: [PATCH] Don't clobber info->mach in gdb_print_insn_mips

[Fred Fish]

> > info->mach field being clobbered by gdb_print_insn_mips.  The attached
> > patch fixes it, though I'm not sure if this is the correct solution.
> 
> What happens if info->mach is never set?  gdb_disassemble_info should 
> have already set it correctly.

Yes, gdb_disassemble_info sets it correctly, and then gdb_print_insn_mips
clobbers it back to zero.  Here is a typescript showing the order that
various functions are called.

Without my patch, gdb_disassemble_info sets it to a correct value,
gdb_print_insn_mips clobbers it back to zero, and then eventually
choose_arch_by_number gets called with mach==0 (my typescript shows
10611501, the expected value for my port, since I have my patch
installed).  Since choose_arch_by_number then returns zero,
the code in set_default_mips_dis_options that sets mips_isa
never gets executed:

  chosen_arch = choose_arch_by_number (info->mach);
  if (chosen_arch != NULL)
    {
      mips_processor = chosen_arch->processor;
      mips_isa = chosen_arch->isa;
      mips_cp0_names = chosen_arch->cp0_names;
      mips_cp0sel_names = chosen_arch->cp0sel_names;
      mips_cp0sel_names_len = chosen_arch->cp0sel_names_len;
      mips_hwr_names = chosen_arch->hwr_names;
    }

and mips_isa remains set to ISA_MIPS3 from earlier in
set_default_mips_dis_options:

  mips_isa = ISA_MIPS3;

-Fred

============================================================================
fred:intrinsity [35] gdb -nw gdb
GNU gdb Red Hat Linux (5.3post-0.20021129.18rh)
Copyright 2003 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "i386-redhat-linux-gnu"...
Setting up the environment for debugging gdb.
Breakpoint 1 at 0x815f7ae: file /src/intrinsity/sdk/sdk/fsf/gdb/gdb/utils.c, line 800.
Breakpoint 2 at 0x8096c2c: file /src/intrinsity/sdk/sdk/fsf/gdb/gdb/cli/cli-cmds.c, line 191.
Breakpoint 3 at 0x82e8c52: file /src/intrinsity/sdk/sdk/fsf/gdb/opcodes/mips-dis.c, line 469.
Breakpoint 4 at 0x82e8d11: file /src/intrinsity/sdk/sdk/fsf/gdb/opcodes/mips-dis.c, line 495.
Breakpoint 5 at 0x82ea158: file /src/intrinsity/sdk/sdk/fsf/gdb/opcodes/mips-dis.c, line 1209.
Breakpoint 6 at 0x81126cb: file /src/intrinsity/sdk/sdk/fsf/gdb/gdb/mips-tdep.c, line 5273.
Breakpoint 7 at 0x80bb7de: file /src/intrinsity/sdk/sdk/fsf/gdb/gdb/disasm.c, line 318.
(top-gdb) run j
Starting program: /links2/build/intrinsity/sdk/T-mipsisa32-intrinsity-elf/fsf/gdb/gdb/gdb j
GNU gdb 2003-06-22-cvs
Copyright 2003 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "--host=i686-pc-linux-gnu --target=mipsisa32-intrinsity-elf"...
Setting up the environment for debugging gdb.
.gdbinit:5: Error in sourced command file:
Function "internal_error" not defined.
(gdb) x/i main

Breakpoint 7, gdb_disassemble_info (gdbarch=0x859c090, file=0x859a170) at /src/intrinsity/sdk/sdk/fsf/gdb/gdb/disasm.c:318
318                                      (fprintf_ftype) fprintf_filtered);
(top-gdb) c
Continuing.

Breakpoint 6, gdb_print_insn_mips (memaddr=18446744071562199548, info=0xbfffd2a0) at /src/intrinsity/sdk/sdk/fsf/gdb/gdb/mips-tdep.c:5273
5273      memaddr = ADDR_BITS_REMOVE (memaddr);
(top-gdb) c
Continuing.

Breakpoint 5, _print_insn_mips (memaddr=18446744071562199548, info=0xbfffd2a0, endianness=BFD_ENDIAN_BIG) at /src/intrinsity/sdk/sdk/fsf/gdb/opcodes/mips-dis.c:1209
1209      set_default_mips_dis_options (info);
(top-gdb) c
Continuing.

Breakpoint 4, set_default_mips_dis_options (info=0xbfffd2a0) at /src/intrinsity/sdk/sdk/fsf/gdb/opcodes/mips-dis.c:495
495       mips_isa = ISA_MIPS3;
(top-gdb) c
Continuing.

Breakpoint 3, choose_arch_by_number (mach=10611501) at /src/intrinsity/sdk/sdk/fsf/gdb/opcodes/mips-dis.c:469
469       if (hint_bfd_mach == mach
(top-gdb) c
Continuing.
0x800201fc <main>:      addiu   sp,sp,-256
(gdb) 

Re: [PATCH] Don't clobber info->mach in gdb_print_insn_mips

[Andrew Cagney]

> info->mach field being clobbered by gdb_print_insn_mips.  The attached
>> > patch fixes it, though I'm not sure if this is the correct solution.
> 
>> 
>> What happens if info->mach is never set?  gdb_disassemble_info should 
>> have already set it correctly.
> 
> 
> Yes, gdb_disassemble_info sets it correctly, and then gdb_print_insn_mips
> clobbers it back to zero.  Here is a typescript showing the order that
> various functions are called.
> 
> Without my patch, gdb_disassemble_info sets it to a correct value,
> gdb_print_insn_mips clobbers it back to zero, and then eventually
> choose_arch_by_number gets called with mach==0 (my typescript shows
> 10611501, the expected value for my port, since I have my patch
> installed).  Since choose_arch_by_number then returns zero,
> the code in set_default_mips_dis_options that sets mips_isa
> never gets executed:

So you're ok if I delete that bit of gdb_print_insn_mips?

Andrew

Re: [PATCH] Don't clobber info->mach in gdb_print_insn_mips

[Fred Fish]

> So you're ok if I delete that bit of gdb_print_insn_mips?

Works for me, though I assume that it was put there for a
reason.  Perhaps a better fix is something like:

  if (proc_desc)
    {
      if (pc_is_mips16 (PROC_LOW_ADDR (proc_desc))
        info->mach =  bfd_mach_mips16;
    }
  else
    {
      if (pc_is_mips16 (memaddr))
       info->mach = bfd_mach_mips16;
    }

which would leave info->mach alone unless pc_is_mips16 is true.

-Fred

Re: [PATCH] Don't clobber info->mach in gdb_print_insn_mips

[Andrew Cagney]

>> So you're ok if I delete that bit of gdb_print_insn_mips?
> 
> 
> Works for me, though I assume that it was put there for a
> reason.

The local-to-gdb disasembler functions pre-date libopcodes.  I think 
what's happened is that these wrappers have been accumulating with 
workarounds to bugs that should have been fixed in libopcodes.

`objdump -d' for instance needs to work, and in that situtation the 
disassembler has no additional information.

 > Perhaps a better fix is something like:
> 
>   if (proc_desc)
>     {
>       if (pc_is_mips16 (PROC_LOW_ADDR (proc_desc))
>         info->mach =  bfd_mach_mips16;
>     }
>   else
>     {
>       if (pc_is_mips16 (memaddr))
>        info->mach = bfd_mach_mips16;
>     }
> 
> which would leave info->mach alone unless pc_is_mips16 is true.

True I'll add it with a comment questioning it's need :-)

Andrew

Re: [PATCH] Don't clobber info->mach in gdb_print_insn_mips

[Andrew Cagney]

[-- Attachment #1: Type: text/plain, Size: 49 bytes --]

I've checked this in trunk and 6 branch.

Andrew

[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 1394 bytes --]

2003-06-26  Andrew Cagney  <cagney@redhat.com>

	* mips-tdep.c (gdb_print_insn_mips): Only explicitly set
	info->mach when MIPS16.  Patch suggested by Fred Fish.

Index: mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.221
diff -u -r1.221 mips-tdep.c
--- mips-tdep.c	21 Jun 2003 23:14:43 -0000	1.221
+++ mips-tdep.c	26 Jun 2003 17:58:48 -0000
@@ -5270,12 +5270,20 @@
      the procedure descriptor exists and the address therein is odd,
      it's definitely a 16-bit function.  Otherwise, we have to just
      guess that if the address passed in is odd, it's 16-bits.  */
+  /* FIXME: cagney/2003-06-26: Is this even necessary?  The
+     disassembler needs to be able to locally determine the ISA, and
+     not rely on GDB.  Otherwize the stand-alone 'objdump -d' will not
+     work.  */
   if (proc_desc)
-    info->mach = pc_is_mips16 (PROC_LOW_ADDR (proc_desc)) ?
-      bfd_mach_mips16 : 0;
+    {
+      if (pc_is_mips16 (PROC_LOW_ADDR (proc_desc)))
+	info->mach =  bfd_mach_mips16;
+    }
   else
-    info->mach = pc_is_mips16 (memaddr) ?
-      bfd_mach_mips16 : 0;
+    {
+      if (pc_is_mips16 (memaddr))
+       info->mach = bfd_mach_mips16;
+    } 
 
   /* Round down the instruction address to the appropriate boundary.  */
   memaddr &= (info->mach == bfd_mach_mips16 ? ~1 : ~3);