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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

This probably needs at least a few testcases... not so much to duplicate the tests you're going to add for exception handling emission, but rather to make sure the error handling works correctly.



================
Comment at: include/llvm/Support/ARMWinEH.h:349
+  template<bool isAArch64>
+  typename std::enable_if<isAArch64, uint8_t>::type
+  EpilogueStartIndex() const {
----------------
Using a template here is not helping make the code more readable.

Maybe there should be one function for both ARM and AArch64 that takes a reference to the ExceptionDataRecord?


================
Comment at: tools/llvm-readobj/ARMWinEHPrinter.cpp:788
       }
       assert(DI < array_lengthof(Ring) && "unhandled opcode");
     }
----------------
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.


================
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");
----------------
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.


================
Comment at: tools/llvm-readobj/ARMWinEHPrinter.cpp:807
+  SW.printNumber("FunctionLength", isAArch64 ?
+                 XData.FunctionLength() << 2 : XData.FunctionLength() << 1);
   SW.printNumber("Version", XData.Vers());
----------------
Probably makes sense to add ExceptionDataRecord::FunctionLengthInBytes(), or something like that.


================
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");
----------------
Not really part of your patch, but I'm not sure why we're checking XData.F() here.  Maybe just leave a FIXME.


================
Comment at: tools/llvm-readobj/ARMWinEHPrinter.cpp:829
   } else {
+    if (true) {
+      ListScope PS(SW, "Prologue");
----------------
"if (true)"?


================
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);
----------------
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).


Repository:
  rL LLVM

https://reviews.llvm.org/D53264