Thanks for all the reviews, Emil.
Inline.
On Wed, 19 Jul 2017 11:35:04 +0100
Emil Velikov <emil.l.veli...@gmail.com> wrote:
> 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.
Done.
>
> > 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...
>
Done.
>
> > +/* 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.
Made macro arguments lowercase.
> I'm wondering about a wrapper for vX vs "current" - would it help or
> make things more confusing?
No strong opinion on this, so added a _CURRENT version of the macro as
you suggested.
>
>
> > +#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.
Yes. Added a _CURRENT version of the macro for the specific check.
>
> Thanks
> Emil
Thanks.
--
Miguel
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
