[PATCH] D61312: [llvm-objdump] - Print relocation record in a GNU format.

[George Rimar via Phabricator via llvm-commits]

grimar marked 2 inline comments as done.
grimar added inline comments.


================
Comment at: include/llvm/Object/ObjectFile.h:426
 inline bool SectionRef::operator==(const SectionRef &Other) const {
+  if (OwningObject != Other.OwningObject)
+    return false;
----------------
jhenderson wrote:
> 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?
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));
}
```


================
Comment at: test/tools/llvm-objdump/relocations-elf.test:78
+# RUN: not llvm-objdump --reloc %t2 2>&1 | FileCheck %s --check-prefix=ERR
+# ERR: LLVM ERROR: Invalid data was encountered while parsing the file
+
----------------
MaskRay wrote:
> jhenderson wrote:
> > This is not a nice error. Can anything be done easily to improve it?
> 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.
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61312/new/

https://reviews.llvm.org/D61312