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

undefined behaviour with wl_container_of #7

Closed
gulafaran opened this issue Jun 26, 2024 · 6 comments · Fixed by #8
Closed

undefined behaviour with wl_container_of #7

gulafaran opened this issue Jun 26, 2024 · 6 comments · Fixed by #8

Comments

@gulafaran
Copy link
Contributor

to safely use that C macro the class has to be "no virtual functions, no inheritance, and uniform access control (e.g., all public)"
currently hyprland breaks this. easiest solution to work around this is store both the listener and the type *data; inside

struct DestroyWrapper {
   wl_listener listener;
   Type *data;
};

but im hitting std::format struggles complaining all from not being a consteval expression when trying to make a PR, std::format struggles hehe.

examples in wayfire https://github.com/WayfireWM/wayfire/blob/master/src/wl-listener-wrapper.tpp#L8
decleration https://github.com/WayfireWM/wayfire/blob/master/src/api/wayfire/util.hpp#L19

or example in wrapland https://github.com/winft/wrapland/blob/master/server/buffer.cpp#L344
decleration https://github.com/winft/wrapland/blob/master/server/buffer_p.h#L32

the reason behind this is the macro wl_container_of, which relies on offsetof to compute the offset of a member within a structure. and classes can work with this if adhereing to those rules mentioned at top.

this can be seen when compiling hyprland with extra warnings or with clang.

/home/tom/dev/Hyprland/protocols/wlr-output-management-unstable-v1.cpp:64:39: warning: offset of on non-standard-layout type 'typeof (*pResource)' (aka 'CZwlrOutputManagerV1') [-Winvalid-offsetof]
   64 |     CZwlrOutputManagerV1* pResource = wl_container_of(l, pResource, resourceDestroyListener);
      |                                       ^                             ~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/wayland-util.h:418:9: note: expanded from macro 'wl_container_of'
  418 |                              offsetof(WL_TYPEOF(*sample), member))
      |                              ^                            ~~~~~~
/usr/lib/llvm/18/bin/../../../../lib/clang/18/include/__stddef_offsetof.h:16:24: note: expanded from macro 'offsetof'
   16 | #define offsetof(t, d) __builtin_offsetof(t, d)
@vaxerski
Copy link
Member

Thanks for bringing this to my attention, gcc didn't complain :P

you wanna wrap a MR or should I do it?

@gulafaran
Copy link
Contributor Author

Thanks for bringing this to my attention, gcc didn't complain :P

you wanna wrap a MR or should I do it?

im actually out on an island in a second home? cottage? with the kids and i think im gonna end up throwing the laptop into the sea before i figure out this std::format madness xD, so if you can please <3

@gulafaran
Copy link
Contributor Author

if you wanna sneak peak into how QT did their wayland scanner the source is here https://code.qt.io/cgit/qt/qtwayland.git/tree/src/qtwaylandscanner/qtwaylandscanner.cpp

@vaxerski
Copy link
Member

with the kids and i think im gonna end up throwing the laptop into the sea before i figure out this std::format madness xD

my bad, enjoy your time with the kids. I don't have any. Or a wife. Or a single woman who'd not hate me. Heh.

@gulafaran
Copy link
Contributor Author

with the kids and i think im gonna end up throwing the laptop into the sea before i figure out this std::format madness xD

my bad, enjoy your time with the kids. I don't have any. Or a wife. Or a single woman who'd not hate me. Heh.

heh it changes fast, when you least expect it to. but i managed to somewhat figure out something without throwing the laptop into the sea #8 :D

@vaxerski
Copy link
Member

yeah, left a small review

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 a pull request may close this issue.

2 participants