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

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Paul Carroll-3
(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.


8363.log (440 bytes) Download Attachment
8363_upstream.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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
Reply | Threaded
Open this post in threaded view
|

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

Paul Carroll-3
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.


Reply | Threaded
Open this post in threaded view
|

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
Reply | Threaded
Open this post in threaded view
|

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

Paul Carroll-3
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<[hidden email]>

        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;
-
 
  /* 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,

Reply | Threaded
Open this post in threaded view
|

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

Paul Brook
> 2011-04-07  Paul Carroll<[hidden email]>
>
> 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
Reply | Threaded
Open this post in threaded view
|

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

Alan Modra-3
On Fri, Apr 08, 2011 at 12:45:26PM +0100, Paul Brook wrote:
> > 2011-04-07  Paul Carroll<[hidden email]>
> > 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
Reply | Threaded
Open this post in threaded view
|

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

Paul Carroll-3
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<[hidden email]>
>>> 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.
Reply | Threaded
Open this post in threaded view
|

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  <[hidden email]>

        * 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"
Reply | Threaded
Open this post in threaded view
|

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

Paul Carroll-3
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 <[hidden email]>
>
>     * 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...