[llvm] r255898 - Always sort by offset first. NFC.

[David Blaikie via llvm-commits]

On Thu, Dec 17, 2015 at 7:08 AM, Rafael Espindola via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> Author: rafael
> Date: Thu Dec 17 09:08:24 2015
> New Revision: 255898
>
> URL: http://llvm.org/viewvc/llvm-project?rev=255898&view=rev
> Log:
> Always sort by offset first. NFC.
>
> Every target changing sortRelocs was first calling the parent
> implementation. Just run that first.
>
> Modified:
>     llvm/trunk/lib/MC/ELFObjectWriter.cpp
>     llvm/trunk/lib/MC/MCELFObjectTargetWriter.cpp
>     llvm/trunk/lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp
>     llvm/trunk/lib/Target/SystemZ/MCTargetDesc/SystemZMCObjectWriter.cpp
>
> Modified: llvm/trunk/lib/MC/ELFObjectWriter.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/ELFObjectWriter.cpp?rev=255898&r1=255897&r2=255898&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/MC/ELFObjectWriter.cpp (original)
> +++ llvm/trunk/lib/MC/ELFObjectWriter.cpp Thu Dec 17 09:08:24 2015
> @@ -1046,10 +1046,26 @@ void ELFObjectWriter::WriteSecHdrEntry(u
>    WriteWord(EntrySize); // sh_entsize
>  }
>
> +// ELF doesn't require relocations to be in any order. We sort by the
> Offset,
> +// just to match gnu as for easier comparison. The use type is an
> arbitrary way
> +// of making the sort deterministic.
> +static int cmpRel(const ELFRelocationEntry *AP, const ELFRelocationEntry
> *BP) {
> +  const ELFRelocationEntry &A = *AP;
> +  const ELFRelocationEntry &B = *BP;
> +  if (A.Offset != B.Offset)
> +    return B.Offset - A.Offset;
> +  if (B.Type != A.Type)
> +    return A.Type - B.Type;
> +  llvm_unreachable("ELFRelocs might be unstable!");
> +  return 0;
>

Return after unreachable looks bogus.

Also return as an "else" looks a bit wrong. Perhaps the last 4 lines of
this function should be phrased as:

    assert(B.Type != A.Type)
    return A.Type - B.Type;


> +}
> +
>  void ELFObjectWriter::writeRelocations(const MCAssembler &Asm,
>                                         const MCSectionELF &Sec) {
>    std::vector<ELFRelocationEntry> &Relocs = Relocations[&Sec];
>
> +  array_pod_sort(Relocs.begin(), Relocs.end(), cmpRel);
> +
>    // Sort the relocation entries. Most targets just sort by Offset, but
> some
>    // (e.g., MIPS) have additional constraints.
>    TargetObjectWriter->sortRelocs(Asm, Relocs);
>
> Modified: llvm/trunk/lib/MC/MCELFObjectTargetWriter.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCELFObjectTargetWriter.cpp?rev=255898&r1=255897&r2=255898&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/MC/MCELFObjectTargetWriter.cpp (original)
> +++ llvm/trunk/lib/MC/MCELFObjectTargetWriter.cpp Thu Dec 17 09:08:24 2015
> @@ -29,23 +29,7 @@ bool MCELFObjectTargetWriter::needsReloc
>    return false;
>  }
>
> -// ELF doesn't require relocations to be in any order. We sort by the
> Offset,
> -// just to match gnu as for easier comparison. The use type is an
> arbitrary way
> -// of making the sort deterministic.
> -static int cmpRel(const ELFRelocationEntry *AP, const ELFRelocationEntry
> *BP) {
> -  const ELFRelocationEntry &A = *AP;
> -  const ELFRelocationEntry &B = *BP;
> -  if (A.Offset != B.Offset)
> -    return B.Offset - A.Offset;
> -  if (B.Type != A.Type)
> -    return A.Type - B.Type;
> -  //llvm_unreachable("ELFRelocs might be unstable!");
> -  return 0;
> -}
> -
> -
>  void
>  MCELFObjectTargetWriter::sortRelocs(const MCAssembler &Asm,
>                                      std::vector<ELFRelocationEntry>
> &Relocs) {
> -  array_pod_sort(Relocs.begin(), Relocs.end(), cmpRel);
>  }
>
> Modified: llvm/trunk/lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp?rev=255898&r1=255897&r2=255898&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp
> (original)
> +++ llvm/trunk/lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp Thu
> Dec 17 09:08:24 2015
> @@ -332,9 +332,6 @@ void MipsELFObjectWriter::sortRelocs(con
>    if (Relocs.size() < 2)
>      return;
>
> -  // The default function sorts entries by Offset in descending order.
> -  MCELFObjectTargetWriter::sortRelocs(Asm, Relocs);
> -
>    // Init MipsRelocs from Relocs.
>    std::vector<MipsRelocationEntry> MipsRelocs;
>    for (unsigned I = 0, E = Relocs.size(); I != E; ++I)
>
> Modified:
> llvm/trunk/lib/Target/SystemZ/MCTargetDesc/SystemZMCObjectWriter.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/SystemZ/MCTargetDesc/SystemZMCObjectWriter.cpp?rev=255898&r1=255897&r2=255898&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Target/SystemZ/MCTargetDesc/SystemZMCObjectWriter.cpp
> (original)
> +++ llvm/trunk/lib/Target/SystemZ/MCTargetDesc/SystemZMCObjectWriter.cpp
> Thu Dec 17 09:08:24 2015
> @@ -156,9 +156,6 @@ unsigned SystemZObjectWriter::GetRelocTy
>
>  void SystemZObjectWriter::sortRelocs(const MCAssembler &Asm,
>                                       std::vector<ELFRelocationEntry>
> &Relocs) {
> -  // The default function sorts entries by Offset in descending order.
> -  MCELFObjectTargetWriter::sortRelocs(Asm, Relocs);
> -
>    // This is OK for SystemZ, except for R_390_TLS_GDCALL/LDCALL relocs.
>    // There is typically another reloc, a R_390_PLT32DBL, on the same
>    // instruction.  This other reloc must come *before* the GDCALL reloc,
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160108/086c3e8b/attachment.html>