Page MenuHomePhabricator

[llvm-objdump] - Print relocation record in a GNU format.
ClosedPublic

Authored by grimar on Apr 30 2019, 7:13 AM.

Details

Summary

This fixes the https://bugs.llvm.org/show_bug.cgi?id=41355.

Previously with -r we printed relocation section name instead of the target section name.
It was like this: "RELOCATION RECORDS FOR [.rel.text]"
Now it is: "RELOCATION RECORDS FOR [.text]"

Also when relocation target section has more than one relocation section,
we did not combine the output. Now we do.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Apr 30 2019, 7:13 AM
rupprecht accepted this revision.Apr 30 2019, 3:46 PM

lgtm, thanks!

This revision is now accepted and ready to land.Apr 30 2019, 3:46 PM
jhenderson added inline comments.May 1 2019, 1:42 AM
include/llvm/Object/ObjectFile.h
426 ↗(On Diff #197321)

I'm not sure I understand what the purpose of this check and the similar ones below is? In what cases could SectionPimpl ever equal Other.SectionPimpl and need disambugiating on the OwningObject?

test/tools/llvm-objdump/relocations-elf.test
78 ↗(On Diff #197321)

This is not a nice error. Can anything be done easily to improve it?

MaskRay added a subscriber: MaskRay.May 1 2019, 1:53 AM
MaskRay added inline comments.
test/tools/llvm-objdump/relocations-elf.test
78 ↗(On Diff #197321)

This message is associated with

static ManagedStatic<_object_error_category> error_category;

const std::error_category &object::object_category() {
  return *error_category;
}

I think more ErrorOr in lib/Object should migrate to Expected/Error to fix this. That improvement is not related to this patch.

grimar marked 2 inline comments as done.May 1 2019, 3:21 AM
grimar added inline comments.
include/llvm/Object/ObjectFile.h
426 ↗(On Diff #197321)

In this patch I had to implement DenseMapInfo<object::SectionRef>
and particulary the getEmptyKey():

template <> struct DenseMapInfo<object::SectionRef> {
...
  static object::SectionRef getEmptyKey() {
    return object::SectionRef({}, nullptr);
  }

In this case OwningObject == nullptr and SectionPimpl is zeroed.

Problem was observer in the following test cases:

    LLVM :: tools/llvm-objdump/AArch64/macho-reloc-addend.test
    LLVM :: tools/llvm-objdump/ARM/macho-reloc-half.test

Assertion failed: !KeyInfoT::isEqual(Val, EmptyKey) && !KeyInfoT::isEqual(Val, T
ombstoneKey) && "Empty/Tombstone value shouldn't be inserted into map!"

It is coming from MachO implementation of section_begin and section_end:

section_iterator MachOObjectFile::section_begin() const {
  DataRefImpl DRI;
  return section_iterator(SectionRef(DRI, this));
}

section_iterator MachOObjectFile::section_end() const {
  DataRefImpl DRI;
  DRI.d.a = Sections.size();
  return section_iterator(SectionRef(DRI, this));
}

SectionRef created by section_begin has SectionPimpl (DataRefImpl) that is zeroed.
The only way to distinguish it from empty key is to check the owning object.

BTW, the same would probably be observed for Wasm:

section_iterator WasmObjectFile::section_begin() const {
  DataRefImpl Ref;
  Ref.d.a = 0;
  return section_iterator(SectionRef(Ref, this));
}

section_iterator WasmObjectFile::section_end() const {
  DataRefImpl Ref;
  Ref.d.a = Sections.size();
  return section_iterator(SectionRef(Ref, this));
}
test/tools/llvm-objdump/relocations-elf.test
78 ↗(On Diff #197321)

It is coming from llvm/Object/ELFObjectFile.h:
https://github.com/llvm-mirror/llvm/blob/master/include/llvm/Object/ELFObjectFile.h#L844

auto R = EF.getSection(EShdr->sh_info);
if (!R)
  report_fatal_error(errorToErrorCode(R.takeError()).message());

Which finally calls report_fatal_error which is LLVM_ATTRIBUTE_NORETURN.
So answering your question - I do not see a easy way to improve it in this patch.

MaskRay added inline comments.May 1 2019, 3:37 AM
include/llvm/Object/ObjectFile.h
432 ↗(On Diff #197321)

Just use && for operator== and implement operator!= in terms of operator==?

grimar updated this revision to Diff 197562.May 1 2019, 8:58 AM
grimar marked an inline comment as done.
  • Addressed review comments.
  • Cosmetic change in the test case.
include/llvm/Object/ObjectFile.h
432 ↗(On Diff #197321)

Yes, that is better. Done.

grimar added a comment.May 6 2019, 1:42 AM

James, are you OK with this patch?

jhenderson accepted this revision.May 7 2019, 4:14 AM

LGTM, with a nit.

test/tools/llvm-objdump/relocations-elf.test
74 ↗(On Diff #197562)

I think this would be a bit clearer:
"Check we report an error if the relocated section identified by the sh_info field of a relocation section is invalid."

78 ↗(On Diff #197321)

Okay, fair enough. There are definitely some improvements that could be made there. We shouldn't really be hitting report_fatal_error inside this sort of code, if we don't absolutely have to.

MaskRay accepted this revision.May 7 2019, 4:18 AM
This revision was automatically updated to reflect the committed changes.
grimar marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2019, 6:14 AM