GCC 14 compatibility fixes #54974

fweimer-rh · 2023-12-20T16:22:55Z

These changes should make it possible to build the C parts of Ceph with GCC 14.

Related to:

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

idryomov

Please sign off commits per https://github.com/ceph/ceph/blob/main/SubmittingPatches.rst#sign-your-work.

idryomov · 2023-12-20T16:59:23Z

src/pybind/rbd/c_rbd.pxd

@@ -525,7 +526,7 @@ cdef extern from "rbd/librbd.h" nogil:
    int rbd_snap_unprotect(rbd_image_t image, const char *snap_name)
    int rbd_snap_is_protected(rbd_image_t image, const char *snap_name,
                              int *is_protected)
-    int rbd_snap_exists(rbd_image_t image, const char *snapname, bint *exists)
+    int rbd_snap_exists(rbd_image_t image, const char *snapname, libcpp.bool *exists)


Several Ceph APIs use bool * types, which correspond to libcpp.bool * types in Cython. The bint type has an incorrect size 4 and cannot be used as a replacement.

Part of the GCC 14 preparations, but this looks like a real bug.

These Cython bindings are against our C API, not C++ API. I thought Cython's libcpp was for working with C++?

On top of that, bint and libcpp.bool appear to be the same thing, at least at first sight:

https://github.com/cython/cython/blob/e57193ccc5337f08b6efd4225d21594906df0cb8/Cython/Includes/libcpp/__init__.pxd#L2

Could you please elaborate?

These Cython bindings are against our C API, not C++ API. I thought Cython's libcpp was for working with C++?

The Cython type libcpp.bool is translated bool, which works because the Ceph API uses bool and <stdbool.h>.

I'm not a Cython expert, but I couldn't find better type to use here.

On top of that, bint and libcpp.bool appear to be the same thing, at least at first sight:

https://github.com/cython/cython/blob/e57193ccc5337f08b6efd4225d21594906df0cb8/Cython/Includes/libcpp/__init__.pxd#L2

Could you please elaborate?

I think the way this works is that from the Python view, it's treated as bint (with conversion to the Python bool type etc.). Because it's an extern cdef, bool is preserved in the translation, which gives us the right type with the right ABI.

EDIT Earlier comment was wrong.

Makes sense, although it's clear that libcpp was intended only for C++ because it doesn't actually pull in stdbool.h. Looks like someone else noticed this too: cython/cython#5730.

I'm still confused about

Part of the GCC 14 preparations, but this looks like a real bug.

though. Given that it's initialized (bint _exists = False, translated to int <generated_name> = 0;), that the API sets it to either true or false, and that we then do bool(_exists != 0), what can go wrong? Can you include the warning or error reported by GCC 14 in the commit message?

I'm asking because if it's a real bug, we would need to file a tracker ticket and backport this.

If all the callers do this bint _exists = False step, then this should be okay-ish, as long as LTO isn't used and a compiler barrier exists.

I've updated the commit message to include the compiler error.

we could add:

cdef extern from "<stdbool.h>": ctypedef bint bool

I think that would mess with some bool(...) syntax in Python code, like:

ceph/src/pybind/rbd/rbd.pyx

Line 2598 in 86bb77e

ret = (<object>cb)(offset, length, bool(write))

If it's easy to adjust, let's do it.

Also, as I look at this more, in particular src/pybind/rbd/c_rbd.pxd, there are more instances of bint being used in place of a bool, both in structs:

ctypedef struct rbd_snap_mirror_namespace_t: rbd_snap_mirror_state_t state size_t mirror_peer_uuids_count char *mirror_peer_uuids bint complete char *primary_mirror_uuid uint64_t primary_snap_id uint64_t last_copied_object_number

for

typedef struct { rbd_snap_mirror_state_t state; size_t mirror_peer_uuids_count; char *mirror_peer_uuids; bool complete; char *primary_mirror_uuid; uint64_t primary_snap_id; uint64_t last_copied_object_number; } rbd_snap_mirror_namespace_t;

and as a non-pointer parameter:

int rbd_mirror_image_disable(rbd_image_t image, bint force)

for

CEPH_RBD_API int rbd_mirror_image_disable(rbd_image_t image, bool force);

We probably need to clean up all of bint usage here...

Adding

cdef extern from "<stdbool.h>": pass

just to src/pybind/rbd/mock_rbd.pxi should work around it. Feel free to force push that and we will see.

I had to add a similar change to src/pybind/rgw/rgw.pyx, but the documentation builds now.

Can you move the RBD one to .pyx or the RGW one to .pxi so that we are consistent?

Can you move the RBD one to .pyx or the RGW one to .pxi so that we are consistent?

This should be fixed in the latest rebase.

We probably need to clean up all of bint usage here...

I think use of bint is only a problem if it is used in such a way that Cython emits an int type for it. If it's nested in a struct or used to describe another type (like libcpp does for exposing bool), the C layout discrepancy does not matter. Not ideal, but not an immediate problem either.

fweimer-rh · 2023-12-21T00:00:29Z

Please sign off commits per https://github.com/ceph/ceph/blob/main/SubmittingPatches.rst#sign-your-work.

Done.

src/pybind/rgw/mock_rgw.pxi

src/pybind/rgw/rgw.pyx

src/tracing/librados.tp

This fixes type errors like this: In file included from /usr/include/lttng/tracepoint-event.h:69, from …-build/include/tracing/librados.h:4143, from …/src/tracing/librados.c:6 : …-build/include/tracing/librados.h: In function ‘lttng_ust__event_probe__librados___rados_mon_command_exit’: …-build/include/tracing/librados.h:477:9: error: initialization of ‘size_t’ {aka ‘long unsigned int’} from ‘size_t *’ {aka ‘long unsigned int *’} makes integer from pointer without a cast 477 | ceph_ctf_integerp(size_t, outslen, outslen) | ^~~~~~~~~~~~~~~~~ GCC 14 will likely treat these type mismatches as an error and fail the build. Signed-off-by: Florian Weimer <fweimer@redhat.com>

Several Ceph APIs use bool * types, which correspond to libcpp.bool * types in Cython. The bint type has an incorrect size 4 and cannot be used as a replacement. This prevents a compilation failure with future compilers: …-build/src/pybind/rbd/rbd.c: In function ‘__pyx_pf_3rbd_3RBD_104namespace_exists’: …-build/src/pybind/rbd/rbd.c:42165:76: error: passing argument 3 of ‘rbd_namespace_exists’ from incompatible pointer type 42165 | __pyx_v_ret = rbd_namespace_exists(__pyx_v__ioctx, __pyx_v__name, (&__pyx_v__exists)); | ~^~~~~~~~~~~~~~~~~ | | | int * In file included from …-build/src/pybind/rbd/rbd.c:1268: …/src/include/rbd/librbd.h:1496:45: note: expected ‘_Bool *’ but argument is of type ‘int *’ 1496 | bool *exists); | ^ Signed-off-by: Florian Weimer <fweimer@redhat.com>

idryomov · 2024-01-08T15:30:34Z

@cbodley A change in RGW bindings here, PTAL.

idryomov · 2024-01-14T10:18:27Z

No related failures:

https://pulpito.ceph.com/dis-2024-01-12_00:14:46-rbd-wip-dis-testing-distro-default-smithi/

idryomov · 2024-01-14T10:19:37Z

@cbodley @mattbenjamin This is good to merge from the RBD perspective. Can I get a +1 from the RGW side?

fweimer-rh requested a review from a team as a code owner December 20, 2023 16:22

github-actions bot added pybind rbd labels Dec 20, 2023

idryomov reviewed Dec 20, 2023

View reviewed changes

fweimer-rh force-pushed the c99 branch from 8642d01 to 0fee478 Compare December 20, 2023 23:59

fweimer-rh force-pushed the c99 branch 5 times, most recently from 9cc05b0 to d8645ae Compare December 22, 2023 22:07

fweimer-rh force-pushed the c99 branch from d8645ae to 1686b78 Compare January 3, 2024 10:34

idryomov reviewed Jan 4, 2024

View reviewed changes

src/pybind/rgw/mock_rgw.pxi Outdated Show resolved Hide resolved

src/pybind/rgw/rgw.pyx Show resolved Hide resolved

src/tracing/librados.tp Outdated Show resolved Hide resolved

src/tracing/librados.tp Outdated Show resolved Hide resolved

fweimer-rh added 2 commits January 8, 2024 12:02

fweimer-rh force-pushed the c99 branch from 1686b78 to a49d154 Compare January 8, 2024 12:37

idryomov approved these changes Jan 8, 2024

View reviewed changes

idryomov added the rgw label Jan 8, 2024

idryomov requested a review from a team January 8, 2024 15:30

cbodley requested a review from mattbenjamin January 8, 2024 15:31

idryomov added the wip-dis-testing label Jan 8, 2024

cbodley approved these changes Jan 14, 2024

View reviewed changes

idryomov merged commit d5457a2 into ceph:main Jan 14, 2024
10 of 11 checks passed

tserong mentioned this pull request Apr 9, 2024

Fix FTBFS on GCC 14 (boo#1222493) SUSE/ceph#516

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GCC 14 compatibility fixes #54974

GCC 14 compatibility fixes #54974

fweimer-rh commented Dec 20, 2023

idryomov left a comment

idryomov Dec 20, 2023 •

edited

fweimer-rh Dec 21, 2023 •

edited

idryomov Dec 21, 2023

idryomov Dec 21, 2023 •

edited

fweimer-rh Dec 22, 2023

idryomov Dec 22, 2023

idryomov Dec 22, 2023

fweimer-rh Dec 22, 2023

idryomov Dec 22, 2023

fweimer-rh Jan 3, 2024

fweimer-rh commented Dec 21, 2023

idryomov commented Jan 8, 2024

idryomov commented Jan 14, 2024

idryomov commented Jan 14, 2024

GCC 14 compatibility fixes #54974

GCC 14 compatibility fixes #54974

Conversation

fweimer-rh commented Dec 20, 2023

Checklist

idryomov left a comment

Choose a reason for hiding this comment

idryomov Dec 20, 2023 • edited

Choose a reason for hiding this comment

fweimer-rh Dec 21, 2023 • edited

Choose a reason for hiding this comment

idryomov Dec 21, 2023

Choose a reason for hiding this comment

idryomov Dec 21, 2023 • edited

Choose a reason for hiding this comment

fweimer-rh Dec 22, 2023

Choose a reason for hiding this comment

idryomov Dec 22, 2023

Choose a reason for hiding this comment

idryomov Dec 22, 2023

Choose a reason for hiding this comment

fweimer-rh Dec 22, 2023

Choose a reason for hiding this comment

idryomov Dec 22, 2023

Choose a reason for hiding this comment

fweimer-rh Jan 3, 2024

Choose a reason for hiding this comment

fweimer-rh commented Dec 21, 2023

idryomov commented Jan 8, 2024

idryomov commented Jan 14, 2024

idryomov commented Jan 14, 2024

idryomov Dec 20, 2023 •

edited

fweimer-rh Dec 21, 2023 •

edited

idryomov Dec 21, 2023 •

edited