This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Update addends in non-allocatable sections for REL targets when creating a relocatable output.
ClosedPublic

Authored by ikudrin on Jul 4 2018, 3:30 AM.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

ikudrin created this revision.Jul 4 2018, 3:30 AM
grimar added inline comments.Jul 4 2018, 4:22 AM
ELF/InputSection.cpp
732

Should instead of all other changes in this patch this line just be a

(Flags & SHF_ALLOC || Config->Relocatable) ?

With the corresponding renaming/commenting of things may be.

We have relocateAlloc/relocateNonAlloc splitted mostly for speedup of
processing debug relocations I think.

I do not feel it is very critical for -r. It seems better to reuse the existent
code than to add new.

Rui, what do you think?

834

This function is a bit overloaded with excessive variables.
It could be shorter:

void InputSectionBase::relocateNonAllocForRelocatable(uint8_t *Buf, uint8_t *BufEnd) {
  for (const Relocation &Rel : Relocations) {
    // InputSection::copyRelocations() adds only R_ABS relocations.
    assert(Rel.Expr == R_ABS);
    uint8_t *BufLoc = Rel.Offset + cast<InputSection>(this)->OutSecOff + Offset;
    uint64_t TargetVA =
        SignExtend64(Rel.Sym->getVA(Rel.Addend), Config->Wordsize * 8);
    Target->relocateOne(BufLoc, Rel.Type, TargetVA);
  }
}

And then probably inlined. (If will be decided to not to reuse the code).

ikudrin added inline comments.Jul 4 2018, 4:57 AM
ELF/InputSection.cpp
732

Yes, that's possible, and, actually, the first variant of the fix was made this way. I switched to the current version because I found it more straightforward.

With the corresponding renaming/commenting of things may be.

What renaming are you suggesting?

grimar added inline comments.Jul 4 2018, 5:12 AM
ELF/InputSection.cpp
732

Maybe something like relocateAlloc->relocateAll?

ruiu added inline comments.Jul 5 2018, 10:56 AM
ELF/InputSection.cpp
817

Can this be a non-member function? I mean if you explicitly pass cast<InputSection>(this) as an argument for this function, can't you make this a non-member, static function? Non-member functions are generally preferred over member functions if they don't depend on non-public part of a class.

818

You are not using BufEnd, so please remove.

819

I believe const unsigned Bits = Config->Is64 ? 64 : 32 would produce slightly better code because by doing that, a compiler knows Bits is either 32 or 64 and not any other value.

ikudrin updated this revision to Diff 154350.Jul 5 2018, 9:23 PM
  • Shortened relocateNonAllocForRelocatable();
  • Made it static;
  • Removed an unused argument;
  • Changed an expression for Bits.
ikudrin marked 4 inline comments as done.Jul 5 2018, 9:23 PM
grimar accepted this revision.Jul 10 2018, 6:55 AM

This LGTM. The minor suggestion is below.

ELF/InputSection.cpp
744

Maybe I would make it even shorter:

const unsigned Bits = Config->Is64 ? 64 : 32;
for (const Relocation &Rel : Sec->Relocations) {
  assert(Rel.Expr == R_ABS);
  Target->relocateOne(Buf + Rel.Offset + Sec->OutSecOff, Rel.Type,
                      SignExtend64(Rel.Sym->getVA(Rel.Addend), Bits));
}
This revision is now accepted and ready to land.Jul 10 2018, 6:55 AM
ruiu accepted this revision.Jul 10 2018, 6:35 PM

LGTM

ikudrin added inline comments.Jul 11 2018, 5:05 AM
ELF/InputSection.cpp
744

Thanks for the suggestion, but I prefer to stay with the original version. I believe that the shortened variant is more complicated while giving distinct names to different things improves readability.

This revision was automatically updated to reflect the committed changes.

Note that the interaction with --compress-debug-sections={zlib,zstd} is not handled in this patch. For relocatable linking of .rel.debug_* sections, the implicit addends are not applied to the output.
I am trying to figure out a solution.

Writer<ELFT>::run
  maybeCompress
    OutputSection::writeTo
      InputSection::writeTo
        relocate  # relocations is empty
  writeSections
    OutputSection::writeTo
      InputSection::writeTo
        if SHT_REL copyRelocations
          addReloc R_ABS
        elif non-compress relocate
          relocateNonAllocForRelocatable handle R_ABS
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2023, 9:19 PM

Note that the interaction with --compress-debug-sections={zlib,zstd} is not handled in this patch. For relocatable linking of .rel.debug_* sections, the implicit addends are not applied to the output.
I am trying to figure out a solution.

Writer<ELFT>::run
  maybeCompress
    OutputSection::writeTo
      InputSection::writeTo
        relocate  # relocations is empty
  writeSections
    OutputSection::writeTo
      InputSection::writeTo
        if SHT_REL copyRelocations
          addReloc R_ABS
        elif non-compress relocate
          relocateNonAllocForRelocatable handle R_ABS

Filed https://github.com/llvm/llvm-project/issues/66738 and pushed 345f532f3fe9bd4b6d55a490683455ee542d90d9 to add a test demonstrating the issue.