Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GIT PULL] Hide io_uring_get_sqe symbol #1336

Closed
wants to merge 1 commit into from

Conversation

cmazakas
Copy link
Contributor

The user-facing API for liburing has the user interface with io_uring_get_sqe via a static inline function. However, this function is also defined in the plain liburing archive, which can potential lead to ODR violations.

We instead define it properly only for the liburing-ffi case and keep it hidden when the liburing archive itself is built.

See:

exbigboss@pleiades ~/cpp/liburing (symbol-fixes)
❯ nm -an src/liburing-ffi.a | grep io_uring_get_sqe
00000000000013d0 T _io_uring_get_sqe
0000000000001540 T io_uring_get_sqe
exbigboss@pleiades ~/cpp/liburing (symbol-fixes)
❯ nm -an src/liburing.a | grep io_uring_get_sqe
exbigboss@pleiades ~/cpp/liburing (symbol-fixes) [1]
❯

git request-pull output:

The following changes since commit 6c509e2b0c881a13b83b259a221bf15fc9b3f681:

  test/sqwait: cleanup allocated iovecs (2025-01-22 19:48:36 -0700)

are available in the Git repository at:

  https://github.com/cmazakas/liburing.git symbol-fixes

for you to fetch changes up to 659c39092a7ddfaa68e4f4560475a0e9232af361:

  Hide io_uring_get_sqe symbol (2025-01-27 09:22:34 -0800)

----------------------------------------------------------------
Christian Mazakas (1):
      Hide io_uring_get_sqe symbol

 debian/liburing2.symbols | 1 -
 src/ffi.c                | 5 +++++
 src/include/liburing.h   | 2 --
 src/liburing.map         | 1 -
 src/queue.c              | 4 +++-
 5 files changed, 8 insertions(+), 5 deletions(-)

By submitting this pull request, I acknowledge that:

  1. I have followed the above pull request guidelines.
  2. I have the rights to submit this work under the same license.
  3. I agree to a Developer Certificate of Origin (see https://developercertificate.org for more information).

The user-facing API for liburing has the user interface with
io_uring_get_sqe via a static inline function. However, this function
is also defined in the plain liburing archive, which can potential lead
to ODR violations.

We instead define it properly only for the liburing-ffi case and keep
it hidden when the liburing archive itself is built.

Signed-off-by: Christian Mazakas <[email protected]>
@ammarfaizi2
Copy link
Contributor

ammarfaizi2 commented Jan 28, 2025

No, you can't omit that.

It will break applications that are linked against liburing-2.2 and
earlier versions. Jens did it on purpose to maintain backward
compatibility after marking io_uring_get_sqe as a static inline
function.

From: Jens Axboe <[email protected]>
Date: Wed, 23 Feb 2022 16:28:09 -0700
Subject: [PATCH] queue: provide io_uring_get_sqe() symbol again

With a recent change, io_uring_get_sqe() was moved to the header file
to avoid a function call for this hot function. However, this will break
updates of the library, where the symbol is then not provided:

./app: symbol lookup error: ./app: undefined symbol: io_uring_get_sqe, version LIBURING_2.0

Provide one internally so we still export it, but allow newly compiled
applications to grab the one from the header. That should provide the
best of both worlds.

Fixes: 6cf23f9a6470 ("Move io_uring_get_sqe() inline in the liburing.h header")
Signed-off-by: Jens Axboe <[email protected]>
---
 src/Makefile           |  2 +-
 src/include/liburing.h |  4 ++++
 src/queue.c            | 14 ++++++++++++++
 3 files changed, 19 insertions(+), 1 deletion(-)

@ammarfaizi2
Copy link
Contributor

However, this function is also defined in the plain liburing archive, which can potential lead to ODR violations.

Do you have a real example of an ODR violation that leads to catastrophic
consequences? (e.g., compiler error, runtime crash, etc.).

@cmazakas
Copy link
Contributor Author

It will break applications that are linked against liburing-2.0 and
earlier versions. Jens did it on purpose to maintain backward
compatibility after marking io_uring_get_sqe as a static inline
function.

Ah, this makes a ton of sense, actually.

I wasn't aware of the history of the change. I thought it was an honest mistake. I hadn't considered it's a break for existing applications which rely on semver.

Do you have a real example of an ODR violation that leads to catastrophic
consequences? (e.g., compiler error, runtime crash, etc.).

Nope. The concern here is mostly theoretical.

The extra symbol was just something I ran into during development with liburing locally and I figured I'd make a PR to "fix the bug".

Thanks for the explanation. There's nothing else to do for now so I'll just simply close the PR.

@cmazakas cmazakas closed this Jan 29, 2025
@cmazakas cmazakas deleted the symbol-fixes branch January 29, 2025 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants