MIPS: Add `.insn' to ensure a text label is defined as code not data
diff mbox

Message ID alpine.DEB.2.20.17.1611171225000.10580@tp.orcam.me.uk
State Accepted
Headers show

Commit Message

Maciej W. Rozycki Nov. 17, 2016, 7:15 p.m. UTC
Avoid a build error with microMIPS compilation and recent versions of 
GAS which complain if a branch targets a label which is marked as data
rather than microMIPS code:

../sysdeps/mips/mips32/crti.S: Assembler messages:
../sysdeps/mips/mips32/crti.S:72: Error: branch to a symbol in another ISA mode
make[2]: *** [.../csu/crti.o] Error 1

as commit 9d862524f6ae ("MIPS: Verify the ISA mode and alignment of 
branch and jump targets") closed a hole in branch processing, making 
relocation calculation respect the ISA mode of the symbol referred.  
This allowed diagnosing the situation where an attempt is made to pass 
control from code assembled for one ISA mode to code assembled for a 
different ISA mode and either relaxing the branch to a cross-mode jump 
or if that is not possible, then reporting this as an error rather than 
letting such code build and then fail unpredictably at the run time.

This however requires the correct annotation of branch targets as code, 
because the ISA mode is not relevant for data symbols and is therefore 
not recorded for them.  The `.insn' pseudo-op is used for this purpose 
and has been supported by GAS since:

Wed Feb 12 14:36:29 1997  Ian Lance Taylor  <ian@cygnus.com>

	* config/tc-mips.c (mips_pseudo_table): Add "insn".
	(s_insn): New static function.
	* doc/c-mips.texi: Document .insn.

so there has been no reason to avoid it where required.  More recently 
this pseudo-op has been documented, by the microMIPS architecture 
specification[1][2], as required for the correct interpretation of any 
code label which is not followed by an actual instruction in an assembly 
source.

Use it in our crti.S files then, to mark that the trailing label there 
with no instructions following is indeed not a code bug and the branch 
is legitimate.

References:

[1] "MIPS Architecture for Programmers, Volume II-B: The microMIPS32
    Instruction Set", MIPS Technologies, Inc., Document Number: MD00582,
    Revision 5.04, January 15, 2014, Section 7.1 "Assembly-Level
    Compatibility", p. 533

[2] "MIPS Architecture for Programmers, Volume II-B: The microMIPS64
    Instruction Set", MIPS Technologies, Inc., Document Number: MD00594,
    Revision 5.04, January 15, 2014, Section 8.1 "Assembly-Level
    Compatibility", p. 623

2016-11-17  Matthew Fortune  <Matthew.Fortune@imgtec.com>
            Maciej W. Rozycki  <macro@imgtec.com>

	* sysdeps/mips/mips32/crti.S (_init): Add `.insn' pseudo-op at 
	`.Lno_weak_fn' label.
	* sysdeps/mips/mips64/n32/crti.S (_init): Likewise.
	* sysdeps/mips/mips64/n64/crti.S (_init): Likewise.
---
 This should qualify as obvious, however for good measure I pushed it 
through full regression testing with the `mips-mti-linux-gnu' target and 
big-endian regular MIPS o32 and n64 multilibs as well as little-endian o32 
MIPS16 and microMIPS multilibs, all with binutils 2.27 predating the extra 
branch/jump checks (or otherwise glibc wouldn't build without this patch 
for the microMIPS multilib).

 While working on this change I've noticed the R_MIPS_JALR regular MIPS 
relocations, used unconditionally even with microMIPS compilations where 
R_MICROMIPS_JALR relocations (if any) need to be used instead.  Support 
for R_MICROMIPS_JALR handling is incomplete in the BFD linker meaning 
these relocs are always ignored, however this may change sometime and we 
ought to have correct code, so I'll send a separate change to correct it, 
once it has passed regression testing with a similar set of multilibs.

 Meanwhile OK to apply this change?

  Maciej

glibc-umips-insn.diff

Comments

Joseph Myers Nov. 17, 2016, 11:22 p.m. UTC | #1
On Thu, 17 Nov 2016, Maciej W. Rozycki wrote:

> 2016-11-17  Matthew Fortune  <Matthew.Fortune@imgtec.com>
>             Maciej W. Rozycki  <macro@imgtec.com>
> 
> 	* sysdeps/mips/mips32/crti.S (_init): Add `.insn' pseudo-op at 
> 	`.Lno_weak_fn' label.
> 	* sysdeps/mips/mips64/n32/crti.S (_init): Likewise.
> 	* sysdeps/mips/mips64/n64/crti.S (_init): Likewise.

OK.  We may want to backport this to release branches.
Maciej W. Rozycki Nov. 18, 2016, 12:49 p.m. UTC | #2
On Thu, 17 Nov 2016, Joseph Myers wrote:

> > 	* sysdeps/mips/mips32/crti.S (_init): Add `.insn' pseudo-op at 
> > 	`.Lno_weak_fn' label.
> > 	* sysdeps/mips/mips64/n32/crti.S (_init): Likewise.
> > 	* sysdeps/mips/mips64/n64/crti.S (_init): Likewise.
> 
> OK.  We may want to backport this to release branches.

 Indeed and I can do it while I am at it.  How far back do you think it 
would make sense to go with it?

 Patch committed now, thanks for your review.

  Maciej
Joseph Myers Nov. 18, 2016, 5:55 p.m. UTC | #3
On Fri, 18 Nov 2016, Maciej W. Rozycki wrote:

> On Thu, 17 Nov 2016, Joseph Myers wrote:
> 
> > > 	* sysdeps/mips/mips32/crti.S (_init): Add `.insn' pseudo-op at 
> > > 	`.Lno_weak_fn' label.
> > > 	* sysdeps/mips/mips64/n32/crti.S (_init): Likewise.
> > > 	* sysdeps/mips/mips64/n64/crti.S (_init): Likewise.
> > 
> > OK.  We may want to backport this to release branches.
> 
>  Indeed and I can do it while I am at it.  How far back do you think it 
> would make sense to go with it?

The last couple (2.24 and 2.23) seem to be in reasonably active use (i.e. 
getting patches backported to them).  I don't know how likely people are 
to use any older branches with current binutils.
Maciej W. Rozycki Nov. 23, 2016, 1:41 p.m. UTC | #4
On Fri, 18 Nov 2016, Joseph Myers wrote:

> >  Indeed and I can do it while I am at it.  How far back do you think it 
> > would make sense to go with it?
> 
> The last couple (2.24 and 2.23) seem to be in reasonably active use (i.e. 
> getting patches backported to them).  I don't know how likely people are 
> to use any older branches with current binutils.

 Me neither and myself I tend to stick to the master branch.  Patch 
backported now, to both versions.

  Maciej

Patch
diff mbox

Index: glibc/sysdeps/mips/mips32/crti.S
===================================================================
--- glibc.orig/sysdeps/mips/mips32/crti.S	2016-11-17 12:38:44.330014101 +0000
+++ glibc/sysdeps/mips/mips32/crti.S	2016-11-17 15:28:32.203511190 +0000
@@ -74,6 +74,7 @@ 
 	.reloc 1f,R_MIPS_JALR,PREINIT_FUNCTION
 1:	jalr $25
 .Lno_weak_fn:
+	.insn
 #else
 	lw $25,%got(PREINIT_FUNCTION)($28)
 	.reloc 1f,R_MIPS_JALR,PREINIT_FUNCTION
Index: glibc/sysdeps/mips/mips64/n32/crti.S
===================================================================
--- glibc.orig/sysdeps/mips/mips64/n32/crti.S	2016-11-17 15:30:23.000000000 +0000
+++ glibc/sysdeps/mips/mips64/n32/crti.S	2016-11-17 15:30:48.510029889 +0000
@@ -74,6 +74,7 @@ 
 	.reloc 1f,R_MIPS_JALR,PREINIT_FUNCTION
 1:	jalr $25
 .Lno_weak_fn:
+	.insn
 #else
 	lw $25,%got_disp(PREINIT_FUNCTION)($28)
 	.reloc 1f,R_MIPS_JALR,PREINIT_FUNCTION
Index: glibc/sysdeps/mips/mips64/n64/crti.S
===================================================================
--- glibc.orig/sysdeps/mips/mips64/n64/crti.S	2016-11-17 15:30:30.000000000 +0000
+++ glibc/sysdeps/mips/mips64/n64/crti.S	2016-11-17 15:30:56.644097664 +0000
@@ -74,6 +74,7 @@ 
 	.reloc 1f,R_MIPS_JALR,PREINIT_FUNCTION
 1:	jalr $25
 .Lno_weak_fn:
+	.insn
 #else
 	ld $25,%got_disp(PREINIT_FUNCTION)($28)
 	.reloc 1f,R_MIPS_JALR,PREINIT_FUNCTION