[PATCH] D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information.

[Greg Clayton via Phabricator via llvm-commits]

clayborg added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:920
+    OS << "  error: decoding the CIE opcodes into rows failed: "
+       << toString(RowsOrErr.takeError()) << "\n";
+  OS << "\n";
----------------
This function dumps the CFIProgram contents to the stream that is supplied. I don't believe that returning an error from a dumping function is a good idea as I like to see all errors in a file, not just the first error and have the tool stop dumping.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:945
+  else
+    OS << "  error: decoding the CIE opcodes into rows failed: "
+       << toString(RowsOrErr.takeError()) << "\n";
----------------
This function dumps the FDE to the stream that is supplied. Same argument as before. If this was a parsing function, then yes, an error should be returned.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:368
+llvm::Expected<uint64_t>
+CFIProgram::Instruction::getOperandAsUnsigned(const CFIProgram &CFIP,
+                                              uint32_t OperandIdx) const {
----------------
Yes I did miss the error message string part. I will fix!


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:432
+CFIProgram::Instruction::getOperandAsSigned(const CFIProgram &CFIP,
+                                            uint32_t OperandIdx) const {
+  if (OperandIdx >= 2)
----------------
Will do


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:432
+  switch (Type) {
+  case OT_Unset:
+    return createStringError(std::errc::invalid_argument,
----------------
jhenderson wrote:
> Some of these cases are identical. Can we combine them? Same goes above in `getOperandAsUnsigned`.
They have different error messages as they mention the "Type" by the enumeration name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89845