This is an archive of the discontinued LLVM Phabricator instance.

msan: Handle musttail calls
ClosedPublic

Authored by rnk on Jun 27 2014, 3:46 PM.

Details

Summary

First, avoid calling setTailCall(false) on musttail calls. The funciton
prototypes should be "congruent", so the shadow layout should be exactly
the same.

Second, avoid inserting instrumentation after a musttail call to
propagate the return value shadow. We don't need to propagate the
result of a tail call, it should already be in the right place.

Diff Detail

Event Timeline

rnk updated this revision to Diff 10949.Jun 27 2014, 3:46 PM
rnk retitled this revision from to msan: Handle musttail calls.
rnk updated this object.
rnk added a reviewer: eugenis.
rnk added a subscriber: Unknown Object (MLST).
eugenis accepted this revision.Jun 30 2014, 1:18 AM
eugenis edited edge metadata.

LGTM w/ nits

include/llvm/IR/Instructions.h
2418

Are you sure it should be part of ReturnInst interface?
Makes more sense as a utility function to me.
Anyway, this is an unrelated refactoring, please commit separately.

lib/Transforms/Instrumentation/MemorySanitizer.cpp
2276

I no longer understand what is this code trying to achieve. The comment suggests that there is a problem when caller returns void, and that's exactly the case you are enabling for musttail calls.

But this whole thing does not make sense, and I can't figure out what is it that tail call optimization can break. Perhaps it had something to do with the hybrid tool and had been made obsolete by us resetting retval shadow to 0 _before_ every call.

Anyway, the tests still pass if this chunk is removed. I suggest you just do that. If I'm missing something, we will know it soon enough.

This revision is now accepted and ready to land.Jun 30 2014, 1:18 AM
rnk added inline comments.Jun 30 2014, 1:09 PM
include/llvm/IR/Instructions.h
2418

I agree, the IR classes are generally very lightweight and don't have these kinds of analysis helpers.

+chandlerc, WDYT? Is there a better place to put this?

lib/Transforms/Instrumentation/MemorySanitizer.cpp
2276

OK. :) I'll commit that as a separate change without any of the actual musttail handling stuff.

rnk updated this revision to Diff 11231.Jul 9 2014, 4:26 PM

rebase

rnk closed this revision.Aug 11 2014, 5:21 PM
rnk updated this revision to Diff 12374.

Closed by commit rL215415 (authored by @rnk).