This is an archive of the discontinued LLVM Phabricator instance.

[WPD] Add an optional checking mode for debugging devirtualization
ClosedPublic

Authored by tejohnson on Feb 3 2021, 1:17 PM.

Details

Summary

This adds an internal option -wholeprogramdevirt-check which if enabled
will guard each devirtualization with a runtime check against the
expected target, and an invocation of a debug trap if the check fails.
This is useful for debugging WPD failures involving undefined behavior
(e.g. casting to another class type not in the inheritance chain).

Diff Detail

Event Timeline

tejohnson created this revision.Feb 3 2021, 1:17 PM
tejohnson requested review of this revision.Feb 3 2021, 1:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2021, 1:17 PM

I think this is probably worth a user level option as well.

I think this is probably worth a user level option as well.

I guess from the perspective of it being used to identify undefined behavior, however, it will only flush that out if we happen to do a single implementation WPD right now. It's also useful for debugging WPD. For now I'd like to get it in as an internal option for debugging, and figure out the appropriate name for a user level option in a follow on patch - wdyt?

davidxl added inline comments.Feb 12 2021, 9:51 AM
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
1065

is versioning necessary here? Can it just be:

if (target != direct_target)

trap();

direct_target();

@pcc You mentioned over chat that you were ok with this patch, but did you have any specific comments?

@davidxl response to your suggestion below.

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
1065

It could, but invoking versionCallSite was very convenient so I didn't need to replicate a bunch of the necessary code. With versionCallSite we currently get:

if (target == direct_target)

direct_target()

else

trap()

I'm not sure it matters much given that this is for debugging. Wdyt?

davidxl accepted this revision.Feb 16 2021, 2:30 PM

lgtm

This revision is now accepted and ready to land.Feb 16 2021, 2:30 PM
pcc added a comment.Feb 16 2021, 2:57 PM

Can you use SplitBlockAndInsertIfThen instead of versionCallSite here? I personally don't find the name versionCallSite very intuitive here.

In D95969#2566916, @pcc wrote:

Can you use SplitBlockAndInsertIfThen instead of versionCallSite here? I personally don't find the name versionCallSite very intuitive here.

There's a bunch of handling in versionCallSite for things like must tail calls and invoke instructions that I think I would end up needing to replicate, along with the general case. Calling versionCallSite makes things easy since it handles all of this. Would it help to expand the comment above the call?

pcc added a comment.Feb 16 2021, 4:27 PM
In D95969#2566916, @pcc wrote:

Can you use SplitBlockAndInsertIfThen instead of versionCallSite here? I personally don't find the name versionCallSite very intuitive here.

There's a bunch of handling in versionCallSite for things like must tail calls and invoke instructions that I think I would end up needing to replicate, along with the general case. Calling versionCallSite makes things easy since it handles all of this. Would it help to expand the comment above the call?

You're just adding some code before the call, no? That means that you don't need to touch the call itself.

In D95969#2567073, @pcc wrote:
In D95969#2566916, @pcc wrote:

Can you use SplitBlockAndInsertIfThen instead of versionCallSite here? I personally don't find the name versionCallSite very intuitive here.

There's a bunch of handling in versionCallSite for things like must tail calls and invoke instructions that I think I would end up needing to replicate, along with the general case. Calling versionCallSite makes things easy since it handles all of this. Would it help to expand the comment above the call?

You're just adding some code before the call, no? That means that you don't need to touch the call itself.

Yeah actually this is a good reason to go with David's earlier suggestion of changing the if condition. Let me go ahead and do that.

Change to use SplitBlockAndInsertIfThen to insert guarded trap before call.

tejohnson edited the summary of this revision. (Show Details)Feb 17 2021, 11:45 AM
In D95969#2567073, @pcc wrote:
In D95969#2566916, @pcc wrote:

Can you use SplitBlockAndInsertIfThen instead of versionCallSite here? I personally don't find the name versionCallSite very intuitive here.

There's a bunch of handling in versionCallSite for things like must tail calls and invoke instructions that I think I would end up needing to replicate, along with the general case. Calling versionCallSite makes things easy since it handles all of this. Would it help to expand the comment above the call?

You're just adding some code before the call, no? That means that you don't need to touch the call itself.

Yeah actually this is a good reason to go with David's earlier suggestion of changing the if condition. Let me go ahead and do that.

Ok I have changed this along the lines of what both of you suggested. PTAL

thanks for the update. lgtm

pcc accepted this revision.Feb 17 2021, 1:01 PM

LGTM

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
1062

No need for an if here, just do the bitcast and it will constant fold away if the types are the same.

tejohnson updated this revision to Diff 324443.Feb 17 2021, 3:29 PM

Address comments

This revision was landed with ongoing or failed builds.Feb 17 2021, 4:46 PM
This revision was automatically updated to reflect the committed changes.