[PATCH] ARM: objdump produces incorrect disassembly on multiple inputs

[PATCH] ARM: objdump produces incorrect disassembly on multiple inputs

[Paul Carroll]

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

(I don't have write permission into the Binutils CVS, so someone else 
will be merging the final patch.)

The 'objdump' utility will not disassembly instructions correctly when 
it processes multiple ARM input files.
That is, if you compare the disassembly of each file individually 
against the output produced by processing all of the files at once, the 
output is different.
This is due to the ARM disassembler using a couple of global symbols to 
track symbol entries in a given input file.
The problem is that these global symbols are initialized once, but never 
reset back to their starting values when a new input file begins 
processing.  As a result, 'objdump's view of the symbols for the new 
input file is messed up.

This patch adds a call to initialize those 2 values at the start of 
processing each ARM input file.  This fix is ARM-specific.

The added test case is the stripped-down assembly output for a C file.  
It is necessary to have ARM instructions in the test case, since this is 
for testing the ARM disassembler.

Out of the 67 disassemblers in the binutils/opcodes directory, it is 
possible that there are issues with global symbols in the CRIS, 
Microblaze, S390, and tic4x (TI TMS320C[34]X) files.  I won't say there 
is a problem, but there could be one.


[-- Attachment #2: 8363.log --]
[-- Type: text/plain, Size: 427 bytes --]

2011-03-08  Paul Carroll <pcarroll@codesourcery.com>

	opcodes/
	* disassemble.c (disassembler): Added ARM initialize call
	* arm-dis.c (initialize_arm_disassemble): init global vars

	include/
	* dis-asm.h: Added extern for new arm disassembler initialization

	binutils/testsuite/
	* binutils-all/arm/simple.s: Demo issue with objdump with 
	multiple input files
	* binutils-all/arm/objdump.exp: added new ARM test case code

[-- Attachment #3: 8363_upstream.patch --]
[-- Type: text/plain, Size: 4211 bytes --]

Index: src/binutils/testsuite/binutils-all/arm/objdump.exp
===================================================================
RCS file: /cvs/src/src/binutils/testsuite/binutils-all/arm/objdump.exp,v
retrieving revision 1.6
diff -u -p -r1.6 objdump.exp
--- src/binutils/testsuite/binutils-all/arm/objdump.exp	2 Sep 2009 07:22:33 -0000	1.6
+++ src/binutils/testsuite/binutils-all/arm/objdump.exp	8 Mar 2011 22:10:24 -0000
@@ -61,3 +61,29 @@ if [regexp $want $got] then {
 } else {
     fail "thumb2-cond test2"
 }
+
+###########################
+# Set up the test of multiple disassemblies
+###########################
+
+if {![binutils_assemble $srcdir/$subdir/simple.s tmpdir/simple.o]} then {
+    return
+}
+
+if [is_remote host] {
+    set objfile [remote_download host tmpdir/simple.o]
+} else {
+    set objfile tmpdir/simple.o
+}
+
+# Make sure multiple disassemblies come out the same
+
+set got [binutils_run $OBJDUMP "-dr $objfile $objfile"]
+
+set want "$objfile:\[ \]*file format.*$objfile:\[ \]*file format.*push.*add.*sub.*str.*add.*pop"
+
+if [regexp $want $got] then {
+    pass "multiple input files"
+} else {
+    fail "multiple input files"
+}
Index: src/binutils/testsuite/binutils-all/arm/simple.s
===================================================================
RCS file: src/binutils/testsuite/binutils-all/arm/simple.s
diff -N src/binutils/testsuite/binutils-all/arm/simple.s
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ src/binutils/testsuite/binutils-all/arm/simple.s	8 Mar 2011 22:10:24 -0000
@@ -0,0 +1,35 @@
+	.cpu arm7tdmi-s
+	.fpu softvfp
+	.file	"y.c"
+	.bss
+	.align	2
+l:
+	.space	4
+	.text
+	.align	2
+	.global	f1
+	.type	f1, %function
+f1:
+	str	fp, [sp, #-4]!
+	add	fp, sp, #0
+	sub	sp, sp, #12
+	str	r0, [fp, #-8]
+	add	sp, fp, #0
+	ldmfd	sp!, {fp}
+	bx	lr
+	.align	2
+	.word	l
+	.size	f1, .-f1
+	.align	2
+	.global	main
+	.type	main, %function
+main:
+	stmfd	sp!, {fp, lr}
+	add	fp, sp, #4
+	bx	lr
+	.align	2
+	.word	1717986919
+	.word	-1840700269
+	.word	l
+	.size	main, .-main
+	.ident	"GCC: (Sourcery G++ 2011.03-999999 - Preview) 4.5.1"
Index: src/include/dis-asm.h
===================================================================
RCS file: /cvs/src/src/include/dis-asm.h,v
retrieving revision 1.80
diff -u -p -r1.80 dis-asm.h
--- src/include/dis-asm.h	29 Jun 2010 04:17:27 -0000	1.80
+++ src/include/dis-asm.h	8 Mar 2011 22:10:24 -0000
@@ -310,6 +310,7 @@ extern int  get_arm_regname_num_options 
 extern int  set_arm_regname_option (int);
 extern int  get_arm_regnames (int, const char **, const char **, const char *const **);
 extern bfd_boolean arm_symbol_is_valid (asymbol *, struct disassemble_info *);
+extern void initialize_arm_disassemble (void);
 
 /* Fetch the disassembler for a given BFD, if that support is available.  */
 extern disassembler_ftype disassembler (bfd *);
Index: src/opcodes/arm-dis.c
===================================================================
RCS file: /cvs/src/src/opcodes/arm-dis.c,v
retrieving revision 1.136
diff -u -p -r1.136 arm-dis.c
--- src/opcodes/arm-dis.c	6 Jan 2011 14:30:43 -0000	1.136
+++ src/opcodes/arm-dis.c	8 Mar 2011 22:10:25 -0000
@@ -1651,8 +1651,8 @@ enum map_type
 };
 
 enum map_type last_type;
-int last_mapping_sym = -1;
-bfd_vma last_mapping_addr = 0;
+int last_mapping_sym;
+bfd_vma last_mapping_addr;
 
 \f
 /* Functions.  */
@@ -4960,3 +4960,10 @@ the -M switch:\n"));
   fprintf (stream, "  force-thumb              Assume all insns are Thumb insns\n");
   fprintf (stream, "  no-force-thumb           Examine preceeding label to determine an insn's type\n\n");
 }
+
+void
+initialize_arm_disassemble()
+{
+  last_mapping_sym = -1;
+  last_mapping_addr = 0;
+}
Index: src/opcodes/disassemble.c
===================================================================
RCS file: /cvs/src/src/opcodes/disassemble.c,v
retrieving revision 1.78
diff -u -p -r1.78 disassemble.c
--- src/opcodes/disassemble.c	6 Aug 2010 03:59:49 -0000	1.78
+++ src/opcodes/disassemble.c	8 Mar 2011 22:10:25 -0000
@@ -119,6 +119,7 @@ disassembler (abfd)
 #endif
 #ifdef ARCH_arm
     case bfd_arch_arm:
+      initialize_arm_disassemble();
       if (bfd_big_endian (abfd))
 	disassemble = print_insn_big_arm;
       else

Re: [PATCH] ARM: objdump produces incorrect disassembly on multiple inputs

[Paul Brook]

> The 'objdump' utility will not disassembly instructions correctly when
> it processes multiple ARM input files.
> That is, if you compare the disassembly of each file individually
> against the output produced by processing all of the files at once, the
> output is different.
> This is due to the ARM disassembler using a couple of global symbols to
> track symbol entries in a given input file.
> The problem is that these global symbols are initialized once, but never
> reset back to their starting values when a new input file begins
> processing.  As a result, 'objdump's view of the symbols for the new
> input file is messed up.
> 
> This patch adds a call to initialize those 2 values at the start of
> processing each ARM input file.  This fix is ARM-specific.

Wouldn't it be better to use info->private_data ?

Paul

Re: [PATCH] ARM: objdump produces incorrect disassembly on multiple inputs

[Paul Carroll]

On 4/7/2011 7:27 AM, Paul Brook wrote:
>> The 'objdump' utility will not disassembly instructions correctly when
>> it processes multiple ARM input files.
>>
>> This is due to the ARM disassembler using a couple of global symbols to
>> track symbol entries in a given input file.
>>
>>
>> This patch adds a call to initialize those 2 values at the start of
>> processing each ARM input file.  This fix is ARM-specific.
> Wouldn't it be better to use info->private_data ?
Possibly.
As a test, I implemented the fix using 'info->private_data'.
That moves the 3 global variables, used by the ARM disassembler, into 
this private structure.
But, the initialization call before starting disassembly is still 
necessary since those variables need to be reset back to their initial 
values.  Otherwise, this issue isn't solved.
And, since the 'info' data structure is not presently available to the 
common 'disassembler()' function where I added the initialization call, 
that would need to have this additional 'info' argument passed into it, 
so it could pass that on to the new ARM initialization call.
There is also a little bit of rearranging of the 'info->private_data' 
initialization in the ARM disassembler so it now occurs as part of the 
initialization of the variables that are at fault.

Perhaps an alternative would be to use the fix I proposed and make those 
3 variables 'static'.  I don't think there is any reason any code 
outside of the ARM disassembler would want to access those variables.

Re: [PATCH] ARM: objdump produces incorrect disassembly on multiple inputs

[Paul Brook]

> On 4/7/2011 7:27 AM, Paul Brook wrote:
> >> The 'objdump' utility will not disassembly instructions correctly when
> >> it processes multiple ARM input files.
> >> 
> >> This is due to the ARM disassembler using a couple of global symbols to
> >> track symbol entries in a given input file.
> >> 
> >> 
> >> This patch adds a call to initialize those 2 values at the start of
> >> processing each ARM input file.  This fix is ARM-specific.
> > 
> > Wouldn't it be better to use info->private_data ?
> 
> Possibly.
> As a test, I implemented the fix using 'info->private_data'.
> That moves the 3 global variables, used by the ARM disassembler, into
> this private structure.
> But, the initialization call before starting disassembly is still
> necessary since those variables need to be reset back to their initial
> values.

Rubbish.

The disassemble() function is documented to return an appropriate disassembly 
callback.  Having it also reset unspecified state is at somewhat surprising.

Putting the state in private_data will do exactly what you want.  If fact it's 
more reliable as it's linked to the actual disassembler state (of which there 
may be many), rather than when the user happens to request the callback 
function.  Note how a new instance of struct disassemble_info is created for 
each object.

Paul

Re: [PATCH] ARM: objdump produces incorrect disassembly on multiple inputs

[Paul Carroll]

On 4/7/2011 3:19 PM, Paul Brook wrote:
> Rubbish.
>
> The disassemble() function is documented to return an appropriate disassembly
> callback.  Having it also reset unspecified state is at somewhat surprising.
>
> Putting the state in private_data will do exactly what you want.  If fact it's
> more reliable as it's linked to the actual disassembler state (of which there
> may be many), rather than when the user happens to request the callback
> function.  Note how a new instance of struct disassemble_info is created for
> each object.

Looks like I either didn't dig deep enough or I got confused with the 
BFD 'info' structure.
In any event, yes, moving the global variables into the private data 
structure and initializing them there results in the exact same behavior 
as my earlier fix, with less impact on other files.
Here would be the alternative patch, with the test case:

2011-04-07  Paul Carroll<pcarroll@codesourcery.com>

	opcodes/
	* arm-dis.c (print_insn): init vars moved into private_data structure

	binutils/testsuite/
	* binutils-all/arm/simple.s: Demo issue with objdump with
	multiple input files
	* binutils-all/arm/objdump.exp: added new ARM test case code



Index: src/binutils/testsuite/binutils-all/arm/objdump.exp
===================================================================
RCS file: /cvs/src/src/binutils/testsuite/binutils-all/arm/objdump.exp,v
retrieving revision 1.6
diff -u -p -r1.6 objdump.exp
--- src/binutils/testsuite/binutils-all/arm/objdump.exp    2 Sep 2009 
07:22:33 -0000    1.6
+++ src/binutils/testsuite/binutils-all/arm/objdump.exp    7 Apr 2011 
23:33:17 -0000
@@ -61,3 +61,29 @@ if [regexp $want $got] then {
  } else {
      fail "thumb2-cond test2"
  }
+
+###########################
+# Set up the test of multiple disassemblies
+###########################
+
+if {![binutils_assemble $srcdir/$subdir/simple.s tmpdir/simple.o]} then {
+    return
+}
+
+if [is_remote host] {
+    set objfile [remote_download host tmpdir/simple.o]
+} else {
+    set objfile tmpdir/simple.o
+}
+
+# Make sure multiple disassemblies come out the same
+
+set got [binutils_run $OBJDUMP "-dr $objfile $objfile"]
+
+set want "$objfile:\[ \]*file format.*$objfile:\[ \]*file 
format.*push.*add.*sub.*str.*add.*pop"
+
+if [regexp $want $got] then {
+    pass "multiple input files"
+} else {
+    fail "multiple input files"
+}
Index: src/binutils/testsuite/binutils-all/arm/simple.s
===================================================================
RCS file: src/binutils/testsuite/binutils-all/arm/simple.s
diff -N src/binutils/testsuite/binutils-all/arm/simple.s
--- /dev/null    1 Jan 1970 00:00:00 -0000
+++ src/binutils/testsuite/binutils-all/arm/simple.s    7 Apr 2011 
23:33:17 -0000
@@ -0,0 +1,35 @@
+    .cpu arm7tdmi-s
+    .fpu softvfp
+    .file    "y.c"
+    .bss
+    .align    2
+l:
+    .space    4
+    .text
+    .align    2
+    .global    f1
+    .type    f1, %function
+f1:
+    str    fp, [sp, #-4]!
+    add    fp, sp, #0
+    sub    sp, sp, #12
+    str    r0, [fp, #-8]
+    add    sp, fp, #0
+    ldmfd    sp!, {fp}
+    bx    lr
+    .align    2
+    .word    l
+    .size    f1, .-f1
+    .align    2
+    .global    main
+    .type    main, %function
+main:
+    stmfd    sp!, {fp, lr}
+    add    fp, sp, #4
+    bx    lr
+    .align    2
+    .word    1717986919
+    .word    -1840700269
+    .word    l
+    .size    main, .-main
+    .ident    "GCC: (Sourcery G++ 2011.03) 4.5.1"
Index: src/opcodes/arm-dis.c
===================================================================
RCS file: /cvs/src/src/opcodes/arm-dis.c,v
retrieving revision 1.138
diff -u -p -r1.138 arm-dis.c
--- src/opcodes/arm-dis.c    14 Mar 2011 16:04:08 -0000    1.138
+++ src/opcodes/arm-dis.c    7 Apr 2011 23:33:18 -0000
@@ -45,6 +45,14 @@
  #define NUM_ELEM(a)     (sizeof (a) / sizeof (a)[0])
  #endif

+/* Cached mapping symbol state.  */
+enum map_type
+{
+  MAP_ARM,
+  MAP_THUMB,
+  MAP_DATA
+};
+
  struct arm_private_data
  {
    /* The features to use when disassembling optional instructions.  */
@@ -53,6 +61,13 @@ struct arm_private_data
    /* Whether any mapping symbols are present in the provided symbol
       table.  -1 if we do not know yet, otherwise 0 or 1.  */
    int has_mapping_symbols;
+
+  /* Track the last type (although this doesn't seem to be useful) */
+  enum map_type last_type;
+
+  /* Tracking symbol table information */
+  int last_mapping_sym;
+  bfd_vma last_mapping_addr;
  };

  struct opcode32
@@ -1642,18 +1657,6 @@ static unsigned int ifthen_next_state;
  static bfd_vma ifthen_address;
  #define IFTHEN_COND ((ifthen_state >> 4) & 0xf)

-/* Cached mapping symbol state.  */
-enum map_type
-{
-  MAP_ARM,
-  MAP_THUMB,
-  MAP_DATA
-};
-
-enum map_type last_type;
-int last_mapping_sym = -1;
-bfd_vma last_mapping_addr = 0;
-
  \f
  /* Functions.  */
  int
@@ -4635,6 +4638,8 @@ print_insn (bfd_vma pc, struct disassemb
        select_arm_features (info->mach, & private.features);

        private.has_mapping_symbols = -1;
+      private.last_mapping_sym = -1;
+      private.last_mapping_addr = 0;

        info->private_data = & private;
      }
@@ -4658,8 +4663,8 @@ print_insn (bfd_vma pc, struct disassemb
        /* Start scanning at the start of the function, or wherever
       we finished last time.  */
        start = info->symtab_pos + 1;
-      if (start < last_mapping_sym)
-    start = last_mapping_sym;
+      if (start < private_data->last_mapping_sym)
+    start = private_data->last_mapping_sym;
        found = FALSE;

        /* First, look for mapping symbols.  */
@@ -4754,10 +4759,10 @@ print_insn (bfd_vma pc, struct disassemb
          }
      }

-      last_mapping_sym = last_sym;
-      last_type = type;
-      is_thumb = (last_type == MAP_THUMB);
-      is_data = (last_type == MAP_DATA);
+      private_data->last_mapping_sym = last_sym;
+      private_data->last_type = type;
+      is_thumb = (private_data->last_type == MAP_THUMB);
+      is_data = (private_data->last_type == MAP_DATA);

        /* Look a little bit ahead to see if we should print out
       two or four bytes of data.  If there's a symbol,

Re: [PATCH] ARM: objdump produces incorrect disassembly on multiple inputs

[Paul Brook]

> 2011-04-07  Paul Carroll<pcarroll@codesourcery.com>
> 
> 	opcodes/
> 	* arm-dis.c (print_insn): init vars moved into private_data structure
> 
> 	binutils/testsuite/
> 	* binutils-all/arm/simple.s: Demo issue with objdump with
> 	multiple input files
> 	* binutils-all/arm/objdump.exp: added new ARM test case code


Applied, thanks.

The patch you provided was a bit mangled, and I had to manually fixup most of 
the whitespace.  You need to have words with your email client about how to 
attach patches without messing with the formatting.

Paul

Re: [PATCH] ARM: objdump produces incorrect disassembly on multiple inputs

[Alan Modra]

On Fri, Apr 08, 2011 at 12:45:26PM +0100, Paul Brook wrote:
> > 2011-04-07  Paul Carroll<pcarroll@codesourcery.com>
> > 	binutils/testsuite/
> > 	* binutils-all/arm/simple.s: Demo issue with objdump with
> > 	multiple input files
> > 	* binutils-all/arm/objdump.exp: added new ARM test case code

arm-coff  +ERROR: /src/binutils-current/binutils/testsuite/binutils-all/arm/simple.s: assembly failed
arm-epoc-pe  +ERROR: /src/binutils-current/binutils/testsuite/binutils-all/arm/simple.s: assembly failed
arm-pe  +ERROR: /src/binutils-current/binutils/testsuite/binutils-all/arm/simple.s: assembly failed
arm-wince-pe  +ERROR: /src/binutils-current/binutils/testsuite/binutils-all/arm/simple.s: assembly failed

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] ARM: objdump produces incorrect disassembly on multiple inputs

[Paul Carroll]

On 4/10/2011 6:15 PM, Alan Modra wrote:
> On Fri, Apr 08, 2011 at 12:45:26PM +0100, Paul Brook wrote:
>>> 2011-04-07  Paul Carroll<pcarroll@codesourcery.com>
>>> 	binutils/testsuite/
>>> 	* binutils-all/arm/simple.s: Demo issue with objdump with
>>> 	multiple input files
>>> 	* binutils-all/arm/objdump.exp: added new ARM test case code
> arm-coff  +ERROR: /src/binutils-current/binutils/testsuite/binutils-all/arm/simple.s: assembly failed
> arm-epoc-pe  +ERROR: /src/binutils-current/binutils/testsuite/binutils-all/arm/simple.s: assembly failed
> arm-pe  +ERROR: /src/binutils-current/binutils/testsuite/binutils-all/arm/simple.s: assembly failed
> arm-wince-pe  +ERROR: /src/binutils-current/binutils/testsuite/binutils-all/arm/simple.s: assembly failed
Sounds like that patch needs to be backed out until I either make the 
test case's assembly code more generic or, more likely, modify the 
driving script so it is only invoked for certain toolkits.
My apologies.

Re: [PATCH] ARM: objdump produces incorrect disassembly on multiple inputs

[Nick Clifton]

Hi Paul,

>> /src/binutils-current/binutils/testsuite/binutils-all/arm/simple.s:
>> assembly failed

> Sounds like that patch needs to be backed out until I either make the
> test case's assembly code more generic or, more likely, modify the
> driving script so it is only invoked for certain toolkits.

Or do what I have done and remove the .type and .size directives from 
simple.s.  They are not needed for the test and they have different 
semantics on COFF based targets, so removing them is the easy solution.

Cheers
   Nick

binutils/testsuite/ChangeLog
2011-04-11  Nick Clifton  <nickc@redhat.com>

	* binutils-all/arm/simple.s: Fix assembly problems for COFF based
	ARM toolchaisn by removing .type and .size directives.

--- binutils-all/arm/simple.s   8 Apr 2011 11:42:18 -0000       1.1
+++ binutils-all/arm/simple.s   11 Apr 2011 15:12:29 -0000      1.2
@@ -8,7 +8,7 @@ l:
      .text
      .align    2
      .global    f1
-    .type    f1, %function
+
  f1:
      str    fp, [sp, #-4]!
      add    fp, sp, #0
@@ -19,10 +19,10 @@ f1:
      bx    lr
      .align    2
      .word    l
-    .size    f1, .-f1
+
      .align    2
      .global    main
-    .type    main, %function
+
  main:
      stmfd    sp!, {fp, lr}
      add    fp, sp, #4
@@ -31,5 +31,5 @@ main:
      .word    1717986919
      .word    -1840700269
      .word    l
-    .size    main, .-main
+
      .ident    "GCC: (Sourcery G++ 2011.03) 4.5.1"

Re: [PATCH] ARM: objdump produces incorrect disassembly on multiple inputs

[Paul Carroll]

On 4/11/2011 9:16 AM, Nick Clifton wrote:
> Or do what I have done and remove the .type and .size directives from 
> simple.s.  They are not needed for the test and they have different 
> semantics on COFF based targets, so removing them is the easy solution.
>
> binutils/testsuite/ChangeLog
> 2011-04-11  Nick Clifton <nickc@redhat.com>
>
>     * binutils-all/arm/simple.s: Fix assembly problems for COFF based
>     ARM toolchaisn by removing .type and .size directives.
If that is all that is needed to satisfy Alan and his builds, then I'm 
satisfied.
Definitely something to watch out for in the future.
Thanks, Nick...