[Mesa-dev] [PATCH mesa 5/5] wayland-egl: Update ABI checker

[Miguel Angel Vico]

On Wed, 19 Jul 2017 12:19:30 +0100
Emil Velikov <emil.l.velikov at gmail.com> wrote:

> On 18 July 2017 at 21:49, Miguel A. Vico <mvicomoya at nvidia.com> wrote:
> > This change updates wayland-egl-abi-check.c with the latest changes to
> > wl_egl_window.
> >
> > Signed-off-by: Miguel A. Vico <mvicomoya at nvidia.com>
> > Reviewed-by: James Jones <jajones at nvidia.com>
> > ---
> >  .../wayland/wayland-egl/wayland-egl-abi-check.c    | 78 ++++++++++++++++++----
> >  1 file changed, 65 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/egl/wayland/wayland-egl/wayland-egl-abi-check.c b/src/egl/wayland/wayland-egl/wayland-egl-abi-check.c
> > index 1962f05850..6bdd71b6e0 100644
> > --- a/src/egl/wayland/wayland-egl/wayland-egl-abi-check.c
> > +++ b/src/egl/wayland/wayland-egl/wayland-egl-abi-check.c
> > @@ -31,7 +31,28 @@
> >   * DO NOT EVER CHANGE!
> >   */
> >
> > +/* From: a2ab5c2588 - Miguel A. Vico : wayland-egl: Make wl_egl_window a versioned struct */  
> Please keep the sha as XXX - we'll update it as the commit lands.
> 

Done.

> > +#define WL_EGL_WINDOW_VERSION_v3 3
> > +struct wl_egl_window_v3 {
> > +    const intptr_t version;
> > +
> > +    int width;
> > +    int height;
> > +    int dx;
> > +    int dy;
> > +
> > +    int attached_width;
> > +    int attached_height;
> > +
> > +    void *private;
> > +    void (*resize_callback)(struct wl_egl_window *, void *);
> > +    void (*destroy_window_callback)(void *);
> > +
> > +    struct wl_surface *surface;
> > +};
> > +
> >  /* From: 690ead4a13 - Stencel, Joanna : egl/wayland-egl: Fix for segfault in dri2_wl_destroy_surface. */
> > +#define WL_EGL_WINDOW_VERSION_v2 2
> >  struct wl_egl_window_v2 {
> >      struct wl_surface *surface;
> >
> > @@ -123,6 +144,20 @@ struct wl_egl_window_v0 {
> >          }                                                                           \
> >      } while (0)
> >
> > +#define CHECK_VERSION(A_VER, B_VER, MATCH)                                          \
> > +    do {                                                                            \
> > +        if (((MATCH)  && (WL_EGL_WINDOW_VERSION ## A_VER) !=                        \
> > +                         (WL_EGL_WINDOW_VERSION ## B_VER)) ||                       \
> > +            (!(MATCH) && (WL_EGL_WINDOW_VERSION ## A_VER) >=                        \
> > +                         (WL_EGL_WINDOW_VERSION ## B_VER))) {                       \
> > +            printf("Backards incompatible change detected!\n   "                    \
> > +                   "WL_EGL_WINDOW_VERSION" #A_VER " %s "                            \
> > +                   "WL_EGL_WINDOW_VERSION" #B_VER "\n",                             \
> > +                   ((MATCH) ? "!=" : ">="));                                        \
> > +            return 1;                                                               \
> > +        }                                                                           \
> > +    } while (0)
> > +  
> Same crazy idea as CHECK_SIZE - worth having separate macros?

Yup. Done.

> 
> >  int main(int argc, char **argv)
> >  {
> >      /* Check wl_egl_window_v1 ABI against wl_egl_window_v0 */
> > @@ -149,19 +184,36 @@ int main(int argc, char **argv)
> >
> >      CHECK_SIZE(_v1, _v2, FALSE);
> >
> > -    /* Check wl_egl_window ABI against wl_egl_window_v2 */
> > -    CHECK_MEMBER(_v2,, surface,                 surface);
> > -    CHECK_MEMBER(_v2,, width,                   width);
> > -    CHECK_MEMBER(_v2,, height,                  height);
> > -    CHECK_MEMBER(_v2,, dx,                      dx);
> > -    CHECK_MEMBER(_v2,, dy,                      dy);
> > -    CHECK_MEMBER(_v2,, attached_width,          attached_width);
> > -    CHECK_MEMBER(_v2,, attached_height,         attached_height);
> > -    CHECK_MEMBER(_v2,, private,                 private);
> > -    CHECK_MEMBER(_v2,, resize_callback,         resize_callback);
> > -    CHECK_MEMBER(_v2,, destroy_window_callback, destroy_window_callback);
> > -
> > -    CHECK_SIZE(_v2,, TRUE);
> > +    /* Check wl_egl_window_v3 ABI against wl_egl_window_v2 */
> > +    CHECK_MEMBER(_v2, _v3, surface,                 version);  
> Just hit me that with the current CHECK_MEMBER macro changes like the
> above will be easier to miss. Or someone will attempt to "correct" it.
> The following seem a bit more obvious, imho.
> 
> CHECK_RENAMED_MEMBER(_v2, _v3, surface,                 version);
> CHECK_MEMBER(_v2, _v3, width);

Sure. Done.

> 
> > +    CHECK_MEMBER(_v2, _v3, width,                   width);
> > +    CHECK_MEMBER(_v2, _v3, height,                  height);
> > +    CHECK_MEMBER(_v2, _v3, dx,                      dx);
> > +    CHECK_MEMBER(_v2, _v3, dy,                      dy);
> > +    CHECK_MEMBER(_v2, _v3, attached_width,          attached_width);
> > +    CHECK_MEMBER(_v2, _v3, attached_height,         attached_height);
> > +    CHECK_MEMBER(_v2, _v3, private,                 private);
> > +    CHECK_MEMBER(_v2, _v3, resize_callback,         resize_callback);
> > +    CHECK_MEMBER(_v2, _v3, destroy_window_callback, destroy_window_callback);
> > +
> > +    CHECK_SIZE   (_v2, _v3, FALSE);
> > +    CHECK_VERSION(_v2, _v3, FALSE);
> > +  
> Hmm what does the v2 vs v3 version check bring?

Not much in this case.

We need to check VERSION is increased every time we modify
wl_egl_window. This is to prevent someone from changing wl_egl_window
without increasing the VERSION number.

Rather than adding that check whenever we add wl_egl_window_v4, I
thought I'd just add the macro now and use it with v2 vs v3 (just to
use it somewhere).

> 
> Thanks
> Emil

Thanks,
-- 
Miguel