Hi Miguel,
Thanks for looking into this. There's a couple of small nits below but
it overall looks good.
On 18 July 2017 at 21:49, Miguel A. Vico <mvicom...@nvidia.com> wrote:
> Add a small ABI checker for wl_egl_window so that we can check for
> backwards incompatible changes at 'make check' time.
>
> Signed-off-by: Miguel A. Vico <mvicom...@nvidia.com>
> Reviewed-by: James Jones <jajo...@nvidia.com>
> ---
> src/egl/wayland/wayland-egl/Makefile.am | 10 +-
> .../wayland/wayland-egl/wayland-egl-abi-check.c | 167
> +++++++++++++++++++++
> 2 files changed, 176 insertions(+), 1 deletion(-)
> create mode 100644 src/egl/wayland/wayland-egl/wayland-egl-abi-check.c
>
> diff --git a/src/egl/wayland/wayland-egl/Makefile.am
> b/src/egl/wayland/wayland-egl/Makefile.am
> index 8c45e8e26d..74a52027c6 100644
> --- a/src/egl/wayland/wayland-egl/Makefile.am
> +++ b/src/egl/wayland/wayland-egl/Makefile.am
> @@ -14,7 +14,15 @@ libwayland_egl_la_LDFLAGS = \
> $(GC_SECTIONS) \
> $(LD_NO_UNDEFINED)
>
> -TESTS = wayland-egl-symbols-check
> +TESTS =
> +check_PROGRAMS =
> +
> +TESTS += wayland-egl-symbols-check
> EXTRA_DIST = wayland-egl-symbols-check
>
> +TESTS += wayland-egl-abi-check
> +check_PROGRAMS += wayland-egl-abi-check
> +
Please add into the respective variables directly.
TESTS = \
foo \
bar
check_PROGRAMS = wayland-egl-abi-check
> +wayland_egl_abi_check_SOURCES = wayland-egl-abi-check.c
> +
Feel free to drop this - default extension is ".c" so this will be
picked automatically.
> include $(top_srcdir)/install-lib-links.mk
> diff --git a/src/egl/wayland/wayland-egl/wayland-egl-abi-check.c
> b/src/egl/wayland/wayland-egl/wayland-egl-abi-check.c
> new file mode 100644
> index 0000000000..1962f05850
> --- /dev/null
> +++ b/src/egl/wayland/wayland-egl/wayland-egl-abi-check.c
> @@ -0,0 +1,167 @@
> +/*
> + * Copyright (c) 2017, NVIDIA CORPORATION. All rights reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include <stddef.h> // offsetof
> +#include <stdio.h> // printf
> +
> +#include "wayland-egl-priv.h" // Current struct wl_egl_window implementation
> +
> +/*
> + * Following are previous implementations of wl_egl_window.
> + *
> + * DO NOT EVER CHANGE!
> + */
> +
> +/* From: 690ead4a13 - Stencel, Joanna : egl/wayland-egl: Fix for segfault in
> dri2_wl_destroy_surface. */
> +struct wl_egl_window_v2 {
> +/* From: ca3ed3e024 - Ander Conselvan de Oliveira : egl/wayland: Don't
> invalidate drawable on swap buffers */
> +struct wl_egl_window_v1 {
> +/* From: 214fc6e850 - Benjamin Franzke : egl: Implement libwayland-egl */
> +struct wl_egl_window_v0 {
Hats off for digging all the commits and adding references!
Can we declare the structs in the same order to how they are used - 0...
> +/* This program checks we keep a backwards-compatible struct wl_egl_window
> + * definition whenever it is modified in wayland-egl-priv.h.
> + *
> + * The previous definition should be added above as a new struct
> + * wl_egl_window_vN, and the appropriate checks should be added below
> + */
> +
> +#define TRUE 1
> +#define FALSE 0
> +
Use stdbool.h's true/false if the suggestion, below seem too much?
> +#define MEMBER_SIZE(TYPE, MEMBER) sizeof(((TYPE *)0)->MEMBER)
> +
> +#define CHECK_MEMBER(A_VER, B_VER, A_MEMBER, B_MEMBER)
> \
> + do {
> \
> + if (offsetof(struct wl_egl_window ## A_VER, A_MEMBER) !=
> \
> + offsetof(struct wl_egl_window ## B_VER, B_MEMBER)) {
> \
> + printf("Backards incompatible change detected!\n "
> \
> + "offsetof(struct wl_egl_window" #A_VER "::" #A_MEMBER ")
> != " \
> + "offsetof(struct wl_egl_window" #B_VER "::" #B_MEMBER
> ")\n"); \
> + return 1;
> \
> + }
> \
> +
> \
> + if (MEMBER_SIZE(struct wl_egl_window ## A_VER, A_MEMBER) !=
> \
> + MEMBER_SIZE(struct wl_egl_window ## B_VER, B_MEMBER)) {
> \
> + printf("Backards incompatible change detected!\n "
> \
> + "MEMBER_SIZE(struct wl_egl_window" #A_VER "::" #A_MEMBER
> ") != " \
> + "MEMBER_SIZE(struct wl_egl_window" #B_VER "::" #B_MEMBER
> ")\n"); \
> + return 1;
> \
> + }
> \
> + } while (0)
> +
Using arguments in all caps, is a bit too much personally. The newer
code in Mesa tries to keep only the macro name in uppercase.
I'm wondering about a wrapper for vX vs "current" - would it help or
make things more confusing?
> +#define CHECK_SIZE(A_VER, B_VER, MATCH)
> \
> + do {
> \
> + if (((MATCH) && sizeof(struct wl_egl_window ## A_VER) !=
> \
> + sizeof(struct wl_egl_window ## B_VER)) ||
> \
> + (!(MATCH) && sizeof(struct wl_egl_window ## A_VER) >
> \
> + sizeof(struct wl_egl_window ## B_VER))) {
> \
> + printf("Backards incompatible change detected!\n "
> \
> + "sizeof(struct wl_egl_window" #A_VER ") %s "
> \
> + "sizeof(struct wl_egl_window" #B_VER ")\n",
> \
> + ((MATCH) ? "!=" : ">"));
> \
> + return 1;
> \
> + }
> \
> + } while (0)
> +
This seems like a cool way of handling both vX <> vY and vX vs.
"current". It feels a bit overkill bth.
I would have kept those as separate macros - both the macros will be
easier to read and the code will be more obvious.
Thanks
Emil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
