Page MenuHomePhabricator

[DSE] fix - missing store to runtime stack in thunk with tail call bvval arg
Needs ReviewPublic

Authored by Gerolf on Jul 1 2016, 5:41 PM.

Details

Summary

In a thunk DSE deletes a store to the stack. This causes an incorrect
parameter value (for a byval argument) to be passed on to a tail call.

This exposes a corner case that requires clarification. I believe for a byval in
a tail call, the pass by value semantics of byval holds but not the ‘owned by
caller’ aspect of it. That part does not make sense for tail call. And this
justifies the code in getModRefInfo. DSE looks at the parameter list of the
call, but asks whether or not the call could mod ref a stack location. Since
this is a tail call I think mod ref analysis correctly asserts “ no mod ref “.
Here is the relevant code in BasicAliasAnalysis.cpp/getModRefInfo():

If this is a tail call and Loc.Ptr points to a stack location, we know that
the tail call cannot access or modify the local stack.
We cannot exclude byval arguments here; these belong to the caller of
the current function not to the current function, and a tail callee
// may reference them.
if (isa<AllocaInst>(Object))

if (const CallInst *CI = dyn_cast<CallInst>(CS.getInstruction()))
  if (CI->isTailCall())
    return MRI_NoModRef;

The information DSE misses out on is whether the call arguments need values from
the runtime stack. The patch is adding that check. Does anyone have different
opinion about the fix?

Also, FWIW, I think a tail call with a byval argument can happen only in a
thunk. That assertion is missing yet.

Diff Detail

Event Timeline

Gerolf updated this revision to Diff 62592.Jul 1 2016, 5:41 PM
Gerolf retitled this revision from to [DSE] fix - missing store to runtime stack in thunk with tail call bvval arg.
Gerolf updated this object.
Gerolf added a subscriber: llvm-commits.

Special-casing this in DSE makes no sense... alias analysis should always return the correct result.

In this case, alias analysis is correct; the definition of "tail" doesn't allow marking this call. If clang is generating code like this, it's a bug in clang.

Gerolf added a subscriber: Gerolf.Jul 7 2016, 8:36 PM

Ahmed pointed out the clang commit causing the issue is r244207 - Mark calls in thunk functions as tail-call optimization candidates

The purpose of the fix was to prevent thunks from showing up on the call stack.

It seems that this patch overlooked the implications of tail calls with byval parameters and should be backed out. Any objections?

Thanks
Gerolf

reames resigned from this revision.Oct 7 2016, 3:09 PM
reames removed a reviewer: reames.

Appears to be a stale revision. Feel free to readd as reviewer if further review needed.

dexonsmith resigned from this revision.Mon, Oct 19, 6:52 PM