This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] MIPS/binutils: microMIPS linker relaxation fixes
- From: Richard Sandiford <rdsandiford at googlemail dot com>
- To: "Maciej W. Rozycki" <macro at codesourcery dot com>
- Cc: <binutils at sourceware dot org>, Tristan Gingold <gingold at adacore dot com>
- Date: Sun, 13 Nov 2011 11:39:14 +0000
- Subject: Re: [PATCH] MIPS/binutils: microMIPS linker relaxation fixes
- References: <alpine.DEB.1.10.1110272117590.28657@tp.orcam.me.uk>
"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