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]

[PATCH v2] microMIPS/GAS: Unbreak fixed-size delay slot handling


Hi,

 [I sent the wrong version of the patch, sorry, here's the correct one.]

 Here is a fix for a corner-case issue where a 32-bit instruction is 
requested for a 16-bit fixed-size delay slot, e.g.:

$ cat foo.s
	.set	micromips
	.set	noreorder
	jalrs	$2
	 and	$2,$3,$4
$ mips-sde-elf-as -o foo.o foo.s
foo.s: Assembler messages:
foo.s:4: Error: absolute expression required `and $2,$3,$4'
$ cat bar.s
	.set	micromips
	.set	noreorder
	jalrs	$2
	 sw	$18,208($28)
$ mips-sde-elf-as -o bar.o bar.s
bar.s: Assembler messages:
bar.s:4: Error: Macro used $at after ".set noat"
bar.s:4: Warning: Macro instruction expanded into a wrong size instruction in a 16-bit branch delay slot
$

 Of course the run-time behaviour of this code is unspecified, but 
nevertheless GAS should do the right thing and having failed to find a 
suitable 16-bit instruction output a 32-bit one as if outside a delay 
slot.  It is actually supposed to be handled (there's been a suitable 
warning message defined too), and for this purpose GAS makes two passes 
over the opcode table, in the first one ignoring any 32-bit instructions 
and in the second -- taking all instructions into account.

 What happens here is a fallback macro matches the instruction (in the 
case of foo.s this is also incorrect, but I'll be handling that 
separately; if not the bug handled here that would be benign) and code to 
handle the macro has been written under the assumption a preceding real 
instruction has already matched for any operand cases that are handled by 
one.  But in the case of a 16-bit fixed-size delay slot all the operand 
cases that could only be handled with a 32-bit instruction have been 
skipped.  This is already triggered by our test suite as seen in the 
changes below to micromips-branch-delay.l, but the incorrect warning 
message produced was obviously easily lost in the noise.

 To solve this problem I have decided we should simply ignore macros in 
the first pass for 16-bit delay slots.  I believe this is the right 
approach as firstly that avoids the assumption macros rely on noted above, 
and secondly there are no macros that produce a 16-bit instruction and are 
preceded by a 32-bit instruction both at a time.  Therefore any macros 
that are intended to emit a 16-bit instruction into a delay slot will 
happily do so in the second pass.

 There are actually only three/four macros that can produce a 16-bit 
instruction and these are: JAL/JALS, BGT and BALIGN -- emitting a 
JALR/JALRS, a NOP and a NOP, respectively.  And of these only BALIGN makes 
any sense in a delay slot.  Nevertheless all work correctly with my 
change, fulfilling the delay slot requirement.

 To complement this change I have adjusted code throughout to emit a 
32-bit JALR instruction, where it is produced from a macro, if placed in a 
fixed-size 32-bit delay slot -- this issue was revealed with one of the 
tests included.

 Thanks to Chao-ying for reporting the issue and providing one of the test 
cases.

 No regressions in testing across the usual MIPS targets.  OK to apply?

2012-10-23  Maciej W. Rozycki  <macro@codesourcery.com>

	gas/
	* config/tc-mips.c (is_delay_slot_valid): Don't accept macros
	in 16-bit delay slots.
	(macro_build_jalr): Emit 32-bit JALR if placed in a 32-bit delay 
	slot.
	(macro) <M_JAL_2>: Likewise

2012-10-23  Chao-ying Fu  <fu@mips.com>
            Maciej W. Rozycki  <macro@codesourcery.com>

	gas/testsuite/
	* gas/mips/micromips-branch-delay.l: Update messages for 16-bit 
	delay slot changes.
	* gas/mips/micromips-warn-branch-delay.d: New test.
	* gas/mips/micromips-warn-branch-delay.l: Stderr output for the 
	new test.
	* gas/mips/micromips-warn-branch-delay-1.d: New test.
	* gas/mips/micromips-warn-branch-delay.s: New test source.
	* gas/mips/micromips-warn-branch-delay-1.s: New test source.
	* gas/mips/mips.exp: Run the new tests.

  Maciej

binutils-mips-gas-fixed-delayed-slot-macro.diff
Index: binutils-fsf-trunk-quilt/gas/config/tc-mips.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/config/tc-mips.c	2012-10-22 22:14:38.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/config/tc-mips.c	2012-10-22 22:15:11.031769292 +0100
@@ -2301,7 +2301,18 @@ is_size_valid (const struct mips_opcode 
 }
 
 /* Return TRUE if the microMIPS opcode MO is valid for the delay slot
-   of the preceding instruction.  Always TRUE in the standard MIPS mode.  */
+   of the preceding instruction.  Always TRUE in the standard MIPS mode.
+
+   We don't accept macros in 16-bit delay slots to avoid a case where
+   a macro expansion fails because it relies on a preceding 32-bit real
+   instruction to have matched and does not handle the operands correctly.
+   The only macros that may expand to 16-bit instructions are JAL that
+   cannot be placed in a delay slot anyway, and corner cases of BALIGN
+   and BGT (that likewise cannot be placed in a delay slot) that decay to
+   a NOP.  In all these cases the macros precede any corresponding real
+   instruction definitions in the opcode table, so they will match in the
+   second pass where the size of the delay slot is ignored and therefore
+   produce correct code.  */
 
 static bfd_boolean
 is_delay_slot_valid (const struct mips_opcode *mo)
@@ -2310,7 +2321,8 @@ is_delay_slot_valid (const struct mips_o
     return TRUE;
 
   if (mo->pinfo == INSN_MACRO)
-    return TRUE;
+    return ((history[0].insn_mo->pinfo2 & INSN2_BRANCH_DELAY_16BIT) == 0
+	    ? TRUE : FALSE);
   if ((history[0].insn_mo->pinfo2 & INSN2_BRANCH_DELAY_32BIT) != 0
       && micromips_insn_length (mo) != 4)
     return FALSE;
@@ -5328,7 +5340,8 @@ macro_build_jalr (expressionS *ep, int c
   if (mips_opts.micromips)
     {
       jalr = mips_opts.noreorder && !cprestore ? "jalr" : "jalrs";
-      if (MIPS_JALR_HINT_P (ep))
+      if (MIPS_JALR_HINT_P (ep)
+	  || (history[0].insn_mo->pinfo2 & INSN2_BRANCH_DELAY_32BIT))
 	macro_build (NULL, jalr, "t,s", RA, PIC_CALL_REG);
       else
 	macro_build (NULL, jalr, "mj", PIC_CALL_REG);
@@ -7768,7 +7781,9 @@ macro (struct mips_cl_insn *ip)
       if (mips_pic == NO_PIC)
 	{
 	  s = jals ? "jalrs" : "jalr";
-	  if (mips_opts.micromips && dreg == RA)
+	  if (mips_opts.micromips
+	      && dreg == RA
+	      && !(history[0].insn_mo->pinfo2 & INSN2_BRANCH_DELAY_32BIT))
 	    macro_build (NULL, s, "mj", sreg);
 	  else
 	    macro_build (NULL, s, JALR_FMT, dreg, sreg);
@@ -7783,7 +7798,9 @@ macro (struct mips_cl_insn *ip)
 
 	  s = (mips_opts.micromips && (!mips_opts.noreorder || cprestore)
 	       ? "jalrs" : "jalr");
-	  if (mips_opts.micromips && dreg == RA)
+	  if (mips_opts.micromips
+	      && dreg == RA
+	      && !(history[0].insn_mo->pinfo2 & INSN2_BRANCH_DELAY_32BIT))
 	    macro_build (NULL, s, "mj", sreg);
 	  else
 	    macro_build (NULL, s, JALR_FMT, dreg, sreg);
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/micromips-branch-delay.l
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/testsuite/gas/mips/micromips-branch-delay.l	2012-10-22 22:14:28.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/micromips-branch-delay.l	2012-10-22 22:15:19.711764181 +0100
@@ -1,6 +1,6 @@
 .*: Assembler messages:
-.*:17: Warning: Macro instruction expanded into a wrong size instruction in a 16-bit branch delay slot
-.*:19: Warning: Macro instruction expanded into a wrong size instruction in a 16-bit branch delay slot
+.*:17: Warning: Wrong size instruction in a 16-bit branch delay slot
+.*:19: Warning: Wrong size instruction in a 16-bit branch delay slot
 .*:21: Warning: Macro instruction expanded into a wrong size instruction in a 16-bit branch delay slot
 .*:40: Warning: Wrong size instruction in a 16-bit branch delay slot
 .*:44: Warning: Wrong size instruction in a 16-bit branch delay slot
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/micromips-warn-branch-delay-1.d
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/micromips-warn-branch-delay-1.d	2012-10-22 22:15:19.711764181 +0100
@@ -0,0 +1,41 @@
+#objdump: -dr --prefix-addresses --show-raw-insn
+#name: microMIPS fixed-size branch delay slots 1
+#as: -32 -mmicromips
+#source: micromips-warn-branch-delay-1.s
+
+.*: +file format .*mips.*
+
+Disassembly of section \.text:
+([0-9a-f]+) <[^>]*> 4220 fffe 	bltzals	zero,\1 <.*>
+[ 	]*[0-9a-f]+: R_MICROMIPS_PC16_S1	.*
+[0-9a-f]+ <[^>]*> 0c00      	nop
+[0-9a-f]+ <[^>]*> 0c00      	nop
+([0-9a-f]+) <[^>]*> 4220 fffe 	bltzals	zero,\1 <.*>
+[ 	]*[0-9a-f]+: R_MICROMIPS_PC16_S1	.*
+[0-9a-f]+ <[^>]*> 0c00      	nop
+[0-9a-f]+ <[^>]*> 0c00      	nop
+([0-9a-f]+) <[^>]*> 4220 fffe 	bltzals	zero,\1 <.*>
+[ 	]*[0-9a-f]+: R_MICROMIPS_PC16_S1	.*
+[0-9a-f]+ <[^>]*> 45e2      	jalrs	v0
+[0-9a-f]+ <[^>]*> 0c00      	nop
+([0-9a-f]+) <[^>]*> 4220 fffe 	bltzals	zero,\1 <.*>
+[ 	]*[0-9a-f]+: R_MICROMIPS_PC16_S1	.*
+[0-9a-f]+ <[^>]*> 0c00      	nop
+[0-9a-f]+ <[^>]*> 0c00      	nop
+([0-9a-f]+) <[^>]*> 4020 fffe 	bltzal	zero,\1 <.*>
+[ 	]*[0-9a-f]+: R_MICROMIPS_PC16_S1	.*
+[0-9a-f]+ <[^>]*> 0000 0000 	nop
+[0-9a-f]+ <[^>]*> 0c00      	nop
+([0-9a-f]+) <[^>]*> 4020 fffe 	bltzal	zero,\1 <.*>
+[ 	]*[0-9a-f]+: R_MICROMIPS_PC16_S1	.*
+[0-9a-f]+ <[^>]*> 0000 0000 	nop
+[0-9a-f]+ <[^>]*> 0c00      	nop
+([0-9a-f]+) <[^>]*> 4020 fffe 	bltzal	zero,\1 <.*>
+[ 	]*[0-9a-f]+: R_MICROMIPS_PC16_S1	.*
+[0-9a-f]+ <[^>]*> 03e2 4f3c 	jalrs	v0
+[0-9a-f]+ <[^>]*> 0c00      	nop
+([0-9a-f]+) <[^>]*> 4020 fffe 	bltzal	zero,\1 <.*>
+[ 	]*[0-9a-f]+: R_MICROMIPS_PC16_S1	.*
+[0-9a-f]+ <[^>]*> 0000 0000 	nop
+[0-9a-f]+ <[^>]*> 0c00      	nop
+	\.\.\.
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/micromips-warn-branch-delay-1.s
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/micromips-warn-branch-delay-1.s	2012-10-22 22:15:19.711764181 +0100
@@ -0,0 +1,43 @@
+# Source code used to test correct macro expansion in microMIPS fixed-size
+# branch delay slots.
+
+	.text
+	.set	dspr2
+	.set	noreorder
+	.set	noat
+test:
+	bltzals	$0, .
+	 nop
+	nop
+
+	bltzals	$0, .
+	 bgt	$2, 0x7fffffff, .
+	 nop
+
+	bltzals	$0, .
+	 jals	$2
+	 nop
+
+	bltzals	$0, .
+	 balign	$2, $2, 0
+	nop
+
+	bltzal	$0, .
+	 nop
+	nop
+
+	bltzal	$0, .
+	 bgt	$2, 0x7fffffff, .
+	 nop
+
+	bltzal	$0, .
+	 jals	$2
+	 nop
+
+	bltzal	$0, .
+	 balign	$2, $2, 0
+	nop
+
+# Force some (non-delay-slot) zero bytes, to make 'objdump' print ...
+	.align	4, 0
+	.space	16
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/micromips-warn-branch-delay.d
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/micromips-warn-branch-delay.d	2012-10-22 22:15:19.711764181 +0100
@@ -0,0 +1,26 @@
+#objdump: -dr --show-raw-insn
+#name: microMIPS fixed-size branch delay slots
+#as: -mmicromips
+#source: micromips-warn-branch-delay.s
+#stderr: micromips-warn-branch-delay.l
+
+.*: +file format .*mips.*
+
+Disassembly of section \.text:
+
+[0-9a-f]+ <foo>:
+[ 0-9a-f]+:	45e2      	jalrs	v0
+[ 0-9a-f]+:	0083 1250 	and	v0,v1,a0
+[ 0-9a-f]+:	45e2      	jalrs	v0
+[ 0-9a-f]+:	6043 9000 	swr	v0,0\(v1\)
+[ 0-9a-f]+:	45e2      	jalrs	v0
+[ 0-9a-f]+:	6043 8000 	swl	v0,0\(v1\)
+[ 0-9a-f]+:	45e2      	jalrs	v0
+[ 0-9a-f]+:	0272 8210 	mul	s0,s2,s3
+[ 0-9a-f]+:	45e2      	jalrs	v0
+[ 0-9a-f]+:	001f 8b90 	sltu	s1,ra,zero
+[ 0-9a-f]+:	45e2      	jalrs	v0
+[ 0-9a-f]+:	0220 8910 	add	s1,zero,s1
+[ 0-9a-f]+:	45e2      	jalrs	v0
+[ 0-9a-f]+:	01b1 8990 	sub	s1,s1,t5
+#pass
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/micromips-warn-branch-delay.l
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/micromips-warn-branch-delay.l	2012-10-22 22:15:19.711764181 +0100
@@ -0,0 +1,8 @@
+.*: Assembler messages:
+.*:8: Warning: Wrong size instruction in a 16-bit branch delay slot
+.*:10: Warning: Wrong size instruction in a 16-bit branch delay slot
+.*:12: Warning: Wrong size instruction in a 16-bit branch delay slot
+.*:14: Warning: Wrong size instruction in a 16-bit branch delay slot
+.*:16: Warning: Wrong size instruction in a 16-bit branch delay slot
+.*:18: Warning: Wrong size instruction in a 16-bit branch delay slot
+.*:20: Warning: Wrong size instruction in a 16-bit branch delay slot
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/micromips-warn-branch-delay.s
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/micromips-warn-branch-delay.s	2012-10-22 22:15:19.711764181 +0100
@@ -0,0 +1,20 @@
+# Source file used to test microMIPS fixed-size branch delay slots.
+
+	.text
+	.set	noreorder
+	.set	noat
+foo:
+	jalrs	$2
+	 and	$2,$3,$4
+	jalrs	$2
+	 swr	$2,0($3)
+	jalrs	$2
+	 swl	$2,0($3)
+	jalrs	$2
+	 mul	$16,$18,$19
+	jalrs	$2
+	 sltu	$17,$31,$0
+	jalrs	$2
+	 add	$17,$0,$17
+	jalrs	$2
+	 sub	$17,$17,$13
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips.exp
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/testsuite/gas/mips/mips.exp	2012-10-22 22:14:46.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips.exp	2012-10-22 22:15:19.711764181 +0100
@@ -1113,6 +1113,8 @@ if { [istarget mips*-*-vxworks*] } {
 	run_dump_test "micromips-branch-relax"
 	run_dump_test "micromips-branch-relax-pic"
 	run_dump_test "micromips-branch-delay"
+	run_dump_test "micromips-warn-branch-delay"
+	run_dump_test "micromips-warn-branch-delay-1"
     }
 
     run_dump_test_arches "mcu"		[mips_arch_list_matching mips32r2 \


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