This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
[PATCH] MIPS/GAS: Fix branch swapping with relaxed macros
- From: "Maciej W. Rozycki" <macro at codesourcery dot com>
- To: <binutils at sourceware dot org>
- Cc: Richard Sandiford <rdsandiford at googlemail dot com>, Meador Inge <meadori at codesourcery dot com>, Tristan Gingold <gingold at adacore dot com>
- Date: Mon, 24 Oct 2011 16:08:47 +0100
- Subject: [PATCH] MIPS/GAS: Fix branch swapping with relaxed macros
Hi,
The change to enable microMIPS branch swapping caused a regression, where
if an instruction preceding a branch is actually a relaxed macro, then GAS
tries to fiddle with the preceding flag without realising it is a variant
frag causing hell to break loose and the following effect:
$ mips-sde-elf-as -32 -march=mips64 -mmicromips -o relax-swap3.o relax-swap3.s
relax-swap3.s: Assembler messages:
relax-swap3.s:5: Error: internal error: fixup not contained within frag
relax-swap3.s:5: Error: internal error: fixup not contained within frag
Unfortunately it has turned out such a case is not covered by the test
suite and was only triggered with some code Meador tried to assemble.
While in principle it's possible to handle swapping a tail of a variant
frag with a branch (one way of doing it would be splitting the last
instruction of both variants off into another, single-instruction variant
frag placed in the delay slot) I believe it's been a deliberate decision
not to complicate GAS to handle such corner case. I have therefore
decided to disable branch swapping with relaxed macros for microMIPS code
like it's already done for standard MIPS code.
The following change fixes the problem and adds a suitable test case. I
have made the test case to run across all the three ISA modes we support,
to cover the standard MIPS case too and the case of the slightly different
MIPS16 LA macro that nevertheless cannot be swapped with a jump either,
although for different reasons (and guarded with a different conditional
in can_swap_branch_p -- history[0].fixed_p).
I had to add the #pass (which as you may know I'm a bit opposed to) at
the end of the MIPS16 test case as MIPS16 code uses odd alignment rules --
apparently twice the amount used for standard or microMIPS code (not sure
where it comes from offhand) -- and then pads with the 0x6500 16-byte NOP
opcode. Perhaps we could use:
.align 4
.space 16
to deal with that, but let's keep the change as it is for now; I'll have a
more systematic look into it as my time permits to see if that's worth the
hassle in the first place.
Regression-tested successfully, for the mips-linux-gnu and mips-sde-elf
targets. OK to apply?
I believe this is 2.22 material too -- OK there as well?
2011-10-24 Maciej W. Rozycki <macro@codesourcery.com>
gas/
* config/tc-mips.c (can_swap_branch_p): Exclude microMIPS
variant frags too.
gas/testsuite/
* gas/mips/relax-swap3.d: New test.
* gas/mips/mips16@relax-swap3.d: Likewise.
* gas/mips/micromips@relax-swap3.d: Likewise.
* gas/mips/relax-swap3.s: New test source.
* gas/mips/mips.exp: Run the new tests.
Maciej
binutils-gas-umips-relax-fix.diff
Index: binutils-fsf-trunk-quilt/gas/config/tc-mips.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/config/tc-mips.c 2011-10-24 14:49:15.795929714 +0100
+++ binutils-fsf-trunk-quilt/gas/config/tc-mips.c 2011-10-24 14:49:16.135923384 +0100
@@ -3728,9 +3728,8 @@ can_swap_branch_p (struct mips_cl_insn *
/* If the previous instruction is in a variant frag other than this
branch's one, we cannot do the swap. This does not apply to
- MIPS16/microMIPS code, which uses variant frags for different
- purposes. */
- if (!HAVE_CODE_COMPRESSION
+ MIPS16 code, which uses variant frags for different purposes. */
+ if (!mips_opts.mips16
&& history[0].frag
&& history[0].frag->fr_type == rs_machine_dependent)
return FALSE;
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/micromips@relax-swap3.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/micromips@relax-swap3.d 2011-10-24 14:49:16.135923384 +0100
@@ -0,0 +1,22 @@
+#objdump: -dr --prefix-addresses --show-raw-insn
+#name: MIPS relaxed macro with branch swapping
+#as: -32
+#source: relax-swap3.s
+
+.*: +file format .*mips.*
+
+Disassembly of section \.text:
+[0-9a-f]+ <[^>]*> 41a2 0000 lui v0,0x0
+[ ]*[0-9a-f]+: R_MICROMIPS_HI16 bar
+[0-9a-f]+ <[^>]*> 3042 0000 addiu v0,v0,0
+[ ]*[0-9a-f]+: R_MICROMIPS_LO16 bar
+[0-9a-f]+ <[^>]*> 4583 jr v1
+[0-9a-f]+ <[^>]*> 0c00 nop
+[0-9a-f]+ <[^>]*> 41a2 0000 lui v0,0x0
+[ ]*[0-9a-f]+: R_MICROMIPS_HI16 bar
+[0-9a-f]+ <[^>]*> 3042 0000 addiu v0,v0,0
+[ ]*[0-9a-f]+: R_MICROMIPS_LO16 bar
+[0-9a-f]+ <[^>]*> 8dff beqz v1,[0-9a-f]+ <[^>]*>
+[ ]*[0-9a-f]+: R_MICROMIPS_PC7_S1 .*
+[0-9a-f]+ <[^>]*> 0c00 nop
+ \.\.\.
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips.exp
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/testsuite/gas/mips/mips.exp 2011-10-24 14:49:14.585861691 +0100
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips.exp 2011-10-24 14:49:16.135923384 +0100
@@ -749,6 +749,7 @@ if { [istarget mips*-*-vxworks*] } {
run_dump_test "relax-swap1-mips1"
run_dump_test "relax-swap1-mips2"
run_dump_test "relax-swap2"
+ run_dump_test_arches "relax-swap3" [mips_arch_list_all]
run_list_test_arches "relax-bposge" "-mdsp -relax-branch" \
[mips_arch_list_matching mips64r2 \
!micromips]
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips16@relax-swap3.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips16@relax-swap3.d 2011-10-24 14:49:16.135923384 +0100
@@ -0,0 +1,15 @@
+#objdump: -dr --prefix-addresses --show-raw-insn
+#name: MIPS relaxed macro with branch swapping
+#as: -32
+#source: relax-swap3.s
+
+.*: +file format .*mips.*
+
+Disassembly of section \.text:
+[0-9a-f]+ <[^>]*> 0a00 la v0,[0-9a-f]+ <[^>]*>
+[0-9a-f]+ <[^>]*> eb00 jr v1
+[0-9a-f]+ <[^>]*> 6500 nop
+[0-9a-f]+ <[^>]*> f7ff 0a1c la v0,[0-9a-f]+ <[^>]*>
+[0-9a-f]+ <[^>]*> 2300 beqz v1,[0-9a-f]+ <[^>]*>
+ \.\.\.
+#pass
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/relax-swap3.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/relax-swap3.d 2011-10-24 14:49:16.135923384 +0100
@@ -0,0 +1,21 @@
+#objdump: -dr --prefix-addresses --show-raw-insn
+#name: MIPS relaxed macro with branch swapping
+#as: -32
+#source: relax-swap3.s
+
+.*: +file format .*mips.*
+
+Disassembly of section \.text:
+[0-9a-f]+ <[^>]*> 3c020000 lui v0,0x0
+[ ]*[0-9a-f]+: R_MIPS_HI16 bar
+[0-9a-f]+ <[^>]*> 24420000 addiu v0,v0,0
+[ ]*[0-9a-f]+: R_MIPS_LO16 bar
+[0-9a-f]+ <[^>]*> 00600008 jr v1
+[0-9a-f]+ <[^>]*> 00000000 nop
+[0-9a-f]+ <[^>]*> 3c020000 lui v0,0x0
+[ ]*[0-9a-f]+: R_MIPS_HI16 bar
+[0-9a-f]+ <[^>]*> 24420000 addiu v0,v0,0
+[ ]*[0-9a-f]+: R_MIPS_LO16 bar
+[0-9a-f]+ <[^>]*> 10600001 beqz v1,[0-9a-f]+ <[^>]*>
+[0-9a-f]+ <[^>]*> 00000000 nop
+ \.\.\.
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/relax-swap3.s
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/relax-swap3.s 2011-10-24 14:49:16.135923384 +0100
@@ -0,0 +1,14 @@
+# Source file used to check the lack of branch swapping with a relaxed macro.
+
+ .text
+foo:
+ la $2, bar
+ jr $3
+
+ la $2, bar
+ beqz $3, 0f
+0:
+
+# Force at least 8 (non-delay-slot) zero bytes, to make 'objdump' print ...
+ .align 2
+ .space 8