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

[Christophe Lyon <christophe dot lyon at linaro dot org>]

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.
>>>
>