[PATCH] D59020: [StackMaps] Update llvm-readobj to parse V3 Stackmaps

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

Looks largely reasonable.  Several comments to clarify diff to ensure there's not something subtle I'm missing.  Once those are done, likely to be a quick LGTM.



================
Comment at: llvm/include/llvm/Object/StackMapParser.h:13
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/Endian.h"
----------------
Please reupload w/ full context.


================
Comment at: llvm/include/llvm/Object/StackMapParser.h:118
+    /// Get the Location Size for this location.
+    uint16_t getLocationSize() const {
+        return read<uint16_t>(P + LocationSizeOffset);
----------------
Please rename to getSize.


================
Comment at: llvm/include/llvm/Object/StackMapParser.h:157
     static const int KindOffset = 0;
-    static const int DwarfRegNumOffset = KindOffset + sizeof(uint16_t);
-    static const int SmallConstantOffset = DwarfRegNumOffset + sizeof(uint16_t);
-    static const int LocationAccessorSize = sizeof(uint64_t);
+    static const int LocationSizeOffset = KindOffset + sizeof(uint16_t);
+    static const int DwarfRegNumOffset = LocationSizeOffset + sizeof(uint16_t);
----------------
You've got a couple of additions of new accessors, constants, and one constant rename.  Please separate that into it's own patch for the old V2 parser, commit it, and then rebase.

(i.e. please remove the renames and added stuff from the diff so that the meaninful changes are obvious.)


================
Comment at: llvm/test/Object/stackmap-dump.test:1
-RUN: llvm-readobj -stackmap %p/Inputs/stackmap-test.macho-x86-64 | FileCheck %s
+RUN: llvm-readobj -stackmap %p/Inputs/stackmap-test.elf-x86-64 | FileCheck %s
 
----------------
Rather than switching from macho to elf, please add an extra run line for ELF.  (i.e. test both)


================
Comment at: llvm/tools/llvm-readobj/StackMapPrinter.h:67
       }
+      OS << ", size: " << Loc.getLocationSize() << "\n";
     }
----------------
Adding the size is a completely separate change.  Please post this separately so that all the test diffs are clear as to which change they related to.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59020