<html><head></head><body>1) Of course, it's easy to use. But that is exactly why it is a very bad precedent. All (other) assertions need a debugger anyway. Yet we can't add printf debugging to all assertions just because it's easier than attaching a debugger. We also cannot add all the relevant infos even in this assertion.<br><br>2) This pokes into the internals and ossifies the implementation. And the developer most likely to have to fight with this upon future refactoring is me or my successor. You're optimising for your convenience at the expense of somebody else here.<br><br>And in all likelihood, it takes no more code to script this within a memory debugger than this patch.<br><br><div class="gmail_quote">Le 21 février 2019 13:03:12 GMT+02:00, Alexandre Janniaux <ajanni@videolabs.io> a écrit :<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<pre class="k9mail">Fundamentally, you're completely right, there are far better solutions.<br><br>But I don't think it fits every situation we encounter when developing<br>VLC, and working on ASan or even making similar tools works on platforms<br>like windows, whether it's memory debugging tools or full feature fast<br>debugger, might be a lot more work than a 30 lines patch.<br><br>There are also edge cases where it becomes very useful in current and<br>future works. The sandbox embodies this issue quite well: yes you can<br>attach gdb, have coredump, check memory. But debugging many processes<br>with many threads (and potentially many coredumps) is a lot harder with<br>gdb than just launching `gdb vlc`, especially for newcomers. Again we<br>could develop more sophisticated tools but it takes times, what do we<br>do until they are available, useful and easy to use?<br><br>I think having this tool could prove very very helpful in these cases<br>as a simple development sanity check. If it's not correct in the current<br>state, it might be a good idea to find a common ground on this.<br><br>It's printf debugging, but on a very specific kind of bug that happens<br>a lot in VLC on the spine of it's architecture. In some way you can<br>check memory issues with gdb too but it's a lot harder than compiling<br>and launching with ASan, which produces a printf-like output too. I see<br>this patch the same: you can hack the codebase to produce useful<br>information with ASan but most of the time it's harder than having a<br>dedicated tool to signal the issue and highlight which object is<br>responsible.<br><br><br>On 2019-02-21 11:37, Rémi Denis-Courmont wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #729fcf; padding-left: 1ex;">Hi,<br><br>My GDB skills are bad so I just walked the objects tree by printing<br>memory content on the abort. It would be much easier with VLC-specific<br>GDB macros, but the point is that you can debug that assertion just as<br>any.<br><br>It also works after the fact for pseudorandom failures, if you enable<br>core dumps. Much better than this patch since you have access to all<br>memory, not just the objects tree.<br><br>Adding special snow flake debug code for assertion is a bad idea that<br>sets a very bad precedent - as opposed to improving integration with<br>proper debugging tools.<br><br>Le 21 février 2019 12:26:47 GMT+02:00, Thomas Guillem <thomas@gllm.fr> <br>a écrit :<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #ad7fa8; padding-left: 1ex;">On Thu, Feb 21, 2019, at 11:18, Rémi Denis-Courmont wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;">Hi,<br><br>Well I did debug that assertion failure using (ASan +) gdb just fine<br></blockquote>last time. The code has not changed. So this kludge is definitely not<br>necessary for its alleged purpose.<br><br>Even if it works with gdb (please tell us how), evelopers don't always<br>run VLC with gdb. This cause an issue if the leak is not<br>reproducible...<br><br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;">Not only that but stderr is not universally the output for debug nor<br></blockquote>is it universally where assertion failures are dumped to,<br>So if there is no stderr, you don't see the error. I don't see the<br>problem at all.<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;">so this patch does not even work properly<br></blockquote>Works on Windows, Android, Linux, macOS when VLC is run via command<br>line. This is the case for most developers usecase.<br><br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;">And in most cases you can see the leak by stopping all inputs and<br></blockquote>dumping the objects tree before quitting, anyway.<br><br>What about leaks not related to playback ? Interface, service<br>discovery, playlist that are only cleaned when vlc is closed.<br><br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;">Le 21 février 2019 11:24:57 GMT+02:00, Thomas Guillem<br></blockquote><thomas@gllm.fr> a écrit :<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;"><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #fcaf3e; padding-left: 1ex;">Comment:<br>A lot of developers asked me to propose this patch again. It was<br></blockquote></blockquote>previously<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;"><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #fcaf3e; padding-left: 1ex;">refused because developers could use proper debugging tools. I<br></blockquote></blockquote>disagree with<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;"><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #fcaf3e; padding-left: 1ex;">that assumption. Even with Asan, when the leak assert fail, it will<br></blockquote></blockquote>abort()<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;"><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #fcaf3e; padding-left: 1ex;">without prompting any leaks, so you'll have to edit libvlc.c to<br></blockquote></blockquote>remove this<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;"><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #fcaf3e; padding-left: 1ex;">assert. If your leak was not reproducible, you'll lose a precious<br></blockquote></blockquote>information.<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;"><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #fcaf3e; padding-left: 1ex;">And good luck finding the leak with GDB, you really need to know the<br></blockquote></blockquote>internal<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;"><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #fcaf3e; padding-left: 1ex;">of vlc_object_t to iterate through the different list to get the<br></blockquote></blockquote>culprit.<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;"><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #fcaf3e; padding-left: 1ex;">Commit log:<br><br>In case of a vlc_object_t leak, and if asserts are enabled, the<br></blockquote></blockquote>error output<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;"><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #fcaf3e; padding-left: 1ex;">will be like the following (when leaking intentionally decoder_t<br></blockquote></blockquote>objects):<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;"><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #fcaf3e; padding-left: 1ex;">=== vlc_object LEAKS detected ===<br> \ playlist<br> | \ input<br> | | \ decoder avcodec<br> | | | decoder faad<br> | | | decoder stl src/libvlc.c | 30<br></blockquote></blockquote>++++++++++++++++++++++++++++++<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;"><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #fcaf3e; padding-left: 1ex;"> 1 file changed, 30 insertions(+)<br><br>diff --git a/src/libvlc.c b/src/libvlc.c<br>index 8e8d2ca8b2..d9ff96de84 100644<br>--- a/src/libvlc.c<br>+++ b/src/libvlc.c<br>@@ -436,6 +436,24 @@ void libvlc_InternalCleanup( libvlc_int_t<br></blockquote></blockquote>*p_libvlc )<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;"><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #fcaf3e; padding-left: 1ex;"> #endif<br> }<br><br>+#ifndef NDEBUG<br>+static void DumpObjectLeaks(vlc_object_internals_t *priv, unsigned<br></blockquote></blockquote>level)<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;"><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #fcaf3e; padding-left: 1ex;">+{<br>+ bool first = true;<br>+ vlc_list_foreach(priv, &priv->children, siblings)<br>+ {<br>+ vlc_object_t *obj = vlc_externals(priv);<br>+ for (unsigned i = 0; i < level; i++)<br>+ fprintf(stderr, " %s ", first && i == level -1 ? "\\"<br></blockquote></blockquote>: "|");<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;"><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #fcaf3e; padding-left: 1ex;">+ char *name = vlc_object_get_name(obj);<br>+ fprintf(stderr, "%s %s\n", vlc_object_typename(obj), name ?<br></blockquote></blockquote>name : "");<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;"><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #fcaf3e; padding-left: 1ex;">+ free(name);<br>+ DumpObjectLeaks(priv, level + 1);<br>+ first = false;<br>+ }<br>+}<br>+#endif<br>+<br> /**<br> * Destroy everything.<br> * This function requests the running threads to finish, waits for<br></blockquote></blockquote>their<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;"><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #fcaf3e; padding-left: 1ex;">@@ -449,6 +467,18 @@ void libvlc_InternalDestroy( libvlc_int_t<br></blockquote></blockquote>*p_libvlc )<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;"><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #fcaf3e; padding-left: 1ex;"> vlc_ExitDestroy( &priv->exit );<br><br>+#ifndef NDEBUG<br>+ {<br>+ vlc_object_internals_t *internal = vlc_internals(p_libvlc);<br>+ if (atomic_load(&internal->refs) != 1)<br>+ {<br>+ vlc_mutex_lock(&internal->tree_lock);<br>+ fprintf(stderr, "=== vlc_object LEAKS detected ===\n");<br>+ DumpObjectLeaks(internal, 1);<br>+ vlc_mutex_unlock(&internal->tree_lock);<br>+ }<br>+ }<br>+#endif<br> assert( atomic_load(&(vlc_internals(p_libvlc)->refs)) == 1 );<br> vlc_object_release( p_libvlc );<br> }<br></blockquote>--<br>Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez<br></blockquote>excuser ma brièveté.<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;"><hr>vlc-devel mailing list<br>To unsubscribe or modify your subscription options:<br><a href="https://mailman.videolan.org/listinfo/vlc-devel">https://mailman.videolan.org/listinfo/vlc-devel</a><br></blockquote></blockquote><hr>vlc-devel mailing list<br>To unsubscribe or modify your subscription options:<br><a href="https://mailman.videolan.org/listinfo/vlc-devel">https://mailman.videolan.org/listinfo/vlc-devel</a><br></blockquote><hr>vlc-devel mailing list<br>To unsubscribe or modify your subscription options:<br><a href="https://mailman.videolan.org/listinfo/vlc-devel">https://mailman.videolan.org/listinfo/vlc-devel</a></pre></blockquote></div><br>-- <br>Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.</body></html>