[PATCH] D70720: [llvm-objdump] Display locations of variables alongside disassembly

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: llvm/include/llvm/Support/FormattedStream.h:108
 
   /// getColumn - Return the column number
+  unsigned getColumn() {
----------------
please don't repeat the function name in the doxygen comment.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:551
 namespace {
+struct PrintedExpr {
+  enum ExprKind {
----------------
doxygen comments explaining the purpose of this?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:558
+  unsigned Precedence;
+  SmallString<20> String;
+
----------------
20 seems short. What's expected in here?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:566
+  // precedence as C, with numbers taken from the table at
+  // https://en.cppreference.com/w/c/language/operator_precedence.
+  void AddParens(unsigned NewPrecedence) {
----------------
///


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:589
+    unsigned Opcode = Op.getCode();
+    if (Opcode == dwarf::DW_OP_piece) {
+      // DW_OP_piece - record piece of larger object.
----------------
Can you convert this into a switch()?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:796
+  // not seen any code which uses anything other than DW_OP_call_frame_cfa
+  // here, so might not be worth the extra complexity.
+  SmallString<20> FrameBase;
----------------
make this the doxygen comment


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:816
+// Stores a single expression representing the location of a source-level
+// variable, along with the PC range for which that expression is valid.
+struct LiveVariable {
----------------
///


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:845
+
+class LiveVariablePrinter {
+  // Information we want to track about one column in which we are printing a
----------------
what does this do?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:873
+    auto Locs = VarDie.getLocations(dwarf::DW_AT_location);
+    if (Locs) {
+      for (auto &LocExpr : *Locs) {
----------------
early exit


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:968
+  // NextAddr are in different functions, so live ranges starting at NextAddr
+  // will be ignored, because they belong to the next function.
+  void update(object::SectionedAddress ThisAddr,
----------------
///


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1043
+  // true, we have to print at least one line (with the continuation of any
+  // already-active live ranges) because something has already been printed
+  // earlier on this line.
----------------
///


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1101
+
+  // Print the live variable ranges to the right of a disassembled instruction.
+  void printAfterInst(formatted_raw_ostream &OS) {
----------------
///


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70720