masaori335 commented on PR #13830:
URL: https://github.com/apache/trafficserver/pull/13830#issuecomment-6138843115
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
```cpp
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:
```cpp
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
