This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
[committed] MIPS16/GAS: Fix delay slot filling across frags
- From: "Maciej W. Rozycki" <macro at imgtec dot com>
- To: Matthew Fortune <Matthew dot Fortune at imgtec dot com>
- Cc: "binutils at sourceware dot org" <binutils at sourceware dot org>
- Date: Thu, 30 Jun 2016 15:13:54 +0100
- Subject: [committed] MIPS16/GAS: Fix delay slot filling across frags
- Authentication-results: sourceware.org; auth=none
- References: <6D39441BF12EF246A7ABCE6654B023537E448CF7 at HHMAIL01 dot hh dot imgtec dot org>
From: Matthew Fortune <Matthew.Fortune@imgtec.com>
Fix an assertion failure like:
test.s: Assembler messages:
test.s:3: Internal error!
Assertion failure in append_insn at .../gas/config/tc-mips.c:7523.
Please report this bug.
triggered by assembling MIPS16 code like:
hello:
addiu $4, $4, 4
jr $31
with the generation of a listing file enabled, e.g.:
$ as -mips16 -O2 -aln=test.lst
The cause of the problem is the lack of support for moving instructions
across frags in MIPS16 jump swapping, which triggers more easily with
listing enabled as in that case every instruction gets placed in its own
frag. It would trigger even with listing disabled though if the
instruction to swap a MIPS16 jump with was unfortunately enough placed
as last in a frag that became full.
This scenario is already handled correctly with branch swapping in
regular MIPS and microMIPS code, so reuse it for MIPS16 code as well,
and now that all MIPS16 handling has become the same as the regular MIPS
and microMIPS cases remove MIPS16 special casing altogether.
This effectively complements:
commit 464ab0e55ade01d2bb0b4fa45c429af7a2f85a26
Author: Maciej W. Rozycki <macro@linux-mips.org>
Date: Mon Aug 6 20:33:00 2012 +0000
<https://sourceware.org/ml/binutils/2012-08/msg00043.html>, ("MIPS/GAS:
Correct microMIPS branch swapping assertion") for the MIPS16 case.
The assertion itself was introduced with:
commit 1e91584932efd70020c8c98037d0cb93a0552a20
Author: Richard Sandiford <rdsandiford@googlemail.com>
Date: Wed Mar 9 09:17:02 2005 +0000
<https://sourceware.org/ml/binutils/2005-03/msg00217.html>, ("Rework
MIPS nop-insertion code, add -mfix-vr4130 [5/11]"), but its introduction
merely noted our existing lack of support for MIPS16 jump swapping
across frags.
gas/
* config/tc-mips.c (append_insn) <APPEND_SWAP>: Do not special
case MIPS16 handling.
* testsuite/gas/mips/branch-swap-3.d: New test.
* testsuite/gas/mips/branch-swap-4.d: New test.
* testsuite/gas/mips/mips16@branch-swap-3.d: New test.
* testsuite/gas/mips/mips16@branch-swap-4.d: New test.
* testsuite/gas/mips/micromips@branch-swap-3.d: New test.
* testsuite/gas/mips/micromips@branch-swap-4.d: New test.
* testsuite/gas/mips/branch-swap-3.s: New test source.
* testsuite/gas/mips/mips.exp: Run the new tests.
---
Hi Matthew,
On Tue, 14 Jun 2016, Matthew Fortune wrote:
> The following test fails with an assert in append_insn when built with:
>
> mips...as -mips32r2 -mips16 -O2 -aln=test.lst
>
> hello:
> addiu $4, $4, 4
> jr $31
>
> When generating a listing file gas seems to place every instruction in
> its own fragment which breaks the 'rule' that MIPS16 delay slot and
> jump must be in the same fragment. I don't know if the requirement
> to have these in the same fragment is valid or not but I looked at
> supporting them being in different fragments. The logic in this area
> seems rather delicate so I'm not sure of the fix.
>
> After following all the logic through for the various cases in
> APPEND_SWAP it seems there is nothing different about the MIPS16 case
> and the normal case for when DS and jump are in the same fragment.
> There is also nothing obviously wrong with the code for mis-matching
> fragments if applied to MIPS16.
>
> So... My best guess at a fix is to remove the MIPS16 special case and
> use the same code as MIPS32 and microMIPS.
>
> Relevant history in this area are three commits (most recent first):
> a4e064680 <no useful one-liner>
> b8ee1a6e8 <no useful one-liner>
> e9df6573b <no useful one-liner>
>
> And a mailing list discussion at:
> https://www.sourceware.org/ml/binutils/2005-09/msg00005.html
Thanks for your submission and the detailed research of the origins of
this code. This assertion failure actually dates a little bit further
back, or specifically:
commit 1e91584932efd70020c8c98037d0cb93a0552a20
Author: Richard Sandiford <rdsandiford@googlemail.com>
Date: Wed Mar 9 09:17:02 2005 +0000
<https://sourceware.org/ml/binutils/2005-03/msg00217.html>, ("Rework MIPS
nop-insertion code, add -mfix-vr4130 [5/11]"), when our branch swapping
code was modernised into its current form. The addition of the assertion
merely reflected the assumption MIPS16 handling has had since forever,
that is that both the branch and the instruction swapped remain in the
same frag. If this assumption did not stand, then previously code would
produce rubbish or maybe an assertion failure elsewhere, or maybe even
crashed instead.
Obviously this shows how often the listing feature is used these days
(i.e. nobody requested it with MIPS16 code before!), and also means we've
relied on pure luck with this handling as we'd hit the case even without
listing if the instruction to be swapped with a jump happened to be last
in the previous frag for other reasons; cf.:
commit 464ab0e55ade01d2bb0b4fa45c429af7a2f85a26
Author: Maciej W. Rozycki <macro@linux-mips.org>
Date: Mon Aug 6 20:33:00 2012 +0000
<https://sourceware.org/ml/binutils/2012-08/msg00043.html> ("MIPS/GAS:
Correct microMIPS branch swapping assertion"), where by the lone merit of
being handled separately I incorrectly assumed the issue does not trigger
for MIPS16 code.
At first I thought our current code special-cases MIPS16 handling beacuse
of MIPS16 standard vs extended instruction relaxation. However we don't
actually attempt to swap jumps with instructions in MIPS16 variant frags,
even though we could: we could add the jump to such a variant frag such
that the standard variant instruction is reordered into the delay slot and
the extended variant gets the jump and a delay-slot NOP appended. This
might be worth implementing sometime for a little performance gain, but
but not a size gain with the modern MIPS16 ISA however, as we switch the
jump to its compact form in this case then. So it looks to me like your
proposal is actually safe.
I have committed the change below then, with just a minor update to your
proposed code change to have the declaration and statements separated from
each other in the piece of code affected. I decided your test case was
insufficient though, as I think we want to make sure not only that code
assembles successfully, but also that the same code is produced with and
without a listing requested as well, and that both 16-bit and 32-bit
MIPS16 jump instructions are handled correctly across frags.
I also thought it would be worthwhile to have variant frags covered,
which revealed an interesting phenomenon where a variant instruction is
actually swapped with a fixed-slot-size microMIPS jump where a fixed
instruction is not. This looks like an area for a potential separate
improvement.
Maciej
binutils-matthewf-mips16-gas-delay-slot-frag.diff
Index: binutils/gas/config/tc-mips.c
===================================================================
--- binutils.orig/gas/config/tc-mips.c 2016-06-30 11:12:19.276873304 +0100
+++ binutils/gas/config/tc-mips.c 2016-06-30 11:12:53.984295960 +0100
@@ -7518,13 +7518,8 @@ append_insn (struct mips_cl_insn *ip, ex
case APPEND_SWAP:
{
struct mips_cl_insn delay = history[0];
- if (mips_opts.mips16)
- {
- know (delay.frag == ip->frag);
- move_insn (ip, delay.frag, delay.where);
- move_insn (&delay, ip->frag, ip->where + insn_length (ip));
- }
- else if (relaxed_branch || delay.frag != ip->frag)
+
+ if (relaxed_branch || delay.frag != ip->frag)
{
/* Add the delay slot instruction to the end of the
current frag and shrink the fixed part of the
Index: binutils/gas/testsuite/gas/mips/branch-swap-3.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils/gas/testsuite/gas/mips/branch-swap-3.d 2016-06-30 11:12:54.031553889 +0100
@@ -0,0 +1,38 @@
+#objdump: -dr -M reg-names=numeric
+#as: -32 -O2 -aln=branch-swap-lst.lst
+#name: MIPS branch swapping with assembler listing
+#source: branch-swap-3.s
+
+# Check delay slot filling with a listing file works
+
+.*: +file format .*mips.*
+
+Disassembly of section \.text:
+
+[0-9a-f]+ <test>:
+[ 0-9a-f]+: 0c000000 jal 0 <test>
+[ ]*[0-9a-f]+: R_MIPS_26 func
+[ 0-9a-f]+: 00408025 move \$16,\$2
+[ 0-9a-f]+: 0c000000 jal 0 <test>
+[ ]*[0-9a-f]+: R_MIPS_26 func
+[ 0-9a-f]+: 26300001 addiu \$16,\$17,1
+[ 0-9a-f]+: 0c000000 jal 0 <test>
+[ ]*[0-9a-f]+: R_MIPS_26 func
+[ 0-9a-f]+: 26300001 addiu \$16,\$17,1
+[ 0-9a-f]+: 0c000000 jal 0 <test>
+[ ]*[0-9a-f]+: R_MIPS_26 func
+[ 0-9a-f]+: 26303fff addiu \$16,\$17,16383
+[ 0-9a-f]+: 0c000000 jal 0 <test>
+[ ]*[0-9a-f]+: R_MIPS_26 func
+[ 0-9a-f]+: 26303fff addiu \$16,\$17,16383
+[ 0-9a-f]+: 03e0000[89] jr \$31
+[ 0-9a-f]+: 00408025 move \$16,\$2
+[ 0-9a-f]+: 03e0000[89] jr \$31
+[ 0-9a-f]+: 26300001 addiu \$16,\$17,1
+[ 0-9a-f]+: 03e0000[89] jr \$31
+[ 0-9a-f]+: 26300001 addiu \$16,\$17,1
+[ 0-9a-f]+: 03e0000[89] jr \$31
+[ 0-9a-f]+: 26303fff addiu \$16,\$17,16383
+[ 0-9a-f]+: 03e0000[89] jr \$31
+[ 0-9a-f]+: 26303fff addiu \$16,\$17,16383
+ \.\.\.
Index: binutils/gas/testsuite/gas/mips/branch-swap-3.s
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils/gas/testsuite/gas/mips/branch-swap-3.s 2016-06-30 11:12:54.054323118 +0100
@@ -0,0 +1,30 @@
+ .text
+test:
+ move $16, $2
+ jal func
+ addiu $16, $17, 1
+ jal func
+ addiu $16, $17, foo
+ jal func
+ addiu $16, $17, 0x3fff
+ jal func
+ addiu $16, $17, bar
+ jal func
+
+ move $16, $2
+ jr $31
+ addiu $16, $17, 1
+ jr $31
+ addiu $16, $17, foo
+ jr $31
+ addiu $16, $17, 0x3fff
+ jr $31
+ addiu $16, $17, bar
+ jr $31
+
+ .set foo, 1
+ .set bar, 0x3fff
+
+ # Force some (non-delay-slot) zero bytes, to make 'objdump' print ...
+ .space 16
+ .align 4, 0
Index: binutils/gas/testsuite/gas/mips/branch-swap-4.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils/gas/testsuite/gas/mips/branch-swap-4.d 2016-06-30 11:12:54.067622326 +0100
@@ -0,0 +1,5 @@
+#objdump: -dr -M reg-names=numeric
+#as: -32 -O2
+#name: MIPS branch swapping without assembler listing
+#source: branch-swap-3.s
+#dump: branch-swap-3.d
Index: binutils/gas/testsuite/gas/mips/micromips@branch-swap-3.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils/gas/testsuite/gas/mips/micromips@branch-swap-3.d 2016-06-30 11:12:54.094592181 +0100
@@ -0,0 +1,40 @@
+#objdump: -dr -M reg-names=numeric
+#as: -32 -O2 -aln=branch-swap-lst.lst
+#name: MIPS branch swapping with assembler listing
+#source: branch-swap-3.s
+
+# Check delay slot filling with a listing file works (microMIPS)
+
+.*: +file format .*mips.*
+
+Disassembly of section \.text:
+
+[0-9a-f]+ <test>:
+[ 0-9a-f]+: 0e02 move \$16,\$2
+[ 0-9a-f]+: f400 0000 jal 0 <test>
+[ ]*[0-9a-f]+: R_MICROMIPS_26_S1 func
+[ 0-9a-f]+: 0000 0000 nop
+[ 0-9a-f]+: 6c10 addiu \$16,\$17,1
+[ 0-9a-f]+: f400 0000 jal 0 <test>
+[ ]*[0-9a-f]+: R_MICROMIPS_26_S1 func
+[ 0-9a-f]+: 0000 0000 nop
+[ 0-9a-f]+: f400 0000 jal 0 <test>
+[ ]*[0-9a-f]+: R_MICROMIPS_26_S1 func
+[ 0-9a-f]+: 3211 0001 addiu \$16,\$17,1
+[ 0-9a-f]+: f400 0000 jal 0 <test>
+[ ]*[0-9a-f]+: R_MICROMIPS_26_S1 func
+[ 0-9a-f]+: 3211 3fff addiu \$16,\$17,16383
+[ 0-9a-f]+: f400 0000 jal 0 <test>
+[ ]*[0-9a-f]+: R_MICROMIPS_26_S1 func
+[ 0-9a-f]+: 3211 3fff addiu \$16,\$17,16383
+[ 0-9a-f]+: 459f jr \$31
+[ 0-9a-f]+: 0e02 move \$16,\$2
+[ 0-9a-f]+: 459f jr \$31
+[ 0-9a-f]+: 6c10 addiu \$16,\$17,1
+[ 0-9a-f]+: 459f jr \$31
+[ 0-9a-f]+: 3211 0001 addiu \$16,\$17,1
+[ 0-9a-f]+: 459f jr \$31
+[ 0-9a-f]+: 3211 3fff addiu \$16,\$17,16383
+[ 0-9a-f]+: 459f jr \$31
+[ 0-9a-f]+: 3211 3fff addiu \$16,\$17,16383
+ \.\.\.
Index: binutils/gas/testsuite/gas/mips/micromips@branch-swap-4.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils/gas/testsuite/gas/mips/micromips@branch-swap-4.d 2016-06-30 11:12:54.111020354 +0100
@@ -0,0 +1,5 @@
+#objdump: -dr -M reg-names=numeric
+#as: -32 -O2
+#name: MIPS branch swapping without assembler listing
+#source: branch-swap-3.s
+#dump: micromips@branch-swap-3.d
Index: binutils/gas/testsuite/gas/mips/mips.exp
===================================================================
--- binutils.orig/gas/testsuite/gas/mips/mips.exp 2016-06-30 11:12:16.147061498 +0100
+++ binutils/gas/testsuite/gas/mips/mips.exp 2016-06-30 11:12:54.127546518 +0100
@@ -575,6 +575,9 @@ if { [istarget mips*-*-vxworks*] } {
"MIPS branch swapping ($count)"
}
+ run_dump_test_arches "branch-swap-3" [mips_arch_list_all]
+ run_dump_test_arches "branch-swap-4" [mips_arch_list_all]
+
run_dump_test "branch-section-1"
run_dump_test "branch-section-2"
run_dump_test "branch-section-3"
Index: binutils/gas/testsuite/gas/mips/mips16@branch-swap-3.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils/gas/testsuite/gas/mips/mips16@branch-swap-3.d 2016-06-30 11:12:54.145044177 +0100
@@ -0,0 +1,44 @@
+#objdump: -dr -M reg-names=numeric
+#as: -32 -O2 -aln=branch-swap-lst.lst
+#name: MIPS branch swapping with assembler listing
+#source: branch-swap-3.s
+
+# Check delay slot filling with a listing file works (MIPS16)
+
+.*: +file format .*mips.*
+
+Disassembly of section \.text:
+
+[0-9a-f]+ <test>:
+[ 0-9a-f]+: 1800 0000 jal 0 <test>
+[ ]*[0-9a-f]+: R_MIPS16_26 func
+[ 0-9a-f]+: 6702 move \$16,\$2
+[ 0-9a-f]+: 1800 0000 jal 0 <test>
+[ ]*[0-9a-f]+: R_MIPS16_26 func
+[ 0-9a-f]+: 4101 addiu \$16,\$17,1
+[ 0-9a-f]+: 4101 addiu \$16,\$17,1
+[ 0-9a-f]+: 1800 0000 jal 0 <test>
+[ ]*[0-9a-f]+: R_MIPS16_26 func
+[ 0-9a-f]+: 6500 nop
+[ 0-9a-f]+: f7f7 410f addiu \$16,\$17,16383
+[ 0-9a-f]+: 1800 0000 jal 0 <test>
+[ ]*[0-9a-f]+: R_MIPS16_26 func
+[ 0-9a-f]+: 6500 nop
+[ 0-9a-f]+: f7f7 410f addiu \$16,\$17,16383
+[ 0-9a-f]+: 1800 0000 jal 0 <test>
+[ ]*[0-9a-f]+: R_MIPS16_26 func
+[ 0-9a-f]+: 6500 nop
+[ 0-9a-f]+: e820 jr \$31
+[ 0-9a-f]+: 6702 move \$16,\$2
+[ 0-9a-f]+: e820 jr \$31
+[ 0-9a-f]+: 4101 addiu \$16,\$17,1
+[ 0-9a-f]+: 4101 addiu \$16,\$17,1
+[ 0-9a-f]+: e820 jr \$31
+[ 0-9a-f]+: 6500 nop
+[ 0-9a-f]+: f7f7 410f addiu \$16,\$17,16383
+[ 0-9a-f]+: e820 jr \$31
+[ 0-9a-f]+: 6500 nop
+[ 0-9a-f]+: f7f7 410f addiu \$16,\$17,16383
+[ 0-9a-f]+: e820 jr \$31
+[ 0-9a-f]+: 6500 nop
+ \.\.\.
Index: binutils/gas/testsuite/gas/mips/mips16@branch-swap-4.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils/gas/testsuite/gas/mips/mips16@branch-swap-4.d 2016-06-30 11:12:54.156301917 +0100
@@ -0,0 +1,5 @@
+#objdump: -dr -M reg-names=numeric
+#as: -32 -O2
+#name: MIPS branch swapping without assembler listing
+#source: branch-swap-3.s
+#dump: mips16@branch-swap-3.d