Re: [PATCH wayland] scanner: Fix broken private-code generation

2018-02-23 Thread Emil Velikov 
On 23 February 2018 at 22:24, Derek Foreman  wrote:
> Missing a closing bracket.
>
Eek, that's a embarasing.
Reviewed-by: Emil Velikov 

We might want to tweak the existing infra to check that, bonus points
if we also check the headers actually safe for inclusion in C and C++
sources.
A simple runner like weston-tests-env should do it.

-Emil
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland] scanner: Fix broken private-code generation

2018-02-23 Thread Derek Foreman 

Pushed with Daniel's RB.

Thanks,
Derek

On 2018-02-23 04:39 PM, Daniel Stone wrote:

Rb me (sorry for mangled phone client formatting ...)

On Fri, 23 Feb 2018, 10:25 pm Derek Foreman, > wrote:


Missing a closing bracket.

Signed-off-by: Derek Foreman >
---
  src/scanner.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/scanner.c b/src/scanner.c
index c93070c..1737911 100644
--- a/src/scanner.c
+++ b/src/scanner.c
@@ -1748,7 +1748,7 @@ emit_code(struct protocol *protocol, enum
visibility vis)
                        "# define __has_attribute(x) 0  /*
Compatibility with non-clang compilers. */\n"
                        "#endif\n\n");

-               printf("#if (__has_attribute(visibility) ||
defined(__GNUC__) && __GNUC__ >= 4\n"
+               printf("#if (__has_attribute(visibility) ||
defined(__GNUC__) && __GNUC__ >= 4)\n"
                        "#define WL_PRIVATE __attribute__
((visibility(\"hidden\")))\n"
                        "#else\n"
                        "#define WL_PRIVATE\n"
--
2.14.3

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org

https://lists.freedesktop.org/mailman/listinfo/wayland-devel



___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH wayland] scanner: Fix broken private-code generation

2018-02-23 Thread Derek Foreman 
Missing a closing bracket.

Signed-off-by: Derek Foreman 
---
 src/scanner.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/scanner.c b/src/scanner.c
index c93070c..1737911 100644
--- a/src/scanner.c
+++ b/src/scanner.c
@@ -1748,7 +1748,7 @@ emit_code(struct protocol *protocol, enum visibility vis)
   "# define __has_attribute(x) 0  /* Compatibility with 
non-clang compilers. */\n"
   "#endif\n\n");
 
-   printf("#if (__has_attribute(visibility) || defined(__GNUC__) 
&& __GNUC__ >= 4\n"
+   printf("#if (__has_attribute(visibility) || defined(__GNUC__) 
&& __GNUC__ >= 4)\n"
   "#define WL_PRIVATE __attribute__ 
((visibility(\"hidden\")))\n"
   "#else\n"
   "#define WL_PRIVATE\n"
-- 
2.14.3

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland] wayland-egl: use correct `nm` path when cross-compiling

2018-02-23 Thread Eric Engestrom 
On Friday, 2018-02-23 17:31:53 +, Emil Velikov wrote:
> From: Emil Velikov 
> 
> Inspired by Heiko Becker and Eric's work in libdrm and Mesa
> respectively.
> 
> Cc: Eric Engestrom 
> Signed-off-by: Emil Velikov 

Reviewed-by: Eric Engestrom 

I haven't really followed that, but I think the meson stuff is in
a branch somewhere? I guess it should get the other half of this patch
so that it doesn't need fixing when it lands :)

> ---
>  configure.ac  | 1 +
>  egl/wayland-egl-symbols-check | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 2542243..91f837d 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -29,6 +29,7 @@ AC_PROG_CC
>  AC_PROG_CXX
>  AC_PROG_GREP
>  AM_PROG_AS
> +AC_PROG_NM
>  
>  # check if we have C++ compiler. This is hacky workaround,
>  # for a reason why it is this way see
> diff --git a/egl/wayland-egl-symbols-check b/egl/wayland-egl-symbols-check
> index e107362..6ad28f3 100755
> --- a/egl/wayland-egl-symbols-check
> +++ b/egl/wayland-egl-symbols-check
> @@ -1,6 +1,6 @@
>  #!/bin/sh
>  
> -FUNCS=$(nm -D --defined-only ${WAYLAND_EGL_LIB} | grep -o "T .*" | cut -c 3- 
> | while read func; do
> +FUNCS=$($NM -D --defined-only ${WAYLAND_EGL_LIB} | grep -o "T .*" | cut -c 
> 3- | while read func; do
>  ( grep -q "^$func$" || echo $func )  <  wl_egl_window_resize
>  wl_egl_window_create
> -- 
> 2.16.0
> 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland] wayland-egl: use correct `nm` path when cross-compiling

2018-02-23 Thread Daniel Stone 
Hi,

On 23 February 2018 at 17:31, Emil Velikov  wrote:
> Inspired by Heiko Becker and Eric's work in libdrm and Mesa
> respectively.

Seems trivially correct and also worked here - pushed, thanks!

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH wayland] wayland-egl: use correct `nm` path when cross-compiling

2018-02-23 Thread Emil Velikov 
From: Emil Velikov 

Inspired by Heiko Becker and Eric's work in libdrm and Mesa
respectively.

Cc: Eric Engestrom 
Signed-off-by: Emil Velikov 
---
 configure.ac  | 1 +
 egl/wayland-egl-symbols-check | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 2542243..91f837d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -29,6 +29,7 @@ AC_PROG_CC
 AC_PROG_CXX
 AC_PROG_GREP
 AM_PROG_AS
+AC_PROG_NM
 
 # check if we have C++ compiler. This is hacky workaround,
 # for a reason why it is this way see
diff --git a/egl/wayland-egl-symbols-check b/egl/wayland-egl-symbols-check
index e107362..6ad28f3 100755
--- a/egl/wayland-egl-symbols-check
+++ b/egl/wayland-egl-symbols-check
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-FUNCS=$(nm -D --defined-only ${WAYLAND_EGL_LIB} | grep -o "T .*" | cut -c 3- | 
while read func; do
+FUNCS=$($NM -D --defined-only ${WAYLAND_EGL_LIB} | grep -o "T .*" | cut -c 3- 
| while read func; do
 ( grep -q "^$func$" || echo $func )

Re: [PATCH v14 19/41] compositor-drm: Extract buffer->plane co-ord translation

2018-02-23 Thread Daniel Stone 
Hi Pekka,

On 23 January 2018 at 14:20, Pekka Paalanen  wrote:
> On Wed, 20 Dec 2017 12:26:36 + Daniel Stone  wrote:
>> + if (sx1 < 0)
>> + sx1 = 0;
>> + if (sy1 < 0)
>> + sy1 = 0;
>> + if (sx2 > wl_fixed_from_int(ev->surface->width))
>> + sx2 = wl_fixed_from_int(ev->surface->width);
>> + if (sy2 > wl_fixed_from_int(ev->surface->height))
>> + sy2 = wl_fixed_from_int(ev->surface->height);
>
> If the clamps changed something, we'd need equivalent adjustment in the
> dest, but that seems to be ignored for now.

Hm ... I am feeling particularly dense today, but I can't get my head
around this. Why/how would we need to translate the dest region?

(The rest is all fine though: thankyou!)

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 1/1] tests: Add free-without-remove test

2018-02-23 Thread Markus Ongyerth 
On 2018/2月/23 08:53, Derek Foreman wrote:
> On 2018-02-23 07:44 AM, Markus Ongyerth wrote:
> > On 2018/2月/23 07:31, Derek Foreman wrote:
> > > On 2018-02-23 03:52 AM, w...@ongy.net wrote:
> > > > From: Markus Ongyerth 
> > > > 
> > > > This is related to the discussion earlier on the mailing list and
> > > > demonstrates a problem with current wl_priv_signal_emit and wl_listener
> > > > that free themselves, but do not remove from the list.
> > > > 
> > > > This testcase asserts that the wl_list inside wl_listener is not written
> > > > to anymore after it got emitted.
> > > > ---
> > > >Makefile.am |  3 +++
> > > >tests/free-without-remove.c | 63 
> > > > +
> > > >2 files changed, 66 insertions(+)
> > > >create mode 100644 tests/free-without-remove.c
> > > > 
> > > > diff --git a/Makefile.am b/Makefile.am
> > > > index 322d6b8..c51f328 100644
> > > > --- a/Makefile.am
> > > > +++ b/Makefile.am
> > > > @@ -158,6 +158,7 @@ pkgconfig_DATA += egl/wayland-egl-backend.pc
> > > >built_test_programs =\
> > > > array-test  \
> > > > +   free-without-remove-test\
> > > > client-test \
> > > > display-test\
> > > > connection-test \
> > > > @@ -223,6 +224,8 @@ libtest_runner_la_LIBADD =  \
> > > >array_test_SOURCES = tests/array-test.c
> > > >array_test_LDADD = libtest-runner.la
> > > > +free_without_remove_test_SOURCES = tests/free-without-remove.c
> > > > +free_without_remove_test_LDADD = libtest-runner.la
> > > >client_test_SOURCES = tests/client-test.c
> > > >client_test_LDADD = libtest-runner.la
> > > >display_test_SOURCES = tests/display-test.c
> > > > diff --git a/tests/free-without-remove.c b/tests/free-without-remove.c
> > > > new file mode 100644
> > > > index 000..ff468d7
> > > > --- /dev/null
> > > > +++ b/tests/free-without-remove.c
> > > > @@ -0,0 +1,63 @@
> > > > +/*
> > > > + * Copyright © 2018 Markus Ongyerth
> > > > + *
> > > > + * 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 (including the
> > > > + * next paragraph) 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 
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > > +
> > > > +#include "wayland-private.h"
> > > > +#include "wayland-server.h"
> > > > +#include "test-runner.h"
> > > > +
> > > > +static void
> > > > +display_destroy_notify(struct wl_listener *l, void *data)
> > > > +{
> > > > +   l->link.prev = l->link.next = NULL;
> > > > +}
> > > > +
> > > > +TEST(free_without_remove)
> > > > +{
> > > > +   struct wl_display *display;
> > > > +   struct wl_listener a, b;
> > > > +
> > > > +   display = wl_display_create();
> > > > +   a.notify = display_destroy_notify;
> > > > +   b.notify = display_destroy_notify;
> > > > +
> > > > +   wl_display_add_destroy_listener(display, );
> > > > +   wl_display_add_destroy_listener(display, );
> > > > +
> > > > +   wl_display_destroy(display);
> > > > +
> > > > +   assert(a.link.next == a.link.prev && a.link.next == NULL);
> > > > +   assert(b.link.next == b.link.prev && b.link.next == NULL);
> > > 
> > > I feel this would be slightly more clear if it tested
> > > a.link.next == NULL && a.link.prev == NULL
> > Possibly.
> > > 
> > > As my previously submit test did... is this significantly more rigorous 
> > > than
> > > my test here: https://patchwork.freedesktop.org/patch/206013/  ?
> > Did you try to run this one? It *fails* on the

Re: [PATCH 3/3] rdp-backend.so: OpenGL hardware acceleration

2018-02-23 Thread Raimundo Sagarzazu 
Hi,
 
Two patches that improve performance in my case. 
 
First, supporting ARGB gbm-format for the output allows a much better 
performance of the intelReadPixels function of the i965 driver of Mesa, which 
is my case:
 
diff -rup a/libweston/compositor-rdp.c b/libweston/compositor-rdp.c
--- a/libweston/compositor-rdp.c   2018-02-22 11:35:14.0 +0100
+++ b/libweston/compositor-rdp.c2018-02-22 11:37:20.312159332 +0100
@@ -592,6 +592,8 @@ parse_gbm_format(const char *s, uint32_t
  *gbm_format = default_value;
   else if (strcmp(s, "xrgb") == 0)
  *gbm_format = GBM_FORMAT_XRGB;
+ else if (strcmp(s, "argb") == 0)
+ *gbm_format = GBM_FORMAT_ARGB;
   else if (strcmp(s, "rgb565") == 0)
  *gbm_format = GBM_FORMAT_RGB565;
   else if (strcmp(s, "xrgb2101010") == 0)
 
Second, reading just the damaged pixels and y-flipping back the image:
 
diff -rup a/libweston/compositor-rdp.c b/libweston/compositor-rdp.c
--- a/libweston/compositor-rdp.c   2018-02-20 16:04:20.0 +0100
+++ b/libweston/compositor-rdp.c2018-02-20 16:10:37.575768246 +0100
@@ -146,6 +146,12 @@ struct rdp_output {
   struct gbm_surface *gbm_surface;
   pixman_image_t *shadow_surface;
+ int format;
+ int stride;
+ int height;
+ uint32_t *tmpdata, *image;
+ uint32_t tmpdata_size;
+
   struct wl_list peers;
};
@@ -391,12 +397,25 @@ rdp_output_repaint(struct weston_output
   if (pixman_region32_not_empty(damage)) {
  if (!b->use_pixman) {
  /* TODO: Performance: Only read 
pixels that was actually repained by renderer->repaint_output. */
- 
ec->renderer->read_pixels(output_base,
+/*
ec->renderer->read_pixels(output_base,
 
pixman_image_get_format(output->shadow_surface),
 
pixman_image_get_data(output->shadow_surface),
 0, 
0,
 
pixman_image_get_width(output->shadow_surface),
- 
pixman_image_get_height(output->shadow_surface));
+
pixman_image_get_height(output->shadow_surface));*/
+int
 i, nrects;
+int
 x, y, width, height;
+pixman_box32_t 
*rect=pixman_region32_rectangles(damage, );
+for (i=0; irenderer->read_pixels(output_base, output->format, output->tmpdata, x, 
output->height-rect[i].y2, width, height);
+
pixman_blt(output->tmpdata, output->image, -width, output->stride, 32, 32, 0, 
1-height, x, y, width, height);
+}
  }
   wl_list_for_each(outputPeer, >peers, 
link) {
@@ -650,6 +669,30 @@ rdp_output_fini_egl(struct rdp_output *o
}
 static int
+rdp_get_image_size(struct rdp_output *output)
+{
+ uint32_t  
image_size=pixman_image_get_width(output->shadow_surface)*pixman_image_get_height(output->shadow_surface)*sizeof(uint32_t);
+
+ if (output->tmpdata_size < image_size) {
+ free(output->tmpdata);
+ output->tmpdata = malloc(image_size);
+ if (output->tmpdata == NULL) {
+output->tmpdata_size = 0;
+errno = ENOMEM;
+return -1;
+ }
+ 
+ output->tmpdata_size =

Re: [PATCH 1/1] tests: Add free-without-remove test

2018-02-23 Thread Derek Foreman 

On 2018-02-23 07:44 AM, Markus Ongyerth wrote:

On 2018/2月/23 07:31, Derek Foreman wrote:

On 2018-02-23 03:52 AM, w...@ongy.net wrote:

From: Markus Ongyerth 

This is related to the discussion earlier on the mailing list and
demonstrates a problem with current wl_priv_signal_emit and wl_listener
that free themselves, but do not remove from the list.

This testcase asserts that the wl_list inside wl_listener is not written
to anymore after it got emitted.
---
   Makefile.am |  3 +++
   tests/free-without-remove.c | 63 
+
   2 files changed, 66 insertions(+)
   create mode 100644 tests/free-without-remove.c

diff --git a/Makefile.am b/Makefile.am
index 322d6b8..c51f328 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -158,6 +158,7 @@ pkgconfig_DATA += egl/wayland-egl-backend.pc
   built_test_programs =\
array-test  \
+   free-without-remove-test\
client-test \
display-test\
connection-test \
@@ -223,6 +224,8 @@ libtest_runner_la_LIBADD =  \
   array_test_SOURCES = tests/array-test.c
   array_test_LDADD = libtest-runner.la
+free_without_remove_test_SOURCES = tests/free-without-remove.c
+free_without_remove_test_LDADD = libtest-runner.la
   client_test_SOURCES = tests/client-test.c
   client_test_LDADD = libtest-runner.la
   display_test_SOURCES = tests/display-test.c
diff --git a/tests/free-without-remove.c b/tests/free-without-remove.c
new file mode 100644
index 000..ff468d7
--- /dev/null
+++ b/tests/free-without-remove.c
@@ -0,0 +1,63 @@
+/*
+ * Copyright © 2018 Markus Ongyerth
+ *
+ * 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 (including the
+ * next paragraph) 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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "wayland-private.h"
+#include "wayland-server.h"
+#include "test-runner.h"
+
+static void
+display_destroy_notify(struct wl_listener *l, void *data)
+{
+   l->link.prev = l->link.next = NULL;
+}
+
+TEST(free_without_remove)
+{
+   struct wl_display *display;
+   struct wl_listener a, b;
+
+   display = wl_display_create();
+   a.notify = display_destroy_notify;
+   b.notify = display_destroy_notify;
+
+   wl_display_add_destroy_listener(display, );
+   wl_display_add_destroy_listener(display, );
+
+   wl_display_destroy(display);
+
+   assert(a.link.next == a.link.prev && a.link.next == NULL);
+   assert(b.link.next == b.link.prev && b.link.next == NULL);


I feel this would be slightly more clear if it tested
a.link.next == NULL && a.link.prev == NULL

Possibly.


As my previously submit test did... is this significantly more rigorous than
my test here: https://patchwork.freedesktop.org/patch/206013/  ?

Did you try to run this one? It *fails* on the current version of libwayland,
because wl_priv_signal_emit will write into one of the wl_listeners after it
got emitted.

This only happens where there's more than one listener in the signal list,
which is the reason your test doesn't catch it.


More rigorous indeed - but we can't land it without fixing the bug first. :)

Also, I'm not sure it warrants a whole new file when it could just piggy 
back an existing test...  I guess now that I think about it, signal-test 
seems most appropriate?


Thanks,
Derek



Thanks,
Derek


+}



___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel


___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 1/1] tests: Add free-without-remove test

2018-02-23 Thread Markus Ongyerth 
On 2018/2月/23 07:31, Derek Foreman wrote:
> On 2018-02-23 03:52 AM, w...@ongy.net wrote:
> > From: Markus Ongyerth 
> > 
> > This is related to the discussion earlier on the mailing list and
> > demonstrates a problem with current wl_priv_signal_emit and wl_listener
> > that free themselves, but do not remove from the list.
> > 
> > This testcase asserts that the wl_list inside wl_listener is not written
> > to anymore after it got emitted.
> > ---
> >   Makefile.am |  3 +++
> >   tests/free-without-remove.c | 63 
> > +
> >   2 files changed, 66 insertions(+)
> >   create mode 100644 tests/free-without-remove.c
> > 
> > diff --git a/Makefile.am b/Makefile.am
> > index 322d6b8..c51f328 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -158,6 +158,7 @@ pkgconfig_DATA += egl/wayland-egl-backend.pc
> >   built_test_programs = \
> > array-test  \
> > +   free-without-remove-test\
> > client-test \
> > display-test\
> > connection-test \
> > @@ -223,6 +224,8 @@ libtest_runner_la_LIBADD =  \
> >   array_test_SOURCES = tests/array-test.c
> >   array_test_LDADD = libtest-runner.la
> > +free_without_remove_test_SOURCES = tests/free-without-remove.c
> > +free_without_remove_test_LDADD = libtest-runner.la
> >   client_test_SOURCES = tests/client-test.c
> >   client_test_LDADD = libtest-runner.la
> >   display_test_SOURCES = tests/display-test.c
> > diff --git a/tests/free-without-remove.c b/tests/free-without-remove.c
> > new file mode 100644
> > index 000..ff468d7
> > --- /dev/null
> > +++ b/tests/free-without-remove.c
> > @@ -0,0 +1,63 @@
> > +/*
> > + * Copyright © 2018 Markus Ongyerth
> > + *
> > + * 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 (including the
> > + * next paragraph) 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 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include "wayland-private.h"
> > +#include "wayland-server.h"
> > +#include "test-runner.h"
> > +
> > +static void
> > +display_destroy_notify(struct wl_listener *l, void *data)
> > +{
> > +   l->link.prev = l->link.next = NULL;
> > +}
> > +
> > +TEST(free_without_remove)
> > +{
> > +   struct wl_display *display;
> > +   struct wl_listener a, b;
> > +
> > +   display = wl_display_create();
> > +   a.notify = display_destroy_notify;
> > +   b.notify = display_destroy_notify;
> > +
> > +   wl_display_add_destroy_listener(display, );
> > +   wl_display_add_destroy_listener(display, );
> > +
> > +   wl_display_destroy(display);
> > +
> > +   assert(a.link.next == a.link.prev && a.link.next == NULL);
> > +   assert(b.link.next == b.link.prev && b.link.next == NULL);
> 
> I feel this would be slightly more clear if it tested
> a.link.next == NULL && a.link.prev == NULL
Possibly.
> 
> As my previously submit test did... is this significantly more rigorous than
> my test here: https://patchwork.freedesktop.org/patch/206013/  ?
Did you try to run this one? It *fails* on the current version of libwayland, 
because wl_priv_signal_emit will write into one of the wl_listeners after it 
got emitted.

This only happens where there's more than one listener in the signal list, 
which is the reason your test doesn't catch it.
> 
> Thanks,
> Derek
> 
> > +}
> > 
> 
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel


signature.asc
Description: PGP signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org

Re: [PATCH 1/1] tests: Add free-without-remove test

2018-02-23 Thread Derek Foreman 

On 2018-02-23 03:52 AM, w...@ongy.net wrote:

From: Markus Ongyerth 

This is related to the discussion earlier on the mailing list and
demonstrates a problem with current wl_priv_signal_emit and wl_listener
that free themselves, but do not remove from the list.

This testcase asserts that the wl_list inside wl_listener is not written
to anymore after it got emitted.
---
  Makefile.am |  3 +++
  tests/free-without-remove.c | 63 +
  2 files changed, 66 insertions(+)
  create mode 100644 tests/free-without-remove.c

diff --git a/Makefile.am b/Makefile.am
index 322d6b8..c51f328 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -158,6 +158,7 @@ pkgconfig_DATA += egl/wayland-egl-backend.pc
  
  built_test_programs =\

array-test  \
+   free-without-remove-test\
client-test \
display-test\
connection-test \
@@ -223,6 +224,8 @@ libtest_runner_la_LIBADD =  \
  
  array_test_SOURCES = tests/array-test.c

  array_test_LDADD = libtest-runner.la
+free_without_remove_test_SOURCES = tests/free-without-remove.c
+free_without_remove_test_LDADD = libtest-runner.la
  client_test_SOURCES = tests/client-test.c
  client_test_LDADD = libtest-runner.la
  display_test_SOURCES = tests/display-test.c
diff --git a/tests/free-without-remove.c b/tests/free-without-remove.c
new file mode 100644
index 000..ff468d7
--- /dev/null
+++ b/tests/free-without-remove.c
@@ -0,0 +1,63 @@
+/*
+ * Copyright © 2018 Markus Ongyerth
+ *
+ * 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 (including the
+ * next paragraph) 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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "wayland-private.h"
+#include "wayland-server.h"
+#include "test-runner.h"
+
+static void
+display_destroy_notify(struct wl_listener *l, void *data)
+{
+   l->link.prev = l->link.next = NULL;
+}
+
+TEST(free_without_remove)
+{
+   struct wl_display *display;
+   struct wl_listener a, b;
+
+   display = wl_display_create();
+   a.notify = display_destroy_notify;
+   b.notify = display_destroy_notify;
+
+   wl_display_add_destroy_listener(display, );
+   wl_display_add_destroy_listener(display, );
+
+   wl_display_destroy(display);
+
+   assert(a.link.next == a.link.prev && a.link.next == NULL);
+   assert(b.link.next == b.link.prev && b.link.next == NULL);


I feel this would be slightly more clear if it tested
a.link.next == NULL && a.link.prev == NULL

As my previously submit test did... is this significantly more rigorous 
than my test here: https://patchwork.freedesktop.org/patch/206013/  ?


Thanks,
Derek


+}



___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [RFC wayland] server: Add special case destroy signal emitter

2018-02-23 Thread Derek Foreman 

On 2018-02-23 02:15 AM, Pekka Paalanen wrote:

On Thu, 22 Feb 2018 16:02:49 -0600
Derek Foreman  wrote:


In the past much code (weston, efl/enlightenment, mutter) has
freed structures containing wl_listeners from destroy handlers
without first removing the listener from the signal.  As the
destroy notifier only fires once, this has largely gone
unnoticed until recently.

Other code does not (Qt, wlroots) - and removes itself from
the signal before free.

If somehow a destroy signal is listened to by code from both
kinds of callers, those that free will corrupt the lists for
those that don't, and Bad Things will happen.

To avoid these bad things, remove every item from the signal list
during destroy emit, and put it in a list all its own.  This way
whether the listener is removed or not has no impact on the
following emits.

Signed-off-by: Derek Foreman 
---

  src/wayland-private.h |  3 +++
  src/wayland-server.c  | 49 ++---
  2 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/src/wayland-private.h b/src/wayland-private.h
index 12b9032..29516ec 100644
--- a/src/wayland-private.h
+++ b/src/wayland-private.h
@@ -253,6 +253,9 @@ wl_priv_signal_get(struct wl_priv_signal *signal, 
wl_notify_func_t notify);
  void
  wl_priv_signal_emit(struct wl_priv_signal *signal, void *data);
  
+void

+wl_priv_signal_final_emit(struct wl_priv_signal *signal, void *data);
+
  void
  wl_connection_close_fds_in(struct wl_connection *connection, int max);
  
diff --git a/src/wayland-server.c b/src/wayland-server.c

index eb1e500..df5c16a 100644
--- a/src/wayland-server.c
+++ b/src/wayland-server.c
@@ -682,7 +682,7 @@ destroy_resource(void *element, void *data, uint32_t flags)
/* Don't emit the new signal for deprecated resources, as that would
 * access memory outside the bounds of the deprecated struct */
if (!resource_is_deprecated(resource))
-   wl_priv_signal_emit(>destroy_signal, resource);
+   wl_priv_signal_final_emit(>destroy_signal, resource);
  
  	if (resource->destroy)

resource->destroy(resource);
@@ -841,7 +841,7 @@ wl_client_destroy(struct wl_client *client)
  {
uint32_t serial = 0;
  
-	wl_priv_signal_emit(>destroy_signal, client);

+   wl_priv_signal_final_emit(>destroy_signal, client);
  
  	wl_client_flush(client);

wl_map_for_each(>objects, destroy_resource, );
@@ -1089,7 +1089,7 @@ wl_display_destroy(struct wl_display *display)
struct wl_socket *s, *next;
struct wl_global *global, *gnext;
  
-	wl_priv_signal_emit(>destroy_signal, display);

+   wl_priv_signal_final_emit(>destroy_signal, display);
  
  	wl_list_for_each_safe(s, next, >socket_list, link) {

wl_socket_destroy(s);
@@ -2025,6 +2025,49 @@ wl_priv_signal_emit(struct wl_priv_signal *signal, void 
*data)
}
  }
  
+/** Emit the signal for the last time, calling all the installed listeners

+ *
+ * Iterate over all the listeners added to this \a signal and call
+ * their \a notify function pointer, passing on the given \a data.
+ * Removing or adding a listener from within wl_priv_signal_emit()
+ * is safe, as is freeing the structure containing the listener.
+ *
+ * A large body of external code assumes it's ok to free a destruction
+ * listener without removing that listener from the list.  Mixing code
+ * that acts like this and code that doesn't will result in list
+ * corruption.
+ *
+ * We resolve this by removing each item from the list and isolating it
+ * in another list.  We discard it completely after firing the notifier.
+ * This should allow interoperability between code that unlinks its
+ * destruction listeners and code that just frees structures they're in.
+ *
+ */
+void
+wl_priv_signal_final_emit(struct wl_priv_signal *signal, void *data)
+{
+   struct wl_listener *l;
+   struct wl_list *pos;
+
+   wl_list_insert_list(>emit_list, >listener_list);
+
+   /* During a destructor notifier isolate every list item before
+* notifying.  This renders harmless the long standing misuse
+* of freeing listeners without removing them, but allows
+* callers that do choose to remove them to interoperate with
+* ones that don't. */
+   while (!wl_list_empty(>emit_list)) {
+   wl_list_init(>listener_list);
+   pos = signal->emit_list.next;
+   l = wl_container_of(pos, l, link);
+
+   wl_list_remove(pos);
+   wl_list_insert(>listener_list, pos);


Just a quick fly-by comment; is there a reason to actually have a head
in the personal list for 'pos'? Wouldn't wl_list_init(pos) be enough?


I was trying to keep wl_priv_signal_get() "working" - though I'm fairly 
solidly convinced it's totally useless for that case...



Or would you want keep e.g. checks like
wl_list_empty(_destroy_listener->link) working? Not

[PATCH 1/1] tests: Add free-without-remove test

2018-02-23 Thread wl 
From: Markus Ongyerth 

This is related to the discussion earlier on the mailing list and
demonstrates a problem with current wl_priv_signal_emit and wl_listener
that free themselves, but do not remove from the list.

This testcase asserts that the wl_list inside wl_listener is not written
to anymore after it got emitted.
---
 Makefile.am |  3 +++
 tests/free-without-remove.c | 63 +
 2 files changed, 66 insertions(+)
 create mode 100644 tests/free-without-remove.c

diff --git a/Makefile.am b/Makefile.am
index 322d6b8..c51f328 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -158,6 +158,7 @@ pkgconfig_DATA += egl/wayland-egl-backend.pc
 
 built_test_programs =  \
array-test  \
+   free-without-remove-test\
client-test \
display-test\
connection-test \
@@ -223,6 +224,8 @@ libtest_runner_la_LIBADD =  \
 
 array_test_SOURCES = tests/array-test.c
 array_test_LDADD = libtest-runner.la
+free_without_remove_test_SOURCES = tests/free-without-remove.c
+free_without_remove_test_LDADD = libtest-runner.la
 client_test_SOURCES = tests/client-test.c
 client_test_LDADD = libtest-runner.la
 display_test_SOURCES = tests/display-test.c
diff --git a/tests/free-without-remove.c b/tests/free-without-remove.c
new file mode 100644
index 000..ff468d7
--- /dev/null
+++ b/tests/free-without-remove.c
@@ -0,0 +1,63 @@
+/*
+ * Copyright © 2018 Markus Ongyerth
+ *
+ * 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 (including the
+ * next paragraph) 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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "wayland-private.h"
+#include "wayland-server.h"
+#include "test-runner.h"
+
+static void
+display_destroy_notify(struct wl_listener *l, void *data)
+{
+   l->link.prev = l->link.next = NULL;
+}
+
+TEST(free_without_remove)
+{
+   struct wl_display *display;
+   struct wl_listener a, b;
+
+   display = wl_display_create();
+   a.notify = display_destroy_notify;
+   b.notify = display_destroy_notify;
+
+   wl_display_add_destroy_listener(display, );
+   wl_display_add_destroy_listener(display, );
+
+   wl_display_destroy(display);
+
+   assert(a.link.next == a.link.prev && a.link.next == NULL);
+   assert(b.link.next == b.link.prev && b.link.next == NULL);
+}
-- 
2.16.2

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH 0/1] free-without-remove v2

2018-02-23 Thread wl 
From: Markus Ongyerth 

This is a v2 for the free-without-remove test, that actually fails the test and
does not require the use of valgrind to make observe the problem.

This is intended to fail at the moment, as the primary purpose is to show a
problem with current days wl_priv_signal_emit.

Or rather in code of consumers, that made asumptions that were never true :)

Markus Ongyerth (1):
  tests: Add free-without-remove test

 Makefile.am |  3 +++
 tests/free-without-remove.c | 63 +
 2 files changed, 66 insertions(+)
 create mode 100644 tests/free-without-remove.c

-- 
2.16.2

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [RFC wayland] server: Add special case destroy signal emitter

2018-02-23 Thread Markus Ongyerth 
Hi, I have a v2 RFC _emit_final based on your idea.
It passes `make check` of libwayland and weston.
It also passes the remove-without-free test I sent in another mail.

---

(This patch isn't quite intended to be merged, but to get feedback on
the approach)

This adds a wl_priv_signal_emit_final and a wl_signal_emit_final.
The `_final` emit functions have the additional asumption that every
listener currently in the list will be removed after the _emit. Either
gracefully, or simply free()d under the asumption that a destroy signal
will not be emitted more than once.

These functions take advantage of this and allow to use both tyles in
the same signal list.
It obsoletes the wl_priv_signal for destroy signals.

Non destroy wl_priv_signals currently have a stronger guarantee than
normal signals, which forces us to either keep the type around, or use
this as destroy signal path, and another safe version for other signals.
---
 src/wayland-private.h |  5 +
 src/wayland-server.c  | 50 +++---
 2 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/src/wayland-private.h b/src/wayland-private.h
index 12b9032..76adb32 100644
--- a/src/wayland-private.h
+++ b/src/wayland-private.h
@@ -250,6 +250,11 @@ wl_priv_signal_add(struct wl_priv_signal *signal, struct 
wl_listener *listener);
 struct wl_listener *
 wl_priv_signal_get(struct wl_priv_signal *signal, wl_notify_func_t notify);
 
+void
+wl_signal_emit_final(struct wl_list *signal, void *data);
+
+void
+wl_priv_signal_emit_final(struct wl_priv_signal *signal, void *data);
 void
 wl_priv_signal_emit(struct wl_priv_signal *signal, void *data);
 
diff --git a/src/wayland-server.c b/src/wayland-server.c
index eb1e500..62fc71a 100644
--- a/src/wayland-server.c
+++ b/src/wayland-server.c
@@ -682,7 +682,7 @@ destroy_resource(void *element, void *data, uint32_t flags)
/* Don't emit the new signal for deprecated resources, as that would
 * access memory outside the bounds of the deprecated struct */
if (!resource_is_deprecated(resource))
-   wl_priv_signal_emit(>destroy_signal, resource);
+   wl_priv_signal_emit_final(>destroy_signal, resource);
 
if (resource->destroy)
resource->destroy(resource);
@@ -841,7 +841,7 @@ wl_client_destroy(struct wl_client *client)
 {
uint32_t serial = 0;
 
-   wl_priv_signal_emit(>destroy_signal, client);
+   wl_priv_signal_emit_final(>destroy_signal, client);
 
wl_client_flush(client);
wl_map_for_each(>objects, destroy_resource, );
@@ -1089,7 +1089,7 @@ wl_display_destroy(struct wl_display *display)
struct wl_socket *s, *next;
struct wl_global *global, *gnext;
 
-   wl_priv_signal_emit(>destroy_signal, display);
+   wl_priv_signal_emit_final(>destroy_signal, display);
 
wl_list_for_each_safe(s, next, >socket_list, link) {
wl_socket_destroy(s);
@@ -1992,6 +1992,50 @@ wl_priv_signal_get(struct wl_priv_signal *signal, 
wl_notify_func_t notify)
return NULL;
 }
 
+/**
+ * This emits a signal under the asumpton that every listener in it will be
+ * removed from the list in some way.
+ * 
+ * It supports listeners that remove themselves without issues.
+ *
+ * The situation where a listener is only free()d, but not removed from the
+ * list is also supported, to not break existing library users.
+ *
+ * The notify function can remove arbitrary elements from the wl_signal, and
+ * the wl_signal will be in a valid state at all times (s.t. later elements in
+ * the list not beeing free()d without removal, but that broke at any time).
+ *
+ * The idea is based on wl_priv_signal_emit, we iterate until a list is empty.
+ * In this case we don't need the emit_list, since we know our actual signal
+ * list will be empty in the end.
+ */
+void
+wl_signal_emit_final(struct wl_list *signal, void *data)
+{
+   struct wl_listener *l;
+   struct wl_list *pos;
+
+   while (!wl_list_empty(signal)) {
+   pos = signal->next;
+   
+   /* Remove and ensure validity of position element */
+   wl_list_remove(pos);
+   wl_list_init(pos);
+
+   l = wl_container_of(pos, l, link);
+   l->notify(l, data);
+   }
+}
+
+/**
+ * see wl_signal_emit_final for more details
+ */
+void
+wl_priv_signal_emit_final(struct wl_priv_signal *signal, void *data)
+{
+   wl_signal_emit_final(>listener_list, data);
+}
+
 /** Emit the signal, calling all the installed listeners
  *
  * Iterate over all the listeners added to this \a signal and call
-- 
2.16.2



On 2018/2月/22 04:02, Derek Foreman wrote:
> In the past much code (weston, efl/enlightenment, mutter) has
> freed structures containing wl_listeners from destroy handlers
> without first removing the listener from the signal.  As the
> destroy notifier only fires once, this has largely gone
> unnoticed until recently.
> 
>

[PATCH 1/1] tests: Add free-without-remove test

2018-02-23 Thread wl 
From: Markus Ongyerth 

This is related to the discussion earlier on the mailing list and
demonstrates a problem with current wl_priv_signal_emit and wl_listener
that free themselves, but do not remove from the list.

The testcase itself passes. And I'm not sure if that can be prevented.
To reliably observe the "Invalid write of size 8" in valgrind (memcheck)
simply run `valgrind ./.libs/remove-without-free`.

Signed-of-by: Markus Ongyerth 
---
 Makefile.am |  3 ++
 tests/free-without-remove.c | 70 +
 2 files changed, 73 insertions(+)
 create mode 100644 tests/free-without-remove.c

diff --git a/Makefile.am b/Makefile.am
index 322d6b8..c51f328 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -158,6 +158,7 @@ pkgconfig_DATA += egl/wayland-egl-backend.pc
 
 built_test_programs =  \
array-test  \
+   free-without-remove-test\
client-test \
display-test\
connection-test \
@@ -223,6 +224,8 @@ libtest_runner_la_LIBADD =  \
 
 array_test_SOURCES = tests/array-test.c
 array_test_LDADD = libtest-runner.la
+free_without_remove_test_SOURCES = tests/free-without-remove.c
+free_without_remove_test_LDADD = libtest-runner.la
 client_test_SOURCES = tests/client-test.c
 client_test_LDADD = libtest-runner.la
 display_test_SOURCES = tests/display-test.c
diff --git a/tests/free-without-remove.c b/tests/free-without-remove.c
new file mode 100644
index 000..3bc335d
--- /dev/null
+++ b/tests/free-without-remove.c
@@ -0,0 +1,70 @@
+/*
+ * Copyright C 2018 Markus Ongyerth
+ *
+ * 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 (including the
+ * next paragraph) 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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "wayland-private.h"
+#include "wayland-server.h"
+#include "test-runner.h"
+
+struct display_destroy_listener {
+   struct wl_listener listener;
+};
+
+static void
+display_destroy_notify(struct wl_listener *l, void *data)
+{
+   struct display_destroy_listener *listener =
+   container_of(l, struct display_destroy_listener, listener);
+
+   free(listener);
+}
+TEST(client_destroy_listener)
+{
+   struct wl_display *display;
+   struct display_destroy_listener *a;
+   struct display_destroy_listener *b;
+
+   a = malloc(sizeof(struct display_destroy_listener));
+   b = malloc(sizeof(struct display_destroy_listener));
+
+   display = wl_display_create();
+   a->listener.notify = display_destroy_notify;
+   b->listener.notify = display_destroy_notify;
+
+   wl_display_add_destroy_listener(display, >listener);
+   wl_display_add_destroy_listener(display, >listener);
+
+   wl_display_destroy(display);
+}
-- 
2.16.2

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH 0/1] Add remove-without-free test

2018-02-23 Thread wl 
From: Markus Ongyerth 

In [1] it was pointed out, that the proposed patch breaks code, that does not
remove their listener from a wl_signal list, but just free()s them.

From libwayland's perspective, this has never been different. People just got
lucky.
wl_priv_signal_emit will touch the recently free()d memory again IFF there is
another listener behind it in the signal list.
The attached patch makes this obvious under valgrind (memcheck). I would prefer
if it failed the testcase, but I'm not sure how to do that (some sanitizer
options?).

Going forward, it would be relativly easy to add a special case for single
listener signals to the original patch, but in the rather lengthy discussion we
had earlier, we concluded that we should try to fix clients from libwayland
side.
But this could cause problems for code that uses wl_signal and wl_signal_emit
internally, since that should actually not touch any previously called
wl_listener in the signal list (until next emit).

A secondary emit_destroy (emit_final) path as suggested in [2] seems to be the
better way.


[1] 
https://lists.freedesktop.org/archives/wayland-devel/2018-February/037168.html
[2] 
https://lists.freedesktop.org/archives/wayland-devel/2018-February/037203.html

Markus Ongyerth (1):
  tests: Add free-without-remove test

 Makefile.am |  3 ++
 tests/free-without-remove.c | 70 +
 2 files changed, 73 insertions(+)
 create mode 100644 tests/free-without-remove.c

-- 
2.16.2

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [RFC wayland] server: Add special case destroy signal emitter

2018-02-23 Thread Pekka Paalanen 
On Thu, 22 Feb 2018 16:02:49 -0600
Derek Foreman  wrote:

> In the past much code (weston, efl/enlightenment, mutter) has
> freed structures containing wl_listeners from destroy handlers
> without first removing the listener from the signal.  As the
> destroy notifier only fires once, this has largely gone
> unnoticed until recently.
> 
> Other code does not (Qt, wlroots) - and removes itself from
> the signal before free.
> 
> If somehow a destroy signal is listened to by code from both
> kinds of callers, those that free will corrupt the lists for
> those that don't, and Bad Things will happen.
> 
> To avoid these bad things, remove every item from the signal list
> during destroy emit, and put it in a list all its own.  This way
> whether the listener is removed or not has no impact on the
> following emits.
> 
> Signed-off-by: Derek Foreman 
> ---
> 
>  src/wayland-private.h |  3 +++
>  src/wayland-server.c  | 49 ++---
>  2 files changed, 49 insertions(+), 3 deletions(-)
> 
> diff --git a/src/wayland-private.h b/src/wayland-private.h
> index 12b9032..29516ec 100644
> --- a/src/wayland-private.h
> +++ b/src/wayland-private.h
> @@ -253,6 +253,9 @@ wl_priv_signal_get(struct wl_priv_signal *signal, 
> wl_notify_func_t notify);
>  void
>  wl_priv_signal_emit(struct wl_priv_signal *signal, void *data);
>  
> +void
> +wl_priv_signal_final_emit(struct wl_priv_signal *signal, void *data);
> +
>  void
>  wl_connection_close_fds_in(struct wl_connection *connection, int max);
>  
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index eb1e500..df5c16a 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -682,7 +682,7 @@ destroy_resource(void *element, void *data, uint32_t 
> flags)
>   /* Don't emit the new signal for deprecated resources, as that would
>* access memory outside the bounds of the deprecated struct */
>   if (!resource_is_deprecated(resource))
> - wl_priv_signal_emit(>destroy_signal, resource);
> + wl_priv_signal_final_emit(>destroy_signal, resource);
>  
>   if (resource->destroy)
>   resource->destroy(resource);
> @@ -841,7 +841,7 @@ wl_client_destroy(struct wl_client *client)
>  {
>   uint32_t serial = 0;
>  
> - wl_priv_signal_emit(>destroy_signal, client);
> + wl_priv_signal_final_emit(>destroy_signal, client);
>  
>   wl_client_flush(client);
>   wl_map_for_each(>objects, destroy_resource, );
> @@ -1089,7 +1089,7 @@ wl_display_destroy(struct wl_display *display)
>   struct wl_socket *s, *next;
>   struct wl_global *global, *gnext;
>  
> - wl_priv_signal_emit(>destroy_signal, display);
> + wl_priv_signal_final_emit(>destroy_signal, display);
>  
>   wl_list_for_each_safe(s, next, >socket_list, link) {
>   wl_socket_destroy(s);
> @@ -2025,6 +2025,49 @@ wl_priv_signal_emit(struct wl_priv_signal *signal, 
> void *data)
>   }
>  }
>  
> +/** Emit the signal for the last time, calling all the installed listeners
> + *
> + * Iterate over all the listeners added to this \a signal and call
> + * their \a notify function pointer, passing on the given \a data.
> + * Removing or adding a listener from within wl_priv_signal_emit()
> + * is safe, as is freeing the structure containing the listener.
> + *
> + * A large body of external code assumes it's ok to free a destruction
> + * listener without removing that listener from the list.  Mixing code
> + * that acts like this and code that doesn't will result in list
> + * corruption.
> + *
> + * We resolve this by removing each item from the list and isolating it
> + * in another list.  We discard it completely after firing the notifier.
> + * This should allow interoperability between code that unlinks its
> + * destruction listeners and code that just frees structures they're in.
> + *
> + */
> +void
> +wl_priv_signal_final_emit(struct wl_priv_signal *signal, void *data)
> +{
> + struct wl_listener *l;
> + struct wl_list *pos;
> +
> + wl_list_insert_list(>emit_list, >listener_list);
> +
> + /* During a destructor notifier isolate every list item before
> +  * notifying.  This renders harmless the long standing misuse
> +  * of freeing listeners without removing them, but allows
> +  * callers that do choose to remove them to interoperate with
> +  * ones that don't. */
> + while (!wl_list_empty(>emit_list)) {
> + wl_list_init(>listener_list);
> + pos = signal->emit_list.next;
> + l = wl_container_of(pos, l, link);
> +
> + wl_list_remove(pos);
> + wl_list_insert(>listener_list, pos);

Just a quick fly-by comment; is there a reason to actually have a head
in the personal list for 'pos'? Wouldn't wl_list_init(pos) be enough?

Or would you want keep e.g. checks like
wl_list_empty(_destroy_listener->link) working? Not sure how that
would

Re: [PATCH wayland v2 4/4] tests: add code, public-code and private-code tests

2018-02-23 Thread Pekka Paalanen 
On Thu, 22 Feb 2018 13:03:49 -0600
Derek Foreman  wrote:

> On 2018-02-22 05:55 AM, Pekka Paalanen wrote:
> > On Thu, 22 Feb 2018 11:23:39 +
> > Emil Velikov  wrote:
> >   
> >> From: Emil Velikov 
> >>
> >> First one is deprecated in favour of the second option.
> >>
> >> The latter is newly introduced and annotates the generated symbols
> >> accordingly.
> >>
> >> v2: Don't introduce small-public-code.c - reuse small-code.c (Pekka)
> >>
> >> Cc: Pekka Paalanen 
> >> Signed-off-by: Emil Velikov 
> >> ---
> >>   Makefile.am |  3 +-
> >>   tests/data/small-private-code.c | 71 
> >> +
> >>   tests/scanner-test.sh   |  4 +++
> >>   3 files changed, 77 insertions(+), 1 deletion(-)
> >>   create mode 100644 tests/data/small-private-code.c

> >> --- a/tests/scanner-test.sh
> >> +++ b/tests/scanner-test.sh
> >> @@ -48,4 +48,8 @@ generate_and_compare "-c code" "small.xml" 
> >> "small-code-core.c"
> >>   generate_and_compare "-c client-header" "small.xml" "small-client-core.h"
> >>   generate_and_compare "-c server-header" "small.xml" "small-server-core.h"
> >>   
> >> +# The existing "code" must produce result identical to "public-code"
> >> +generate_and_compare "code" "small.xml" "small-code.c"
> >> +generate_and_compare "public-code" "small.xml" "small-code.c"
> >> +generate_and_compare "private-code" "small.xml" "small-private-code.c"
> >>   exit $RETCODE  
> > 
> > Hi Emil,
> > 
> > this is exactly what I had in mind.
> > 
> > Reviewed-by: Pekka Paalanen 
> > 
> > I'll push this series tomorrow if nothing else is heard.
> >   
> 
> All looks good to me as well,
> 
> Reviewed-by: Derek Foreman 
> 
> for the series.

All pushed:
   76a4e42..b02c401  master -> master


Thanks,
pq


pgp0jdTJKzHrs.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel