[Mesa-dev] [PATCH mesa 1/5] wayland-egl: Add wl_egl_window ABI checker

Miguel Angel Vico mvicomoya at nvidia.com
Thu Jul 20 00:31:38 UTC 2017


Thanks for all the reviews, Emil.

Inline.

On Wed, 19 Jul 2017 11:35:04 +0100
Emil Velikov <emil.l.velikov at 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 <mvicomoya at 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 <mvicomoya at nvidia.com>
> > Reviewed-by: James Jones <jajones at 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




More information about the mesa-dev mailing list