[StackMaps] Update llvm-readobj to parse V3 Stackmaps

[jacob-hughes]

Summary

This updates the StackMap parser in the llvm-readobj tool to parse version 3 StackMaps, which were bumped in this revision[1].

Version 3 StackMaps differ in that they have a uint16 sized "location size" field which was added to the Location block in a StackMap record. The record has additional padding for alignment. This was a backwards incompatible change resulting in a StackMap version bump.

[1]: https://reviews.llvm.org/D32629

[jacob-hughes]

Removed missed debug logging info

[reames]

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.

llvm/include/llvm/Object/StackMapParser.h

13 ↗

(On Diff #189483)

Please reupload w/ full context.

118 ↗

(On Diff #189483)

Please rename to getSize.

157 ↗

(On Diff #189483)

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

llvm/test/Object/stackmap-dump.test

1 ↗

(On Diff #189483)

Rather than switching from macho to elf, please add an extra run line for ELF. (i.e. test both)

llvm/tools/llvm-readobj/StackMapPrinter.h

67 ↗

(On Diff #189483)

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.

[jacob-hughes]

Thanks for the review Philip. I've addressed your comments and split this into 3 separate revisions: the first adds the new getSize() accessor; the second (this one) updates the stackmap parser to parse v3 maps; and the third includes the size of the locations in the llvm-readobj -stackmap output.

[jacob-hughes]

llvm/test/Object/stackmap-dump.test

1 ↗

(On Diff #189483)

The reason for this change was that the macho file was created back when v2 stackmaps would have been generated. I recompiled the .ll with with the most recent version of LLVM on my machine (linux) and am not entirely sure how to cross-compile to macho.

If you think it's necessary to have an up to date macho object file too, then I can look into how to make one.

[reames]

LGTM if you're confident in the correctness of the Register -> Indirect changes commented on, otherwise, let's discuss.

llvm/test/Object/stackmap-dump.test

106 ↗

(On Diff #189969)

These Register to Indirect changes bug me. Do you know why these changed?

1 ↗

(On Diff #189483)

Ah, that makes sense.

Given we're effectively removing the V2 parser, and replacing it with a V3 parser, I'm fine with the switch to ELF. Just do me a favour and add a comment to the top of the file about how when the parser format changes, the object file input needs regenerated.

Ideally, I'd like to find a way to reduce the diff (i.e. remove the Macho->ELF pieces and leave only the version pieces), but I don't see an obvious way to do so.

[jacob-hughes]

Sorry for the delayed reply.

I'm fairly certain that the changes in location from Register to Indirect were not caused by this patch. I did a git bisect and it seems that Record 12 starting spilling to the stack instead of registers as a result of this patch [0].

[0] https://reviews.llvm.org/D24103

[reames]

Doesn't look like this has landed. Were you waiting for me on anything? I'm comfortable with your explanation.

[jacob-hughes]

This is my first LLVM PR so I'm not quite sure what happens next. I don't have commit access to the project so will someone need to push this on my behalf? Thanks

[jacob-hughes]

Hi Philip, do you need anything further from me? If not, please could you merge this on my behalf?

[reames]

I think this needs rebased. I tried applying the patch to land it, and got build errors.

[jacob-hughes]

I've updated the diffs for all three revisions after rebasing from the latest master branch. I didn't get any conflicts and it seems to build and pass all the tests fine for me so I'm not sure how much this will help.

If this fails again could you send me the build log so I can see what's going on? The only thing I think it could be right now is the new elf file is not running correctly on a different machine.

[wks]

llvm/include/llvm/Object/StackMapParser.h

324 ↗

(On Diff #192798)

The comment should be “Always returns 3” to match the code.

[jacob-hughes]

Updated comment to mention v3 stack maps

[jacob-hughes]

Ready for re-review and (if accepted) someone to merge on my behalf please

[reames]

I tried applying the patch again, and discovered the problem. Phabricator was stripping out the binary portions, so I wasn't getting the test changes. (I'd also seen a build failure last time, but didn't see that again.)

I went ahead and rebuilt the test locally. There's a few diffs in the tests caused by differences in codegen since the original object file was created. The biggest is we appear to be more aggressive about folding stack references.

Please let me know if you spot any problem with what I submitted.

llvm/trunk/include/llvm/Object/StackMapParser.h

public:
   public:
     /// Get the Kind for this location.
     LocationKind getKind() const {
       return LocationKind(P[KindOffset]);
     }
 
     /// Get the Size for this location.
     unsigned getSizeInBytes() const {
-        return read<uint8_t>(P + SizeOffset);
+        return read<uint16_t>(P + SizeOffset);
 
     }
 
     /// Get the Dwarf register number for this location.
     uint16_t getDwarfRegNum() const {
       return read<uint16_t>(P + DwarfRegNumOffset);
     }
 
public:
   private:
     LocationAccessor(const uint8_t *P) : P(P) {}
 
     LocationAccessor next() const {
       return LocationAccessor(P + LocationAccessorSize);
     }
 
     static const int KindOffset = 0;
-    static const int SizeOffset = KindOffset + sizeof(uint8_t);
-    static const int DwarfRegNumOffset = SizeOffset + sizeof(uint8_t);
-    static const int SmallConstantOffset = DwarfRegNumOffset + sizeof(uint16_t);
-    static const int LocationAccessorSize = sizeof(uint64_t);
+    static const int SizeOffset = KindOffset + sizeof(uint16_t);
+    static const int DwarfRegNumOffset = SizeOffset + sizeof(uint16_t);
+    static const int SmallConstantOffset = DwarfRegNumOffset + sizeof(uint32_t);
+    static const int LocationAccessorSize = sizeof(uint64_t) + sizeof(uint32_t);
 
     const uint8_t *P;
   };
 
   /// Accessor for stackmap live-out fields.
   class LiveOutAccessor {
     friend class StackMapParser;
     friend class RecordAccessor;
public:
     iterator_range<liveout_iterator> liveouts() const {
       return make_range(liveouts_begin(), liveouts_end());
     }
 
   private:
     RecordAccessor(const uint8_t *P) : P(P) {}
 
     unsigned getNumLiveOutsOffset() const {
-      return LocationListOffset + LocationSize * getNumLocations() +
-             sizeof(uint16_t);
+      unsigned LocOffset = 
+          ((LocationListOffset + LocationSize * getNumLocations()) + 7) & ~0x7; 
+      return LocOffset + sizeof(uint16_t);
     }
 
     unsigned getSizeInBytes() const {
       unsigned RecordSize =
         getNumLiveOutsOffset() + sizeof(uint16_t) + getNumLiveOuts() * LiveOutSize;
       return (RecordSize + 7) & ~0x7;
     }
 
     RecordAccessor next() const {
       return RecordAccessor(P + getSizeInBytes());
     }
 
     static const unsigned PatchpointIDOffset = 0;
     static const unsigned InstructionOffsetOffset =
       PatchpointIDOffset + sizeof(uint64_t);
     static const unsigned NumLocationsOffset =
       InstructionOffsetOffset + sizeof(uint32_t) + sizeof(uint16_t);
     static const unsigned LocationListOffset =
       NumLocationsOffset + sizeof(uint16_t);
-    static const unsigned LocationSize = sizeof(uint64_t);
+    static const unsigned LocationSize = sizeof(uint64_t) + sizeof(uint32_t);
     static const unsigned LiveOutSize = sizeof(uint32_t);
 
     const uint8_t *P;
   };
 
   /// Construct a parser for a version-2 stackmap. StackMap data will be read
   /// from the given array.
   StackMapParser(ArrayRef<uint8_t> StackMapSection)
       : StackMapSection(StackMapSection) {
     ConstantsListOffset = FunctionListOffset + getNumFunctions() * FunctionSize;
 
-    assert(StackMapSection[0] == 2 &&
-           "StackMapParser can only parse version 2 stackmaps");
+    assert(StackMapSection[0] == 3 &&
+           "StackMapParser can only parse version 3 stackmaps");
 
     unsigned CurrentRecordOffset =
       ConstantsListOffset + getNumConstants() * ConstantSize;
 
     for (unsigned I = 0, E = getNumRecords(); I != E; ++I) {
       StackMapRecordOffsets.push_back(CurrentRecordOffset);
       CurrentRecordOffset +=
         RecordAccessor(&StackMapSection[CurrentRecordOffset]).getSizeInBytes();
     }
   }
 
   using function_iterator = AccessorIterator<FunctionAccessor>;
   using constant_iterator = AccessorIterator<ConstantAccessor>;
   using record_iterator = AccessorIterator<RecordAccessor>;
 
-  /// Get the version number of this stackmap. (Always returns 2).
-  unsigned getVersion() const { return 2; }
+  /// Get the version number of this stackmap. (Always returns 3).
+  unsigned getVersion() const { return 3; }
 
   /// Get the number of functions in the stack map.
   uint32_t getNumFunctions() const {
     return read<uint32_t>(&StackMapSection[NumFunctionsOffset]);
   }
 
   /// Get the number of large constants in the stack map.
   uint32_t getNumConstants() const {

llvm/trunk/test/Object/stackmap-dump.test

 RUN: llvm-readobj -stackmap %p/Inputs/stackmap-test.macho-x86-64 | FileCheck %s
 
 ; Note: the macho object file in this test was generated in the following way:
-; llc -mtriple=x86_64-apple-darwin %p/test/CodeGen/X86/stackmap.ll -o stackmap.s
-; clang -c stackmap.s -o %p/test/Object/Inputs/stackmap-test.macho-x86-64
+; llc -mtriple=x86_64-apple-darwin test/CodeGen/X86/stackmap.ll -o test/Object/Inputs/stackmap-test.macho-x86-64 -filetype=obj
 
-CHECK: LLVM StackMap Version: 2
+CHECK: LLVM StackMap Version: 3
 CHECK-NEXT: Num Functions: 16
 CHECK-NEXT:   Function address: 0, stack size: 8, callsite record count: 1
 CHECK-NEXT:   Function address: 0, stack size: 24, callsite record count: 1
 CHECK-NEXT:   Function address: 0, stack size: 8, callsite record count: 1
 CHECK-NEXT:   Function address: 0, stack size: 8, callsite record count: 1
 CHECK-NEXT:   Function address: 0, stack size: 8, callsite record count: 1
 CHECK-NEXT:   Function address: 0, stack size: 8, callsite record count: 1
 CHECK-NEXT:   Function address: 0, stack size: 8, callsite record count: 1
-CHECK-NEXT:   Function address: 0, stack size: 56, callsite record count: 1
-CHECK-NEXT:   Function address: 0, stack size: 56, callsite record count: 1
+CHECK-NEXT:   Function address: 0, stack size: 8, callsite record count: 1
+CHECK-NEXT:   Function address: 0, stack size: 8, callsite record count: 1
 CHECK-NEXT:   Function address: 0, stack size: 56, callsite record count: 1
 CHECK-NEXT:   Function address: 0, stack size: 56, callsite record count: 1
 CHECK-NEXT:   Function address: 0, stack size: 8, callsite record count: 1
 CHECK-NEXT:   Function address: 0, stack size: 56, callsite record count: 2
 CHECK-NEXT:   Function address: 0, stack size: 8, callsite record count: 4
 CHECK-NEXT:   Function address: 0, stack size: 56, callsite record count: 1
 CHECK-NEXT:   Function address: 0, stack size: 18446744073709551615, callsite record count: 1
 CHECK-NEXT: Num Constants: 3

 CHECK-NEXT:     1 live-outs: [ R#7 (8-bytes) ]
 
 CHECK:        Record ID: 8, instruction offset: 10
 CHECK-NEXT:     2 locations:
 CHECK-NEXT:       #1: Register R#2, size: 8
 CHECK-NEXT:       #2: Register R#8, size: 8
 CHECK-NEXT:     2 live-outs: [ R#0 (8-bytes) R#7 (8-bytes) ]
 
-CHECK:        Record ID: 11, instruction offset: 42
+CHECK:        Record ID: 11, instruction offset: 4
 CHECK-NEXT:     17 locations:
 CHECK-NEXT:       #1: Register R#9, size: 8
-CHECK-NEXT:       #2: Register R#14, size: 8
-CHECK-NEXT:       #3: Register R#10, size: 8
-CHECK-NEXT:       #4: Register R#3, size: 8
-CHECK-NEXT:       #5: Register R#0, size: 8
-CHECK-NEXT:       #6: Register R#13, size: 8
-CHECK-NEXT:       #7: Register R#12, size: 8
-CHECK-NEXT:       #8: Register R#15, size: 8
+CHECK-NEXT:       #2: Indirect [R#6 + 16], size: 8
+CHECK-NEXT:       #3: Indirect [R#6 + 24], size: 8
+CHECK-NEXT:       #4: Indirect [R#6 + 32], size: 8
+CHECK-NEXT:       #5: Indirect [R#6 + 40], size: 8
+CHECK-NEXT:       #6: Indirect [R#6 + 48], size: 8
+CHECK-NEXT:       #7: Indirect [R#6 + 56], size: 8
+CHECK-NEXT:       #8: Indirect [R#6 + 64], size: 8
 CHECK-NEXT:       #9: Indirect [R#6 + 72], size: 8
 CHECK-NEXT:       #10: Indirect [R#6 + 80], size: 8
 CHECK-NEXT:       #11: Indirect [R#6 + 88], size: 8
 CHECK-NEXT:       #12: Indirect [R#6 + 96], size: 8
 CHECK-NEXT:       #13: Indirect [R#6 + 104], size: 8
 CHECK-NEXT:       #14: Indirect [R#6 + 112], size: 8
 CHECK-NEXT:       #15: Indirect [R#6 + 120], size: 8
 CHECK-NEXT:       #16: Indirect [R#6 + 128], size: 8
 CHECK-NEXT:       #17: Indirect [R#6 + 136], size: 8
 CHECK-NEXT:     1 live-outs: [ R#7 (8-bytes) ]
 
-CHECK:        Record ID: 12, instruction offset: 62
+CHECK:        Record ID: 12, instruction offset: 4
 CHECK-NEXT:     17 locations:
 CHECK-NEXT:       #1: Register R#0, size: 8
-CHECK-NEXT:       #2: Register R#14, size: 8
-CHECK-NEXT:       #3: Register R#10, size: 8
-CHECK-NEXT:       #4: Register R#9, size: 8
-CHECK-NEXT:       #5: Register R#8, size: 8
-CHECK-NEXT:       #6: Register R#4, size: 8
-CHECK-NEXT:       #7: Register R#1, size: 8
-CHECK-NEXT:       #8: Register R#2, size: 8
-CHECK-NEXT:       #9: Register R#5, size: 8
-CHECK-NEXT:       #10: Register R#3, size: 8
-CHECK-NEXT:       #11: Register R#13, size: 8
-CHECK-NEXT:       #12: Register R#12, size: 8
-CHECK-NEXT:       #13: Register R#15, size: 8
+CHECK-NEXT:       #2: Indirect [R#6 + 16], size: 8
+CHECK-NEXT:       #3: Indirect [R#6 + 24], size: 8
+CHECK-NEXT:       #4: Indirect [R#6 + 32], size: 8
+CHECK-NEXT:       #5: Indirect [R#6 + 40], size: 8
+CHECK-NEXT:       #6: Indirect [R#6 + 48], size: 8
+CHECK-NEXT:       #7: Indirect [R#6 + 56], size: 8
+CHECK-NEXT:       #8: Indirect [R#6 + 64], size: 8
+CHECK-NEXT:       #9: Indirect [R#6 + 72], size: 8
+CHECK-NEXT:       #10: Indirect [R#6 + 80], size: 8
+CHECK-NEXT:       #11: Indirect [R#6 + 88], size: 8
+CHECK-NEXT:       #12: Indirect [R#6 + 96], size: 8
+CHECK-NEXT:       #13: Indirect [R#6 + 104], size: 8
 CHECK-NEXT:       #14: Indirect [R#6 + 112], size: 8
 CHECK-NEXT:       #15: Indirect [R#6 + 120], size: 8
 CHECK-NEXT:       #16: Indirect [R#6 + 128], size: 8
 CHECK-NEXT:       #17: Indirect [R#6 + 136], size: 8
 CHECK-NEXT:     0 live-outs: [ ]
 
 CHECK:        Record ID: 13, instruction offset: 50
 CHECK-NEXT:     1 locations:
 CHECK-NEXT:       #1: Indirect [R#6 + -48], size: 4
 CHECK-NEXT:     0 live-outs: [ ]
 
 CHECK:        Record ID: 14, instruction offset: 24
 CHECK-NEXT:     2 locations:
 CHECK-NEXT:       #1: Register R#0, size: 1
 CHECK-NEXT:       #2: Register R#3, size: 1
 CHECK-NEXT:     0 live-outs: [ ]
 
 CHECK:        Record ID: 15, instruction offset: 4
 CHECK-NEXT:     1 locations:
 CHECK-NEXT:       #1: Constant 33, size: 8
 CHECK-NEXT:     0 live-outs: [ ]
 
-CHECK:        Record ID: 16, instruction offset: 32
+CHECK:        Record ID: 16, instruction offset: 16
 CHECK-NEXT:     1 locations:
-CHECK-NEXT:       #1: Direct R#6 + -32, size: 8
+CHECK-NEXT:       #1: Direct R#6 + -40, size: 8
 CHECK-NEXT:     0 live-outs: [ ]
 
-CHECK:        Record ID: 17, instruction offset: 32
+CHECK:        Record ID: 17, instruction offset: 16
 CHECK-NEXT:     2 locations:
 CHECK-NEXT:       #1: Direct R#6 + -8, size: 8
-CHECK-NEXT:       #2: Direct R#6 + -40, size: 8
+CHECK-NEXT:       #2: Direct R#6 + -16, size: 8
 CHECK-NEXT:     1 live-outs: [ R#7 (8-bytes) ]
 
 CHECK:        Record ID: 4294967295, instruction offset: 4
 CHECK-NEXT:     0 locations:
 CHECK-NEXT:     0 live-outs: [ ]
 
 CHECK:        Record ID: 4294967296, instruction offset: 4
 CHECK-NEXT:     0 locations:
 CHECK-NEXT:     0 live-outs: [ ]
 
 CHECK:        Record ID: 9223372036854775807, instruction offset: 4
 CHECK-NEXT:     0 locations:
 CHECK-NEXT:     0 live-outs: [ ]
 
 CHECK:        Record ID: 18446744073709551615, instruction offset: 4
 CHECK-NEXT:     0 locations:
 CHECK-NEXT:     1 live-outs: [ R#7 (8-bytes) ]
 
 CHECK:        Record ID: 16, instruction offset: 18
 CHECK-NEXT:     1 locations:
 CHECK-NEXT:       #1: Indirect [R#6 + -44], size: 4
 CHECK-NEXT:     0 live-outs: [ ]
 
-CHECK:        Record ID: 0, instruction offset: 26
+CHECK:        Record ID: 0, instruction offset: 25
 CHECK-NEXT:     0 locations:
 CHECK-NEXT:     0 live-outs: [ ]