<html><head></head><body>Yes. I leave it to the user of the script, or this patch, to maintain it, as opposed to whoever refactors the VLC object subsystem.<br><br>That seems fair to me. It's not like I'm erasing this patch out of the spacetime continuum and you won't be able to apply it locally.<br><br><div class="gmail_quote">Le 21 février 2019 13:48:17 GMT+02:00, Steve Lhomme <robux4@ycbcr.xyz> 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">On 21/02/2019 12:39, 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;"> 1) Of course, it's easy to use. But that is exactly why it is a very <br> bad precedent. All (other) assertions need a debugger anyway. Yet we <br> can't add printf debugging to all assertions just because it's easier <br> than attaching a debugger. We also cannot add all the relevant infos <br> even in this assertion.<br><br> 2) This pokes into the internals and ossifies the implementation. And <br> the developer most likely to have to fight with this upon future <br> refactoring is me or my successor. You're optimising for your <br> convenience at the expense of somebody else here.<br><br> And in all likelihood, it takes no more code to script this within a <br> memory debugger than this patch.<br></blockquote><br>Such a script doesn't exist and you're leaving it to every other <br>individual to implement their own and update it when you change the <br>internals (again, hoping they can reproduce the leak).<br><br>><br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #729fcf; padding-left: 1ex;"> Le 21 février 2019 13:03:12 GMT+02:00, Alexandre Janniaux <br> <ajanni@videolabs.io> a écrit :<br><br> 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><br> Hi, My GDB skills are bad so I just walked the objects tree by<br> printing memory content on the abort. It would be much easier<br> with VLC-specific GDB macros, but the point is that you can<br> debug that assertion just as any. It also works after the fact<br> for pseudorandom failures, if you enable core dumps. Much<br> better than this patch since you have access to all memory,<br> not just the objects tree. Adding special snow flake debug<br> code for assertion is a bad idea that sets a very bad<br> precedent - as opposed to improving integration with proper<br> debugging tools. Le 21 février 2019 12:26:47 GMT+02:00, Thomas<br> Guillem <thomas@gllm.fr> a écrit :<br><br> On Thu, Feb 21, 2019, at 11:18, Rémi Denis-Courmont wrote:<br><br> Hi, Well I did debug that assertion failure using<br> (ASan +) gdb just fine <br><br> last time. The code has not changed. So this kludge is<br> definitely not necessary for its alleged purpose. Even if<br> it works with gdb (please tell us how), evelopers don't<br> always run VLC with gdb. This cause an issue if the leak<br> is not reproducible...<br><br> Not only that but stderr is not universally the output<br> for debug nor <br><br> 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<br> see the problem at all.<br><br> so this patch does not even work properly <br><br> Works on Windows, Android, Linux, macOS when VLC is run<br> via command line. This is the case for most developers<br> usecase.<br><br> And in most cases you can see the leak by stopping all<br> inputs and <br><br> dumping the objects tree before quitting, anyway. What<br> about leaks not related to playback ? Interface, service<br> discovery, playlist that are only cleaned when vlc is closed.<br><br> Le 21 février 2019 11:24:57 GMT+02:00, Thomas Guillem <br><br> <thomas@gllm.fr> a écrit :<br><br> Comment: A lot of developers asked me to propose<br> this patch again. It was <br><br> previously<br><br> refused because developers could use proper<br> debugging tools. I <br><br> disagree with<br><br> that assumption. Even with Asan, when the leak<br> assert fail, it will <br><br> abort()<br><br> without prompting any leaks, so you'll have to<br> edit libvlc.c to <br><br> remove this<br><br> assert. If your leak was not reproducible, you'll<br> lose a precious <br><br> information.<br><br> And good luck finding the leak with GDB, you<br> really need to know the <br><br> internal<br><br> of vlc_object_t to iterate through the different<br> list to get the <br><br> culprit.<br><br> Commit log: In case of a vlc_object_t leak, and if<br> asserts are enabled, the <br><br> error output<br><br> will be like the following (when leaking<br> intentionally decoder_t <br><br> objects):<br><br> === vlc_object LEAKS detected === \ playlist | \<br> input | | \ decoder avcodec | | | decoder faad | |<br> | decoder stl src/libvlc.c | 30 <br><br> ++++++++++++++++++++++++++++++<br><br> 1 file changed, 30 insertions(+) diff --git<br> a/src/libvlc.c b/src/libvlc.c index<br> 8e8d2ca8b2..d9ff96de84 100644 --- a/src/libvlc.c<br> +++ b/src/libvlc.c @@ -436,6 +436,24 @@ void<br> libvlc_InternalCleanup( libvlc_int_t <br><br> *p_libvlc )<br><br> #endif } +#ifndef NDEBUG +static void<br> DumpObjectLeaks(vlc_object_internals_t *priv,<br> unsigned <br><br> level)<br><br> +{ + bool first = true; + vlc_list_foreach(priv,<br> &priv->children, siblings) + { + vlc_object_t *obj<br> = vlc_externals(priv); + for (unsigned i = 0; i <<br> level; i++) + fprintf(stderr, " %s ", first && i<br> == level -1 ? "\\" <br><br> : "|");<br><br> + char *name = vlc_object_get_name(obj); +<br> fprintf(stderr, "%s %s\n",<br> vlc_object_typename(obj), name ? <br><br> name : "");<br><br> + free(name); + DumpObjectLeaks(priv, level + 1);<br> + first = false; + } +} +#endif + /** * Destroy<br> everything. * This function requests the running<br> threads to finish, waits for <br><br> their<br><br> @@ -449,6 +467,18 @@ void libvlc_InternalDestroy(<br> libvlc_int_t <br><br> *p_libvlc )<br><br> vlc_ExitDestroy( &priv->exit ); +#ifndef NDEBUG +<br> { + vlc_object_internals_t *internal =<br> vlc_internals(p_libvlc); + if<br> (atomic_load(&internal->refs) != 1) + { +<br> vlc_mutex_lock(&internal->tree_lock); +<br> fprintf(stderr, "=== vlc_object LEAKS detected<br> ===\n"); + DumpObjectLeaks(internal, 1); +<br> vlc_mutex_unlock(&internal->tree_lock); + } + }<br> +#endif assert(<br> atomic_load(&(vlc_internals(p_libvlc)->refs)) == 1<br> ); vlc_object_release( p_libvlc ); } <br><br> -- Envoyé de mon appareil Android avec Courriel K-9<br> Mail. Veuillez <br><br> excuser ma brièveté.<hr> vlc-devel mailing list To unsubscribe or modify your<br> subscription options:<br> <a href="https://mailman.videolan.org/listinfo/vlc-devel">https://mailman.videolan.org/listinfo/vlc-devel</a> <hr> vlc-devel mailing list To unsubscribe or modify your<br> subscription options:<br> <a href="https://mailman.videolan.org/listinfo/vlc-devel">https://mailman.videolan.org/listinfo/vlc-devel</a> <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><br><br> -- <br> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez <br> excuser ma brièveté.<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>