[llvm-dev] Maintaining musttail call contract

[Philip Reames via llvm-dev]

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!
>