<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On 4 December 2014 at 12:12, Pekka Paalanen <span dir="ltr"><<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Wed, 3 Dec 2014 11:44:47 +0100<br>
<div><div class="h5">Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>> wrote:<br>
<br>
> On 1 December 2014 at 11:42, Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com">ppaalanen@gmail.com</a>> wrote:<br>
><br>
> > On Fri, 21 Nov 2014 11:18:33 +0100<br>
> > Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>> wrote:<br>
> ><br>
> > > We didn't free the struct client that we got from client_connect()<br>
> > ><br>
> > > Signed-off-by: Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>><br>
> > > ---<br>
> > > tests/display-test.c | 23 +++++++++++++----------<br>
> > > tests/test-compositor.c | 1 +<br>
> > > 2 files changed, 14 insertions(+), 10 deletions(-)<br>
> ><br>
> > Hi,<br>
> ><br>
> > how did you find these leaks?<br>
> ><br>
><br>
> By valgrind --trace-children=yes<br>
><br>
><br>
> ><br>
> > More importantly, why doesn't our leak checker find these?<br>
> > That is, the code in test-runner.c, run_test().<br>
> ><br>
><br>
> Because it is in another fork. It looks like this:<br>
><br>
> run_test()<br>
> int the alloc_count and fds_count<br>
> run the TEST function<br>
> create_client --> fork<br>
> [in fork] alloc, dealloc, whatever<br>
> wait for client<br>
> check alloc_count and fds_count<br>
><br>
> The problem is that the forked client has the alloc_count and fds_count<br>
> in its own virtual memory, so the leak checker finds only leaks in<br>
> TEST function.<br>
><br>
> I have some patches that extend these leak checks<br>
> into the client too, but I ran into problem with filedescriptors there.<br>
> The problem is that when forking the client, we pass WAYLAND_SOCKET fd<br>
> to wl_display_connect(). wl_display_connect() just uses this fd (no dup),<br>
> so wl_display_disconnect() closes this fd.<br>
> So the result of leak checker is that the client closed<br>
> one more fd that it opened. Hardcoding -1 to client's fds number<br>
> is not a solution too, because client does not have to call<br>
> wl_display_connect()<br>
><br>
> So summing it up: using leak checker in clients is possible, but only for<br>
> memory<br>
> leaks. For fd leaks we'd have to use some more sophisticated solution.<br>
<br>
</div></div>Erf, more complicated than I imagined.<br>
<br>
I think the fd leak thing should be easy to fix though: I'm not sure<br>
the strict equality check for open fds makes too much sense, it would<br>
be also good if there are less fds open in the end than in the<br>
beginning. Right? So fail only if in the end there are more fds open.<br>
<br>
That seems more useful to me than not doing fd leak checks at all.<br>
Maybe we can even do it differently in TEST vs. clients?<br></blockquote><div><br></div><div>In my branch I chose even different solution. Instead of doing it differently in TEST and clients,<br></div><div>I did it differently in normal TESTs and sanity TESTs. Sanity tests are currently the<br></div><div>only one that are not using wl_display_connect, so I added a macro<br></div><div>TEST_CLIENT_NOT_USING_WL_DISPLAY_CONNECT which just fixed the leak checks<br>for the sanity tests (or any other test that do not use wl_display_connect() -- but there is none atm).<br></div><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class=""><br>
> > For one, I think those are not thread-safe. Calling exit() in a test<br>
> > would bypass it too, but I'm not sure that happens.<br>
> ><br>
><br>
> Huh? What is not thread-safe? you mean leak checks when running only one<br>
> test (without fork?)<br>
<br>
</span>I mean e.g. this function<br>
<br>
__attribute__ ((visibility("default"))) void *<br>
malloc(size_t size)<br>
{<br>
num_alloc++;<br>
return sys_malloc(size);<br>
}<br>
<br>
is not thread-safe, because it read-modify-writes a global variable<br>
without any explicit synchronization guarantees. It's not using a mutex<br>
nor a guaranteed-atomic variable or operation. We just get lucky on<br>
most arches, I assume. Or at least on x86?<br></blockquote><div><br></div>Dunno, haven't tried anywhere else that on x86_64...<br>Anyway, in my development branch I added support for atomic increasing/decreasing<br></div><div class="gmail_quote">of the counter.<br> <br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class=""><br>
> Yes, it looks like when calling exit in the test, the leak check is<br>
> bypassed.<br>
> Quick grep though the tests shows that there are no calls to exit in TEST,<br>
> just in<br>
> the forked clients (which is not a problem _now_ with no leak checks in<br>
> clients)<br>
<br>
</span>Yeah.<br>
<span class=""><br>
> > The Valgrind output nowadays is quite messy. Even when I run a single<br>
> > test from one test binary which should not fork at all, I think I see<br>
> > two forks in Valgrind output: one is likely the debugger check, maybe<br>
> > the other is actually a thread for the test compositor or something?<br>
> ><br>
><br>
> Yes, the first fork is the debugger check and the other is the client<br>
> for test compositor. In the tests without the test compositor you should<br>
> see only<br>
> one fork (the debugger check)<br>
<br>
</span>I suppose we need to live with that.<br>
<span class=""><br>
> > I think Valgrind is saying that there are many more leaks to plug.<br>
> ><br>
> ><br>
> Yes<br></span></blockquote><div><br></div><div>Still keep the patches on github, because the leak checker made the test to fail<br>and I'd like to check if it is correct or I have a bug in the code somewhere.<br></div><div>If you're (or anybody is) interested, it is here:<br><br><a href="https://github.com/mchalupa/wayland/tree/tc-client-leaks">https://github.com/mchalupa/wayland/tree/tc-client-leaks</a><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="">
<br>
<br>
</span>Thanks,<br>
pq<br>
</blockquote></div><br></div><div class="gmail_extra">Regards,<br>Marek<br></div></div>