This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] MIPS/GAS: Fix branch swapping with relaxed macros


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]