Verifier regression for caller/callee after enabling opaque pointers

[joker-eph]

I've been tracking a simple case like:

declare void @foo(double %0)
define void @caller(i32 %i) {
  call void @foo(i32 %i)
  ret void
}

We expect the verifier to complain:

bad.ll:3:13: error: '@foo' defined with type 'void (double)*' but expected 'void (i32)*'

And it was until LLVM 15, I bisected to 41d5033 which enabled opaque pointer.

[efriedma-quic]

This is expected. Before opaque pointers, you'd need a bitcast to make a call like this, but with opaque pointers, such a bitcast would be a no-op.

opt -passes=lint will give you a warning about the undefined behavior, if that's what you're looking for.

[joker-eph]

I don't actually understand why a direct function call can't be enforced in the verifier as it has always been? We should have direct access to the function type here don't we?

I don't believe LangRef allows this right now.

[efriedma-quic]

Suppose you have the following:

declare void @foo(double %0)
@z = constant ptr @foo
define void @caller(i32 %i) {
  %p = load ptr, ptr @z
  call void %p(i32 %i)
  ret void
}

And you run this through opt -passes=early-cse. What are you proposing the output should look like?

[joker-eph]

I would consider that turning an indirect call into a direct call would be a transformation that would require introducing a bitcast.

I anticipate that the rebuttal will be that that this is just a RAUW operation here, and that's not generally something that is expected: RAUW isn't changing any type and it shouldn't be possible to create invalid IR from valid IR this way.

But I still see this as a non-trivial defect from the opaque pointer design right now: it's not clear to me how the original motivation for opaque pointers as I remember it justifies such a "hole" in the static IR consistency.

[efriedma-quic]

Internally, the compiler treats call void @foo(i32 %i) with a mismatched type as an indirect call, so compiler optimizations don't get confused. And when you construct a call using IRBuilder, I think we verify that the type of the call matches the type of the arguments. So as a practical matter, I don't think there's much chance for misuse in C++ code.

We could mess with the way we print/parse textual IR, I guess. Maybe add a "indirect" flag to calls in textual IR. So call indirect void %p(i32 %i) or call indirect void @foo(i32 %i) would be valid, but call void @foo(i32 %i) isn't valid. It doesn't seem compelling to me, but I can see why you might want it.

[nikic]

Right, from an IR perspective everything is working perfectly here. We distinguish "direct" calls by whether they call a function with matching signature. Most users get that conservative behavior by dint of using the getCalledFunction() API, which enforces a matching signature. More sophisticated users drop down to getCalledOperand() and perform their own legality checks.

I can see how a human reader might find it hard to spot that a call is using a mismatched signature though. I'd be open to making this more obvious in printed IR, e.g. by putting a ; mismatched signature comment next to such calls.

[joker-eph]

I wouldn't say "everything is working perfectly", but I can see how it would have required more effort in the design of opaque pointer to preserve the stronger guarantee that existed before about direct calls. I still believe this is design work that should have been done: so far it seems to me that the IR consistency has just been weakened in the process.
I find the concept of "we distinguish "direct" calls by whether they call a function with matching signature" rather an unsatisfactory property in the IR actually.

I don't expect anyone to go back and improve this right now, but still there is nothing about what you're describing here that I can figure out by reading LangRef: https://llvm.org/docs/LangRef.html#call-instruction ; this seems just "tribal knowledge" where "the implementation became the spec" at some point, without the spec actually be updated.

Here are the relevant bits to me:

‘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.
‘fnptrval’: An LLVM value containing a pointer to a function to be called. In most cases, this is a direct function call, but indirect call’s are just as possible, calling an arbitrary pointer to function value.

I can see how a human reader might find it hard to spot that a call is using a mismatched signature though. I'd be open to making this more obvious in printed IR, e.g. by putting a ; mismatched signature comment next to such calls.

Direct and indirect call seems different enough in nature to me that this should likely be more prominent than a comment and look like a keyword, Eli's call indirect void @foo(i32 %i) proposal seems better from this point of view.
Also "mismatched signature" does not clearly indicate anything about the nature of the call (indirect vs direct).

[nikic]

I feel like the core misunderstanding here is that you think IR has an intrinsic notion of direct vs indirect calls. Maybe this is different in MLIR land, but LLVM IR has no such distinction. In LLVM IR, all calls are the same. (The only exception are intrinsic calls, which have special requirements -- but that's orthogonal to the issue at hand.)

The distinction between direct and indirect calls is not an IR notion, it's a concept introduced by optimization constraints: Many optimizations only work on calls to known functions, so we like to split calls into two buckets: Direct and indirect ones. However, there is no fundamental difference between them, and a direct call can turn into an indirect call or vice versa just by dint of replacing the callee operand.

Whether optimizations are prepared to handle a call to a known function with mismatching signature depends on the optimization. Some of them do handle this (e.g. something working on function attributes doesn't care), many don't (either because they can't or because it's not worth the bother).

Regarding your LangRef quote, I don't think it contradicts anything said here. The section you're quoting refers to a rarely used syntax where the function type is explicitly specified, e.g. call i32(i32) @foo(i32 0) is legal, but call i32(i32) @foo(ptr null) is not, because the types between the explicitly specified signature and the call arguments differ. However, this has no relation to the signature of the function being called. (This syntax is normally only used for varargs functions.)

It may make sense to add an example that shows that the signatures can differ, but I don't think any normative changes are necessary.

Finally, in regards to the indirect marker, this goes back to what I said before: IR has no intrinsic notion of indirect calls, and this makes it look like it does. I think this would be pretty confusing, especially as the term "indirect" is not really well defined. For example "indirect" in the sense of CallBase::isIndirectCall() considers calls with mismatching signatures to be "direct" -- and this has always been the case, even prior to opaque pointers.

[joker-eph]

I feel like the core misunderstanding here is that you think IR has an intrinsic notion of direct vs indirect calls. Maybe this is different in MLIR land, but LLVM IR has no such distinction. In LLVM IR, all calls are the same. (The only exception are intrinsic calls, which have special requirements -- but that's orthogonal to the issue at hand.)

There is no misunderstanding: you're taking a very "literal" reading of the IR by saying "this is the same C++ class so we don't make a distinction".
I'm taking a more conceptual angle where there is a clear distinction: the purpose of the IR is to sustain transformations, and the way we handle a direct vs indirect call is quite central to any transformation that operates on this instruction.

The distinction between direct and indirect calls is not an IR notion, it's a concept introduced by optimization constraints: Many optimizations only work on calls to known functions, so we like to split calls into two buckets: Direct and indirect ones. However, there is no fundamental difference between them, and a direct call can turn into an indirect call or vice versa just by dint of replacing the callee operand.
Whether optimizations are prepared to handle a call to a known function with mismatching signature depends on the optimization

This didn't exist before opaque pointers though did it? It seems to me that the concept of "IR validity" changed here.

Finally, in regards to the indirect marker, this goes back to what I said before: IR has no intrinsic notion of indirect calls, and this makes it look like it does. I think this would be pretty confusing, especially as the term "indirect" is not really well defined. For example "indirect" in the sense of CallBase::isIndirectCall() considers calls with mismatching signatures to be "direct" -- and this has always been the case, even prior to opaque pointers.

I guess that goes with my overall point: this all look like an IR design issue here, I see inconsistency and fragility involved.

[nikic]

This didn't exist before opaque pointers though did it? It seems to me that the concept of "IR validity" changed here.

No, this did exist before opaque pointer as well. It was just spelled differently. Previously we had the following three cases:

1. Callee is a function ("direct").
2. Callee is a function after stripping bitcasts.
3. Callee is not a function even after stripping bitcasts ("indirect").

Now we have the following equivalent case:

1. Callee is a function with matching signature ("direct").
2. Callee is a function.
3. Callee is not a function ("indirect").

The meaning of plain "callee is a function" has changed, but the different cases are the same, and we need all three of those cases in different places, based on differing capabilities and correctness requirements across transforms.