This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [OPCODE][AARCH64]Check mapping symbol while backward searching for IT block


Hi Christophe,

Thank you for reporting this!

The related patch searches for proper mapping symbols. They are defined in arm elf document.
So the test case should be limited on elf target only. I will post a fix.

Regards,
Renlin

On 25/01/16 18:07, Christophe Lyon wrote:
On 22 January 2016 at 14:34, Renlin Li <renlin.li@foss.arm.com> wrote:
Hi,

Sorry for the late reply.

After discussion with Richard, we agreed that, this patch is fixing a BUG
which gives undesired behavior provided a valid input.
The patch is Okay as it is. I will commit it then.

For an invalid input which leads to unexpected result, it's another issue. I
will try to sum up and create a public ticket .

Hi,
I've noticed that this patch makes:
./gas/testsuite/gas.sum:FAIL: 32-bit Thumb conditional instructions
backward search
on arm-wince-pe.

Can you have a look?


Regards,
Renlin Li




On 15/09/15 11:16, Richard Earnshaw wrote:
On 10/09/15 18:19, Renlin Li wrote:
Hi all,

For the following simple assembly code.

          .text
          .thumb
          .syntax unified
          .thumb_func
f:
          nop.w
          .long 0xbf080000
          nop.w

When objdumped, the following code assembly will be generated.

     0:   f3af 8000       nop.w
     4:   bf080000        .word   0xbf080000
     8:   f3af 8000       nopeq.w

The instruction at pc=0x8 is treated as conditional executed instruction
within an IT block. This is because objdump misinterprets data (at
pc=0x4) as an IT instruction.
During the backward search for a proper IT instruction, mapping state
for the current instruction is not checked.


In this patch, I create a new function mapping_symbol_for_insn to search
the mapping state given an addr.
It's used in find_ifthen_state to further guard the check for an IT
instruction.

A new testcase is also added. Binutils checked without any new issues.
Is Okay to commit on trunk?

Hmm, I'm not entirely sure about this.  Use of .word in this sort of
case is most likely a mistake by the programmer rather than deliberate.
   Furthermore, if you're going to change this case, shouldn't you also
consider:

         itttt eq
         .word   xxxxxxxxx  // 32-bit opcode
         nop

?

R.

Kind regards,
Renlin Li




opcodes/ChangeLog:

2015-09-10  Renlin Li  <renlin.li@arm.com>

      * arm-dis.c (mapping_symbol_for_insn): New function.
      (find_ifthen_state): Call mapping_symbol_for_insn().



gas/testsuite/ChangeLog:

2015-09-10  Renlin Li  <renlin.li@arm.com>

      * gas/arm/thumb2_it_search.d: New.
      * gas/arm/thumb2_it_search.s: New.



new-new.diff


commit 347a76f708bfb31b38ce787bd444005412655ae0
Author: Renlin Li <renlin.li@arm.com>
Date:   Thu Sep 10 16:13:22 2015 +0100

      tmp
           Change-Id: I74905754d21022ba9513f12649e80cd65d4425a9

diff --git a/gas/testsuite/gas/arm/thumb2_it_search.d
b/gas/testsuite/gas/arm/thumb2_it_search.d
new file mode 100644
index 0000000..6758ef8
--- /dev/null
+++ b/gas/testsuite/gas/arm/thumb2_it_search.d
@@ -0,0 +1,12 @@
+#name: 32-bit Thumb conditional instructions backward search
+#as: -march=armv6kt2
+#skip: *-*-*aout*
+#source: thumb2_it_search.s
+#objdump: -dr --prefix-addresses --show-raw-insn
+
+.*: +file format .*arm.*
+
+Disassembly of section .text:
+0+0 <[^>]+> f3af 8000  nop.w
+0+4 <[^>]+> bf080000   .word   0xbf080000
+0+8 <[^>]+> f3af 8000  nop.w
diff --git a/gas/testsuite/gas/arm/thumb2_it_search.s
b/gas/testsuite/gas/arm/thumb2_it_search.s
new file mode 100644
index 0000000..a29cb51
--- /dev/null
+++ b/gas/testsuite/gas/arm/thumb2_it_search.s
@@ -0,0 +1,8 @@
+       .text
+       .thumb
+       .syntax unified
+       .thumb_func
+f:
+       nop.w
+       .long 0xbf080000
+       nop.w
diff --git a/opcodes/arm-dis.c b/opcodes/arm-dis.c
index 430da08..6a097f5 100644
--- a/opcodes/arm-dis.c
+++ b/opcodes/arm-dis.c
@@ -5930,6 +5930,10 @@ parse_disassembler_options (char *options)
       }
   }
   +static bfd_boolean
+mapping_symbol_for_insn (bfd_vma pc, struct disassemble_info *info,
+                        enum map_type *map_symbol);
+
   /* Search back through the insn stream to determine if this instruction
is
      conditionally executed.  */
   @@ -5992,9 +5996,15 @@ find_ifthen_state (bfd_vma pc,
         }
         if ((insn & 0xff00) == 0xbf00 && (insn & 0xf) != 0)
         {
-         /* This could be an IT instruction.  */
-         seen_it = insn;
-         it_count = count >> 1;
+         enum map_type type = MAP_ARM;
+         bfd_boolean found = mapping_symbol_for_insn (addr, info,
&type);
+
+         if (!found || (found && type == MAP_THUMB))
+           {
+             /* This could be an IT instruction.  */
+             seen_it = insn;
+             it_count = count >> 1;
+           }
         }
         if ((insn & 0xf800) >= 0xe800)
         count++;
@@ -6078,6 +6088,71 @@ get_sym_code_type (struct disassemble_info *info,
     return FALSE;
   }
   +/* Search the mapping symbol state for instruction at pc.  This is
only
+   applicable for elf target.
+
+   There is an assumption Here, info->private_data contains the correct
AND
+   up-to-date information about current scan process.  The information
will be
+   used to speed this search process.
+
+   Return TRUE if the mapping state can be determined, and map_symbol
+   will be updated accordingly.  Otherwise, return FALSE.  */
+
+static bfd_boolean
+mapping_symbol_for_insn (bfd_vma pc, struct disassemble_info *info,
+                        enum map_type *map_symbol)
+{
+  bfd_vma addr;
+  int n, start = 0;
+  bfd_boolean found = FALSE;
+  enum map_type type = MAP_ARM;
+  struct arm_private_data *private_data;
+
+  if (info->private_data == NULL || info->symtab_size == 0
+      || bfd_asymbol_flavour (*info->symtab) != bfd_target_elf_flavour)
+    return FALSE;
+
+  private_data = info->private_data;
+  if (pc == 0)
+    start = 0;
+  else
+    start = private_data->last_mapping_sym;
+
+  start = (start == -1)? 0 : start;
+  addr = bfd_asymbol_value (info->symtab[start]);
+
+  if (pc >= addr)
+    {
+      if (get_map_sym_type (info, start, &type))
+      found = TRUE;
+    }
+  else
+    {
+      for (n = start - 1; n >= 0; n--)
+       {
+         if (get_map_sym_type (info, n, &type))
+           {
+             found = TRUE;
+             break;
+           }
+       }
+    }
+
+  /* No mapping symbols were found.  A leading $d may be
+     omitted for sections which start with data; but for
+     compatibility with legacy and stripped binaries, only
+     assume the leading $d if there is at least one mapping
+     symbol in the file.  */
+  if (!found && private_data->has_mapping_symbols == 1)
+    {
+      type = MAP_DATA;
+      found = TRUE;
+    }
+
+  *map_symbol = type;
+  return found;
+}
+
   /* Given a bfd_mach_arm_XXX value, this function fills in the fields
      of the supplied arm_feature_set structure with bitmasks indicating
      the support base architectures and coprocessor extensions.



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]