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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
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
----------------
jhenderson wrote:
> "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?
> I wonder if we should have a 64-bit version of this same case, since it has a different upper bound?

Yes, probably.

It seems was not possible (or too complicated, I was unable to) to produce the right values for the program header with the previous approach (because of overflows happening), so I've also changed how tests are implemented. Now I set needed size/offset values directly for the `PT_GNU_EH_FRAME`. It is the most straightforward way to set them and I it also looks much cleaner than before I think. Not sure why I did not do it in this way from the begining.

I've also added a test (Case D) for "p_memsz does not match p_filesz for GNU_EH_FRAME" error (seems we do not have it anywhere, but it is related). I'd improve this message in a follow-up though to report actual values.


================
Comment at: llvm/tools/llvm-readobj/DwarfCFIEHPrinter.h:113
+  if (!Content)
+    reportError(Content.takeError(), ObjF->getFileName());
+
----------------
jhenderson wrote:
> 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.
Yes. That is what I meant, it is in my TODO list.


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

https://reviews.llvm.org/D80380