Re: [PATCH 1/2] opcodes/mips: use .word/.short for undefined instructions

["Maciej W. Rozycki" <macro@orcam.me.uk>]

On Tue, 17 Jan 2023, Andrew Burgess wrote:

> Sorry for the time taken to prepare this patch.  Let me know if you're
> happy for my to push the below, or if there's anything else that's
> needed.

 This is mostly OK, thank you, but see below.

> diff --git a/binutils/testsuite/binutils-all/mips/mips.exp b/binutils/testsuite/binutils-all/mips/mips.exp
> index 6a0ec25a06f..f43109a75b8 100644
> --- a/binutils/testsuite/binutils-all/mips/mips.exp
> +++ b/binutils/testsuite/binutils-all/mips/mips.exp
> @@ -266,3 +266,7 @@ run_dump_test_n64 "global-local-symtab-sort-n64${tmips}"
>  run_dump_test_o32 "global-local-symtab-final-o32" useld
>  run_dump_test_n32 "global-local-symtab-final-n32" useld
>  run_dump_test_n64 "global-local-symtab-final-n64" useld
> +
> +run_dump_test_o32 "micromips-reserved-enc"
> +run_dump_test_n32 "micromips-reserved-enc"
> +run_dump_test_n64 "micromips-reserved-enc"

 Our convention has been not to have duplicate test names, so please 
create separate .d files for each of these three tests (we have no better 
way at the moment, although one could envisage appending the ABI name 
automatically).  See e.g. global-local-symtab-sort-n32.d in the same 
directory and the `source' and `dump' keywords within for how you can 
avoid duplicating identical sources and dumps.

> diff --git a/opcodes/mips-dis.c b/opcodes/mips-dis.c
> index 6a513cd8946..80c35f4a5e0 100644
> --- a/opcodes/mips-dis.c
> +++ b/opcodes/mips-dis.c
> @@ -2601,11 +2601,19 @@ print_insn_micromips (bfd_vma memaddr, struct disassemble_info *info)
>      }
>  
>    if (length == 2)
> -    infprintf (is, dis_style_assembler_directive, ".short");
> +    {
> +      infprintf (is, dis_style_assembler_directive, ".short");
> +      infprintf (is, dis_style_text, "\t");
> +      infprintf (is, dis_style_immediate, "0x%x", insn);
> +    }
>    else
> -    infprintf (is, dis_style_assembler_directive, ".word");
> -  infprintf (is, dis_style_text, "\t");
> -  infprintf (is, dis_style_immediate, "0x%x", insn);
> +    {
> +      infprintf (is, dis_style_assembler_directive, ".short");
> +      infprintf (is, dis_style_text, "\t");
> +      infprintf (is, dis_style_immediate, "0x%x", (insn >> 16) & 0xffff);
> +      infprintf (is, dis_style_text, ", ");
> +      infprintf (is, dis_style_immediate, "0x%x", (insn & 0xffff));
> +    }

 Now that I have looked at it again I've been wondering if:

  infprintf (is, dis_style_assembler_directive, ".short");
  infprintf (is, dis_style_text, "\t");
  if (length != 2)
    {
      infprintf (is, dis_style_immediate, "0x%x", (insn >> 16) & 0xffff);
      infprintf (is, dis_style_text, ", ");
    }
  infprintf (is, dis_style_immediate, "0x%x", (insn & 0xffff));

might be more desirably avoiding some code duplication, but I'll leave it 
up to you to decide if to keep your original proposal or whether to switch 
to this alternative.

  Maciej