[PATCH] D80380: [llvm-readobj] - Do not crash when an invalid .eh_frame_hdr is dumped using --unwind.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/ELF.h:72
+  if (Headers)
+    return "[index " + std::to_string(Phdr - &Headers->front()) + "]";
+  // See the comment in the getSecIndexForError() above.
----------------
`Twine` instead of `std::to_string`?


================
Comment at: llvm/include/llvm/Object/ELF.h:73
+    return "[index " + std::to_string(Phdr - &Headers->front()) + "]";
+  // See the comment in the getSecIndexForError() above.
+  llvm::consumeError(Headers.takeError());
----------------
Delete "the".


================
Comment at: llvm/test/tools/llvm-readobj/ELF/unwind.test:287
+## Case C: test we report an error when the offset + the file size of the PT_GNU_EH_FRAME is so
+##         large value that overflows the platform address size type.
+# RUN: yaml2obj --docnum=3 %s -o %t5 -DOFFSET=0x100 -DSIZE=0xffffffff
----------------
"so large a value that it overflows" is slightly more grammatically correct, I think (English is weird - most native speakers don't even have a good grasp of all the rules, but this feels right!).

I wonder if we should have a 64-bit version of this same case, since it has a different upper bound?


================
Comment at: llvm/tools/llvm-readobj/DwarfCFIEHPrinter.h:113
+  if (!Content)
+    reportError(Content.takeError(), ObjF->getFileName());
+
----------------
grimar wrote:
> Note: we might want to change this and all other `reportError` calls in the `PrinterContext<ELFT>` to report warnings.
Sounds reasonable to me, but probably a separate change.


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

https://reviews.llvm.org/D80380