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

Steve Lhomme robux4 at ycbcr.xyz
Thu Feb 21 12:48:17 CET 2019


On 21/02/2019 12:39, Rémi Denis-Courmont wrote:
> 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.
>
> 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.
>
> And in all likelihood, it takes no more code to script this within a 
> memory debugger than this patch.

Such a script doesn't exist and you're leaving it to every other 
individual to implement their own and update it when you change the 
internals (again, hoping they can reproduce the leak).

>
> Le 21 février 2019 13:03:12 GMT+02:00, Alexandre Janniaux 
> <ajanni at videolabs.io> a écrit :
>
>     Fundamentally, you're completely right, there are far better solutions.
>
>     But I don't think it fits every situation we encounter when developing
>     VLC, and working on ASan or even making similar tools works on platforms
>     like windows, whether it's memory debugging tools or full feature fast
>     debugger, might be a lot more work than a 30 lines patch.
>
>     There are also edge cases where it becomes very useful in current and
>     future works. The sandbox embodies this issue quite well: yes you can
>     attach gdb, have coredump, check memory. But debugging many processes
>     with many threads (and potentially many coredumps) is a lot harder with
>     gdb than just launching `gdb vlc`, especially for newcomers. Again we
>     could develop more sophisticated tools but it takes times, what do we
>     do until they are available, useful and easy to use?
>
>     I think having this tool could prove very very helpful in these cases
>     as a simple development sanity check. If it's not correct in the current
>     state, it might be a good idea to find a common ground on this.
>
>     It's printf debugging, but on a very specific kind of bug that happens
>     a lot in VLC on the spine of it's architecture. In some way you can
>     check memory issues with gdb too but it's a lot harder than compiling
>     and launching with ASan, which produces a printf-like output too. I see
>     this patch the same: you can hack the codebase to produce useful
>     information with ASan but most of the time it's harder than having a
>     dedicated tool to signal the issue and highlight which object is
>     responsible.
>
>
>     On 2019-02-21 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. 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 
>
>         ------------------------------------------------------------------------
>         vlc-devel mailing list To unsubscribe or modify your
>         subscription options:
>         https://mailman.videolan.org/listinfo/vlc-devel 
>
>     ------------------------------------------------------------------------
>     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



More information about the vlc-devel mailing list