[PATCH] MIPS/GAS: Fix ISA bit handling in local symbol assignments

["Maciej W. Rozycki" <macro at codesourcery dot com>]

Hi,

 This issue has escaped me because we (Mentor) have a local change in the 
CodeBench toolchain corresponding to the proposal archived here:

http://sourceware.org/ml/binutils/2011-10/msg00274.html

that covers the bug by deferring relocation processing for local symbols 
to the link stage.  Thanks to Robert for making Mentor aware of this 
problem.

 We have a problem with relocations against microMIPS local function 
symbols that have been created as aliases to a global symbol.  This is a 
common scenario with GCC-generated code for static functions, e.g. code 
like this (slightly edited for brevity):

	.section .mdebug.abi32
	.previous
	.nan	legacy
	.module	fp=32
	.module	oddspreg
	.abicalls
	.text
$Ltext0:
	.cfi_sections	.debug_frame
#APP
	memmove = __GI_memmove
	memset = __GI_memset
	memcpy = __GI_memcpy
	__divdi3 = __divdi3_internal
	__udivdi3 = __udivdi3_internal
	__moddi3 = __moddi3_internal
	__umoddi3 = __umoddi3_internal
#NO_APP
	.align	2
	.globl	__libc_fini
$LFB25 = .
	.file 1 "soinit.c"
	.loc 1 37 0
	.cfi_startproc
	.set	nomips16
	.set	micromips
	.ent	__libc_fini
	.type	__libc_fini, @function
__libc_fini:
	.frame	$sp,0,$31		# vars= 0, regs= 0/0, args= 0, gp= 0
	.mask	0x00000000,0
	.fmask	0x00000000,0
	.set	noreorder
	.set	nomacro
$LVL0 = .
	jrc	$31
	.set	macro
	.set	reorder
	.end	__libc_fini
	.cfi_endproc
$LFE25:
	.size	__libc_fini, .-__libc_fini
	__libc_fini.localalias.0 = __libc_fini
	.align	2
	.globl	__libc_global_ctors
$LFB27 = .
	.cfi_startproc
	.set	nomips16
	.set	micromips
	.ent	__libc_global_ctors
	.type	__libc_global_ctors, @function
__libc_global_ctors:
	.frame	$sp,0,$31		# vars= 0, regs= 0/0, args= 0, gp= 0
	.mask	0x00000000,0
	.fmask	0x00000000,0
	.set	noreorder
	.cpload	$25
	.set	nomacro
	lw	$25,%got(__libc_fini.localalias.0)($28)
	addiu	$25,$25,%lo(__libc_fini.localalias.0)
	jrc	$25
	.set	macro
	.set	reorder
	.end	__libc_global_ctors
	.cfi_endproc
$LFE27:
	.size	__libc_global_ctors, .-__libc_global_ctors
	.globl	_fini_ptr
	.section	.fini_array,"aw"
	.align	2
	.type	_fini_ptr, @object
	.size	_fini_ptr, 4
_fini_ptr:
	.word	__libc_fini
	.section	.dtors,"a",@progbits
	.align	2
	.type	__DTOR_LIST__, @object
	.size	__DTOR_LIST__, 4
__DTOR_LIST__:
	.word	-1
	.section	.ctors,"a",@progbits
	.align	2
	.type	__CTOR_LIST__, @object
	.size	__CTOR_LIST__, 4
__CTOR_LIST__:
	.word	-1
	.text
$Letext0:

is produced from elf/soinit.c in glibc sources where the corresponding 
source code is like this (again, edited a bit):

static void (*const __CTOR_LIST__[1]) (void)
  __attribute__ ((used, section (".ctors")))
  = { (void (*) (void)) -1 };
static void (*const __DTOR_LIST__[1]) (void)
  __attribute__ ((used, section (".dtors")))
  = { (void (*) (void)) -1 };

static inline void
run_hooks (void (*const list[]) (void))
{
  while (*++list)
    (**list) ();
}

/* This function will be called from _init in init-first.c.  */
void
__libc_global_ctors (void)
{
  /* Call constructor functions.  */
  run_hooks (__CTOR_LIST__);
}


/* This function becomes the DT_FINI termination function
   for the C library.  */
void
__libc_fini (void)
{
  /* Call destructor functions.  */
  run_hooks (__DTOR_LIST__);
}

void (*_fini_ptr) (void) __attribute__ ((section (".fini_array")))
     = &__libc_fini;

 Here `__libc_fini.localalias.0' is set to equal `__libc_fini' as a local 
alias and its ISA bit is lost as relocations are reduced to section 
symbols in `adjust_reloc_syms', because the section symbol does not have 
the STO_MICROMIPS flag set in its `st_other' field as the original symbol 
did.  As a result the function is called from `__libc_global_ctors' as 
standard MIPS code and it crashes with SIGILL.  As this happens early on 
in `ld.so.1' startup no dynamically linked microMIPS program works.

 The bit would be set by GAS in `mips_compressed_mark_label' if 
`__libc_fini.localalias.0' was set directly as a label.  Furthermore it 
would be set for `__libc_fini' even and consequently 
`__libc_fini.localalias.0' if the former symbol was only declared global 
after the definition, i.e. if the `.globl' directive was moved below.  
That makes the handling of global symbols inconsistent, they have the ISA 
bit set in processing or not depending on where the directive to make them 
global has been placed in the assembly source.  The bit is going to be 
cleared anyway in `adjust_symtab' before entering the final symbol table, 
as per the comment for `mips_compressed_mark_label'.

 Therefore I think the obvious fix is to set the ISA bit unconditionally 
in `mips_compressed_mark_label', also for global, weak and linkonce 
symbols, in case such a symbol is aliased to with a local symbol.  This is 
implemented below, fixed the problem observed with `ld.so.1'.  A test case 
is included covering the issue and making sure the immediate offset is 
calculated right both for a local and a global reference and that the 
value of the corresponding symbols is unchanged in the symbol table (i.e. 
the ISA bit is still clear there despite the change to 
`mips_compressed_mark_label').

 The problem does not affect MIPS16 symbols because relocations against 
such symbols are never reduced, for other reasons.  This is probably why 
the bug has survived for so long, code in `mips_compressed_mark_label' 
predates the addition of microMIPS support and can be traced back to 2006 
and `mips16_mark_labels', which is how the function was called back then.

 Regression-tested against my usual MIPS targets; also with the GCC and 
G++ test suites.  OK to apply to trunk and 2.25?

2014-12-22  Maciej W. Rozycki  <macro@codesourcery.com>

	gas/
	* config/tc-mips.c (mips_compressed_mark_label): Always set the
	ISA bit.

	gas/testsuite/
	* gas/mips/micromips-localalias.d: New test.
	* gas/mips/micromips-localalias.s: New test source.
	* gas/mips/mips.exp: Run the new test.

  Maciej

binutils-gas-mips-compressed-mark-label-isa-bit.diff
Index: binutils-fsf-trunk-quilt/gas/config/tc-mips.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/config/tc-mips.c	2014-12-22 04:56:36.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/config/tc-mips.c	2014-12-22 05:41:36.177536456 +0000
@@ -4250,21 +4250,17 @@ s_is_linkonce (symbolS *sym, segT from_s
 static void
 mips_compressed_mark_label (symbolS *label)
 {
+  valueT v;
+
   gas_assert (HAVE_CODE_COMPRESSION);
 
   if (mips_opts.mips16)
     S_SET_OTHER (label, ELF_ST_SET_MIPS16 (S_GET_OTHER (label)));
   else
     S_SET_OTHER (label, ELF_ST_SET_MICROMIPS (S_GET_OTHER (label)));
-  if ((S_GET_VALUE (label) & 1) == 0
-      /* Don't adjust the address if the label is global or weak, or
-	 in a link-once section, since we'll be emitting symbol reloc
-	 references to it which will be patched up by the linker, and
-	 the final value of the symbol may or may not be MIPS16/microMIPS.  */
-      && !S_IS_WEAK (label)
-      && !S_IS_EXTERNAL (label)
-      && !s_is_linkonce (label, now_seg))
-    S_SET_VALUE (label, S_GET_VALUE (label) | 1);
+  v = S_GET_VALUE (label);
+  if ((v & 1) == 0)
+    S_SET_VALUE (label, v | 1);
 }
 
 /* Mark preceding MIPS16 or microMIPS instruction labels.  */
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/micromips-localalias.d
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/micromips-localalias.d	2014-12-22 13:19:22.627805154 +0000
@@ -0,0 +1,40 @@
+#objdump: -drt --prefix-addresses
+#as: -32
+#name: microMIPS local label alias
+
+.*: +file format .*mips.*
+
+SYMBOL TABLE:
+00000000 l    d  \.text	00000000 \.text
+#...
+00000000 l     F \.text	00000002 0x80 foo\.localalias
+#...
+00000000 g     F \.text	00000002 0x80 foo
+#...
+
+Disassembly of section \.text:
+00000000 <[^>]*> jrc	ra
+00000002 <[^>]*> nop
+00000004 <[^>]*> lui	gp,0x0
+			4: R_MICROMIPS_HI16	_gp_disp
+00000008 <[^>]*> addiu	gp,gp,0
+			8: R_MICROMIPS_LO16	_gp_disp
+0000000c <[^>]*> addu	gp,gp,t9
+00000010 <[^>]*> lw	t9,0\(gp\)
+			10: R_MICROMIPS_GOT16	\.text
+00000014 <[^>]*> addiu	t9,t9,1
+			14: R_MICROMIPS_LO16	\.text
+00000018 <[^>]*> jrc	t9
+0000001a <[^>]*> nop
+0000001c <[^>]*> lui	gp,0x0
+			1c: R_MICROMIPS_HI16	_gp_disp
+00000020 <[^>]*> addiu	gp,gp,0
+			20: R_MICROMIPS_LO16	_gp_disp
+00000024 <[^>]*> addu	gp,gp,t9
+00000028 <[^>]*> lw	t9,0\(gp\)
+			28: R_MICROMIPS_GOT16	foo
+0000002c <[^>]*> addiu	t9,t9,0
+			2c: R_MICROMIPS_LO16	foo
+00000030 <[^>]*> jrc	t9
+00000032 <[^>]*> nop
+	\.\.\.
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/micromips-localalias.s
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/micromips-localalias.s	2014-12-22 12:38:54.367684661 +0000
@@ -0,0 +1,55 @@
+	.abicalls
+	.text
+	.align	2
+	.globl	foo
+	.set	micromips
+	.ent	foo
+	.type	foo, @function
+foo:
+	.frame	$sp,0,$31
+	.mask	0x00000000,0
+	.fmask	0x00000000,0
+	jrc	$31
+	.end	foo
+	.size	foo, . - foo
+	foo.localalias = foo
+
+	.align	2
+	.globl	bar
+	.set	micromips
+	.ent	bar
+	.type	bar, @function
+bar:
+	.frame	$sp,0,$31
+	.mask	0x00000000,0
+	.fmask	0x00000000,0
+	.set	noreorder
+	.cpload	$25
+	lw	$25,%got(foo.localalias)($28)
+	addiu	$25,$25,%lo(foo.localalias)
+	jrc	$25
+	.set	reorder
+	.end	bar
+	.size	bar, . - bar
+
+	.align	2
+	.globl	baz
+	.set	micromips
+	.ent	baz
+	.type	baz, @function
+baz:
+	.frame	$sp,0,$31
+	.mask	0x00000000,0
+	.fmask	0x00000000,0
+	.set	noreorder
+	.cpload	$25
+	lw	$25,%got(foo)($28)
+	addiu	$25,$25,%lo(foo)
+	jrc	$25
+	.set	reorder
+	.end	baz
+	.size	baz, . - baz
+
+# Force at least 8 (non-delay-slot) zero bytes, to make 'objdump' print ...
+	.align	2
+	.space	8
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips.exp
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/testsuite/gas/mips/mips.exp	2014-12-22 04:56:36.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips.exp	2014-12-22 05:41:36.177536456 +0000
@@ -1192,6 +1192,7 @@ if { [istarget mips*-*-vxworks*] } {
     run_dump_test "micromips-warn-branch-delay-1"
     run_dump_test "micromips-b16"
     run_list_test "micromips-ill"
+    run_dump_test "micromips-localalias"
 
     run_dump_test_arches "mcu"		[mips_arch_list_matching mips32r2 \
 					    !octeon]