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

[Richard Earnshaw <Richard dot Earnshaw at foss dot arm dot com>]

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