[FFmpeg-devel] [RFC] av_assert

[Michael Niedermayer]

Hi

On Thu, May 17, 2007 at 11:11:11PM +0200, Alex Beregszaszi wrote:
> Hi,
> 
> > > @@ -152,6 +153,14 @@
> > >  
> > >  #define av_abort()      do { av_log(NULL, AV_LOG_ERROR, "Abort at %s:%d\n", __FILE__, __LINE__); abort(); } while (0)
> > >  
> > > +
> > > +#define av_assert(x) do { \
> > > +     if (!(x)) { \
> > > +        av_log(NULL, AV_LOG_ERROR, "Assertion '" #x "' failed in %s at %s:%d\n", __PRETTY_FUNCTION__, __FILE__, __LINE__); \
> > > +	abort(); \
> > > +     } \
> > > +} while(0)
> > 
> > i think the context should not be NULL but rather a argument from
> > av_assert()
> 
> What about the same for av_abort()? Anyway, function name, file name and
> line size gives you mostly the context.

hmm, iam also fine with leaving it NULL if noone objects, i just thought
that passing the context would be better but maybe its not really worth
the extra object size ...


> 
> > 
> > id like to have 3 different classes of asserts
> > 
> > 1. av_slow_assert()
> >     This assert occurs in non speed critical code and can stay enabled even
> >     if we want as fast as possible libav*
> 
> > 2. av_fast_assert()
> >     This assert occurs in speed critical code and should be disabled in non
> >     debug builds
> 
> This type of assert should be used only when the failure of the
> condition only introduces visual bugs / audio glitches, but never
> crashes/undefined behaviour. Right?

the av_fast_assert()
should be used if doing the check will slow libav* down by a
signifcant amount (>0.1%) and failure of the assert() does not lead to
arbitrary code execution bugs, crashes like NULL pointer dereference
arent a problem, after all assert(0) is not very different from a 
crash ...


> 
> > 3. av_security_assert()
> >     This assert checks a security related assumtation and should always be
> >     enabled unless the user explicitly disables such asserts
> 
> What about av_security_check/condition.

iam fine with all of them check/condition/assert


> 
> Anyway, if even 1 and 3 can be disabled explicitly, there is no
> distinction between the two. 

well, the distinction is in when they are used
x= complicated_and_slow_func()
av_slow_assert(x < 256)
write_byte(x);

vs.

x= complicated_and_slow_func()
av_security_assert(x>=0 && x < sizeof(a))
a[x]= *b++;


> Or do you want different error messages
> printed?

no


[...]
>  /* dprintf macros */
>  #ifdef DEBUG
>  #    define dprintf(pctx, ...) av_log(pctx, AV_LOG_DEBUG, __VA_ARGS__)
> @@ -152,6 +153,22 @@
>  
>  #define av_abort()      do { av_log(NULL, AV_LOG_ERROR, "Abort at %s:%d\n", __FILE__, __LINE__); abort(); } while (0)
>  
> +
> +#define av_assert(c, x) do { \
> +     if (!(x)) { \
> +        av_log(c, AV_LOG_ERROR, "Assertion '" #x "' failed in %s at %s:%d\n", __PRETTY_FUNCTION__, __FILE__, __LINE__); \
> +        abort(); \
> +     } \
> +} while(0)
> +

> +#define av_slow_assert(c, x) av_assert(c, x)

this maybe could be under !CONFIG_SMALL || DEBUG


> +#ifndef DEBUG
> +#define av_fast_assert(c, x) av_assert(c, x)
> +#else
> +#define av_fast_assert(c, x)
> +#endif

isnt this wrong around?


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you really think that XML is the answer, then you definitly missunderstood
the question -- Attila Kinali
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070518/d7b2bb75/attachment.pgp>