On Thu, Feb 26, 2015 at 6:25 PM, Janne Grunau <[email protected]> wrote:
> On 2015-02-24 15:33:53 +0000, Vittorio Giovara wrote:
>> Enabled only in debug mode and only when run under Valgrind.
>>
>> Signed-off-by: Vittorio Giovara <[email protected]>
>> ---
>> This is a neat trick I found reading
>> https://blog.mozilla.org/nnethercote/2011/01/11/using-valgrind-to-get-stack-traces/
>>
>> It should help in identifying where an error occurred if it is logged.
>>
>> I was looking for such a thing for some time, and other solutions like
>> debugbreak https://github.com/scottt/debugbreak are simply too fragile.
>>
>> The function is a static import so there are no additional libraries or
>> dependencies. Additionally it only appears on valgrind runs so it doesn't
>> pollute normal log and it is possible to turn it off if desired.
>>
>> I am not sure if I should keep everything under a single CONFIG_ or if it's
>> better to have the header check serpatedly (like in the proposed patch).
>>
>> Vittorio
>>
>> configure | 10 ++++++++++
>> libavutil/log.c | 7 +++++++
>> 2 files changed, 17 insertions(+)
>>
>> diff --git a/configure b/configure
>> index c1d673c..4e8fd6e 100755
>> --- a/configure
>> +++ b/configure
>> @@ -315,6 +315,8 @@ Developer options (useful when working on Libav itself):
>> (group) and PROB the probability associated with
>> NAME (default 0.5).
>> --random-seed=VALUE seed value for --enable/disable-random
>> + --disable-backtrace-printf when debug is enabled, do not print a backtrace
>> + under Valgrind when an error is logged
>
> The name is a little strange, log{-error}?-backtrace would be better.
ok, I can change it
> I also would invert it to --enable-... so it's clear that it can be
> enabled without debug. Whether it should be enabled with debug is
> another question.
iirc when debug is off, we strip the names of the functions so it
would just print a stack of addresses, but I haven't tested that.
I'd rather not have this opt-in, since it's not something you see by
default anyway, and the overhead for a log operation can be
negligible. Actually since we might use an extra option as mentioned
below I'd rather remove the configure option completely.
>> NOTE: Object files are built at the place where configure is launched.
>> EOF
>> @@ -1408,6 +1411,7 @@ HEADERS_LIST="
>> sys_un_h
>> sys_videoio_h
>> unistd_h
>> + valgrind_valgrind_h
>> windows_h
>> winsock2_h
>> "
>> @@ -161,6 +165,9 @@ void av_log_default_callback(void *avcl, int level,
>> const char *fmt, va_list vl)
>> }
>> colored_fputs(av_clip(level >> 3, 0, 6), tint >> 8, line);
>> av_strlcpy(prev, line, sizeof line);
>> +
>> + if (CONFIG_BACKTRACE_PRINTF && level <= AV_LOG_ERROR)
>> + VALGRIND_PRINTF_BACKTRACE("");
>
> Not sure how much overhead this has when not running under valgrind, it
> might be nicer to have a flag/option for this. It's conceivable that it
> would be useful for AV_LOG_WARNING too
How would be a good way to handle this? I'm fine with adding a log
flag, but I think it should be on by default.
Regarding the level, the only way I see it is something similar to
av_log_level, a static global variable but not sure if it's the right
way.
Any further advice is welcome.
--
Vittorio
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
