[vlc-devel] [PATCH] libvlc: dump object types abd names when leaking
Rémi Denis-Courmont
remi at remlab.net
Thu Feb 21 12:51:37 CET 2019
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.
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.
Le 21 février 2019 13:48:17 GMT+02:00, Steve Lhomme <robux4 at ycbcr.xyz> a écrit :
>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
>
>_______________________________________________
>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é.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20190221/dac875fc/attachment.html>
More information about the vlc-devel
mailing list