sh-elf disassembly broken (Was: Re: RFC: Moving disassembler_command to cli land and using newer disassembler code)

sh-elf disassembly broken (Was: Re: RFC: Moving disassembler_command to cli land and using newer disassembler code)

[Joern Rennecke]

This patch:

2002-11-07  Fernando Nasser  <fnasser@redhat.com>

        * printcmd.c (disassemble_command): Remove obsolete function.
        (_initialize_printcmd): Do not create disassemble command here.
        * cli/cli-cmds.c (disassemble_command): New function.  Implements
        disassemble command.
        (init_cli_cmds): Create disassemble command here instead.

breaks disassembly for sh-elf (just try to disassemble any
function in any sh-elf object file).

gdb_disassembly uses TARGET_PRINT_INSN_INFO->mach to initialize
di.mach, but TARGET_PRINT_INSN_INFO->mach is never initialized.

Before your patch, print_insn had set TARGET_PRINT_INSN_INFO->mach
to TARGET_ARCHITECTURE->mach, which gets expanded to 
(gdbarch_bfd_arch_info (current_gdbarch))->mach .


-- 
--------------------------
SuperH (UK) Ltd.
2410 Aztec West / Almondsbury / BRISTOL / BS32 4QX
T:+44 1454 465658

Re: sh-elf disassembly broken (Was: Re: RFC: Moving disassembler_command to cli land and using newer disassembler code)

[Joern Rennecke]

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

I wrote:
> 
> This patch:
> 
> 2002-11-07  Fernando Nasser  <fnasser@redhat.com>
> 
>         * printcmd.c (disassemble_command): Remove obsolete function.
>         (_initialize_printcmd): Do not create disassemble command here.
>         * cli/cli-cmds.c (disassemble_command): New function.  Implements
>         disassemble command.
>         (init_cli_cmds): Create disassemble command here instead.
> 
> breaks disassembly for sh-elf (just try to disassemble any
> function in any sh-elf object file).
> 
> gdb_disassembly uses TARGET_PRINT_INSN_INFO->mach to initialize
> di.mach, but TARGET_PRINT_INSN_INFO->mach is never initialized.
> 
> Before your patch, print_insn had set TARGET_PRINT_INSN_INFO->mach
> to TARGET_ARCHITECTURE->mach, which gets expanded to
> (gdbarch_bfd_arch_info (current_gdbarch))->mach .

And didn't get any reply.

So I had a go at writing a conservative patch, which handles the mach
field likee tui/tuiDisassem.c:tui_disassemble .

Although I have the feeling that TARGET_PRINT_INSN_INFO should be really
a local variable of print_insn, and if any information might be missing
in the gdbarch struct to initialize TARGET_PRINT_INSN_INFO and asm_info
in tui/tuiDisassem.c:tui_disassemble ,  that should rather be added to
the gdbarch struct.
	
-- 
--------------------------
SuperH (UK) Ltd.
2410 Aztec West / Almondsbury / BRISTOL / BS32 4QX
T:+44 1454 465658

[-- Attachment #2: disas-diff --]
[-- Type: text/plain, Size: 992 bytes --]

Mon Feb 24 19:59:50 2003  J"orn Rennecke <joern.rennecke@superh.com>

	* disasm.c (gdb_disassembly): If TARGET_ARCHITECTURE is non-zero,
	initialize di.mach from there.

Index: disasm.c
===================================================================
RCS file: /cvs/src/src/gdb/disasm.c,v
retrieving revision 1.4
diff -p -r1.4 disasm.c
*** disasm.c	3 Feb 2003 01:18:37 -0000	1.4
--- disasm.c	24 Feb 2003 19:59:47 -0000
*************** gdb_disassembly (struct ui_out *uiout,
*** 338,344 ****
--- 338,350 ----
        di_initialized = 1;
      }
  
+   /* ??? TARGET_PRINT_INSN_INFO is undocumented and only initialized inside
+      printcmd.c:print_insn , and some strange hacks that access fields of
+      the variable that underlies the implementation of this macro.  */
    di.mach = TARGET_PRINT_INSN_INFO->mach;
+   if (TARGET_ARCHITECTURE != NULL)
+     di.mach = TARGET_ARCHITECTURE->mach;
+ 
    if (TARGET_BYTE_ORDER == BFD_ENDIAN_BIG)
      di.endian = BFD_ENDIAN_BIG;
    else

Re: sh-elf disassembly broken (Was: Re: RFC: Moving disassembler_command to cli land and using newer disassembler code)

[Elena Zannoni]

Joern, would something like this work, instead? 
Index: sh-tdep.c
===================================================================
RCS file: /cvs/uberbaum/gdb/sh-tdep.c,v
retrieving revision 1.96
diff -u -p -r1.96 sh-tdep.c
--- sh-tdep.c	3 Feb 2003 14:38:05 -0000	1.96
+++ sh-tdep.c	24 Feb 2003 21:48:59 -0000
@@ -937,6 +937,7 @@ sh_store_struct_return (CORE_ADDR addr, 
 static int
 gdb_print_insn_sh (bfd_vma memaddr, disassemble_info *info)
 {
+  info->mach = gdbarch_bfd_arch_info (current_gdbarch)->mach;
   info->endian = TARGET_BYTE_ORDER;
   return print_insn_sh (memaddr, info);
 }



Joern Rennecke writes:
 > I wrote:
 > > 
 > > This patch:
 > > 
 > > 2002-11-07  Fernando Nasser  <fnasser@redhat.com>
 > > 
 > >         * printcmd.c (disassemble_command): Remove obsolete function.
 > >         (_initialize_printcmd): Do not create disassemble command here.
 > >         * cli/cli-cmds.c (disassemble_command): New function.  Implements
 > >         disassemble command.
 > >         (init_cli_cmds): Create disassemble command here instead.
 > > 
 > > breaks disassembly for sh-elf (just try to disassemble any
 > > function in any sh-elf object file).
 > > 
 > > gdb_disassembly uses TARGET_PRINT_INSN_INFO->mach to initialize
 > > di.mach, but TARGET_PRINT_INSN_INFO->mach is never initialized.
 > > 
 > > Before your patch, print_insn had set TARGET_PRINT_INSN_INFO->mach
 > > to TARGET_ARCHITECTURE->mach, which gets expanded to
 > > (gdbarch_bfd_arch_info (current_gdbarch))->mach .
 > 
 > And didn't get any reply.
 > 
 > So I had a go at writing a conservative patch, which handles the mach
 > field likee tui/tuiDisassem.c:tui_disassemble .
 > 
 > Although I have the feeling that TARGET_PRINT_INSN_INFO should be really
 > a local variable of print_insn, and if any information might be missing
 > in the gdbarch struct to initialize TARGET_PRINT_INSN_INFO and asm_info
 > in tui/tuiDisassem.c:tui_disassemble ,  that should rather be added to
 > the gdbarch struct.
 > 	
 > -- 
 > --------------------------
 > SuperH (UK) Ltd.
 > 2410 Aztec West / Almondsbury / BRISTOL / BS32 4QX
 > T:+44 1454 465658Mon Feb 24 19:59:50 2003  J"orn Rennecke <joern.rennecke@superh.com>
 > 
 > 	* disasm.c (gdb_disassembly): If TARGET_ARCHITECTURE is non-zero,
 > 	initialize di.mach from there.
 > 
 > Index: disasm.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/disasm.c,v
 > retrieving revision 1.4
 > diff -p -r1.4 disasm.c
 > *** disasm.c	3 Feb 2003 01:18:37 -0000	1.4
 > --- disasm.c	24 Feb 2003 19:59:47 -0000
 > *************** gdb_disassembly (struct ui_out *uiout,
 > *** 338,344 ****
 > --- 338,350 ----
 >         di_initialized = 1;
 >       }
 >   
 > +   /* ??? TARGET_PRINT_INSN_INFO is undocumented and only initialized inside
 > +      printcmd.c:print_insn , and some strange hacks that access fields of
 > +      the variable that underlies the implementation of this macro.  */
 >     di.mach = TARGET_PRINT_INSN_INFO->mach;
 > +   if (TARGET_ARCHITECTURE != NULL)
 > +     di.mach = TARGET_ARCHITECTURE->mach;
 > + 
 >     if (TARGET_BYTE_ORDER == BFD_ENDIAN_BIG)
 >       di.endian = BFD_ENDIAN_BIG;
 >     else

Re: sh-elf disassembly broken (Was: Re: RFC: Moving disassembler_command to cli land and using newer disassembler code)

[Joern Rennecke]

Elena Zannoni wrote:
> 
> Joern, would something like this work, instead?
> Index: sh-tdep.c
> ===================================================================
> RCS file: /cvs/uberbaum/gdb/sh-tdep.c,v
> retrieving revision 1.96
> diff -u -p -r1.96 sh-tdep.c
> --- sh-tdep.c   3 Feb 2003 14:38:05 -0000       1.96
> +++ sh-tdep.c   24 Feb 2003 21:48:59 -0000
> @@ -937,6 +937,7 @@ sh_store_struct_return (CORE_ADDR addr,
>  static int
>  gdb_print_insn_sh (bfd_vma memaddr, disassemble_info *info)
>  {
> +  info->mach = gdbarch_bfd_arch_info (current_gdbarch)->mach;
>    info->endian = TARGET_BYTE_ORDER;
>    return print_insn_sh (memaddr, info);
>  }
> 

It looks like it should work for the way disassembling is used
right now, but it would mean that we don't honour the setting
of mach in the input parameter.  So if you want to add a command
that disassembles a region of memory assuming another machine
setting than the current one, you'd be out of luck.
			
-- 
--------------------------
SuperH (UK) Ltd.
2410 Aztec West / Almondsbury / BRISTOL / BS32 4QX
T:+44 1454 465658

Re: sh-elf disassembly broken (Was: Re: RFC: Moving disassembler_command to cli land and using newer disassembler code)

[Elena Zannoni]

Joern Rennecke writes:
 > Elena Zannoni wrote:
 > > 
 > > Joern, would something like this work, instead?
 > > Index: sh-tdep.c
 > > ===================================================================
 > > RCS file: /cvs/uberbaum/gdb/sh-tdep.c,v
 > > retrieving revision 1.96
 > > diff -u -p -r1.96 sh-tdep.c
 > > --- sh-tdep.c   3 Feb 2003 14:38:05 -0000       1.96
 > > +++ sh-tdep.c   24 Feb 2003 21:48:59 -0000
 > > @@ -937,6 +937,7 @@ sh_store_struct_return (CORE_ADDR addr,
 > >  static int
 > >  gdb_print_insn_sh (bfd_vma memaddr, disassemble_info *info)
 > >  {
 > > +  info->mach = gdbarch_bfd_arch_info (current_gdbarch)->mach;
 > >    info->endian = TARGET_BYTE_ORDER;
 > >    return print_insn_sh (memaddr, info);
 > >  }
 > > 
 > 
 > It looks like it should work for the way disassembling is used
 > right now, but it would mean that we don't honour the setting
 > of mach in the input parameter.  So if you want to add a command
 > that disassembles a region of memory assuming another machine
 > setting than the current one, you'd be out of luck.

Oh, I see, I guess you have such a command. In any case, I'll commit
this, so at least it works right now, vanilla flavor.

elena


 > 			
 > -- 
 > --------------------------
 > SuperH (UK) Ltd.
 > 2410 Aztec West / Almondsbury / BRISTOL / BS32 4QX
 > T:+44 1454 465658

Re: sh-elf disassembly broken (Was: Re: RFC: Moving disassembler_command to cli land and using newer disassembler code)

[Joern Rennecke]

Elena Zannoni wrote:
> Oh, I see, I guess you have such a command. In any case, I'll commit
> this, so at least it works right now, vanilla flavor.

No, but it would be a sensible addition - e.g. to disassemble trampolines
that are coded in a different mode than the current mode.
In any case, you didn't commit the patch in the fsf repository, so it's
still completely broken.
	
-- 
--------------------------
SuperH (UK) Ltd.
2410 Aztec West / Almondsbury / BRISTOL / BS32 4QX
T:+44 1454 465658