This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] MIPS/GAS: Fix branch swapping with relaxed macros
- From: Tristan Gingold <gingold at adacore dot com>
- To: Maciej W. Rozycki <macro at codesourcery dot com>
- Cc: <binutils at sourceware dot org>, Richard Sandiford <rdsandiford at googlemail dot com>, Meador Inge <meadori at codesourcery dot com>
- Date: Wed, 2 Nov 2011 12:51:05 +0100
- Subject: Re: [PATCH] MIPS/GAS: Fix branch swapping with relaxed macros
- References: <alpine.DEB.1.10.1110220443000.28657@tp.orcam.me.uk>
On Oct 24, 2011, at 5:08 PM, Maciej W. Rozycki wrote:
> 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?
Yes, ok for the back port.
Tristan.
>
> 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