This is an archive of the discontinued LLVM Phabricator instance.

[CGProfile] allows bitcast in metadata node storing function pointers
ClosedPublic

Authored by ychen on Sep 28 2020, 10:35 AM.

Details

Summary

For example, during RAUW in IRMover, the Function ValueAsMetadata in "CG Profile" could become bitcast.

Diff Detail

Event Timeline

ychen created this revision.Sep 28 2020, 10:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2020, 10:35 AM
ychen requested review of this revision.Sep 28 2020, 10:35 AM

ping. This is relatively minor, but it still breaks some of our use cases. Does this look good?

It seems a bit odd to have/need a custom fixup for one type of MDNode here. Can visitModuleFlagCGProfileEntry be fixed to handle the bitcast?

ychen added a comment.Oct 22 2020, 9:54 AM

It seems a bit odd to have/need a custom fixup for one type of MDNode here. Can visitModuleFlagCGProfileEntry be fixed to handle the bitcast?

It is indeed a bit odd but that's close to where the artifact is produced. Debug metadata nodes involving globals/functions could potentially have this issue too (either their verification doesn't enforce similar logic as CGProfile node does or they just don't care I'm not sure).

Fixing visitModuleFlagCGProfileEntry works too although it addresses the consumer rather than the producer, which logically also seems odd. If this is the preferred method to address the issue, I have no strong opinion against it.

It seems a bit odd to have/need a custom fixup for one type of MDNode here. Can visitModuleFlagCGProfileEntry be fixed to handle the bitcast?

It is indeed a bit odd but that's close to where the artifact is produced. Debug metadata nodes involving globals/functions could potentially have this issue too (either their verification doesn't enforce similar logic as CGProfile node does or they just don't care I'm not sure).

Fixing visitModuleFlagCGProfileEntry works too although it addresses the consumer rather than the producer, which logically also seems odd. If this is the preferred method to address the issue, I have no strong opinion against it.

Barring other issues from the way the metadata nodes were constructed, I guess I would prefer the change to the Verifier. It seems a bit odd to be fixing up this one metadata type here. It would be interesting to know how or why this is not an issue for the debug metadata nodes, but the fix doesn't need to wait on that.

ychen updated this revision to Diff 302329.Nov 2 2020, 10:22 AM
  • revert fixup in IRMover
  • allow CGProfile metadata node representing edge to use bitcast.
tejohnson accepted this revision.Nov 12 2020, 7:59 AM

LGTM with a couple minor changes noted below, also the description needs an update as it still references fixing up the nodes.

llvm/test/Transforms/FunctionImport/cg_profile.ll
2

Comment is stale.

6

I guess the test just makes sure this doesn't fail. Would be good to do a check on the output file via llvm-dis that it in fact does have the CG Profile metadata node and that it looks as expected (which I guess now will have the bitcast).

This revision is now accepted and ready to land.Nov 12 2020, 7:59 AM
ychen updated this revision to Diff 305003.Nov 12 2020, 5:22 PM
  • address comments: update comment, check output making sure bitcast is there.
ychen updated this revision to Diff 305005.Nov 12 2020, 5:25 PM
  • formatting

LGTM again except for the patch title and summary which need an update. Go ahead and commit once those are updated.

ychen retitled this revision from [IRMover] Fix up "CG Profile" MDNode after RAUW to [CGProfile] allows bitcast in metadata node storing function pointers.Nov 13 2020, 9:24 AM
ychen edited the summary of this revision. (Show Details)