On Tue, Feb 04, 2014 at 02:21:48PM +0000, Neil Roberts wrote:
> The previous implementation of the wl_container_of macro was
> dereferencing the sample pointer in order to get an address of the
> member to calculate the offset. Ideally this shouldn't cause any
> problems because the dereference doesn't actually cause the address to
> be read from so it shouldn't matter if the pointer is uninitialised.
> However this is probably technically invalid and could cause undefined
> behavior. Clang appears to take advantage of this undefined behavior
> and doesn't bother doing the subtraction. It also gives a warning when
> it does this.
>
> The documentation for wl_container_of implies that it should only be
> given an initialised pointer and if that is done then there is no
> problem with clang. However this is quite easy to forget and doesn't
> cause any problems or warnings with gcc so it's quite easy to
> accidentally break clang.
>
> To fix the problem this changes the macro to use pointer -
> offsetof(__typeof__(sample), member) so that it doesn't need to deref
> the sample pointer. This does however require that the __typeof__
> operator is supported by the compiler. In practice we probably only
> care about gcc and clang and both of these happily support the
> operator.
>
> The previous implementation was also using __typeof__ but it had a
> fallback path avoiding it when the operator isn't available. The
> fallback effectively has undefined behaviour and it is targetting
> unknown compilers so it is probably not a good idea to leave it in.
> Instead, this patch just removes it. If someone finds a compiler that
> doesn't have __typeof__ but does work with the old implementation then
> maybe they could add it back in as a special case.
>
> This patch removes the initialisation anywhere where the sample
> pointer was being unitialised before using wl_container_of. The
> documentation for the macro has also been updated to specify that this
> is OK.
Thanks, that's nice. Patch applied.
Kristian
> ---
> src/wayland-server.c | 2 +-
> src/wayland-server.h | 2 +-
> src/wayland-util.h | 30 +++++++++++++-----------------
> 3 files changed, 15 insertions(+), 19 deletions(-)
>
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index 489b99d..294497d 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -571,7 +571,7 @@ wl_resource_get_link(struct wl_resource *resource)
> WL_EXPORT struct wl_resource *
> wl_resource_from_link(struct wl_list *link)
> {
> - struct wl_resource *resource = NULL;
> + struct wl_resource *resource;
>
> return wl_container_of(link, resource, link);
> }
> diff --git a/src/wayland-server.h b/src/wayland-server.h
> index 3fcaaf6..7fc5b47 100644
> --- a/src/wayland-server.h
> +++ b/src/wayland-server.h
> @@ -162,7 +162,7 @@ wl_client_post_no_memory(struct wl_client *client);
> * \code
> * void your_listener(struct wl_listener *listener, void *data)
> * {
> - * struct your_data *data = NULL;
> + * struct your_data *data;
> *
> * your_data = wl_container_of(listener, data, your_member_name);
> * }
> diff --git a/src/wayland-util.h b/src/wayland-util.h
> index 68d91e2..57764d9 100644
> --- a/src/wayland-util.h
> +++ b/src/wayland-util.h
> @@ -135,7 +135,7 @@ void wl_list_insert_list(struct wl_list *list, struct
> wl_list *other);
> *
> * void example_container_destroy(struct wl_listener *listener, void *data)
> * {
> - * struct example_container *ctr = NULL;
> + * struct example_container *ctr;
> *
> * ctr = wl_container_of(listener, ctr, destroy_listener);
> * \comment{destroy ctr...}
> @@ -144,44 +144,40 @@ void wl_list_insert_list(struct wl_list *list, struct
> wl_list *other);
> *
> * \param ptr A valid pointer to the contained item.
> *
> - * \param sample A pointer to the type of content that the list item stores.
> - * Sample does not need be a valid pointer; a null pointer will suffice.
> + * \param sample A pointer to the type of content that the list item
> + * stores. Sample does not need be a valid pointer; a null or
> + * an uninitialised pointer will suffice.
> *
> * \param member The named location of ptr within the sample type.
> *
> * \return The container for the specified pointer.
> */
> -#ifdef __GNUC__
> #define wl_container_of(ptr, sample, member) \
> - (__typeof__(sample))((char *)(ptr) - \
> - ((char *)&(sample)->member - (char *)(sample)))
> -#else
> -#define wl_container_of(ptr, sample, member) \
> - (void *)((char *)(ptr) - \
> - ((char *)&(sample)->member - (char *)(sample)))
> -#endif
> + (__typeof__(sample))((char *)(ptr) - \
> + offsetof(__typeof__(*sample), member))
> +/* If the above macro causes problems on your compiler you might be
> + * able to find an alternative name for the non-standard __typeof__
> + * operator and add a special case here */
>
> #define wl_list_for_each(pos, head, member) \
> - for (pos = 0, pos = wl_container_of((head)->next, pos, member); \
> + for (pos = wl_container_of((head)->next, pos, member); \
> &pos->member != (head); \
> pos = wl_container_of(pos->member.next, pos, member))
>
> #define wl_list_for_each_safe(pos, tmp, head, member)
> \
> - for (pos = 0, tmp = 0, \
> - pos = wl_container_of((head)->next, pos, member), \
> + for (pos = wl_container_of((head)->next, pos, member), \
> tmp = wl_container_of((pos)->member.next, tmp, member); \
> &pos->member != (head); \
> pos = tmp, \
> tmp = wl_container_of(pos->member.next, tmp, member))
>
> #define wl_list_for_each_reverse(pos, head, member) \
> - for (pos = 0, pos = wl_container_of((head)->prev, pos, member); \
> + for (pos = wl_container_of((head)->prev, pos, member); \
> &pos->member != (head); \
> pos = wl_container_of(pos->member.prev, pos, member))
>
> #define wl_list_for_each_reverse_safe(pos, tmp, head, member)
> \
> - for (pos = 0, tmp = 0, \
> - pos = wl_container_of((head)->prev, pos, member), \
> + for (pos = wl_container_of((head)->prev, pos, member), \
> tmp = wl_container_of((pos)->member.prev, tmp, member); \
> &pos->member != (head); \
> pos = tmp, \
> --
> 1.8.4.2
>
> _______________________________________________
> wayland-devel mailing list
> [email protected]
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
_______________________________________________
wayland-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
