Hi,
A musttail call must be preceded by an optional bitcast instruction
and a return instruction. It seems that there isn't a mechanism to
make sure the musttail call contract isn't broken by any instruction
insertion in the IR.
As far as I can tell, all IR transformations don't really look at
what's before a ReturnInst when inserting instructions. For example,
all Instrumentation passes ignored them, and would insert
instrumentation instructions in between musttail call and ReturnInst,
which would break the contract.
I fixed it for ASAN in https://reviews.llvm.org/D87777 and fixed it
for TSAN in https://reviews.llvm.org/D87620.
But lately I also found that SafeStack pass has the same problem. For
instance, running "opt --safe-stack" on the following IR would crash
Clang:
target triple = "x86_64-unknown-linux-gnu"
declare i32 @foo(i32* %p)
declare void @alloca_test_use([10 x i8]*)
define i32 @call_foo(i32* %a) safestack {
%x = alloca [10 x i8], align 1
call void @alloca_test_use([10 x i8]* %x)
%r = musttail call i32 @foo(i32* %a)
ret i32 %r
}
I am pretty sure that SafeStack isn't the last pass that has this
problem and there will be more like this in the future.
I am trying to see if there are good systematic ways to solve this
problem without having to require every pass author to be careful when
inserting instructions before ReturnInst.
One solution I can think of is to update
IRBuilderBase::SetInsertPoint(Instruction *I) to always check if "I"
is a ReturnInst followed by a musttail call contract, and if so, move
the insertion point to before the call. Since this approach adds
overhead to every insertion point creation, I am not sure if it's the
best solution.
Advice and feedback much appreciated!
--
Xun
Johannes Doerfert via llvm-dev
2020-Oct-31 22:44 UTC
[llvm-dev] Maintaining musttail call contract
Hi Xun, On 10/30/20 9:47 PM, Xun Li via llvm-dev wrote:> Hi, > > A musttail call must be preceded by an optional bitcast instruction > and a return instruction. It seems that there isn't a mechanism to > make sure the musttail call contract isn't broken by any instruction > insertion in the IR. > As far as I can tell, all IR transformations don't really look at > what's before a ReturnInst when inserting instructions. For example, > all Instrumentation passes ignored them, and would insert > instrumentation instructions in between musttail call and ReturnInst, > which would break the contract. > I fixed it for ASAN in https://reviews.llvm.org/D87777 and fixed it > for TSAN in https://reviews.llvm.org/D87620. > But lately I also found that SafeStack pass has the same problem. For > instance, running "opt --safe-stack" on the following IR would crash > Clang: > > target triple = "x86_64-unknown-linux-gnu" > > declare i32 @foo(i32* %p) > declare void @alloca_test_use([10 x i8]*) > > define i32 @call_foo(i32* %a) safestack { > %x = alloca [10 x i8], align 1 > call void @alloca_test_use([10 x i8]* %x) > %r = musttail call i32 @foo(i32* %a) > ret i32 %r > } > > I am pretty sure that SafeStack isn't the last pass that has this > problem and there will be more like this in the future. > I am trying to see if there are good systematic ways to solve this > problem without having to require every pass author to be careful when > inserting instructions before ReturnInst. > > One solution I can think of is to update > IRBuilderBase::SetInsertPoint(Instruction *I) to always check if "I" > is a ReturnInst followed by a musttail call contract, and if so, move > the insertion point to before the call. Since this approach adds > overhead to every insertion point creation, I am not sure if it's the > best solution. > > Advice and feedback much appreciated!I am not a fan of the IRBuilder solution, it doesn't cover all the cases as not everything uses the builder and instructions can be moved, and it will be some behind the scenes magic that could cause other things to break silently. My suggestion: 1) Make the verifier aware of the restriction, it will flag broken IR and catch (almost) all cases (I would assume). 2) If, for some reason I can't imagine right now, we build this broken IR and want to keep it "for a while", write a late pass that tries to "repair" the IR but only if it can keep the semantics otherwise intact, so it cannot unconditionally move instructions but only if it is otherwise correct anyway. If it can't, abort. ~ Johannes
Philip Reames via llvm-dev
2020-Nov-01 20:53 UTC
[llvm-dev] Maintaining musttail call contract
I just skimmed your message, so I might be missing something, but first impression would be that this sounds like a missing verifier rule. Any reason the verifier can't check for this legality restriction? If it can, then the existing verify after all functionality should be sufficient to catch the bugs you're referencing? Philip On 10/30/20 7:47 PM, Xun Li via llvm-dev wrote:> Hi, > > A musttail call must be preceded by an optional bitcast instruction > and a return instruction. It seems that there isn't a mechanism to > make sure the musttail call contract isn't broken by any instruction > insertion in the IR. > As far as I can tell, all IR transformations don't really look at > what's before a ReturnInst when inserting instructions. For example, > all Instrumentation passes ignored them, and would insert > instrumentation instructions in between musttail call and ReturnInst, > which would break the contract. > I fixed it for ASAN in https://reviews.llvm.org/D87777 and fixed it > for TSAN in https://reviews.llvm.org/D87620. > But lately I also found that SafeStack pass has the same problem. For > instance, running "opt --safe-stack" on the following IR would crash > Clang: > > target triple = "x86_64-unknown-linux-gnu" > > declare i32 @foo(i32* %p) > declare void @alloca_test_use([10 x i8]*) > > define i32 @call_foo(i32* %a) safestack { > %x = alloca [10 x i8], align 1 > call void @alloca_test_use([10 x i8]* %x) > %r = musttail call i32 @foo(i32* %a) > ret i32 %r > } > > I am pretty sure that SafeStack isn't the last pass that has this > problem and there will be more like this in the future. > I am trying to see if there are good systematic ways to solve this > problem without having to require every pass author to be careful when > inserting instructions before ReturnInst. > > One solution I can think of is to update > IRBuilderBase::SetInsertPoint(Instruction *I) to always check if "I" > is a ReturnInst followed by a musttail call contract, and if so, move > the insertion point to before the call. Since this approach adds > overhead to every insertion point creation, I am not sure if it's the > best solution. > > Advice and feedback much appreciated! >
Reid Kleckner via llvm-dev
2020-Nov-02 19:03 UTC
[llvm-dev] Maintaining musttail call contract
IIRC, we do already have a verifier rule for this. At least, I'm pretty sure I added one. I was going to say, passes are supposed to handle this by checking ReturnInst::isPrecededByMustTailCall, but apparently that helper doesn't exist yet! I just imagined it. :) Let's add it. I think if we had such a predicate, one that scans backwards skipping debug intrinsics and casts looking for a musttail calls or any other non-cast, non-musttail call instruction, it would make it easier for passes to get this right. You are right, though, this construct is designed in a way that makes it hard for instrumentation passes to be correct by default, and it's not their fault that they are all "broken" according to the LangRef. The only other implementation strategy I can think of for this feature is to have a special "tail call" instruction that is both a call and return, since that's really what we are expressing with two separate instructions. That has a different set of tradeoffs, not all beneficial. On Sun, Nov 1, 2020 at 12:53 PM Philip Reames via llvm-dev < llvm-dev at lists.llvm.org> wrote:> I just skimmed your message, so I might be missing something, but first > impression would be that this sounds like a missing verifier rule. Any > reason the verifier can't check for this legality restriction? If it > can, then the existing verify after all functionality should be > sufficient to catch the bugs you're referencing? > > Philip > > On 10/30/20 7:47 PM, Xun Li via llvm-dev wrote: > > Hi, > > > > A musttail call must be preceded by an optional bitcast instruction > > and a return instruction. It seems that there isn't a mechanism to > > make sure the musttail call contract isn't broken by any instruction > > insertion in the IR. > > As far as I can tell, all IR transformations don't really look at > > what's before a ReturnInst when inserting instructions. For example, > > all Instrumentation passes ignored them, and would insert > > instrumentation instructions in between musttail call and ReturnInst, > > which would break the contract. > > I fixed it for ASAN in https://reviews.llvm.org/D87777 and fixed it > > for TSAN in https://reviews.llvm.org/D87620. > > But lately I also found that SafeStack pass has the same problem. For > > instance, running "opt --safe-stack" on the following IR would crash > > Clang: > > > > target triple = "x86_64-unknown-linux-gnu" > > > > declare i32 @foo(i32* %p) > > declare void @alloca_test_use([10 x i8]*) > > > > define i32 @call_foo(i32* %a) safestack { > > %x = alloca [10 x i8], align 1 > > call void @alloca_test_use([10 x i8]* %x) > > %r = musttail call i32 @foo(i32* %a) > > ret i32 %r > > } > > > > I am pretty sure that SafeStack isn't the last pass that has this > > problem and there will be more like this in the future. > > I am trying to see if there are good systematic ways to solve this > > problem without having to require every pass author to be careful when > > inserting instructions before ReturnInst. > > > > One solution I can think of is to update > > IRBuilderBase::SetInsertPoint(Instruction *I) to always check if "I" > > is a ReturnInst followed by a musttail call contract, and if so, move > > the insertion point to before the call. Since this approach adds > > overhead to every insertion point creation, I am not sure if it's the > > best solution. > > > > Advice and feedback much appreciated! > > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20201102/5b41b5af/attachment.html>