RFC: Requiring callinst function type to match the type of the function being called?

IR & Optimizations
1

I’ve just seen if (CB->getFunctionType() != F.getFunctionType()) return false; written in another place and thought I’d ask if we can make that a verifier error instead.

Currently the type of the callee of a call instruction can be different from the type of the function being called. That’s sometimes OK from a codegen perspective, in target dependent fashion, and it’s probably something we can get out of C codegen in some edge cases. I believe the current perspective is roughly:

  1. The function type indicates what we do with the prolog, where it gets arguments from etc
  2. The callee type indicates what we do with the call site, where we put arguments etc

So provided they kind of match, it’s alright. E.g. we pass too few arguments, the function reads whatever was in the higher register anyway.

I’m not totally sure that’s the proper semantics for the compiler IR though. We could require an exact match and emit hackery at the call site to paste over divergence, e.g. passing undef for arguments that were missing. I suspect that would mostly show up when devirtualising functions.

Thoughts welcome :slight_smile:

There’s good context in Verifier Pass doesn't catch calls with different number of parameters than the definition - #4 by nikic, specifically

IR needs to have a well-formed (but not necessarily well-defined) way to call a function with a signature that differs from its declaration.

^That’s the current state of the system but it’s not clear to me that it’s a feature.

2

Consider something like this:

define void @call_fn(ptr %fn) {
  call void %fn(i32 0)
  ret void
}

declare void @fn(float)

define void @do_call_fn() {
  call void @call_fn(ptr @fn)
  ret void
}

At this point, the call is on an SSA value for which we don’t know the function type. This must be allowed. Then, consider how this looks like after inlining:

declare void @fn(float)

define void @do_call_fn() {
  call void @fn(i32 0)
  ret void
}

Now we have a call to a known function with mismatching function type. This would fail your IR verifier check.

More generally, we cannot really have any IR well-formedness constraints that differ between an arbitrary SSA value and a constant, because this essentially breaks RAUW.

Apart from this more generic concern, I’ll add that “fixing up call site divergence” is very hard, because it requires a precise understanding of the (target-specific) calling convention lowering. And it’s important for certain cases (like cross-language LTO) that this is handled precisely and not just in some approximate fashion that satisfies the IR verifier.

3

This should not be a verifier error. Statically known undefined behavior or undefined behavior-like cases cannot be an IR verifier error. Doing so imposes complexity on every IR transform, which would then need to guard against introducing the invalid pattern, which could have logically existed in the source program.

These kind of cases should be diagnosed with the lint pass

4

This is partly a consequence of the opaque pointer migration. Before the opaque pointer migration, there would be a bitcast wrapping the call target, so mid-level passes would treat these calls as “indirect”, which would block most interesting IPO transforms. Now that we don’t have such casts, mid-level passes need these checks to confirm that the prototypes line up.

However, I wonder if calls themselves should internally carry some notion of whether the call is direct or indirect, rather than implying it from the types. This would allow us to, for example, include checks that the relevant ABI attributes (byval, sret, inreg) match between caller and callee. Currently there are cases where the prototypes match, but the attributes don’t, and IR handling passes can get confused (IIRC this had to do with byval and transparent_union, but I couldn’t find the issue). “Direct” calls could have verifier checks, and calls would only transition from indirect to direct after validating that they meet the requirements. It would make it invalid to RAUW a Function with non-Function Constants, which is perhaps not so great, so this may not be such a great idea.

Regardless, to Jon’s original point, it would be nice if we had a simpler, more readable check for call-direct-ness.

5

Most code uses CallBase::getCalledFunction() to do these checks.

We could do this, maybe? There’s some potential oddness here:

  • There are now more ways to define a “direct” call.
  • What does the API actually look like? Maybe we can do some magic with FunctionCallee to try to hide what’s happening in the common case?
  • LTO needs to do something complicated to avoid generating invalid code.
  • We need an explicit IR optimization to do indirect->direct transforms. When do we run this?
  • Like you mention, RAUW from direct->indirect doesn’t work… but we usually try to avoid doing that anyway.
6

I agree with the “one must not break RAUW” premise. We have perhaps lost a little information here in the opaque pointer conversion, but probably nothing significant.

I think these are both true and that strikes me as a problem. I see lots of getCalledFunction, which checks the function type, but I don’t see where that could be checking the attributes. There’s some danger we’re getting away with that somewhat by luck but hopefully I’m missing something.

Indirect calls have a collection of limitations and calls though mismatched type are either going to be UB or some target specific quirk which may need special casing. Direct calls are somewhat different from a transform perspective. We can do things like mutate call site and caller together (at least when static/internal). I’d like a (cheap and widely used) test on IR which amounts to “is this a call to a known thing, with matching callee and caller type, and matching attributes, and if necessary matching metadata”. That might be a safer thing to call than getCalledFunction, or an extra check to put within getCalledFunction if so inclined.

Suggestions:

  • Add a function to CallBase which checks type and attributes
  • Add a bit somewhere that caches the result of the above
  • Add CallBase derivative, CallK or CallDirect or whatever, which we derive from a call when the checks pass

Thoughts? Provided we don’t go in the direction from known-well-typed to wrong-typed or unknown, which seems likely to be an error anyway, any of those would work out. I especially like the call-known variant as that would be a legitimate IR invariant, and if it turns out that we’re missing attribute checks all over the place as the above somewhat suggests, it’ll burn those errors out.

(if we get a new call instruction or if we do the hidden bit thing instead the call instruction constructor and instcombine are the obvious places to put it. We create the more specific one where we already have the information and we have instcombine record it for when the information turns up later, and RAUW is undisturbed except if someone wants to replace a known symbol with an ssa value using it, which they probably shouldn’t be doing by accident)

7

If we want to check that ABI attributes match between caller and callee, I think the way to do that is to make them part of the function type. Attributes currently mixing optimization and ABI properties is pretty iffy in general.

One thing to keep in mind here is that attributes from the callee generally get inherited to the call-site (if you use the normal call-site APIs). I think that’s why this problem typically does not come up in practice.

8

I’m still thinking about this in the background.

My instinct is to put more information in the function type, which agrees with moving from attributes to the type. However I think that instinct is wrong.

We’ve gone the other way with data types, where now things are ptr and an addrspace, with no particular type information. We’ve got type information associated with call instructions (in particular the parameters), and also sometimes have a callee type, where they might disagree. If we decreased the information associated with the callee, perhaps all the way to a ptr and addrspace, I think we should still be fine. I’ve gone trawling for context from the opaque ptr introduction and can’t find anything calling out this case in particular, but google search is not what it was. Maybe I should try that as a refactor, find out the hard way if anything breaks.

So two things. 1, we might be able to scrap the callee function type on call instructions entirely instead of adding to it 2, this doesn’t address the thing which originally motivated this thread, which is checking for some notion of well formed call repeatedly.

Currently we do work to decide whether we’re calling a known function with arguments (and attributes) which are consistent with that function, for possibly target specific notions of “consistent”. E.g. if the api treats f32 and i32 as the same register, arguably it doesn’t matter if that one is the “wrong” type. We do that work repeatedly, and probably do transforms based on the call being consistent, in which case subsequently changing the call to something with a different type should probably be done cautiously.

We could add a callinst subtype. Call it CallKnown, semantics that of a call except the callee is known and the parameter types and attributes are consistent with the arguments to the call. That the IR verifier can check. Create CallKnown as a means of remembering that the arg checks have already been done, have various passes change from dyn_cast & check type to dyn_cast.

If someone wants to replace the consistently-typed call with an inconsistent one, they can do so by cut&replace. ReplaceAllUsesWith converting unknown calls into known ones without magically updating the call instruction to knowncall seems fine.

It’s work of course and adding an instruction is a nuisance.

9

I’m not sure how that would work. Keep in mind that for indirect calls, the function type (and attributes) on the call is all we have. I don’t see how we could lower the call without the function type. (Well, mostly, if we ignore varargs, the function type is directly implied by the call arguments and the return type of the instruction, so an explicit “function type” is somewhat redundant – but that’s just a question of representation.)

So, in practice, the way to check for a well formed call is to just check that the function types are the same – or use one of the helpers that does it for you, like getCalledFunction(). For the purposes of most code in LLVM, you don’t need to do more than that.

Yes, this does mean that there can be mismatches in ABI attributes, but this is not going to matter unless you are doing some fairly specific things. (Like, for example, we have this extremely dubious InstCombine transform which tries to make the function types match by inserting casts, and in that context things like mismatches in byval attributes can matter. Most of that transform should really get removed though…)

Which isn’t to say that there isn’t a problem here – there is, but it is just one aspect of the larger problem of call ABI representation in LLVM. I could probably fill a book with everything that’s wrong with it. But this is not an area where I’d want to make significant changes without some larger plan.

If we were to make larger changes in this area, the general direction I would probably pursue is having a stronger separation between the function signature (function type) and the way that signature maps to the ABI. That is, the function type would be combined with a separate ABI specification structure, and both being equal would imply a “simple” call.

And by “ABI specification” I don’t just mean saying that this argument should be zero extended, but being able to specify things all the way down to “this i8 argument should be passed as bits 24-32 of register %rcx”.

(There are a lot of motivations to do that: Custom calling conventions, precise control about calling convention at target feature mismatch boundaries, preserving types and optimization attributes that get lost during packing, etc. It would be quite hard to actually pull this off though…)