Should umfPutIPCHandle and umfOpenIPCHandle take const handle data pointers?

[steffenlarsen]

Rationale

umfPutIPCHandle and umfOpenIPCHandle currently take an umf_ipc_handle_t, but since the handles are replicable across processes I would not expect these functions to change the data contained in the handles. Could these be changed to take const handles?

API Changes

Change from

umf_result_t umfPutIPCHandle(umf_ipc_handle_t ipcHandle);
umf_result_t umfOpenIPCHandle(umf_ipc_handler_handle_t hIPCHandler, umf_ipc_handle_t ipcHandle, void **ptr)

to

typedef const struct umf_ipc_data_t *umf_const_ipc_handle_t;

umf_result_t umfPutIPCHandle(umf_const_ipc_handle_t ipcHandle);
umf_result_t umfOpenIPCHandle(umf_ipc_handler_handle_t hIPCHandler, umf_const_ipc_handle_t ipcHandle, void **ptr)

[lplewa]

You’re right — but since we’re already past 1.0, this would be an API break, so we can’t change it now. If we ever do a 2.0 release, we can add the const, but it’s not worth a major version bump on its own.

Note: if someone implements this in the future, keep in mind that const umf_ipc_handle_t makes the pointer itself const (like int * const), not the data it points to (const int *). To make the data const, a new type umf_const_ipc_handle_t would need to be added.

[steffenlarsen]

You’re right — but since we’re already past 1.0, this would be an API break, so we can’t change it now. If we ever do a 2.0 release, we can add the const, but it’s not worth a major version bump on its own.

An option could be to define the const variants in addition to the non-const versions, then redirect the non-const to call the const functions. Since having const arguments is a stronger guarantee, both should then work, unlike if it had been the other way around. If needed, the old versions could be deprecated and then removed in a future 2.0 version (if ever done.)

Note: if someone implements this in the future, keep in mind that const umf_ipc_handle_t makes the pointer itself const (like int * const), not the data it points to (const int *). To make the data const, a new type umf_const_ipc_handle_t would need to be added.

Good point! I have amended the suggestion.

[lplewa]

I’ve been thinking about this a bit more — before the 1.0 release, I actually added a bunch of consts to the UMF API, but I skipped this one. Now I remember why.

We can’t make the handle in umfPutIPCHandle const, because the data becomes invalid after that call — we’re effectively modifying it. In C, all “free”-like functions take non-const pointers, since they modify the memory state (turning the pointed memory into an undefined state).
Also since umfPutIPCHandle internally calls free, we’d have to do a dirty cast to “unconst” the handle before freeing it.

For umfOpenIPCHandle, though, it could technically take a const pointer.
But both options to support that are… well, ugly, and I’m not sure this missing const is serious enough to justify the complications in the header:

umf_result_t umfOpenIPCHandle(umf_ipc_handler_handle_t hIPCHandler,
                              umf_ipc_handle_t ipcHandle, void **ptr);
umf_result_t umfConstOpenIPCHandle(umf_ipc_handler_handle_t hIPCHandler,
                                   umf_const_ipc_handle_t ipcHandle, void **ptr);

or

#ifdef UMF_CONST_OPEN_IPC_HANDLE
umf_result_t umfOpenIPCHandle(umf_ipc_handler_handle_t hIPCHandler,
                              umf_const_ipc_handle_t ipcHandle, void **ptr);
#else
umf_result_t umfOpenIPCHandle(umf_ipc_handler_handle_t hIPCHandler,
                              umf_ipc_handle_t ipcHandle, void **ptr);
#endif

Anyway, missing consts are a very common thing in C, so I’d say this one isn’t worth complicating the header for. Is this const for some reason very important for you?

[steffenlarsen]

We can’t make the handle in umfPutIPCHandle const, because the data becomes invalid after that call — we’re effectively modifying it. In C, all “free”-like functions take non-const pointers, since they modify the memory state (turning the pointed memory into an undefined state). Also since umfPutIPCHandle internally calls free, we’d have to do a dirty cast to “unconst” the handle before freeing it.

If the point of IPC handles are that they can be copied around, why does umfPutIPCHandle need to change the data and/or call free on it? The user could have multiple copies of the data and if the call intends to invalidate it, only the caller's copy would be invalidated.

For umfOpenIPCHandle, though, it could technically take a const pointer. But both options to support that are… well, ugly, and I’m not sure this missing const is serious enough to justify the complications in the header:

Ah, I didn't realize that UMF was a C-interface. That does require some ugly fixes indeed.

That said, would adding const to the interfaces actually break ABI? The size of the arguments would not change and the mangling wouldn't change, given there isn't mangling in extern C interfaces. Only problem I could see is if the compiler would make some decisions based on the signature, but given the new signature is stricter than the old one, I can't think of a case where it would be an issue.

Anyway, missing consts are a very common thing in C, so I’d say this one isn’t worth complicating the header for. Is this const for some reason very important for you?

The missing constness makes the interfaces a little awkward in the SYCL implementation (see discussion in https://github.com/intel/llvm/pull/20018#discussion_r2439639914). We can work around it by making an extra copy of the handle data, but that is sub-optimal as it requires additional memory allocations and copies, just to allow usage of the handles in a context that wouldn't need constness.