Page MenuHomePhabricator

[StackMaps] Update llvm-readobj to parse V3 Stackmaps

Authored by jacob-hughes on Mar 6 2019, 4:16 AM.



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.


Diff Detail


Event Timeline

jacob-hughes created this revision.Mar 6 2019, 4:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2019, 4:16 AM

Removed missed debug logging info

jacob-hughes edited the summary of this revision. (Show Details)Mar 6 2019, 4:32 AM
reames requested changes to this revision.Mar 7 2019, 7:37 PM

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.

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

1 ↗(On Diff #189483)

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

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.

This revision now requires changes to proceed.Mar 7 2019, 7:37 PM
jacob-hughes marked an inline comment as done.
jacob-hughes edited the summary of this revision. (Show Details)

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 added inline comments.Mar 8 2019, 5:15 PM
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 accepted this revision.Mar 11 2019, 10:55 AM

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

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.

This revision is now accepted and ready to land.Mar 11 2019, 10:55 AM

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


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

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

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

reames requested changes to this revision.Mar 26 2019, 6:28 PM

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

This revision now requires changes to proceed.Mar 26 2019, 6:28 PM

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.

sanjoy removed a reviewer: sanjoy.Mar 31 2019, 10:35 AM
wks requested changes to this revision.Apr 3 2019, 10:59 PM
wks added a subscriber: wks.
wks added inline comments.
324 ↗(On Diff #192798)

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

This revision now requires changes to proceed.Apr 3 2019, 10:59 PM

Updated comment to mention v3 stack maps

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

This revision was not accepted when it landed; it landed in state Needs Review.Apr 12 2019, 8:54 PM
This revision was automatically updated to reflect the committed changes.

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.