[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