[PATCH] D53264: [ARM64] [Windows] Add unwind support to llvm-readobj

[Sanjin Sijaric via Phabricator via llvm-commits]

ssijaric added inline comments.


================
Comment at: tools/llvm-readobj/ARMWinEHPrinter.cpp:788
       }
       assert(DI < array_lengthof(Ring) && "unhandled opcode");
     }
----------------
efriedma wrote:
> Not new with your patch, but we need real error handling here, in case of an unknown opcode, or a multi-byte opcode which would run past the end of the opcode array.
I will change this to print something like "bad opcode" and continue scanning.


================
Comment at: tools/llvm-readobj/ARMWinEHPrinter.cpp:804
     reinterpret_cast<const ulittle32_t *>(Contents.data() + Offset);
-  const ExceptionDataRecord XData(Data);
-
+  const ExceptionDataRecord XData(Data, isAArch64);
   DictScope XRS(SW, "ExceptionData");
----------------
efriedma wrote:
> Missing bounds check?  There must be at least 8 bytes of data in the section after the offset, and you need to check the total number of bytes specified by the header vs. the remaining bytes in the section.
Will add a check that the .xdata must be at least 8 bytes.


================
Comment at: tools/llvm-readobj/ARMWinEHPrinter.cpp:820
     ArrayRef<uint8_t> UC = XData.UnwindByteCode();
-    if (!XData.F()) {
+    if (isAArch64 || !XData.F()) {
       ListScope PS(SW, "Prologue");
----------------
efriedma wrote:
> Not really part of your patch, but I'm not sure why we're checking XData.F() here.  Maybe just leave a FIXME.
The check for F() is applicable to ARM only, and indicates that there is no prologue (e.g. a function fragment)  So !XData.F() indicates that there is a prologue, and we dump the opcodes for ARM.  Not used for AArch64.


================
Comment at: tools/llvm-readobj/ARMWinEHPrinter.cpp:829
   } else {
+    if (true) {
+      ListScope PS(SW, "Prologue");
----------------
efriedma wrote:
> "if (true)"?
Yes, otherwise ListScope will not align the prologue printing with epilogue printing when there multiple epilogues.


================
Comment at: tools/llvm-readobj/ARMWinEHPrinter.cpp:1044
     return dumpUnpackedEntry(COFF, Section, Offset, Index, Entry);
+  assert(!isAArch64 && "Packed unwind data not yet supported for ARM64");
   return dumpPackedEntry(COFF, Section, Offset, Index, Entry);
----------------
efriedma wrote:
> It's okay not to implement this for now, but it should probably do something other than crash (e.g. print a message that packed unwind is not yet implemented).
Ok, will do that.


Repository:
  rL LLVM

https://reviews.llvm.org/D53264