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

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.

LGTM, with a nit.



================
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
+
----------------
grimar wrote:
> 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.
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.


================
Comment at: test/tools/llvm-objdump/relocations-elf.test:74
+
+## Check we report if relocated section identified by sh_info field is invalid.
+# RUN: yaml2obj --docnum=2 %s > %t2
----------------
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."


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

https://reviews.llvm.org/D61312