Re: [PATCH] [ARC] Fix disassembler -M option.

[Andrew Burgess <andrew dot burgess at embecosm dot com>]

* Claudiu Zissulescu <Claudiu.Zissulescu@synopsys.com> [2016-11-28 12:01:34 +0100]:

> In ARC backend, we can use -M<feature> option to control how an opcode is disassembled.
> This patch fixes a number of issues introduced by the original commit:
> - fpus and fpud are swaped.
> - quarkse_em doesn't include FPX extensions.
> - auto guessed opcode mechanism may ignore options passed via -M<feature>.
> 
> Ok to apply?
> Claudiu

Looks fine to me (I'm not a maintainer though) except for one minor
whitespace issue highlighted inline.

Thanks,
Andrew

> 
> opcodes/
> 2016-11-28  Claudiu Zissulescu  <claziss@synopsys.com>
> 
> 	* arc-dis.c (is_compatible_p): Remove function.
> 	(skip_this_opcode): Don't add any decoding class to decode list.
> 	Remove warning.
> 	(find_format_from_table): Go through all opcodes, and warn if we
> 	use a guessed mnemonic.
> 
> binutils/
> 2016-11-28  Claudiu Zissulescu  <claziss@synopsys.com>
> 
> 	* testsuite/binutils-all/arc/objdump.exp (Warning test): Update
> 	test.
> ---
>  binutils/testsuite/binutils-all/arc/objdump.exp |  2 +-
>  opcodes/arc-dis.c                               | 95 +++++++++++--------------
>  2 files changed, 41 insertions(+), 56 deletions(-)
> 
> diff --git a/binutils/testsuite/binutils-all/arc/objdump.exp b/binutils/testsuite/binutils-all/arc/objdump.exp
> index 58c4a05..a988823 100644
> --- a/binutils/testsuite/binutils-all/arc/objdump.exp
> +++ b/binutils/testsuite/binutils-all/arc/objdump.exp
> @@ -46,7 +46,7 @@ if [is_remote host] {
>  
>  set got [binutils_run $OBJDUMP "$OBJDUMPFLAGS --disassemble $objfile"]
>  
> -set want "Warning: disassembly.*dsubh12\[ \t\]*r0,r2,r4.*dmulh12.f\[ \t\]*r0,r2,r4.*dmulh11.f"
> +set want "Warning: disassembly.*vmac2hnfr\[ \t\]*r0,r2,r4.*dmulh12.f\[ \t\]*r0,r2,r4.*dmulh11.f"
>  
>  if [regexp $want $got] then {
>      pass "Warning test"
> diff --git a/opcodes/arc-dis.c b/opcodes/arc-dis.c
> index 31b5a91..beb12af 100644
> --- a/opcodes/arc-dis.c
> +++ b/opcodes/arc-dis.c
> @@ -108,21 +108,6 @@ static linkclass decodelist = NULL;
>  
>  /* Functions implementation.  */
>  
> -/* Return TRUE when two classes are not opcode conflicting.  */
> -
> -static bfd_boolean
> -is_compatible_p (insn_class_t     classA,
> -		 insn_subclass_t  sclassA,
> -		 insn_class_t     classB,
> -		 insn_subclass_t  sclassB)
> -{
> -  if (classA == DSP && sclassB == DPX)
> -    return FALSE;
> -  if (sclassA == DPX && classB == DSP)
> -    return FALSE;
> -  return TRUE;
> -}
> -
>  /* Add a new element to the decode list.  */
>  
>  static void
> @@ -141,54 +126,34 @@ add_to_decodelist (insn_class_t     insn_class,
>     disassembled.  */
>  
>  static bfd_boolean
> -skip_this_opcode (const struct arc_opcode *  opcode,
> -		  struct disassemble_info *  info)
> +skip_this_opcode (const struct arc_opcode *  opcode)

This should be 'const struct arc_opcode *opcode' I think.  We might as
well fix it while the line is being edited.

>  {
>    linkclass t = decodelist;
> -  bfd_boolean addme = TRUE;
>  
>    /* Check opcode for major 0x06, return if it is not in.  */
>    if (arc_opcode_len (opcode) == 4
>        && OPCODE_32BIT_INSN (opcode->opcode) != 0x06)
>      return FALSE;
>  
> -  while (t != NULL
> -	 && is_compatible_p (t->insn_class, t->subclass,
> -			     opcode->insn_class, opcode->subclass))
> +  /* or not a known truble class.  */
> +  switch (opcode->insn_class)
> +    {
> +    case FLOAT:
> +    case DSP:
> +      break;
> +    default:
> +      return FALSE;
> +    }
> +
> +  while (t != NULL)
>      {
>        if ((t->insn_class == opcode->insn_class)
>  	  && (t->subclass == opcode->subclass))
> -	addme = FALSE;
> +	return FALSE;
>        t = t->nxt;
>      }
>  
> -  /* If we found an incompatibility then we must skip.  */
> -  if (t != NULL)
> -    return TRUE;
> -
> -  /* Even if we do not precisely know the if the right mnemonics
> -     is correctly displayed, keep the disassmbled code class
> -     consistent.  */
> -  if (addme)
> -    {
> -      switch (opcode->insn_class)
> -	{
> -	case DSP:
> -	case FLOAT:
> -	  /* Add to the conflict list only the classes which
> -	     counts.  */
> -	  add_to_decodelist (opcode->insn_class, opcode->subclass);
> -	  /* Warn if we have to decode an opcode and no preferred
> -	     classes have been chosen.  */
> -	  info->fprintf_func (info->stream, _("\n\
> -Warning: disassembly may be wrong due to guessed opcode class choice.\n\
> - Use -M<class[,class]> to select the correct opcode class(es).\n\t\t\t\t"));
> -	  break;
> -	default:
> -	  break;
> -	}
> -    }
> -  return FALSE;
> +  return TRUE;
>  }
>  
>  static bfd_vma
> @@ -243,8 +208,10 @@ find_format_from_table (struct disassemble_info *info,
>  {
>    unsigned int i = 0;
>    const struct arc_opcode *opcode = NULL;
> +  const struct arc_opcode *t_op = NULL;
>    const unsigned char *opidx;
>    const unsigned char *flgidx;
> +  bfd_boolean warn_p = FALSE;
>  
>    do
>      {
> @@ -337,15 +304,29 @@ find_format_from_table (struct disassemble_info *info,
>  	continue;
>  
>        if (insn_len == 4
> -	  && overlaps
> -	  && skip_this_opcode (opcode, info))
> -	continue;
> +	  && overlaps)
> +	{
> +	  warn_p = TRUE;
> +	  t_op = opcode;
> +	  if (skip_this_opcode (opcode))
> +	    continue;
> +	}
>  
>        /* The instruction is valid.  */
>        return opcode;
>      }
>    while (opcode->mask);
>  
> +  if (warn_p)
> +    {
> +      info->fprintf_func (info->stream,
> +			  _("\nWarning: disassembly may be wrong due to "
> +			    "guessed opcode class choice.\n"
> +			    "Use -M<class[,class]> to select the correct "
> +			    "opcode class(es).\n\t\t\t\t"));
> +      return t_op;
> +    }
> +
>    return NULL;
>  }
>  
> @@ -694,18 +675,22 @@ parse_option (char *option)
>      add_to_decodelist (FLOAT, DPX);
>  
>    else if (CONST_STRNEQ (option, "quarkse_em"))
> -    add_to_decodelist (FLOAT, QUARKSE);
> +    {
> +      add_to_decodelist (FLOAT, DPX);
> +      add_to_decodelist (FLOAT, SPX);
> +      add_to_decodelist (FLOAT, QUARKSE);
> +    }
>  
>    else if (CONST_STRNEQ (option, "fpuda"))
>      add_to_decodelist (FLOAT, DPA);
>  
> -  else if (CONST_STRNEQ (option, "fpud"))
> +  else if (CONST_STRNEQ (option, "fpus"))
>      {
>        add_to_decodelist (FLOAT, SP);
>        add_to_decodelist (FLOAT, CVT);
>      }
>  
> -  else if (CONST_STRNEQ (option, "fpus"))
> +  else if (CONST_STRNEQ (option, "fpud"))
>      {
>        add_to_decodelist (FLOAT, DP);
>        add_to_decodelist (FLOAT, CVT);
> -- 
> 1.9.1
>