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

[Andrew Burgess <aburgess@redhat.com>]

Andrew Burgess <aburgess@redhat.com> writes:

> "Maciej W. Rozycki" <macro@orcam.me.uk> writes:
>
>> Andrew,
>>
>>> I've updated the patch.  Let me know what you think.
>>
>>  I'd say it's OK, except that I put your change through my MIPS regression 
>> tester and that revealed failures from your new case for numerous targets, 
>> e.g.:
>>
>> mips-elf  +FAIL: microMIPS source file contains reserved encoding (o32)
>> mips-img-elf  +FAIL: microMIPS source file contains reserved encoding (o32)
>> mips-img-elf  +FAIL: microMIPS source file contains reserved encoding (n32)
>>
>> etc.  The usual suspect is section padding owing to different alignments 
>> used with individual MIPS targets, e.g.:
>>
>> extra regexps in 
>> .../binutils/testsuite/binutils-all/mips/micromips-reserved-enc-o32.d starting with "^
>> \.\.\.$"
>> EOF from tmpdir/dump.out
>> FAIL: microMIPS source file contains reserved encoding (o32)
>>
>> See e.g. binutils/testsuite/binutils-all/mips/micromips-branch-alias.s for 
>> how to add suitable padding at the end.
>>
>>  And then:
>>
>> mipsisa32r6-elf  +FAIL: microMIPS source file contains reserved encoding (o32)
>> mipsisa32r6-linux  +FAIL: microMIPS source file contains reserved encoding (o32)
>> mipsisa32r6el-elf  +FAIL: microMIPS source file contains reserved encoding (o32)
>>
>> etc., due to:
>>
>> .../binutils/testsuite/binutils-all/mips/micromips-reserved-enc.s: Assembler messages:
>> .../binutils/testsuite/binutils-all/mips/micromips-reserved-enc.s:3: Fatal error: `micromips' cannot be used with `mips32r6'
>>
>> We don't care about different ISA levels here, so let's set a reasonable 
>> one, as in binutils/testsuite/binutils-all/mips/micromips-branch-alias.s 
>> again:
>>
>> 	.module	mips64r3
>>
>> (it could be `.set' too, but let's be consistent, and it has to be a 
>> 64-bit one for the n32/n64 ABIs).
>>
>>  OK with these updates, thank you for your contribution.
>
> Right, I think I've got it this time :)
>
> Updated patch is below.  Given the updates are minor, if I don't hear
> back from you I'll push this sometime next week.
>
> I tested this against all the targets you listed, the only place this
> test still fails is for `mipsisa64sr71k-elf`, but that build looks
> pretty broken anyway.  The failure is in gas, during argument parsing,
> and I see the same failure for many of the tests, so I don't think I
> need to worry about that.  Everything else looks like a PASS or
> UNSUPPORTED, which I think is fine.
>
> Thanks for all your feedback on this one.
>
> Andrew
>
> ---
>
> commit c84e242a7a45787c9ff60a3cf06b3b7f30d85970
> Author: Andrew Burgess <aburgess@redhat.com>
> Date:   Fri Jan 6 16:42:23 2023 +0000
>
>     opcodes/mips: disassemble unknown micromips instructions as two shorts
>     
>     Before commit:
>     
>       commit 2438b771ee07be19d5b01ea55e077dd8b7cef445
>       Date:   Wed Nov 2 15:53:43 2022 +0000
>     
>           opcodes/mips: use .word/.short for undefined instructions
>     
>     unknown 32-bit microMIPS instructions were disassembled as a raw
>     32-bit number with no '.word' directive.  The above commit changed
>     this and added a '.word' directive before the 32-bit number.
>     
>     It was pointed out on the mailing list, that for microMIPS it would be
>     better to display such 32-bit instructions using a '.short' directive
>     followed by two 16-bit values.
>     
>     This commit updates the mips disassembler to do this, and adds a new
>     test that validates this output.

Given the previous version was given the OK once the minor fixes were
merged, I've gone ahead and pushed this patch.

Do let me know if there are any further problems in this area.

Thanks,
Andrew



>
> diff --git a/binutils/testsuite/binutils-all/mips/micromips-reserved-enc-n32.d b/binutils/testsuite/binutils-all/mips/micromips-reserved-enc-n32.d
> new file mode 100644
> index 00000000000..e6608f30265
> --- /dev/null
> +++ b/binutils/testsuite/binutils-all/mips/micromips-reserved-enc-n32.d
> @@ -0,0 +1,5 @@
> +#PROG: objcopy
> +#objdump: -d --prefix-addresses --show-raw-insn
> +#name: microMIPS source file contains reserved encoding (n32)
> +#source: micromips-reserved-enc.s
> +#dump: micromips-reserved-enc-o32.d
> diff --git a/binutils/testsuite/binutils-all/mips/micromips-reserved-enc-n64.d b/binutils/testsuite/binutils-all/mips/micromips-reserved-enc-n64.d
> new file mode 100644
> index 00000000000..f892bfabbe7
> --- /dev/null
> +++ b/binutils/testsuite/binutils-all/mips/micromips-reserved-enc-n64.d
> @@ -0,0 +1,5 @@
> +#PROG: objcopy
> +#objdump: -d --prefix-addresses --show-raw-insn
> +#name: microMIPS source file contains reserved encoding (n64)
> +#source: micromips-reserved-enc.s
> +#dump: micromips-reserved-enc-o32.d
> diff --git a/binutils/testsuite/binutils-all/mips/micromips-reserved-enc-o32.d b/binutils/testsuite/binutils-all/mips/micromips-reserved-enc-o32.d
> new file mode 100644
> index 00000000000..3de3989b37a
> --- /dev/null
> +++ b/binutils/testsuite/binutils-all/mips/micromips-reserved-enc-o32.d
> @@ -0,0 +1,10 @@
> +#PROG: objcopy
> +#objdump: -d --prefix-addresses --show-raw-insn
> +#name: microMIPS source file contains reserved encoding (o32)
> +#source: micromips-reserved-enc.s
> +
> +.*: +file format .*mips.*
> +
> +Disassembly of section \.text:
> +[0-9a-f]+ <[^>]*> 7f6e 5d4c 	\.short	0x7f6e, 0x5d4c
> +	\.\.\.
> diff --git a/binutils/testsuite/binutils-all/mips/micromips-reserved-enc.s b/binutils/testsuite/binutils-all/mips/micromips-reserved-enc.s
> new file mode 100644
> index 00000000000..d4918f36857
> --- /dev/null
> +++ b/binutils/testsuite/binutils-all/mips/micromips-reserved-enc.s
> @@ -0,0 +1,9 @@
> +	.module mips64r3
> +	.module	micromips
> +foo:
> +	.insn
> +	.short	0x7f6e, 0x5d4c
> +
> +# Force some (non-delay-slot) zero bytes, to make 'objdump' print ...
> +	.align	4, 0
> +	.space	16
> diff --git a/binutils/testsuite/binutils-all/mips/mips.exp b/binutils/testsuite/binutils-all/mips/mips.exp
> index 6a0ec25a06f..91bf3274592 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-o32"
> +run_dump_test_n32 "micromips-reserved-enc-n32"
> +run_dump_test_n64 "micromips-reserved-enc-n64"
> diff --git a/opcodes/mips-dis.c b/opcodes/mips-dis.c
> index 6a513cd8946..859d4e3806f 100644
> --- a/opcodes/mips-dis.c
> +++ b/opcodes/mips-dis.c
> @@ -2600,12 +2600,15 @@ print_insn_micromips (bfd_vma memaddr, struct disassemble_info *info)
>  	}
>      }
>  
> -  if (length == 2)
> -    infprintf (is, dis_style_assembler_directive, ".short");
> -  else
> -    infprintf (is, dis_style_assembler_directive, ".word");
> +  infprintf (is, dis_style_assembler_directive, ".short");
>    infprintf (is, dis_style_text, "\t");
> -  infprintf (is, dis_style_immediate, "0x%x", insn);
> +  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));
> +
>    info->insn_type = dis_noninsn;
>  
>    return length;