[PATCH] D76223: [llvm-objdump] Add frame register opcode to variable display

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I don't think I'm in a good position to review most of this code. I've commented on a few things, but somebody else should do the rest.



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp:9
 
+#include "llvm/DebugInfo/DWARF/DWARFContext.h"
 #include "llvm/DebugInfo/DWARF/DWARFExpression.h"
----------------
clang-format the header include list. This is the wrong place for this line.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp:430
+      // that, plus an optional offset.
+      assert(FuncDie && "Cannot render DW_OP_fbreg without a function DIE");
+      raw_svector_ostream S(Stack.emplace_back().String);
----------------
Is it possible to hit this assertion if you are using malformed DWARF?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp:437
+            FrameBaseAttr->getAsBlock()->size());
+        bool LittleEndian = FuncDie.getDwarfUnit()->getContext().isLittleEndian();
+        DataExtractor FrameBaseData(FrameBaseStr, LittleEndian, 0);
----------------
clang-fomat needed on this new code.


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFExpressionCompactPrinterTest.cpp:59-61
+  auto ExpectedDG =
+      dwarfgen::Generator::create(Triple("armv8a-linux-gnueabi"), 4);
+  ASSERT_THAT_EXPECTED(ExpectedDG, Succeeded());
----------------
I'm going to guess that this won't work in a non-ARM build of LLVM...

You probably just want to return if this creation fails.


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFExpressionCompactPrinterTest.cpp:79
+  std::unique_ptr<DWARFContext> DwarfContext = DWARFContext::create(**Obj);
+  uint32_t NumCUs = DwarfContext->getNumCompileUnits();
+  EXPECT_EQ(NumCUs, 1u);
----------------
`uint32_t` seems like an odd type for a number of things. I'd expect `size_t`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76223