Call Continuation handlers with strong data type - #13830

Thanks for tackling this. Fixing the UB in handler dispatch and comparison is worthwhile. Two things:

1. The QUIC handlers still go through a mismatched member-pointer type

These calls still cast to NetVConnHandler, i.e. int (UnixNetVConnection::*)(int, void *):

• src/iocore/net/QUICNetVConnection.cc: lines 81, 253, 356
• src/iocore/net/OpenSSLQUICNetVConnection.cc: lines 102, 313, 418

SET_HANDLER((NetVConnHandler)&QUICNetVConnection::acceptEvent);

2. An alternative: a per-handler trampoline

You may have already considered this, but here's an alternative. Every in-tree SET_HANDLER argument is a compile-time constant &X::y. That means the handler can be a non-type template argument, and we only need to store a single function pointer:

using ContinuationHandler = int (*)(Continuation *, int, void *);

template <typename> struct handler_traits;
template <typename D, typename T> struct handler_traits<int (D::*)(int, T *)> {
  using cls  = D;
  using data = T;
};

template <auto M>
int
trampoline(Continuation *c, int event, void *data)
{
  using Tr = handler_traits<decltype(M)>;
  return (static_cast<typename Tr::cls *>(c)->*M)(event, static_cast<typename Tr::data *>(data));
}

#define SET_HANDLER(_h) (this->handler = &trampoline<_h>)
// comparison: handler == &trampoline<&CacheVC::dead>

What this gets us:

• Like this PR, it lets int f(int, ExactType *) be used directly as a handler: the trampoline reads ExactType from the member pointer's type, so no stored thunk is needed.
• No reinterpret_cast anywhere. The only conversions are static_cast<D *> on the object and static_cast<T *> on the payload.
• A mismatched cast like the QUIC ones above can't be a template argument (it's a reinterpret_cast), so it becomes a compile error instead of silent UB.
• The handler shrinks to 8 bytes. Today it's 16; with this PR it's 24, and that cost lands on every Continuation.
• Dispatch is one indirect call. The member call inside the trampoline goes to a known function, so it can be inlined, which avoids the virtual-bit check and offset adjustment of a member-pointer call.
• Virtual handlers still dispatch dynamically. Continuation doesn't need to be the first base.

The trade-offs:

• Handlers must be compile-time constants. Copying, as in save_handler = handler, still works.
• == compares function addresses. This has the same cross-DSO address-identity caveat as comparing the thunk pointers in this PR, and aggressive identical code folding (--icf=all) could merge trampolines that have the same body.
• Comparison sites (handler == &CacheVC::dead, etc.), SET_CONTINUATION_HANDLER and PUSH_HANDLER would need to spell &trampoline<...>, likely behind a macro.

I ruled out the trampoline alternative because of the code folding optimization, which I did not understand at the time. But the implementation in this patch carries the same code folding risk. I think the trampoline version is both simpler and less wasteful with memory. I will try it.

This PR adds a second dispatch function call to every event. It has a cost in both size and runtime dispatch(extra function call). I agree with Masaori that this could be done statically without either cost. I'm not convinced a change here is necessary, since the type of the second argument is also dependent on the event passed (some handlers will cast it to something else depending on the event anyway) and I don't think there is any combination of types that will cause the thunk template function to produce different code.

The old code comments acknowledge the potential for UB if used outside of the intent, but if every use only passes a handler that is a member of a subclass of Continuation, its gonna be fine.

@cmcfarlen The UB is not a latent risk caused by AI misuse. Dereferencing a function pointer cast to a different function pointer type is UB regardless. Every call to a Continuation handler is UB, even if the data argument has the correct effective type.

I agree that this won't be observable in practice on the architectures we support, but I think it's better to write well defined code, or else write it in assembly. At the very least, it needs a comment that says "we know this isn't correct C++ but we are doing it anyway because of X reason".