llvm dev - Jun 2021 - ABI attributes on arguments vs parameters

Currently ABI attributes are weirdly handled if an argument to a function
is missing an ABI attribute that is present on the called
function's corresponding parameter. e.g.

declare void @f(i32* byval(i32))
define void @g() {
 %a = alloca i32
 call void @f(i32* %a) ; missing the byval(i32) attribute
 ret void
}

CallBase::isByValArgument(unsigned ArgNo) forwards to
CallBase::paramHasAttr(), which first checks the argument attributes, then
if the call is a direct call, checks the called function's parameter
attributes. The existing implementation of CallBase::paramHasAttr() makes
sense for optimization attributes like nocapture, but doesn't really make
sense for ABI attributes like byval. It's weird that lowering a call may be
different depending on whether or not the call is direct.

I attempted to only have lowering inspect the ABI attributes on the
argument and not look through at a potentially direct callee, but there
were cases where LLVM was generating direct calls to functions with ABI
attributes but without properly setting the ABI attributes on the
arguments. I fixed a couple of these but ended up reverting everything
since it was still unclear if we wanted to go in this direction.

Should we go down the path of ignoring ABI attributes on direct callees and
only looking at the attributes on the arguments? And in that case we may
generate code that crashes at runtime if ABI attributes don't properly
match. Otherwise we should document the existing behavior in the LangRef.
The LangRef only mandates that ABI attributes match on a musttail call
<https://llvm.org/docs/LangRef.html#id327>.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210622/959dea5c/attachment.html>

Might be worth CC'ing any already interested parties from previous
discussions & linking to those threads (& maybe linking to this thread
from
those ones).

It does seem pretty questionable that behavior changes if a function
becomes indirect - do you have any rough idea of how deep the rabbit hole
goes if we were to try to finish the work you started of not looking at the
callee to determine these attributes?

On Tue, Jun 22, 2021 at 6:06 PM Arthur Eubanks via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Currently ABI attributes are weirdly handled if an argument to a function
> is missing an ABI attribute that is present on the called
> function's corresponding parameter. e.g.
>
> declare void @f(i32* byval(i32))
> define void @g() {
>  %a = alloca i32
>  call void @f(i32* %a) ; missing the byval(i32) attribute
>  ret void
> }
>
> CallBase::isByValArgument(unsigned ArgNo) forwards to
> CallBase::paramHasAttr(), which first checks the argument attributes, then
> if the call is a direct call, checks the called function's parameter
> attributes. The existing implementation of CallBase::paramHasAttr() makes
> sense for optimization attributes like nocapture, but doesn't really
make
> sense for ABI attributes like byval. It's weird that lowering a call
may be
> different depending on whether or not the call is direct.
>
> I attempted to only have lowering inspect the ABI attributes on the
> argument and not look through at a potentially direct callee, but there
> were cases where LLVM was generating direct calls to functions with ABI
> attributes but without properly setting the ABI attributes on the
> arguments. I fixed a couple of these but ended up reverting everything
> since it was still unclear if we wanted to go in this direction.
>
> Should we go down the path of ignoring ABI attributes on direct callees
> and only looking at the attributes on the arguments? And in that case we
> may generate code that crashes at runtime if ABI attributes don't
properly
> match. Otherwise we should document the existing behavior in the LangRef.
> The LangRef only mandates that ABI attributes match on a musttail call
> <https://llvm.org/docs/LangRef.html#id327>.
> _______________________________________________
> 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/20210622/cb1d893e/attachment.html>

I think the trouble we are having comes from the fact that we don't make it
easy for IR producers (frontends or instrumentation passes) to do the right
thing. If we make it easy (almost automatic), then maybe it will be more
reasonable to declare in LangRef that function prototype mismatch is in
fact UB.

Another developer suggested that the IRBuilder should take responsibility
for setting the attributes on the call site, but I don't like the idea of
IRBuilder::CreateCall inspecting the Callee argument with dyn_cast to
decide its behavior.

This might be an infeasibly difficult ABI breaking change, but what if we
had two overloads, one for direct calls, and one for indirect calls? Maybe
we already have that in the current set of overloads:
  CallInst *CreateCall(FunctionType *FTy, Value *Callee,
                       ArrayRef<Value *> Args = None, const Twine
&Name "",
                       MDNode *FPMathTag = nullptr);
  CallInst *CreateCall(FunctionType *FTy, Value *Callee, ArrayRef<Value *>
Args,
                       ArrayRef<OperandBundleDef> OpBundles,
                       const Twine &Name = "", MDNode *FPMathTag =
nullptr);
  CallInst *CreateCall(FunctionCallee Callee, ArrayRef<Value *> Args =
None,
                       const Twine &Name = "", MDNode *FPMathTag =
nullptr);
  CallInst *CreateCall(FunctionCallee Callee, ArrayRef<Value *> Args,
                       ArrayRef<OperandBundleDef> OpBundles,
                       const Twine &Name = "", MDNode *FPMathTag =
nullptr);

Assuming the vast majority of callers use the FunctionCallee overload,
which is implicitly constructible from a Function*, we could extend
FunctionCallee to carry the calling convention and an attribute list. The
CC and the attributes are actually part of clang::FunctionType, they just
aren't part of llvm::FunctionType. FunctionCallee is sort of incomplete
without them.

So, the task would be to add non-optional parameters to the
FunctionType/Value overload (used for indirect calls) and then fix up all
the call sites. Frontends that generate indirect calls would observe this
as build breakage, not changes to generated code. Instrumentation passes
would magically start doing the right thing without any source changes.
This would also address the long-standing usability issue of calling
conventions [1], which are optimized to unreachable if you forget to set
the calling convention on the call site.

[1]
https://llvm.org/docs/FAQ.html#why-does-instcombine-simplifycfg-turn-a-call-to-a-function-with-a-mismatched-calling-convention-into-unreachable-why-not-make-the-verifier-reject-it

---

Having written this up, this seems like a good direction. Credit goes to
the person that originally suggested the IRBuilder approach, I just had to
think harder about it. :)

On Tue, Jun 22, 2021 at 6:06 PM Arthur Eubanks via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Currently ABI attributes are weirdly handled if an argument to a function
> is missing an ABI attribute that is present on the called
> function's corresponding parameter. e.g.
>
> declare void @f(i32* byval(i32))
> define void @g() {
>  %a = alloca i32
>  call void @f(i32* %a) ; missing the byval(i32) attribute
>  ret void
> }
>
> CallBase::isByValArgument(unsigned ArgNo) forwards to
> CallBase::paramHasAttr(), which first checks the argument attributes, then
> if the call is a direct call, checks the called function's parameter
> attributes. The existing implementation of CallBase::paramHasAttr() makes
> sense for optimization attributes like nocapture, but doesn't really
make
> sense for ABI attributes like byval. It's weird that lowering a call
may be
> different depending on whether or not the call is direct.
>
> I attempted to only have lowering inspect the ABI attributes on the
> argument and not look through at a potentially direct callee, but there
> were cases where LLVM was generating direct calls to functions with ABI
> attributes but without properly setting the ABI attributes on the
> arguments. I fixed a couple of these but ended up reverting everything
> since it was still unclear if we wanted to go in this direction.
>
> Should we go down the path of ignoring ABI attributes on direct callees
> and only looking at the attributes on the arguments? And in that case we
> may generate code that crashes at runtime if ABI attributes don't
properly
> match. Otherwise we should document the existing behavior in the LangRef.
> The LangRef only mandates that ABI attributes match on a musttail call
> <https://llvm.org/docs/LangRef.html#id327>.
> _______________________________________________
> 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/20210623/946d0484/attachment.html>