[vlc-devel] [PATCH] libvlc: dump object types abd names when leaking

[Steve Lhomme]

On 21/02/2019 11:37, Rémi Denis-Courmont wrote:
> Hi,
>
> My GDB skills are bad so I just walked the objects tree by printing 
> memory content on the abort. It would be much easier with VLC-specific 
> GDB macros, but the point is that you can debug that assertion just as 
> any.
>
> It also works after the fact for pseudorandom failures, if you enable 
> core dumps. Much better than this patch since you have access to all 
> memory, not just the objects tree.
>
> Adding special snow flake debug code for assertion is a bad idea that 
> sets a very bad precedent - as opposed to improving integration with 
> proper debugging tools.

What's snow flake about adding debug tooling ?

>
> Le 21 février 2019 12:26:47 GMT+02:00, Thomas Guillem <thomas at gllm.fr> 
> a écrit :
>
>
>     On Thu, Feb 21, 2019, at 11:18, Rémi Denis-Courmont wrote:
>>     Hi,
>>
>>     Well I did debug that assertion failure using (ASan +) gdb just
>>     fine last time. The code has not changed. So this kludge is
>>     definitely not necessary for its alleged purpose.
>
>     Even if it works with gdb (please tell us how), evelopers don't
>     always run VLC with gdb. This cause an issue if the leak is not
>     reproducible...
>
>>
>>     Not only that but stderr is not universally the output for debug
>>     nor is it universally where assertion failures are dumped to,
>     So if there is no stderr, you don't see the error. I don't see the
>     problem at all.
>>     so this patch does not even work properly
>     Works on Windows, Android, Linux, macOS when VLC is run via
>     command line. This is the case for most developers usecase.
>
>>
>>     And in most cases you can see the leak by stopping all inputs and
>>     dumping the objects tree before quitting, anyway.
>
>     What about leaks not related to playback ? Interface, service
>     discovery, playlist that are only cleaned when vlc is closed.
>
>>
>>     Le 21 février 2019 11:24:57 GMT+02:00, Thomas Guillem
>>     <thomas at gllm.fr> a écrit :
>>
>>         Comment:
>>         A lot of developers asked me to propose this patch again. It
>>         was previously
>>         refused because developers could use proper debugging tools.
>>         I disagree with
>>         that assumption. Even with Asan, when the leak assert fail,
>>         it will abort()
>>         without prompting any leaks, so you'll have to edit libvlc.c
>>         to remove this
>>         assert. If your leak was not reproducible, you'll lose a
>>         precious information.
>>         And good luck finding the leak with GDB, you really need to
>>         know the internal
>>         of vlc_object_t to iterate through the different list to get
>>         the culprit.
>>         Commit log:
>>         In case of a vlc_object_t leak, and if asserts are enabled,
>>         the error output
>>         will be like the following (when leaking intentionally
>>         decoder_t objects):
>>         === vlc_object LEAKS detected ===
>>         \ playlist
>>         | \ input
>>         | | \ decoder avcodec
>>         | | | decoder faad
>>         | | | decoder stl
>>         ------------------------------------------------------------------------
>>         src/libvlc.c | 30 ++++++++++++++++++++++++++++++
>>         1 file changed, 30 insertions(+)
>>         diff --git a/src/libvlc.c b/src/libvlc.c
>>         index 8e8d2ca8b2..d9ff96de84 100644
>>         --- a/src/libvlc.c
>>         +++ b/src/libvlc.c
>>         @@ -436,6 +436,24 @@ void libvlc_InternalCleanup(
>>         libvlc_int_t *p_libvlc )
>>         #endif
>>         }
>>         +#ifndef NDEBUG
>>         +static void DumpObjectLeaks(vlc_object_internals_t *priv,
>>         unsigned level)
>>         +{
>>         + bool first = true;
>>         + vlc_list_foreach(priv, &priv->children, siblings)
>>         + {
>>         + vlc_object_t *obj = vlc_externals(priv);
>>         + for (unsigned i = 0; i < level; i++)
>>         + fprintf(stderr, " %s ", first && i == level -1 ? "\\" : "|");
>>         + char *name = vlc_object_get_name(obj);
>>         + fprintf(stderr, "%s %s\n", vlc_object_typename(obj), name ?
>>         name : "");
>>         + free(name);
>>         + DumpObjectLeaks(priv, level + 1);
>>         + first = false;
>>         + }
>>         +}
>>         +#endif
>>         +
>>         /**
>>         * Destroy everything.
>>         * This function requests the running threads to finish, waits
>>         for their
>>         @@ -449,6 +467,18 @@ void libvlc_InternalDestroy(
>>         libvlc_int_t *p_libvlc )
>>         vlc_ExitDestroy( &priv->exit );
>>         +#ifndef NDEBUG
>>         + {
>>         + vlc_object_internals_t *internal = vlc_internals(p_libvlc);
>>         + if (atomic_load(&internal->refs) != 1)
>>         + {
>>         + vlc_mutex_lock(&internal->tree_lock);
>>         + fprintf(stderr, "=== vlc_object LEAKS detected ===\n");
>>         + DumpObjectLeaks(internal, 1);
>>         + vlc_mutex_unlock(&internal->tree_lock);
>>         + }
>>         + }
>>         +#endif
>>         assert( atomic_load(&(vlc_internals(p_libvlc)->refs)) == 1 );
>>         vlc_object_release( p_libvlc );
>>         }
>>
>>
>>     -- 
>>     Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez
>>     excuser ma brièveté.
>>     _______________________________________________
>>     vlc-devel mailing list
>>     To unsubscribe or modify your subscription options:
>>     https://mailman.videolan.org/listinfo/vlc-devel
>
>
> -- 
> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez 
> excuser ma brièveté.
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel