Home Original page

[PATCH] MIPS/binutils: microMIPS linker relaxation fixes

Richard Sandiford rdsandiford@googlemail.com
Sun Nov 13 11:39:00 GMT 2011 
"Maciej W. Rozycki" <macro@codesourcery.com> writes:
> Index: binutils-fsf-trunk-quilt/bfd/elfxx-mips.c
> ===================================================================
> --- binutils-fsf-trunk-quilt.orig/bfd/elfxx-mips.c	2011-10-27 12:23:51.000000000 +0100
> +++ binutils-fsf-trunk-quilt/bfd/elfxx-mips.c	2011-10-27 12:39:12.865860609 +0100
> @@ -5807,6 +5807,19 @@ mips_elf_obtain_contents (reloc_howto_ty
>    return x;
>  }
>  
> +/* Release the field relocated by RELOCATION.  */
> +
> +static void
> +mips_elf_release_contents (reloc_howto_type *howto,
> +			   const Elf_Internal_Rela *relocation,
> +			   bfd *input_bfd, bfd_byte *contents, bfd_vma x)
> +{
> +  bfd_byte *location = contents + relocation->r_offset;
> +
> +  /* Release the bytes.  */
> +  bfd_put (8 * bfd_get_reloc_size (howto), input_bfd, x, location);
> +}
> +
>  /* It has been determined that the result of the RELOCATION is the
>     VALUE.  Use HOWTO to place VALUE into the output file at the
>     appropriate position.  The SECTION is the section to which the

Strange use of "release".  "put" seems better.  Change "obtain"
to "get" if you don't think "put" meshes well with "obtain".

> @@ -11879,46 +11919,124 @@ _bfd_elf_mips_get_relocated_section_cont
>    return NULL;
>  }
>  
> +static void
> +mips_elf_relax_reloc_adjust_addend (bfd *abfd, asection *sec,
> +				    bfd_vma addr, int count,
> +				    Elf_Internal_Rela *internal_relocs,
> +				    Elf_Internal_Rela *irelend,
> +				    Elf_Internal_Rela *irel)
> +{
> +  bfd_byte *contents = elf_section_data (sec)->this_hdr.contents;
> +  unsigned int r_symndx = ELF32_R_SYM (irel->r_info);
> +  unsigned int r_type = ELF32_R_TYPE (irel->r_info);
> +  static bfd_boolean got16_reloc = FALSE;
> +  static unsigned long got16_symndx;
> +  static unsigned long hi16_symndx;
> +  static bfd_vma got16_addend;
> +  static bfd_vma hi16_addend;
> +  reloc_howto_type *howto;
> +  bfd_boolean rel_reloc;
> +  bfd_vma addend;
> +
> +  rel_reloc = mips_elf_rel_relocation_p (abfd, sec, internal_relocs, irel);
> +  howto = MIPS_ELF_RTYPE_TO_HOWTO (abfd, r_type, !rel_reloc);
> +  if (!howto->src_mask)
> +    return;
> +
> +  if (rel_reloc)
> +    {
> +      addend = mips_elf_read_rel_addend (abfd, irel, howto, contents);
> +
> +      if (hi16_reloc_p (r_type))
> +	{
> +	  hi16_addend = addend << howto->rightshift;
> +	  hi16_symndx = r_symndx;
> +	}
> +      if (got16_reloc_p (r_type))
> +	{
> +	  got16_addend = addend << howto->rightshift;
> +	  got16_symndx = r_symndx;
> +	}
> +
> +      if (hi16_reloc_p (r_type) || got16_reloc_p (r_type))
> +	mips_elf_add_lo16_rel_addend (abfd, irel, irelend, contents, &addend);
> +      else if (lo16_reloc_p (r_type))
> +	{
> +	  addend = _bfd_mips_elf_sign_extend (addend << howto->rightshift, 16);
> +	  if (got16_reloc && got16_symndx == r_symndx)
> +	    addend += got16_addend;
> +	  else if (hi16_symndx == r_symndx)
> +	    addend += hi16_addend;
> +	}

I don't like this.  LO16 relocs should be independent of their high parts.
It is possible to have two LO16 relocs for the same HI16 reloc, such as:

     lui  $4,%hi(foo + const)
     lw   $5,%lo(foo + const)($4)
     ...
     sw   $5,%lo(foo + const)($4)

In this case, only one of the LO16 relocs is tied to the HI16.
The other may be placed freely, and might even come before the HI16,
e.g. due to bb reordering[*].  So I think it is wrong to construct
a combined addend for LO16s based on this kind of forward walk.

  [*] Of course, the reverse is not true.  All HI16 relocs must be tied
      to a LO16, with GNU tools allowing several HI16s to be attached to
      the same LO16.

C.f. what gas has to do for mergeable sections, which is another case
where we need to know the combined addend for a LO16 reloc:

  /* If symbol SYM is in a mergeable section, relocations of the form
     SYM + 0 can usually be made section-relative.  The mergeable data
     is then identified by the section offset rather than by the symbol.

     However, if we're generating REL LO16 relocations, the offset is split
     between the LO16 and parterning high part relocation.  The linker will
     need to recalculate the complete offset in order to correctly identify
     the merge data.

     The linker has traditionally not looked for the parterning high part
     relocation, and has thus allowed orphaned R_MIPS_LO16 relocations to be
     placed anywhere.  Rather than break backwards compatibility by changing
     this, it seems better not to force the issue, and instead keep the
     original symbol.  This will work with either linker behavior.  */
  if ((lo16_reloc_p (fixp->fx_r_type)
       || reloc_needs_lo_p (fixp->fx_r_type))
      && HAVE_IN_PLACE_ADDENDS
      && (S_GET_SEGMENT (fixp->fx_addsy)->flags & SEC_MERGE) != 0)
    return 0;

(I think the last part of the comment is misleading: there's no way
the linker could know for certain without help from the assembler.
And yes, I'm guilty of writing that comment...)

My brain is still on holiday, so I don't have any constructive ways
around this yet.  It seems on face value as though it would be better
to disable relaxation on the 2.22 branch.

I stopped here in case reviewing the rest becomes moot.

Richard
More information about the Binutils mailing list