[PATCH] GDB/opcodes: Remove arch/mach/endian disassembler assertions

[PATCH] GDB/opcodes: Remove arch/mach/endian disassembler assertions

[Maciej W. Rozycki]

Fix `set architecture' and `set endian' command disassembly regressions 
from commit 39503f82427e ("Delegate opcodes to select disassembler in 
GDB"), and commit 003ca0fd2286 ("Refactor disassembler selection"), as 
well as a MIPS compressed ISA disassembly target regression from commit 
6394c606997f ("Don't use print_insn_XXX in GDB"), which caused assertion 
failures to trigger.

For example with the `mips-linux-gnu' target we get:

$ cat main.c
int
main (void)
{
  return 0;
}
$ gcc -mips32r2 -O2 main.c -o main
$ gcc -mips16 -mips32r2 -O2 main.c -o main16
$ gdb
GNU gdb (GDB) 8.0.50.20170731-git
[...]
(gdb) file main
Reading symbols from main...done.
(gdb) show architecture
The target architecture is set automatically (currently mips:isa32r2)
(gdb) show endian
The target endianness is set automatically (currently big endian)
(gdb) disassemble main
Dump of assembler code for function main:
   0x00400500 <+0>:	jr	ra
   0x00400504 <+4>:	move	v0,zero
End of assembler dump.
(gdb) set architecture mips:isa64r2
The target architecture is assumed to be mips:isa64r2
(gdb) disassemble main
Dump of assembler code for function main:
   0x00400500 <+0>:	
.../gdb/arch-utils.c:979: internal-error: int default_print_insn(bfd_vma, disassemble_info*): Assertion `info->mach == bfd_get_mach (exec_bfd)' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) n
[...]
Command aborted.
(gdb) set architecture auto
The target architecture is set automatically (currently mips:isa32r2)
(gdb) set endian little
The target is assumed to be little endian
(gdb) disassemble main
Dump of assembler code for function main:
   0x00400500 <+0>:
.../gdb/arch-utils.c:978: internal-error: int default_print_insn(bfd_vma, disassemble_info*): Assertion `info->endian == (bfd_big_endian (exec_bfd) ? BFD_ENDIAN_BIG : BFD_ENDIAN_LITTLE)' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) n
[...]
Command aborted.
(gdb) set endian auto
The target endianness is set automatically (currently big endian)
(gdb) set architecture i386
The target architecture is assumed to be i386
(gdb) disassemble main
Dump of assembler code for function main:
   0x00400500 <+0>:
.../gdb/arch-utils.c:976: internal-error: int default_print_insn(bfd_vma, disassemble_info*): Assertion `info->arch == bfd_get_arch (exec_bfd)' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) n
[...]
Command aborted.
(gdb) set architecture auto
The target architecture is set automatically (currently mips:isa32r2)
(gdb) file main16
Load new symbol table from "main16"? (y or n) y
Reading symbols from main16...done.
(gdb) disassemble main
Dump of assembler code for function main:
   0x00400501 <+0>:	
.../gdb/arch-utils.c:979: internal-error: int default_print_insn(bfd_vma, disassemble_info*): Assertion `info->mach == bfd_get_mach (exec_bfd)' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) n
Command aborted.
(gdb) 

Remove the assertions then, restoring previous semantics:

(gdb) file main
Reading symbols from main...done.
(gdb) set architecture mips:isa64r2
The target architecture is assumed to be mips:isa64r2
(gdb) disassemble main
Dump of assembler code for function main:
   0x00400500 <+0>:	jr	ra
   0x00400504 <+4>:	move	v0,zero
End of assembler dump.
(gdb) set endian little
The target is assumed to be little endian
(gdb) disassemble main
Dump of assembler code for function main:
   0x00400500 <+0>:	j	0x3800c
   0x00400504 <+4>:	addiu	s0,t0,0
End of assembler dump.
(gdb) set architecture i386
The target architecture is assumed to be i386
(gdb) disassemble main
Dump of assembler code for function main:
   0x00400500 <+0>:	add    %eax,%esp
   0x00400502 <+2>:	add    %cl,(%eax)
   0x00400504 <+4>:	add    %al,(%eax)
   0x00400506 <+6>:	adc    %ah,0x0
End of assembler dump.
(gdb) set architecture auto
The target architecture is set automatically (currently mips:isa32r2)
(gdb) set endian auto
The target endianness is set automatically (currently big endian)
(gdb) file main16
Load new symbol table from "main16"? (y or n) y
Reading symbols from main16...done.
(gdb) disassemble main
Dump of assembler code for function main:
   0x00400501 <+0>:	jr	ra
   0x00400503 <+2>:	li	v0,0
End of assembler dump.
(gdb) 

	gdb/
	* arch-utils.c (default_print_insn): Remove arch/mach/endian
	assertions.

	opcodes/
	* disassemble.c (disassembler): Remove arch/mach/endian 
	assertions.
---
Hi,

 Regression-tested with GDB using the `mips-linux-gnu' target, o32 ABI, 
bringing the number of failures down by ~300.  Also no regressions between 
plain commit 6394c606997f^ and commit 6394c606997f with this patch applied 
(as long as the mem-break fix, posted separately, has been also applied).

 OK to apply.

  Maciej

---
 gdb/arch-utils.c      |    7 -------
 opcodes/disassemble.c |   13 +------------
 2 files changed, 1 insertion(+), 19 deletions(-)

gdb-opcode-disassemble-assert.diff
Index: binutils/gdb/arch-utils.c
===================================================================
--- binutils.orig/gdb/arch-utils.c	2017-07-31 16:21:27.410997829 +0100
+++ binutils/gdb/arch-utils.c	2017-07-31 16:22:15.571550657 +0100
@@ -971,13 +971,6 @@ default_print_insn (bfd_vma memaddr, dis
 {
   disassembler_ftype disassemble_fn;
 
-  if (exec_bfd != NULL)
-    {
-      gdb_assert (info->arch == bfd_get_arch (exec_bfd));
-      gdb_assert (info->endian == (bfd_big_endian (exec_bfd)
-				   ? BFD_ENDIAN_BIG : BFD_ENDIAN_LITTLE));
-      gdb_assert (info->mach == bfd_get_mach (exec_bfd));
-    }
   disassemble_fn = disassembler (info->arch, info->endian == BFD_ENDIAN_BIG,
 				 info->mach, exec_bfd);
 
Index: binutils/opcodes/disassemble.c
===================================================================
--- binutils.orig/opcodes/disassemble.c	2017-07-31 11:12:12.000000000 +0100
+++ binutils/opcodes/disassemble.c	2017-07-31 16:23:47.294937646 +0100
@@ -111,21 +111,10 @@
 
 disassembler_ftype
 disassembler (enum bfd_architecture a, bfd_boolean big, unsigned long mach,
-	      bfd *abfd)
+	      bfd *abfd ATTRIBUTE_UNUSED)
 {
   disassembler_ftype disassemble;
 
-  if (abfd != NULL)
-    {
-      /* Do some asserts that the first three parameters should equal
-	 to what we can get from ABFD.  On the other hand, these
-	 asserts help removing some compiler errors on unused
-	 parameter.  */
-      assert (a == bfd_get_arch (abfd));
-      assert (big == bfd_big_endian (abfd));
-      assert (mach == bfd_get_mach (abfd));
-    }
-
   switch (a)
     {
       /* If you add a case to this table, also add it to the

Re: [PATCH] GDB/opcodes: Remove arch/mach/endian disassembler assertions

[Yao Qi]

"Maciej W. Rozycki" <macro@imgtec.com> writes:

Hi Maciej,

> @@ -971,13 +971,6 @@ default_print_insn (bfd_vma memaddr, dis
>  {
>    disassembler_ftype disassemble_fn;
>  
> -  if (exec_bfd != NULL)
> -    {
> -      gdb_assert (info->arch == bfd_get_arch (exec_bfd));
> -      gdb_assert (info->endian == (bfd_big_endian (exec_bfd)
> -				   ? BFD_ENDIAN_BIG : BFD_ENDIAN_LITTLE));
> -      gdb_assert (info->mach == bfd_get_mach (exec_bfd));
> -    }
>    disassemble_fn = disassembler (info->arch, info->endian == BFD_ENDIAN_BIG,
>  				 info->mach, exec_bfd);

I prefer to keep these asserts, as they could check the inconsistent
uses of disassemble_info between gdb and opcodes.  There is another
assert for target armv5te, PR 21818, but the assert exposes an improper
or unexpected usage of disassemble_info.mach in opcodes/arm-dis.c.  I am
fixing it.

If these asserts make few sense to mips, gdb_print_insn_mips doesn't
have to call default_print_insn.  You can just do this at the end of
gdb_print_insn_mips,

-- 
Yao (齐尧)
diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index c1800e4..8ab7886 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -7025,7 +7025,13 @@ gdb_print_insn_mips (bfd_vma memaddr, struct disassemble_info *info)
        register naming conventions specified by the user.  */
     info->disassembler_options = "gpr-names=32";
 
-  return default_print_insn (memaddr, info);
+  disassembler_ftype disassemble_fn;
+
+  disassemble_fn = disassembler (info->arch, info->endian == BFD_ENDIAN_BIG,
+				 info->mach, exec_bfd);
+
+  gdb_assert (disassemble_fn != NULL);
+  return (*disassemble_fn) (memaddr, info);
 }
 
 static int

Re: [PATCH] GDB/opcodes: Remove arch/mach/endian disassembler assertions

[Maciej W. Rozycki]

On Thu, 3 Aug 2017, Yao Qi wrote:

> I prefer to keep these asserts, as they could check the inconsistent
> uses of disassemble_info between gdb and opcodes.  There is another
> assert for target armv5te, PR 21818, but the assert exposes an improper
> or unexpected usage of disassemble_info.mach in opcodes/arm-dis.c.  I am
> fixing it.

 How does the retaining of these assertions interact with `set
architecture' and `set endian' then?

  Maciej

Re: [PATCH] GDB/opcodes: Remove arch/mach/endian disassembler assertions

[Maciej W. Rozycki]

On Thu, 3 Aug 2017, Maciej W. Rozycki wrote:

> > I prefer to keep these asserts, as they could check the inconsistent
> > uses of disassemble_info between gdb and opcodes.  There is another
> > assert for target armv5te, PR 21818, but the assert exposes an improper
> > or unexpected usage of disassemble_info.mach in opcodes/arm-dis.c.  I am
> > fixing it.
> 
>  How does the retaining of these assertions interact with `set
> architecture' and `set endian' then?

 E.g.:

$ x86_64-linux-gnu-gdb
GNU gdb (GDB) 8.0.50.20170729-git
Copyright (C) 2017 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later 
<http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-pc-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
<http://www.gnu.org/software/gdb/documentation/>.
For help, type "help".
Type "apropos word" to search for commands related to "word".
(gdb) file main86
Reading symbols from main86...(no debugging symbols found)...done.
(gdb) show architecture
The target architecture is set automatically (currently i386:x86-64)
(gdb) disassemble main
Dump of assembler code for function main:
   0x0000000000400450 <+0>:	xor    %eax,%eax
   0x0000000000400452 <+2>:	retq
End of assembler dump.
(gdb) set architecture i386
The target architecture is assumed to be i386
(gdb) disassemble main
Dump of assembler code for function main:
   0x00400450 <+0>:
.../gdb/arch-utils.c:979: internal-error: int default_print_insn(bfd_vma, disassemble_info*): Assertion `info->mach == bfd_get_mach (exec_bfd)' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) n

This is a bug, please report it.  For instructions, see:
<http://www.gnu.org/software/gdb/bugs/>.

.../gdb/arch-utils.c:979: internal-error: int default_print_insn(bfd_vma, disassemble_info*): Assertion `info->mach == bfd_get_mach (exec_bfd)' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Create a core file of GDB? (y or n) n
Command aborted.
(gdb)

A change applied to the MIPS backend obviously won't do anything here.

  Maciej

Re: [PATCH] GDB/opcodes: Remove arch/mach/endian disassembler assertions

[Yao Qi]

"Maciej W. Rozycki" <macro@imgtec.com> writes:

> (gdb) set architecture i386
> The target architecture is assumed to be i386
> (gdb) disassemble main
> Dump of assembler code for function main:
>    0x00400450 <+0>:
> .../gdb/arch-utils.c:979: internal-error: int default_print_insn(bfd_vma, disassemble_info*): Assertion `info->mach == bfd_get_mach (exec_bfd)' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n) n

I didn't notice this example in your email.  The gdb patch is good to me.

-- 
Yao (齐尧)

Re: [PATCH] GDB/opcodes: Remove arch/mach/endian disassembler assertions

[Alan Modra]

On Tue, Aug 01, 2017 at 05:39:33PM +0100, Maciej W. Rozycki wrote:
> 	gdb/
> 	* arch-utils.c (default_print_insn): Remove arch/mach/endian
> 	assertions.
> 
> 	opcodes/
> 	* disassemble.c (disassembler): Remove arch/mach/endian 
> 	assertions.

This is OK, obvious really.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] GDB/opcodes: Remove arch/mach/endian disassembler assertions

[Maciej W. Rozycki]

On Mon, 7 Aug 2017, Alan Modra wrote:

> > 	gdb/
> > 	* arch-utils.c (default_print_insn): Remove arch/mach/endian
> > 	assertions.
> > 
> > 	opcodes/
> > 	* disassemble.c (disassembler): Remove arch/mach/endian 
> > 	assertions.
> 
> This is OK, obvious really.

 Applied now, thanks for your review.

  Maciej

Re: [PATCH] GDB/opcodes: Remove arch/mach/endian disassembler assertions

[H.J. Lu]

On Mon, Aug 7, 2017 at 7:56 AM, Maciej W. Rozycki <macro@imgtec.com> wrote:
> On Mon, 7 Aug 2017, Alan Modra wrote:
>
>> >     gdb/
>> >     * arch-utils.c (default_print_insn): Remove arch/mach/endian
>> >     assertions.
>> >
>> >     opcodes/
>> >     * disassemble.c (disassembler): Remove arch/mach/endian
>> >     assertions.
>>
>> This is OK, obvious really.
>
>  Applied now, thanks for your review.
>
>   Maciej

I am checking in:

diff --git a/opcodes/disassemble.c b/opcodes/disassemble.c
index 0058354265..11206c67a5 100644
--- a/opcodes/disassemble.c
+++ b/opcodes/disassemble.c
@@ -110,7 +110,9 @@
 #endif

 disassembler_ftype
-disassembler (enum bfd_architecture a, bfd_boolean big, unsigned long mach,
+disassembler (enum bfd_architecture a,
+        bfd_boolean big ATTRIBUTE_UNUSED,
+        unsigned long mach ATTRIBUTE_UNUSED,
         bfd *abfd ATTRIBUTE_UNUSED)
 {
   disassembler_ftype disassemble;

to fix

/export/gnu/import/git/sources/binutils-gdb/opcodes/disassemble.c: In
function ‘disassembler’:
/export/gnu/import/git/sources/binutils-gdb/opcodes/disassemble.c:113:52:
error: unused parameter ‘big’ [-Werror=unused-parameter]
 disassembler (enum bfd_architecture a, bfd_boolean big, unsigned long mach,
                                                    ^~~
/export/gnu/import/git/sources/binutils-gdb/opcodes/disassemble.c:113:71:
error: unused parameter ‘mach’ [-Werror=unused-parameter]
 disassembler (enum bfd_architecture a, bfd_boolean big, unsigned long mach,
                                                                       ^~~~
cc1: all warnings being treated as errors
make[6]: *** [Makefile:1313: disassemble.lo] Error 1

on x86.

-- 
H.J.

Re: [PATCH] GDB/opcodes: Remove arch/mach/endian disassembler assertions

[Maciej W. Rozycki]

On Mon, 7 Aug 2017, H.J. Lu wrote:

> I am checking in:
> 
> diff --git a/opcodes/disassemble.c b/opcodes/disassemble.c
> index 0058354265..11206c67a5 100644
> --- a/opcodes/disassemble.c
> +++ b/opcodes/disassemble.c
> @@ -110,7 +110,9 @@
>  #endif
> 
>  disassembler_ftype
> -disassembler (enum bfd_architecture a, bfd_boolean big, unsigned long mach,
> +disassembler (enum bfd_architecture a,
> +        bfd_boolean big ATTRIBUTE_UNUSED,
> +        unsigned long mach ATTRIBUTE_UNUSED,
>          bfd *abfd ATTRIBUTE_UNUSED)
>  {
>    disassembler_ftype disassemble;
> 
> to fix
> 
> /export/gnu/import/git/sources/binutils-gdb/opcodes/disassemble.c: In
> function ‘disassembler’:
> /export/gnu/import/git/sources/binutils-gdb/opcodes/disassemble.c:113:52:
> error: unused parameter ‘big’ [-Werror=unused-parameter]
>  disassembler (enum bfd_architecture a, bfd_boolean big, unsigned long mach,
>                                                     ^~~
> /export/gnu/import/git/sources/binutils-gdb/opcodes/disassemble.c:113:71:
> error: unused parameter ‘mach’ [-Werror=unused-parameter]
>  disassembler (enum bfd_architecture a, bfd_boolean big, unsigned long mach,
>                                                                        ^~~~
> cc1: all warnings being treated as errors
> make[6]: *** [Makefile:1313: disassemble.lo] Error 1
> 
> on x86.

 Thanks, and sorry for the breakage.  I only verified it with 
`--enable-targets=all', but should have run full binutils testing across 
my usual targets.

  Maciej