SRET consistency between declaration and call site

Hello,
while debugging assertions when building libm for 32bit Sparc, I hit the
following IR:

  complex_mul_libcall:
    call void @__muldc3({ double, double }* sret %tmp, double %conv, double 0.000000e+00, double %a.real, double %a.imag) #2

  ...

  declare void @__muldc3({ double, double }*, double, double, double, double)

The same IR is essentially generated for i386 too, so it is not Sparc
specific. Unlike i386, Sparc has an assertion in its codegen that the
function called must have the sret attribute on corresponding argument.

The question is now: is the Sparc assertion correct? If yes, this
inconsistency should be checked in the IR verifier, IMO.

Joerg

Hi Joerg,

while debugging assertions when building libm for 32bit Sparc, I hit the
following IR:

  complex_mul_libcall:
    call void @__muldc3({ double, double }* sret %tmp, double %conv, double 0.000000e+00, double %a.real, double %a.imag) #2

  ...

  declare void @__muldc3({ double, double }*, double, double, double, double)

The same IR is essentially generated for i386 too, so it is not Sparc
specific. Unlike i386, Sparc has an assertion in its codegen that the
function called must have the sret attribute on corresponding argument.

I think we should view this as a special case of bitcasting functions
to different types (not quite types because it turns out function
attributes aren't part of the type in IR, but morally the same thing).
LLVM allows those casts normally.

It's more difficult to justify for actually declared globals (rather
than random pointers), but I'm still reluctant to ban it outright.

Tim.

Well, we do consider it an ABI changing attribute for the purpose of
tail call (under "musttail") and we are loosing the dereferencable
information. Removing the assert is easy, I just want to make sure there
is consensus it is the right thing to do.

Joerg

I think the assertion is probably overzealous and would fire when it
shouldn't in an LTO setting, but it sounds like it also caught a real clang
bug.

This seems like one of those things that's supposed to be in the "lint"
pass, but isn't, because lint isn't part of the normal clang pass pipeline.

Can you give an example of where it would trigger in LTO and when should
not?

Joerg

You could imagine that __muldc3 might be implemented in C, and it might be
implemented without using _Complex so that it can be compiled by a compiler
without _Complex support. Instead of using a _Complex double return type,
it would use a pointer outparam as the first parameter, and it would return
that pointer as usual in RAX. Yes, this is a prototype mismatch and
probably UB according to C, but this might be part of an implementation
which knows something about the platform lowering.

Generally we try to make these sorts of things "work". For example, if you
pass parameters to a function pointer call and we later discover the
function takes no parameters, instcombine will discard the arguments rather
than making the call unreachable. IMO the same logic applies to ABI
attributes like sret.

This argument is really not convincing to me. If you have LLVM IR, you
are starting from a frontend that should be supporting complex already.
This really sounds like a case that should be flagged to fix up legacy
code and not something to silently hide. Does code like this (manually
turning structure returns into explicit pointers) actually exist? Note
that the normal way for doing the complex thing is to define a two
element structure, which would still give the sret.

Joerg

Basically, LLVM shouldn't assert if Clang produces prototype mismatches,
because they happen in practice. Clang tries really hard to avoid prototype
mismatches. It even has it's own set of assertions in call lowering to
detect them. However, at the end of the day, sometimes it has incomplete
function pointer type information and mismatches happen. An ABI attribute
mismatch is just another type mismatch, but it doesn't require bitcasts, so
it's less obvious.

I mention LTO because that's where most prototype mismatches come up: one
TU saw a complete pointer to parameter type and the other didn't, and now
things look quite different. I'm just saying it happens, and LLVM should
try to be careful.

Certainly a mismatch between sret and not-sret from caller to callee could not possibly work on sparc, because sparc’s calling convention for struct return is totally bonkers.

If you’re doing an sret return, you need to return to the call address +12, instead of +8, because the instruction at the usual caller+8 return address is expected to be a trap instruction.

Said trap also has the size of the struct encoded in it, and is there in order that it can be inspected by the callee to verify that the size of the struct the caller allocated is the same as what the callee expected. (Most impls don’t bother with said verification step, because, really, wtf is the point.)

So if the caller thinks it’s calling a non-sret function, but it is actually calling an sret function, it’ll put code at +8, and the callee will skip that. Conversely, if the caller thinks it’s calling an sret function, and the function is not actually, the function will return into the trap at +8. So, sure, llvm doesn’t actually need to detect this mismatch, but the generated code is guaranteed to be busted if there is such a mismatch, so really nobody should ever be doing that.

It won't work on AArch64 either: it uses a fixed register to pass struct return addresses, which is not used for other parameters under any circumstances.

Jonas

That seems to make it a pretty good case for consider sret mandatory in
general.

Joerg

That seems to make it a pretty good case for consider sret mandatory in
general.

(Still) no more than any other type cast IMO. If you know what you're
doing you can cast function pointers (at the LLVM level), otherwise
you're going to get into trouble.

Tim.

I think we are talking about two different things here. What I am
advocating is that callsite and function signature must have matching
ABI. I think we already enforce that for things like argument type, so
enforcing sret doesn't seem to be any different. I am not saying that
you should not be able to override it via manual bitcasting, but that's
a completely different topic. If you do that, you are on your own.

Joerg

Unfortunately the two aren't separable because the sret isn't part of
the function's type. You can't even write down a bitcast that adds or
removes it:

$ cat simple.ll
define void @foo(void(i8*)* %fn) {
  %tst = bitcast void(i8*)* %fn to void(i8* sret)*
  ret void
}
$ opt -verify simple.ll -S
opt: simple.ll:2:41: error: argument attributes invalid in function type
  %tst = bitcast void(i8*)* %fn to void(i8* sret)*

So if you ban mismatches at call sites, you're banning the casts too.

Cheers.

Tim.

I agree. Maybe a different analogy based on an FAQ[1] will work. sret is
similar to the function calling convention. The function calling convention
is, for reasons that I don't agree with, not part of the function type.
Therefore the verifier accepts direct calls to functions with mismatched
calling conventions, because it's not a type mismatch. The optimization
passes, however, will transform that call to unreachable.

I argue the same applies to sret. Because the verifier does not reject it,
we cannot assert on a mismatch later. We have to either make sret part of
the type system, so that mismatches cannot occur without a cast, or we have
to silently accept it and generate code that's probably incorrect.

[1]
http://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

Yuck. Yes, that’s a very compelling argument. :slight_smile: It really does seem that the attributes which are required to match should be part of the function type. Oh well – removing the assert is definitely the way to go, then, in the current model.

(And fixing clang so it emits the proper declare would be nice too, but I guess it doesn’t really matter, since that isn’t actually used for anything.)