This is an archive of the discontinued LLVM Phabricator instance.

Differential D115635

[llvm-objcopy] Fix handling of MIPS64 little endian files
ClosedPublic

Authored by atanasyan on Dec 13 2021, 7:02 AM.

Details

Reviewers
jhenderson
nathanchance
rupprecht
alexander-shaposhnikov
Commits
rGd5d8b1c972bc: [llvm-objcopy] Fix handling of MIPS64 little endian files
Summary

MIPS64 little endian target has a "special" encoding of r_info relocation record field. Instead of one 64-bit little endian number, it is a little endian 32-bit number followed by a 32-bit big endian number. For correct reading and writing such fields we must provide information about target machine into the corresponding routine. This patch does this for the llvm-objcopy tool and fix handling of MIPS64 little endian files.

The patch looks ugly but I could not figure out how to hide the nature of MIPS64 ABI better.

The bug was reported in the issue #52647.

Diff Detail

Event Timeline

atanasyan created this revision.Dec 13 2021, 7:02 AM
Herald added a reviewer: alexander-shaposhnikov. · View Herald TranscriptDec 13 2021, 7:02 AM
Herald added subscribers: abrachet, jrtc27, arichardson and 2 others. · View Herald Transcript
atanasyan requested review of this revision.Dec 13 2021, 7:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 13 2021, 7:02 AM
Herald added subscribers: llvm-commits, MaskRay. · View Herald Transcript
jhenderson added a comment.Dec 13 2021, 7:24 AM

Since the "Mips64EL" property is a propert of the input object, I wonder if we should just make it a member of the object, and then put a reference to that object in the relocation sections, so that we can query that, rather than needing to a) pass around the variable, and b) have to have the property in the relocation section? What do you think?

llvm/test/tools/llvm-objcopy/ELF/mips64el.test
5–7 ↗(On Diff #393879)

I'd name the input and output objects differently for the two cases, so that we don't overwrite the first case with the second case. This is sometimes useful for test debugging.

11–14 ↗(On Diff #393879)

We usually get rid of excessive spacing within a block, as per the suggested edit. Same below.

18–19 ↗(On Diff #393879)

I'd be surprised if you need either of these fields.

20 ↗(On Diff #393879)

I'd replace this with a Size field, if the size is important to the test case. If it isn't important, consider just getting rid of the field entirely.

24–25 ↗(On Diff #393879)

These fields are redundant (either they're the default, or simply not needed).

37–42 ↗(On Diff #393879)

Do you need this second symbol? It doesn't seem to be being used.

llvm/tools/llvm-objcopy/ELF/Object.h
800–801
Harbormaster completed remote builds in B138962: Diff 393879.Dec 13 2021, 7:46 AM
atanasyan marked 6 inline comments as done.Dec 13 2021, 9:36 AM

Since the "Mips64EL" property is a property of the input object, I wonder if we should just make it a member of the object,
and then put a reference to that object in the relocation sections, so that we can query that, rather than needing
to a) pass around the variable, and b) have to have the property in the relocation section? What do you think?

Agreed.

atanasyan updated this revision to Diff 393936.Dec 13 2021, 9:39 AM
  • Reduce redundancy of the test case
  • Add IsMips64 field to the Object class.
  • Save reference to an Object in the RelocationSection class.
nathanchance added a comment.Dec 13 2021, 10:21 AM

This resolves the Linux kernel build error, thank you for the quick fix!

Harbormaster completed remote builds in B139001: Diff 393936.Dec 13 2021, 10:48 AM
MaskRay added a comment.Dec 13 2021, 11:05 AM

LG

llvm/test/tools/llvm-objcopy/ELF/mips64.test
2

Add a comment like ## mips64el has a special encoding of the r_info relocation field. Test that we support both endianness.

jhenderson accepted this revision.Dec 14 2021, 12:27 AM

Looks good, with @MaskRay's comment.

llvm/test/tools/llvm-objcopy/ELF/mips64.test
2

I assume that yaml2obj does actually generate the r_info correctly in this case? Easiest way to tell is to make sure the test fails without this fix, if you're not sure.

This revision is now accepted and ready to land.Dec 14 2021, 12:27 AM
This revision was landed with ongoing or failed builds.Dec 14 2021, 6:21 AM
Closed by commit rGd5d8b1c972bc: [llvm-objcopy] Fix handling of MIPS64 little endian files (authored by atanasyan). · Explain Why
This revision was automatically updated to reflect the committed changes.
atanasyan added a commit: rGd5d8b1c972bc: [llvm-objcopy] Fix handling of MIPS64 little endian files.
atanasyan added a comment.Dec 14 2021, 6:22 AM

Thanks for review.

Revision Contents

PathSize
llvm/
test/
tools/
llvm-objcopy/
ELF/
36 lines
tools/
llvm-objcopy/
ELF/
9 lines
36 lines

Diff 394229

llvm/test/tools/llvm-objcopy/ELF/mips64.test

  • This file was added.
# mips64el has a special encoding of the r_info relocation field.
# Test that we support both endianness.
MaskRayUnsubmitted
Not Done
ReplyInline Actions

Add a comment like ## mips64el has a special encoding of the r_info relocation field. Test that we support both endianness.

MaskRay: Add a comment like `## mips64el has a special encoding of the r_info relocation field. Test…
jhendersonUnsubmitted
Not Done
ReplyInline Actions

I assume that yaml2obj does actually generate the r_info correctly in this case? Easiest way to tell is to make sure the test fails without this fix, if you're not sure.

jhenderson: I assume that yaml2obj does actually generate the r_info correctly in this case? Easiest way to…
# RUN: yaml2obj %s -DENDIANNESS=ELFDATA2LSB -o %t1-le
# RUN: llvm-objcopy %t1-le %t2-le
# RUN: llvm-readobj --relocations %t2-le | FileCheck %s
# RUN: yaml2obj %s -DENDIANNESS=ELFDATA2MSB -o %t1-be
# RUN: llvm-objcopy %t1-be %t2-be
# RUN: llvm-readobj --relocations %t2-be | FileCheck %s
!ELF
FileHeader:
Class: ELFCLASS64
Data: [[ENDIANNESS]]
Type: ET_REL
Machine: EM_MIPS
Sections:
- Name: .text
Type: SHT_PROGBITS
- Name: .rela.text
Type: SHT_RELA
Relocations:
- Offset: 8
Symbol: foo
Type: R_MIPS_JALR
Symbols:
- Name: foo
Type: STT_FUNC
Section: .text
# CHECK: Relocations [
# CHECK-NEXT: Section (2) .rela.text {
# CHECK-NEXT: 0x8 R_MIPS_JALR/R_MIPS_NONE/R_MIPS_NONE foo 0x0
# CHECK-NEXT: }
# CHECK-NEXT: ]

llvm/tools/llvm-objcopy/ELF/Object.h

Show First 20 Lines • Show All 777 Lines • ▼ Show 20 Linespublic:
void finalize() override; void finalize() override;
}; };
class RelocationSection class RelocationSection
: public RelocSectionWithSymtabBase<SymbolTableSection> { : public RelocSectionWithSymtabBase<SymbolTableSection> {
MAKE_SEC_WRITER_FRIEND MAKE_SEC_WRITER_FRIEND
std::vector<Relocation> Relocations; std::vector<Relocation> Relocations;
const Object &Obj;
public: public:
RelocationSection(const Object &O) : Obj(O) {}
void addRelocation(Relocation Rel) { Relocations.push_back(Rel); } void addRelocation(Relocation Rel) { Relocations.push_back(Rel); }
Error accept(SectionVisitor &Visitor) const override; Error accept(SectionVisitor &Visitor) const override;
Error accept(MutableSectionVisitor &Visitor) override; Error accept(MutableSectionVisitor &Visitor) override;
Error removeSectionReferences( Error removeSectionReferences(
bool AllowBrokenLinks, bool AllowBrokenLinks,
function_ref<bool(const SectionBase *)> ToRemove) override; function_ref<bool(const SectionBase *)> ToRemove) override;
Error removeSymbols(function_ref<bool(const Symbol &)> ToRemove) override; Error removeSymbols(function_ref<bool(const Symbol &)> ToRemove) override;
void markSymbols() override; void markSymbols() override;
void replaceSectionReferences( void replaceSectionReferences(
const DenseMap<SectionBase *, SectionBase *> &FromTo) override; const DenseMap<SectionBase *, SectionBase *> &FromTo) override;
const Object &getObject() const { return Obj; }
jhendersonUnsubmitted
Not Done
ReplyInline Actions
const DenseMap<SectionBase *, SectionBase *> &FromTo) override;
- bool IsMips64EL() const { return IsObjMips64EL; }
- void SetMips64EL(bool value) { IsObjMips64EL = value; }
+ bool isMips64EL() const { return IsObjMips64EL; }
+ void setMips64EL(bool value) { IsObjMips64EL = value; }
static bool classof(const SectionBase *S) {
jhenderson:
static bool classof(const SectionBase *S) { static bool classof(const SectionBase *S) {
if (S->OriginalFlags & ELF::SHF_ALLOC) if (S->OriginalFlags & ELF::SHF_ALLOC)
return false; return false;
return S->OriginalType == ELF::SHT_REL || S->OriginalType == ELF::SHT_RELA; return S->OriginalType == ELF::SHT_REL || S->OriginalType == ELF::SHT_RELA;
} }
}; };
// TODO: The way stripping and groups interact is complicated // TODO: The way stripping and groups interact is complicated
▲ Show 20 Lines • Show All 159 Lines • ▼ Show 20 Linesprivate:
Error initSymbolTable(SymbolTableSection *SymTab); Error initSymbolTable(SymbolTableSection *SymTab);
Error readSectionHeaders(); Error readSectionHeaders();
Error readSections(bool EnsureSymtab); Error readSections(bool EnsureSymtab);
Error findEhdrOffset(); Error findEhdrOffset();
Expected<SectionBase &> makeSection(const Elf_Shdr &Shdr); Expected<SectionBase &> makeSection(const Elf_Shdr &Shdr);
public: public:
ELFBuilder(const ELFObjectFile<ELFT> &ElfObj, Object &Obj, ELFBuilder(const ELFObjectFile<ELFT> &ElfObj, Object &Obj,
Optional<StringRef> ExtractPartition) Optional<StringRef> ExtractPartition);
: ElfFile(ElfObj.getELFFile()), Obj(Obj),
ExtractPartition(ExtractPartition) {}
Error build(bool EnsureSymtab); Error build(bool EnsureSymtab);
}; };
class BinaryReader : public Reader { class BinaryReader : public Reader {
MemoryBuffer *MemBuf; MemoryBuffer *MemBuf;
uint8_t NewSymbolVisibility; uint8_t NewSymbolVisibility;
▲ Show 20 Lines • Show All 73 Lines • ▼ Show 20 Linespublic:
uint32_t Flags; uint32_t Flags;
bool HadShdrs = true; bool HadShdrs = true;
bool MustBeRelocatable = false; bool MustBeRelocatable = false;
StringTableSection *SectionNames = nullptr; StringTableSection *SectionNames = nullptr;
SymbolTableSection *SymbolTable = nullptr; SymbolTableSection *SymbolTable = nullptr;
SectionIndexSection *SectionIndexTable = nullptr; SectionIndexSection *SectionIndexTable = nullptr;
bool IsMips64EL = false;
SectionTableRef sections() const { return SectionTableRef(Sections); } SectionTableRef sections() const { return SectionTableRef(Sections); }
iterator_range< iterator_range<
filter_iterator<pointee_iterator<std::vector<SecPtr>::const_iterator>, filter_iterator<pointee_iterator<std::vector<SecPtr>::const_iterator>,
decltype(&sectionIsAlloc)>> decltype(&sectionIsAlloc)>>
allocSections() const { allocSections() const {
return make_filter_range(make_pointee_range(Sections), sectionIsAlloc); return make_filter_range(make_pointee_range(Sections), sectionIsAlloc);
} }
Show All 39 Lines

llvm/tools/llvm-objcopy/ELF/Object.cpp

Show First 20 Lines • Show All 972 Lines • ▼ Show 20 Lines
static void setAddend(Elf_Rel_Impl<ELFT, false> &, uint64_t) {} static void setAddend(Elf_Rel_Impl<ELFT, false> &, uint64_t) {}
template <class ELFT> template <class ELFT>
static void setAddend(Elf_Rel_Impl<ELFT, true> &Rela, uint64_t Addend) { static void setAddend(Elf_Rel_Impl<ELFT, true> &Rela, uint64_t Addend) {
Rela.r_addend = Addend; Rela.r_addend = Addend;
} }
template <class RelRange, class T> template <class RelRange, class T>
static void writeRel(const RelRange &Relocations, T *Buf) { static void writeRel(const RelRange &Relocations, T *Buf, bool IsMips64EL) {
for (const auto &Reloc : Relocations) { for (const auto &Reloc : Relocations) {
Buf->r_offset = Reloc.Offset; Buf->r_offset = Reloc.Offset;
setAddend(*Buf, Reloc.Addend); setAddend(*Buf, Reloc.Addend);
Buf->setSymbolAndType(Reloc.RelocSymbol ? Reloc.RelocSymbol->Index : 0, Buf->setSymbolAndType(Reloc.RelocSymbol ? Reloc.RelocSymbol->Index : 0,
Reloc.Type, false); Reloc.Type, IsMips64EL);
++Buf; ++Buf;
} }
} }
template <class ELFT> template <class ELFT>
Error ELFSectionWriter<ELFT>::visit(const RelocationSection &Sec) { Error ELFSectionWriter<ELFT>::visit(const RelocationSection &Sec) {
uint8_t *Buf = reinterpret_cast<uint8_t *>(Out.getBufferStart()) + Sec.Offset; uint8_t *Buf = reinterpret_cast<uint8_t *>(Out.getBufferStart()) + Sec.Offset;
if (Sec.Type == SHT_REL) if (Sec.Type == SHT_REL)
writeRel(Sec.Relocations, reinterpret_cast<Elf_Rel *>(Buf)); writeRel(Sec.Relocations, reinterpret_cast<Elf_Rel *>(Buf),
Sec.getObject().IsMips64EL);
else else
writeRel(Sec.Relocations, reinterpret_cast<Elf_Rela *>(Buf)); writeRel(Sec.Relocations, reinterpret_cast<Elf_Rela *>(Buf),
Sec.getObject().IsMips64EL);
return Error::success(); return Error::success();
} }
Error RelocationSection::accept(SectionVisitor &Visitor) const { Error RelocationSection::accept(SectionVisitor &Visitor) const {
return Visitor.visit(*this); return Visitor.visit(*this);
} }
Error RelocationSection::accept(MutableSectionVisitor &Visitor) { Error RelocationSection::accept(MutableSectionVisitor &Visitor) {
▲ Show 20 Lines • Show All 387 Lines • ▼ Show 20 LinesExpected<std::unique_ptr<Object>> IHexELFBuilder::build() {
addSymTab(StrTab); addSymTab(StrTab);
if (Error Err = initSections()) if (Error Err = initSections())
return std::move(Err); return std::move(Err);
addDataSections(); addDataSections();
return std::move(Obj); return std::move(Obj);
} }
template <class ELFT>
ELFBuilder<ELFT>::ELFBuilder(const ELFObjectFile<ELFT> &ElfObj, Object &Obj,
Optional<StringRef> ExtractPartition)
: ElfFile(ElfObj.getELFFile()), Obj(Obj),
ExtractPartition(ExtractPartition) {
Obj.IsMips64EL = ElfFile.isMips64EL();
}
template <class ELFT> void ELFBuilder<ELFT>::setParentSegment(Segment &Child) { template <class ELFT> void ELFBuilder<ELFT>::setParentSegment(Segment &Child) {
for (Segment &Parent : Obj.segments()) { for (Segment &Parent : Obj.segments()) {
// Every segment will overlap with itself but we don't want a segment to // Every segment will overlap with itself but we don't want a segment to
// be its own parent so we avoid that situation. // be its own parent so we avoid that situation.
if (&Child != &Parent && segmentOverlapsSegment(Child, Parent)) { if (&Child != &Parent && segmentOverlapsSegment(Child, Parent)) {
// We want a canonical "most parental" segment but this requires // We want a canonical "most parental" segment but this requires
// inspecting the ParentSegment. // inspecting the ParentSegment.
if (compareSegmentsByOffset(&Parent, &Child)) if (compareSegmentsByOffset(&Parent, &Child))
▲ Show 20 Lines • Show All 225 Lines • ▼ Show 20 Lines
static void getAddend(uint64_t &, const Elf_Rel_Impl<ELFT, false> &) {} static void getAddend(uint64_t &, const Elf_Rel_Impl<ELFT, false> &) {}
template <class ELFT> template <class ELFT>
static void getAddend(uint64_t &ToSet, const Elf_Rel_Impl<ELFT, true> &Rela) { static void getAddend(uint64_t &ToSet, const Elf_Rel_Impl<ELFT, true> &Rela) {
ToSet = Rela.r_addend; ToSet = Rela.r_addend;
} }
template <class T> template <class T>
static Error initRelocations(RelocationSection *Relocs, static Error initRelocations(RelocationSection *Relocs, T RelRange) {
SymbolTableSection *SymbolTable, T RelRange) {
for (const auto &Rel : RelRange) { for (const auto &Rel : RelRange) {
Relocation ToAdd; Relocation ToAdd;
ToAdd.Offset = Rel.r_offset; ToAdd.Offset = Rel.r_offset;
getAddend(ToAdd.Addend, Rel); getAddend(ToAdd.Addend, Rel);
ToAdd.Type = Rel.getType(false); ToAdd.Type = Rel.getType(Relocs->getObject().IsMips64EL);
if (uint32_t Sym = Rel.getSymbol(false)) { if (uint32_t Sym = Rel.getSymbol(Relocs->getObject().IsMips64EL)) {
if (!SymbolTable) if (!Relocs->getObject().SymbolTable)
return createStringError( return createStringError(
errc::invalid_argument, errc::invalid_argument,
"'" + Relocs->Name + "': relocation references symbol with index " + "'" + Relocs->Name + "': relocation references symbol with index " +
Twine(Sym) + ", but there is no symbol table"); Twine(Sym) + ", but there is no symbol table");
Expected<Symbol *> SymByIndex = SymbolTable->getSymbolByIndex(Sym); Expected<Symbol *> SymByIndex =
Relocs->getObject().SymbolTable->getSymbolByIndex(Sym);
if (!SymByIndex) if (!SymByIndex)
return SymByIndex.takeError(); return SymByIndex.takeError();
ToAdd.RelocSymbol = *SymByIndex; ToAdd.RelocSymbol = *SymByIndex;
} }
Relocs->addRelocation(ToAdd); Relocs->addRelocation(ToAdd);
} }
Show All 28 LinesExpected<SectionBase &> ELFBuilder<ELFT>::makeSection(const Elf_Shdr &Shdr) {
case SHT_REL: case SHT_REL:
case SHT_RELA: case SHT_RELA:
if (Shdr.sh_flags & SHF_ALLOC) { if (Shdr.sh_flags & SHF_ALLOC) {
if (Expected<ArrayRef<uint8_t>> Data = ElfFile.getSectionContents(Shdr)) if (Expected<ArrayRef<uint8_t>> Data = ElfFile.getSectionContents(Shdr))
return Obj.addSection<DynamicRelocationSection>(*Data); return Obj.addSection<DynamicRelocationSection>(*Data);
else else
return Data.takeError(); return Data.takeError();
} }
return Obj.addSection<RelocationSection>(); return Obj.addSection<RelocationSection>(Obj);
case SHT_STRTAB: case SHT_STRTAB:
// If a string table is allocated we don't want to mess with it. That would // If a string table is allocated we don't want to mess with it. That would
// mean altering the memory image. There are no special link types or // mean altering the memory image. There are no special link types or
// anything so we can just use a Section. // anything so we can just use a Section.
if (Shdr.sh_flags & SHF_ALLOC) { if (Shdr.sh_flags & SHF_ALLOC) {
if (Expected<ArrayRef<uint8_t>> Data = ElfFile.getSectionContents(Shdr)) if (Expected<ArrayRef<uint8_t>> Data = ElfFile.getSectionContents(Shdr))
return Obj.addSection<Section>(*Data); return Obj.addSection<Section>(*Data);
else else
▲ Show 20 Lines • Show All 164 Lines • ▼ Show 20 Lines if (auto RelSec = dyn_cast<RelocationSection>(&Sec)) {
const typename ELFFile<ELFT>::Elf_Shdr *Shdr = const typename ELFFile<ELFT>::Elf_Shdr *Shdr =
Sections->begin() + RelSec->Index; Sections->begin() + RelSec->Index;
if (RelSec->Type == SHT_REL) { if (RelSec->Type == SHT_REL) {
Expected<typename ELFFile<ELFT>::Elf_Rel_Range> Rels = Expected<typename ELFFile<ELFT>::Elf_Rel_Range> Rels =
ElfFile.rels(*Shdr); ElfFile.rels(*Shdr);
if (!Rels) if (!Rels)
return Rels.takeError(); return Rels.takeError();
if (Error Err = initRelocations(RelSec, Obj.SymbolTable, *Rels)) if (Error Err = initRelocations(RelSec, *Rels))
return Err; return Err;
} else { } else {
Expected<typename ELFFile<ELFT>::Elf_Rela_Range> Relas = Expected<typename ELFFile<ELFT>::Elf_Rela_Range> Relas =
ElfFile.relas(*Shdr); ElfFile.relas(*Shdr);
if (!Relas) if (!Relas)
return Relas.takeError(); return Relas.takeError();
if (Error Err = initRelocations(RelSec, Obj.SymbolTable, *Relas)) if (Error Err = initRelocations(RelSec, *Relas))
return Err; return Err;
} }
} else if (auto GroupSec = dyn_cast<GroupSection>(&Sec)) { } else if (auto GroupSec = dyn_cast<GroupSection>(&Sec)) {
if (Error Err = initGroupSection(GroupSec)) if (Error Err = initGroupSection(GroupSec))
return Err; return Err;
} }
} }
▲ Show 20 Lines • Show All 918 Lines • Show Last 20 Lines
  翻译：