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

[Andrew Burgess <aburgess@redhat.com>]

"Maciej W. Rozycki" <macro@orcam.me.uk> writes:

> On Fri, 6 Jan 2023, Andrew Burgess wrote:
>
>> >  FYI, I find this questionable as `.word' (at least with the MIPS target) 
>> > implies natural alignment while 32-bit microMIPS encodings, valid or not, 
>> > are not.  Also, given the endianness peculiarity (analogous to the MIPS16 
>> > extended encodings), I think this needs to be ".short\t0x%x, 0x%x" really, 
>> > with the instruction word split into halfwords for any reasonable meaning.  
>> > This is already reflected in the raw hex dump of instruction streams; the 
>> > numbers printed need to match it.
>> >
>> >  With the naked number previously used this obviously didn't matter as it 
>> > stood out without any attempt to pretend to have a meaning.  This is also 
>> > the reason why I chose to keep it as it used to be since forever.
>> 
>> Below is an initial patch.  When I set the environment variable
>> DISABLE_MATCHING then the disassembler fails to match all instructions,
>> so prints .short for everything.
>> 
>> Right now I can't find anything where this doesn't work, but I don't
>> believe that the answer is actually this simple.  Given your deeper
>> knowledge of the target, could you take a look at what I have below and
>> point me at some tests/configurations/whatever where this isn't going to
>> be good enough?
>> 
>> Alternatively, if this is enough, then I'll write this up into a proper
>> patch.
>
>  Your change is probably right.  I'd have thought we have coverage for 
> this in the testsuite, but perhaps we don't.
>
>  If you try this source code (which uses a reserved 32-bit encoding in the 
> microMIPS ISA):
>
> 	.module	micromips
> foo:
> 	.insn
> 	.short	0x7f6e, 0x5d4c
>
> and assemble it for both endiannesses (i.e. with `-EL' and `-EB' passed to 
> GAS respectively), then I'd expect output like:
>
> Disassembly of section .text:
>
> 00000000 <foo>:
>    0:	7f6e 5d4c 	.short	0x7f6e, 0x5d4c
> 	...
>
> from `objdump -d' in both cases.  If this is the case, then the change is 
> right.
>
>  You can use this example, preferably along with the change itself, for a 
> testcase to place in binutils/testsuite/binutils-all/mips/.  I suggest 
> using `run_dump_test_o32'/`run_dump_test_n32'/`run_dump_test_n64' all at a 
> time just as with most of the preexisting test cases just to make sure all 
> the three BFD backends involved handle this right.
>
>  Let me know if you need further information.

Hi Maciej,

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.

Thanks,
Andrew

---

commit 1502245dc2194e8e06a69275b22cffd211218578
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.

diff --git a/binutils/testsuite/binutils-all/mips/micromips-reserved-enc.d b/binutils/testsuite/binutils-all/mips/micromips-reserved-enc.d
new file mode 100644
index 00000000000..fdcc6deae8f
--- /dev/null
+++ b/binutils/testsuite/binutils-all/mips/micromips-reserved-enc.d
@@ -0,0 +1,9 @@
+#PROG: objcopy
+#objdump: -d --prefix-addresses --show-raw-insn
+#name: microMIPS source file contains reserved encoding
+
+.*: +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..59113a7980d
--- /dev/null
+++ b/binutils/testsuite/binutils-all/mips/micromips-reserved-enc.s
@@ -0,0 +1,4 @@
+        .module	micromips
+foo:
+        .insn
+        .short	0x7f6e, 0x5d4c
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"
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));
+    }
   info->insn_type = dis_noninsn;
 
   return length;