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

[Miguel Angel Vico]

Thanks for all the reviews, Emil.

Inline.

On Wed, 19 Jul 2017 11:35:04 +0100
Emil Velikov  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  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 
> > Reviewed-by: James Jones 
> > ---
> >  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 00..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  // offsetof
> > +#include   // 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 ## 

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

[Emil Velikov]

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  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 
> Reviewed-by: James Jones 
> ---
>  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 00..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  // offsetof
> +#include   // 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 

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

[Miguel A. Vico]

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 
Reviewed-by: James Jones 
---
 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
+
+wayland_egl_abi_check_SOURCES = wayland-egl-abi-check.c
+
 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 00..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  // offsetof
+#include   // 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 {
+struct wl_surface *surface;
+
+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 *);
+};
+
+/* From: ca3ed3e024 - Ander Conselvan de Oliveira : egl/wayland: Don't 
invalidate drawable on swap buffers */
+struct wl_egl_window_v1 {
+struct wl_surface *surface;
+
+int width;
+int height;
+int dx;
+int dy;
+
+int attached_width;
+int attached_height;
+
+void *private;
+void (*resize_callback)(struct wl_egl_window *, void *);
+};
+
+/* From: 214fc6e850 - Benjamin Franzke : egl: Implement libwayland-egl */
+struct wl_egl_window_v0 {
+struct wl_surface *surface;
+
+int width;
+int height;
+int dx;
+int dy;
+
+int attached_width;
+int attached_height;
+};
+
+
+/* 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
+
+#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;  
 \
+}