I am trying to prune illegal call targets and as such I need to define what is a legal call (target).
I have a very (=too) aggressive first draft for this in ⚙ D155900 [TTI][NFCI] Introduce two new target transform hooks, however, I really need input on the things we want to allow.
Basically, I’d like us to answer questions like these (and talk about it in the lang ref):
Are missing arguments OK?
Are excess arguments OK?
Can we mismatch the return/parameter type, if so, to what degree?
Are mismatches in ABI-related attributes OK (sext, byval, …)?
What about implicit non-trivial casts, e.g. non-no-op AS casts?
The LangRef already states a number of rules for compatibility. The call site and target function calling conventions need to match (InstCombine will delete straightforward mismatches as-is). The tail calling conventions have more relaxed ABI attribute rules.
‘fnty’: shall be the signature of the function being called. The argument types must match the types implied by this signature.
Var args is special:
The callee must be varargs iff the caller is varargs. Bitcasting a non-varargs function to the appropriate varargs type is legal so long as the non-varargs prefixes obey the other rules.
This one is tricky:
The return type must not undergo automatic conversion to an sret pointer.
Can you please explain what the motivation for this is? I mean, I can see the benefit in theory, but I’m not clear on where this would help in practice.
The benefit in the closed world scenario is not theoretical, in the open world scenario it might be but it’s hard to say w/o data. If our tests are any indication, it clearly will have an impact as it eliminates way more UB calls.
This seems like a very dangerous change to me. If we want to do this, I’d recommend to go for a default implementation of “always valid” and then carve out very specific exceptions based on real-world motivation.
That is fair and the reason I started this thread. I am unsure if we really need to be this conservative about it, we would not in other places where we delete UB calls already. As long as we document what is legal, and adapt based on feedback, I don’t see the difference to InstCombine deleting calls.
Two immediate issues I see here are:
On x86_32 at least, clang will pass arguments by value, even though they really have indirect byval ABI. The reasoning is that LLVM will actually lowering those arguments with the correct ABI, and passing values as SSA values is of course much more amenable to optimization. However, Rust will instead generate the “correct” byval ABI. These calls are incompatible at the IR level (you will have a byval ptr argument on one side an something like i32 on the other – or worse, a non-byval ptr that has a completely different meaning!) but become compatible after lowering. Clang is doing something extremely fishy here, but we still need to make sure that this does not get miscompiled (as your current code will do, on multiple levels).
Right. Arguably, this can help us clean up frontends and avoid some “shady” IR. Given that IPO and inline are less likely to touch those calls, I very much doubt the “much more amenable to optimization part”.
For now, I can allow byval on one side and something else on the other.
(Would you happen to have an example for this? I might open a bug.)
There are recurring problems with callee vs caller ABI attribute mismatches. IIRC there were some attempts to clean this up as part of the opaque pointer migration, by making lowering only consider callee ABI attributes. However, this ran into a whole series of issues with attribute mismatches in practice, and ended up getting reverted. Maybe @aeubanks has more context on that. Missing signext/zeroext attributes are also a recurring problem for targets like s390x. And this also plays into the previous point: A ptr with and without byval are not necessarily an ABI mismatch after lowering.
We can remove “rules” from my patch if we define them to be “fine”. Or we move them into target specific hooks if only some targets are OK with them.
I’m also worried about the cross-language LTO cases, since there aren’t any published rules for how to lower a particular C ABI construct to LLVM IR beyond “copy whatever clang does”. It’s very easy to do something different from clang as long as the right values end up in the right registers.
Does -fsanitize=cfi catch all the calls that would be optimized out by this optimization?
Missing arguments are OK so long as there are paths through the subroutine that do not use any of the missing arguments.
Excess arguments are OK so long as the source language does not forbid. {{So, C would allow, and PASCAL would not.}}
Mismatch of return result is OK so long as it does not cross strong typing boundaries
{{ ints stay ints, floats stay floats, pointers stay pointers }}
ABI related attributes is more tricky:: But certainly by-reference can be mimicked with either a pointer or a dope-vector (and indeed this is how FORTAN on PCC worked.) First order, if the caller is placing the argument in a container of some size and type appropriate for the callee to consume, you should be gentle here.
All inobvious non-no-op casting needs to be on the caller side (my opinion).
I updated ⚙ D155900 [TTI][NFCI] Introduce two new target transform hooks to take a lot of this into account. No pure attribute-based deduction is happening anymore by default. Most UB is derived from UB due to noundef. I still believe this will be really helpful in closed world scenarios as lots of function with too many arguments can be ruled out, as well as those returning the wrong type, etc.
‘fnty’: shall be the signature of the function being called. The argument types must match the types implied by this signature. This type can be omitted if the function is not varargs.
This talks about the type annotation on the call base. Which is gone with opaque pointers. Even before, it did not say you cannot cast the function to whatever type. It’s basically the check the verifier used to run.
The crux is, what does “match” mean. It probably isn’t perfect match, so where is the line.
I think for now, we can start with the weak approach and see how well that solves our problems.