Re: [PATCH] MIPS/binutils: microMIPS linker relaxation fixes

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

Hi Richard,

 Thanks for this initial review.  Further notes below.

> > @@ -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".

 I'll change both then.  I tried to find a good antonym to "obtain", but 
obviously my English sucks. :(

> > @@ -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.

 Here's what the MIPS ABI says about this:

"The AHL addend is a composite computed from the addends of two 
consecutive relocation entries.  Each relocation type of R_MIPS_HI16 must 
have an associated R_MIPS_LO16 entry immediately following it in the list 
of relocations.

"These relocation entries are always processed as a pair and both addend 
fields contribute to the AHL addend.  If AHI and ALO are the addends from 
the paired R_MIPS_HI16 and R_MIPS_LO16 entries, then the addend AHL is 
computed as (AHI << 16) + (short)ALO.  R_MIPS_LO16 entries without an 
R_MIPS_HI16 entry immediately preceding are orphaned and the previously 
defined R_MIPS_HI16 is used for computing the addend."

My calculation is I believe therefore correct -- using "the previously 
defined R_MIPS_HI16."

 If GAS produces any R_MIPS_LO16 relocations without a preceding 
R_MIPS_HI16 relocation, then if any R_MIPS_HI16 relocations at all are 
present in the section concerned in the assembly unit processed, then I 
think it is a bug in GAS -- it should reorder the orphaned R_MIPS_LO16 
relocation to come after an applicable R_MIPS_HI16 relocation.  Obviously 
if there are no R_MIPS_HI16 relocations in the assembly unit at all, then 
the R_MIPS_LO16 relocation has to be truly orphaned, but then the 
calculation does not matter anyway.

 But I agree we have a problem, whether by accident or by deliberately 
being relaxed on ABI implementation.

> 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...)

 That does not help with offsetted symbols anyway, does it?  They need to 
be handled correctly here, while this may be a non-issue with mergeable 
sections (that I reckon are typically used for strings and thus hardly 
ever used with offsetted expressions).

> 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 hope you had a good time.  Yes, that may make sense given the doubts.  
Let's resolve it on trunk for the following release.

 So I have actually given it some more thought and my understanding of the 
ABI remains that while orphaned R_MIPS_LO16 relocations are indeed 
permitted, they still must be preceded by a corresponding R_MIPS_HI16, 
although that is not required to be adjacent.  I believe this is only 
permitted to allow cases like you quoted to avoid unnecessary extra code 
to add missing R_MIPS_HI16 relocations.

 Based on that I think we should actually bite the bullet and make GAS ABI 
compliant.  For backwards compatibility we should probably emit an ELF 
note too to let LD know an object has been produced by a good version of 
GAS and refrain from relaxation if such a note hasn't been found in any of 
the objects being processed.

 Do you have a better idea?

  Maciej