Page MenuHomePhabricator

[Stackmap] Added callsite counts to emitted function information.
ClosedPublic

Authored by kavon on Aug 13 2016, 4:37 PM.

Details

Summary

It was previously not possible for tools to use solely the stackmap information emitted to reconstruct the return addresses of callsites in the map, which is necessary to use the information to walk a stack. This patch adds per-function callsite counts when emitting the stackmap section in order to resolve the problem. Note that this slightly alters the stackmap format, so external tools parsing these maps will need to be updated.

Problem Details:
Records only store their offset from the beginning of the function they belong to. While these records and the functions are output in program order, it is not possible to determine where the end of one function's records are without the callsite count when processing the records to compute return addresses.

Diff Detail

Event Timeline

kavon updated this revision to Diff 67965.Aug 13 2016, 4:37 PM
kavon retitled this revision from to [Stackmap] Added callsite counts to emitted function information..
kavon updated this object.
kavon added a reviewer: sanjoy.
sanjoy edited edge metadata.Aug 13 2016, 5:15 PM

I have not reviewed the code yet, but this looks like the kind of thing that would involve bumping the stackmap version from 1 to 2.

Also CC'ing some other people who might be interested in this.

kavon updated this revision to Diff 68820.Aug 21 2016, 9:14 PM

I updated the original patch so that the record counts are 8 bytes wide instead of 4 bytes wide to fix an alignment issue in the StackMapParser.

The reason this is necessary is that the StackMapParser rounds up the size of every record so that it is divisible by 8 as a means of finding the next 8-byte aligned record, but this method only works if the first record started out 8 byte aligned, which was not guaranteed when I chose to use 4 byte record counts.

kavon added a comment.Aug 21 2016, 9:35 PM

this looks like the kind of thing that would involve bumping the stackmap version from 1 to 2.

@sanjoy and others: if this is decided, then the patch will have to change quite a lot, so please let me know!

I made this patch based on the line in the documentation that says "the stack map format is a contract between an LLVM SVN revision and the runtime." Thus, I didn't think it was worth creating a new version when this is a fix for an oversight of a currently experimental intrinsic.

ributzka edited edge metadata.Aug 22 2016, 9:54 AM

What about reviving the patch in https://reviews.llvm.org/D13110. Wouldn't that solve your problem too?

sanjoy requested changes to this revision.Aug 22 2016, 10:01 AM
sanjoy edited edge metadata.

Looks straightforward. I only have a couple of minor inline comments.

include/llvm/CodeGen/StackMaps.h
201

Can you please use a struct here -- it is just too easy to mess up second and first otherwise.

lib/CodeGen/StackMaps.cpp
342

Indent looks off? Use clang-format.

534

Perhaps call FnInfo FnInfos instead (to go with CSInfos)?

This revision now requires changes to proceed.Aug 22 2016, 10:01 AM

What about reviving the patch in https://reviews.llvm.org/D13110. Wouldn't that solve your problem too?

I will let @kavon make the final call on this, but reviving D13110 looks excessively invasive compared to the problem @kavon has.

If we have to rev the version, which we have to in this case, then I would prefer to have the useful fixes from D13110 in there too. Otherwise this change just feels like a hack on the existing format without addressing any of the other issues.

kavon updated this revision to Diff 68899.Aug 22 2016, 12:20 PM
kavon edited edge metadata.

Updated per sanjoy's inline feedback.

sanjoy requested changes to this revision.Aug 22 2016, 12:28 PM
sanjoy edited edge metadata.

Hi @kavon,

this looks like the kind of thing that would involve bumping the stackmap version from 1 to 2.

@sanjoy and others: if this is decided, then the patch will have to change quite a lot, so please let me know!

Sorry, I forgot about this bit in the last review. I think the version number needs to change. While I agree that this can be seen as "bugfix" kind of change, in practice changing the format without a version bump will silently break backends.

This revision now requires changes to proceed.Aug 22 2016, 12:28 PM
kavon marked 3 inline comments as done.Aug 22 2016, 12:40 PM

@ributzka The bug that is fixed by this patch helps all users of stackmaps (gc.statepoint and patchpoints), because you couldn't associate callsites with their functions. I think we should at least merge this fix in so that the current stackmap format is actually usable.

D13110 goes a lot further in that it adds new features for patchpoints, which is not something I'm using right now (I'm using gc.statepoints), so I don't think I'm the right person to revive such an old patch.

Hi @kavon,

this looks like the kind of thing that would involve bumping the stackmap version from 1 to 2.

@sanjoy and others: if this is decided, then the patch will have to change quite a lot, so please let me know!

Sorry, I forgot about this bit in the last review. I think the version number needs to change. While I agree that this can be seen as "bugfix" kind of change, in practice changing the format without a version bump will silently break backends.

Should we also maintain backwards compatibility with version 1 map generation & parsing, or should I just change it to version number 2 to let others know they need to tweak their code? (I'm in favor the latter).

Hi @kavon,

Should we also maintain backwards compatibility with version 1 map
generation & parsing, or should I just change it to version number 2
to let others know they need to tweak their code? (I'm in favor the
latter).

I'm in favor of the latter too (though I'm interested in what
@ributzka thinks). I'd suggest sending a mail to llvm-dev stating
that you're bumping the stackmap version in a backward incompatible
manner and see if anyone objects.

FWIW, I don't think this should be a problem for us.

include/llvm/CodeGen/StackMaps.h
198

Why do you need this default constructor?

199

Make the constructor explicit.

lib/CodeGen/StackMaps.cpp
342

s/current/Current/ in keeping with LLVM coding style.

343

Why do you need to assign StackSize again?

Can't this whole block be:

auto CurrentIt = FnInfos.find(AP.CurrentFnSym);
if (CurrentIt != FnInfos.end()
  CurrentIt->second.RecordCount++;
else
  FnInfos.insert(...);

?

Hi @kavon,

Should we also maintain backwards compatibility with version 1 map
generation & parsing, or should I just change it to version number 2
to let others know they need to tweak their code? (I'm in favor the
latter).

I'm in favor of the latter too (though I'm interested in what
@ributzka thinks). I'd suggest sending a mail to llvm-dev stating
that you're bumping the stackmap version in a backward incompatible
manner and see if anyone objects.

FWIW, I don't think this should be a problem for us.

kavon marked 4 inline comments as done.Aug 22 2016, 8:41 PM

(added replies to inline comments ahead of adding a new revision)

include/llvm/CodeGen/StackMaps.h
198

The implementation of MapVector requires it. Here's the error if you remove it:

include/llvm/ADT/MapVector.h:80:44: error: 
      no matching constructor for initialization of 'llvm::StackMaps::FunctionInfo'
      Vector.push_back(std::make_pair(Key, ValueT()));
                                           ^
lib/CodeGen/StackMaps.cpp
343

I only reassign StackSize because that's what the previous implementation did, and I didn't want to mess with a good thing ;)

I have now changed the block to match your suggestion.

kavon updated this revision to Diff 68952.Aug 22 2016, 8:46 PM
kavon edited edge metadata.
kavon marked 2 inline comments as done.

This revision includes the changes requested in the inline comments.

@sanjoy I have sent an email to the llvm-dev list about stackmap versioning:

https://groups.google.com/forum/#!topic/llvm-dev/Ci7yLVw6itw

I will create a another revision that also bumps the version from 1 to 2 after giving people a chance to respond to the email.

I'm in favor of the latter too (though I'm interested in what
@ributzka thinks). I'd suggest sending a mail to llvm-dev stating
that you're bumping the stackmap version in a backward incompatible
manner and see if anyone objects.

FWIW, I don't think this should be a problem for us.

I don't have a problem with that either.

kavon updated this revision to Diff 70947.Sep 9 2016, 7:54 PM
kavon edited edge metadata.

Okay, here's a version with the stackmap version bumped to 2.

sanjoy accepted this revision.Sep 12 2016, 10:49 PM
sanjoy edited edge metadata.

lgtm

Do you have commit access? If not let me know here and I'll land this for you.

include/llvm/Object/StackMapParser.h
59

Just to be clear: this was wrong to begin with (for functions with stack frames larger than 4 gb :) )?

lib/CodeGen/StackMaps.cpp
345

I think this can be FnInfos.insert({AP.CurrentFnSym, FunctionInfo(FrameSize)});

This revision is now accepted and ready to land.Sep 12 2016, 10:49 PM
kavon added a comment.Sep 14 2016, 5:59 AM

@sanjoy

I don't have commit access, so if you could merge this patch for me that would be great.

One thing to note is that the diff in this Phabricator review doesn't include one of the binary files needed in the regression tests. Specifically, you'll need to generate test/Object/Inputs/stackmap-test.macho-x86-64 after the patch is applied (see the comment at the top of test/Object/stackmap-dump.test)

include/llvm/Object/StackMapParser.h
59

Yes, this was an existing issue I noticed and fixed, since the field is 64 bits.

This revision was automatically updated to reflect the committed changes.