Re: More problems with MIPS gas relocations

[cgd at broadcom dot com]

r.sandiford@redhat.com ("Richard Sandiford") writes:
> A few weeks back, HJ reported a problem with the way that MIPS gas's
> md_apply_fix handles ELF relocations:
> 
> 	<http://sources.redhat.com/ml/binutils/2001-06/msg00099.html>
>
> The same hunk of code causes the following to be misassembled, but
> for different reasons:

Is that due to a change in the code made over the last few months, or
is it as "historical issue"?


I found another case over the last couple of days where the addition
of checks for extern in that code:

>   if (fixP->fx_addsy != NULL && OUTPUT_FLAVOR == bfd_target_elf_flavour)
>     {
>       if (S_GET_OTHER (fixP->fx_addsy) == STO_MIPS16
> 	  || ((S_IS_WEAK (fixP->fx_addsy)
> 	       || S_IS_EXTERN (fixP->fx_addsy))
> 	      && !S_IS_COMMON (fixP->fx_addsy))

	      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

(this bit), which was changed with the patch:

001-06-08  H.J. Lu  <hjl@gnu.org>

        * config/tc-mips.c (md_apply_fix): Don't adjust common
        extern/weak symbols for ELF.
        (md_estimate_size_before_relax): Treat weak like extern for
        ELF.
        (mips_fix_adjustable): Don't adjust extern/weak symbols for
        ELF.

fails to work.  namely, PC-relative branches generated with
-membedded-pic (on mips-elf, mips64-elf, etc.).  For reference & later
discussion, i've included the entire contents of that patch at the
bottom of this message.


Example source code like:

	.text
	.set noreorder

	nop
	.globl	g1
g1:
l1:
	nop
	nop
	nop

	nop
	nop
	.globl	g2
g2:
l2:
	nop
	nop


	nop
	jal	g1		# R_MIPS_GNU_REL16_S2	.text ?
	nop
	jal	g2		# R_MIPS_GNU_REL16_S2	.text ?
	nop
[ ... ]

when compiled with -membedded-pic used to produce code/relocations like:

0+0000024 R_MIPS_GNU_REL16_S2  \.text
0+000002c R_MIPS_GNU_REL16_S2  \.text

 0020 00000000 04110000 00000000 04110005  .*

("bal"s with 0 or 5 in the offset field.)

i.e. the reloc value 'v' in the instruction was relative to the text
section.  (to get the address, you'd add address of .text + (v + 1) * 4).

In the current code, that produces relocs like:

0+0000024 R_MIPS_GNU_REL16_S2  g1

_with the value in the offset field of the instruction the same_.

Obviously, the linker gets horribly confused by this.  (The linker
also seems to have one other problems w.r.t. -membedded-pic in ELF,
but they're easily fixed and I plan to submit a change for that
soon-ish.)

I figure, the old way was correct, or, if the new way of handling
external symbols really is correct, the reloc against the external
sybol with a -1 (0xffff) value in the instruction also works.  I
accomplished that by changing the code:


> 	  || (symbol_used_in_reloc_p (fixP->fx_addsy)
> 	      && (((bfd_get_section_flags (stdoutput,
> 					   S_GET_SEGMENT (fixP->fx_addsy))
> 		    & SEC_LINK_ONCE) != 0)
> 		  || !strncmp (segment_name (S_GET_SEGMENT (fixP->fx_addsy)),
> 			       ".gnu.linkonce",
> 			       sizeof (".gnu.linkonce") - 1))))
> 
> 	{
> 	  valueT symval = S_GET_VALUE (fixP->fx_addsy);
> 	  value -= symval;
> 	  if (value != 0 && ! fixP->fx_pcrel)

			    ^^^^^^^^^^^^^^^^

to omit that check.

I was planning to submit a more detailed explanation later in the day,
but figured a prompt response to your query might be good.


Looking at the 2001-06-08 patch, included below, I'm actually a bit
curious as to whether or not _other_ parts of it are the Right Thing.


Obviously, the first hunk has caused me (and possibly you) problems.

I've not looked to understand when exactly OBJ_ELF is defined, but for
the last two, I wonder if they should also check (OUTPUT_FLAVOR ==
bfd_target_elf_flavour).  E.g. other code in mips_fix_adjustable()
that's #ifdef'd under OBJ_ELF checks that ELF is being output... so
either the mips_fix_adjustable change in the patch below changed
behaviour for non-ELF, or the other bit of mips_fix_adjustable is
wrong.  8-)


chris
====
Index: tc-mips.c
===================================================================
RCS file: /cvs/src/src/gas/config/tc-mips.c,v
retrieving revision 1.44
retrieving revision 1.45
diff -c -r1.44 -r1.45
*** tc-mips.c	2001/06/08 06:07:13	1.44
--- tc-mips.c	2001/06/09 03:28:04	1.45
***************
*** 9538,9544 ****
    if (fixP->fx_addsy != NULL && OUTPUT_FLAVOR == bfd_target_elf_flavour)
      {
        if (S_GET_OTHER (fixP->fx_addsy) == STO_MIPS16
! 	  || S_IS_WEAK (fixP->fx_addsy)
  	  || (symbol_used_in_reloc_p (fixP->fx_addsy)
  	      && (((bfd_get_section_flags (stdoutput,
  					   S_GET_SEGMENT (fixP->fx_addsy))
--- 9538,9546 ----
    if (fixP->fx_addsy != NULL && OUTPUT_FLAVOR == bfd_target_elf_flavour)
      {
        if (S_GET_OTHER (fixP->fx_addsy) == STO_MIPS16
! 	  || ((S_IS_WEAK (fixP->fx_addsy)
! 	       || S_IS_EXTERN (fixP->fx_addsy))
! 	      && !S_IS_COMMON (fixP->fx_addsy))
  	  || (symbol_used_in_reloc_p (fixP->fx_addsy)
  	      && (((bfd_get_section_flags (stdoutput,
  					   S_GET_SEGMENT (fixP->fx_addsy))
***************
*** 11024,11031 ****
  		&& ! bfd_is_com_section (symsec)
  		&& !linkonce
  #ifdef OBJ_ELF
! 		/* A weak symbol is treated as external.  */
! 		&& ! S_IS_WEAK (sym)
  #endif
  		);
      }
--- 11026,11033 ----
  		&& ! bfd_is_com_section (symsec)
  		&& !linkonce
  #ifdef OBJ_ELF
! 		/* A global or weak symbol is treated as external.  */
! 		&& ! (S_IS_EXTERN (sym) || S_IS_WEAK (sym))
  #endif
  		);
      }
***************
*** 11063,11068 ****
--- 11065,11075 ----
  mips_fix_adjustable (fixp)
       fixS *fixp;
  {
+ #ifdef OBJ_ELF
+   /* Prevent all adjustments to global symbols.  */
+   if (S_IS_EXTERN (fixp->fx_addsy) || S_IS_WEAK (fixp->fx_addsy))
+     return 0;
+ #endif
    if (fixp->fx_r_type == BFD_RELOC_MIPS16_JMP)
      return 0;
    if (fixp->fx_r_type == BFD_RELOC_VTABLE_INHERIT