Re: [FFmpeg-devel] [PATCH 1/9] avutil: add FF_RETURN_ON_OVERFLOW

[Rodger Combs]

> On Jan 7, 2017, at 02:36, Paul B Mahol  wrote:
> 
> On 1/7/17, Michael Niedermayer  > wrote:
>> On Fri, Jan 06, 2017 at 09:11:10PM -0300, James Almer wrote:
>>> On 1/6/2017 4:46 PM, Andreas Cadhalpun wrote:
 Suggested-by: Rodger Combs 
 Signed-off-by: Andreas Cadhalpun 
 ---
 
 Changed the name as suggested by wm4 and the return value as suggested
 by Muhammad Faiz.
 There are also two new overflow checks at the end of the series.
>>> 
>>> This is a good chance to introduce the gcc overflow check builtins.
>>> See https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html,
>>> they
>>> use hardware instructions when possible, like x86's Jump on Overflow.
>>> 
>>> The idea would be to use __builtin_mul_overflow_p(). For example
>>> (untested):
>>> 
>>> #if AV_GCC_VERSION_AT_LEAST(5,1)
>>> #define av_builtin_mul_overflow_p(a, b) __builtin_mul_overflow_p(a, b,
>>> (int) 0)
>>> #else
>>> #define av_builtin_mul_overflow_p(a, b) ((a) > INT_MAX / (b)))
>>> #endif
>>> 
>>> It can also be used all across the codebase, not just for these checks.
>>> 
>> 
 
 ---
 libavutil/common.h | 2 ++
 1 file changed, 2 insertions(+)
 
 diff --git a/libavutil/common.h b/libavutil/common.h
 index 8142b31fdb..6d795a353a 100644
 --- a/libavutil/common.h
 +++ b/libavutil/common.h
 @@ -99,6 +99,8 @@
 #define FFSWAP(type,a,b) do{type SWAP_tmp= b; b= a; a=
 SWAP_tmp;}while(0)
 #define FF_ARRAY_ELEMS(a) (sizeof(a) / sizeof((a)[0]))
 
 +#define FF_RETURN_ON_OVERFLOW(ctx, x) if (x) {av_log(ctx, AV_LOG_ERROR,
 "Overflow check failed: " #x"\n"); return AVERROR(ERANGE);}
>>> 
>>> Printing an error unconditionally seems like log bloat. We do all kinds
>>> of
>>> sanity checks on demuxers and simply return INVALIDDATA without printing
>>> anything if they fail.
>>> Maybe we should do the same here and let the demuxer choose to print an
>>> error or not.
>> 
>> error messages help debuging and thus maintaining code.
>> 
> 
> Not really. Expecially in this case.

Eh, I generally prefer a print-and-return over just returning an error code in 
cases where the source is ambiguous. There are a lot of cases currently where 
in e.g. ffmpeg CLI, the code just gets bubbled up and printed, potentially with 
the name of the codec it came from, but no further information about the 
source, so it can be difficult to track down exactly where the failure 
occurred. Printing a message can be a fair bit more clear.
This seems like something it would be worth talking about and deciding on in 
general, though: as a rule, should we try to print errors about specific 
failures like this?

> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org 
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel 
> 
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/9] avutil: add FF_RETURN_ON_OVERFLOW

[Paul B Mahol]

On 1/7/17, Michael Niedermayer  wrote:
> On Fri, Jan 06, 2017 at 09:11:10PM -0300, James Almer wrote:
>> On 1/6/2017 4:46 PM, Andreas Cadhalpun wrote:
>> > Suggested-by: Rodger Combs 
>> > Signed-off-by: Andreas Cadhalpun 
>> > ---
>> >
>> > Changed the name as suggested by wm4 and the return value as suggested
>> > by Muhammad Faiz.
>> > There are also two new overflow checks at the end of the series.
>>
>> This is a good chance to introduce the gcc overflow check builtins.
>> See https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html,
>> they
>> use hardware instructions when possible, like x86's Jump on Overflow.
>>
>> The idea would be to use __builtin_mul_overflow_p(). For example
>> (untested):
>>
>> #if AV_GCC_VERSION_AT_LEAST(5,1)
>> #define av_builtin_mul_overflow_p(a, b) __builtin_mul_overflow_p(a, b,
>> (int) 0)
>> #else
>> #define av_builtin_mul_overflow_p(a, b) ((a) > INT_MAX / (b)))
>> #endif
>>
>> It can also be used all across the codebase, not just for these checks.
>>
>
>> >
>> > ---
>> >  libavutil/common.h | 2 ++
>> >  1 file changed, 2 insertions(+)
>> >
>> > diff --git a/libavutil/common.h b/libavutil/common.h
>> > index 8142b31fdb..6d795a353a 100644
>> > --- a/libavutil/common.h
>> > +++ b/libavutil/common.h
>> > @@ -99,6 +99,8 @@
>> >  #define FFSWAP(type,a,b) do{type SWAP_tmp= b; b= a; a=
>> > SWAP_tmp;}while(0)
>> >  #define FF_ARRAY_ELEMS(a) (sizeof(a) / sizeof((a)[0]))
>> >
>> > +#define FF_RETURN_ON_OVERFLOW(ctx, x) if (x) {av_log(ctx, AV_LOG_ERROR,
>> > "Overflow check failed: " #x"\n"); return AVERROR(ERANGE);}
>>
>> Printing an error unconditionally seems like log bloat. We do all kinds
>> of
>> sanity checks on demuxers and simply return INVALIDDATA without printing
>> anything if they fail.
>> Maybe we should do the same here and let the demuxer choose to print an
>> error or not.
>
> error messages help debuging and thus maintaining code.
>

Not really. Expecially in this case.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/9] avutil: add FF_RETURN_ON_OVERFLOW

[Michael Niedermayer]

On Fri, Jan 06, 2017 at 09:11:10PM -0300, James Almer wrote:
> On 1/6/2017 4:46 PM, Andreas Cadhalpun wrote:
> > Suggested-by: Rodger Combs 
> > Signed-off-by: Andreas Cadhalpun 
> > ---
> > 
> > Changed the name as suggested by wm4 and the return value as suggested
> > by Muhammad Faiz.
> > There are also two new overflow checks at the end of the series.
> 
> This is a good chance to introduce the gcc overflow check builtins.
> See https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html, they
> use hardware instructions when possible, like x86's Jump on Overflow.
> 
> The idea would be to use __builtin_mul_overflow_p(). For example (untested):
> 
> #if AV_GCC_VERSION_AT_LEAST(5,1)
> #define av_builtin_mul_overflow_p(a, b) __builtin_mul_overflow_p(a, b, (int) 
> 0)
> #else
> #define av_builtin_mul_overflow_p(a, b) ((a) > INT_MAX / (b)))
> #endif
> 
> It can also be used all across the codebase, not just for these checks.
> 

> > 
> > ---
> >  libavutil/common.h | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/libavutil/common.h b/libavutil/common.h
> > index 8142b31fdb..6d795a353a 100644
> > --- a/libavutil/common.h
> > +++ b/libavutil/common.h
> > @@ -99,6 +99,8 @@
> >  #define FFSWAP(type,a,b) do{type SWAP_tmp= b; b= a; a= SWAP_tmp;}while(0)
> >  #define FF_ARRAY_ELEMS(a) (sizeof(a) / sizeof((a)[0]))
> >  
> > +#define FF_RETURN_ON_OVERFLOW(ctx, x) if (x) {av_log(ctx, AV_LOG_ERROR, 
> > "Overflow check failed: " #x"\n"); return AVERROR(ERANGE);}
> 
> Printing an error unconditionally seems like log bloat. We do all kinds of
> sanity checks on demuxers and simply return INVALIDDATA without printing
> anything if they fail.
> Maybe we should do the same here and let the demuxer choose to print an
> error or not.

error messages help debuging and thus maintaining code.



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

If you drop bombs on a foreign country and kill a hundred thousand
innocent people, expect your government to call the consequence
"unprovoked inhuman terrorist attacks" and use it to justify dropping
more bombs and killing more people. The technology changed, the idea is old.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/9] avutil: add FF_RETURN_ON_OVERFLOW

[James Almer]

On 1/6/2017 4:46 PM, Andreas Cadhalpun wrote:
> Suggested-by: Rodger Combs 
> Signed-off-by: Andreas Cadhalpun 
> ---
> 
> Changed the name as suggested by wm4 and the return value as suggested
> by Muhammad Faiz.
> There are also two new overflow checks at the end of the series.

This is a good chance to introduce the gcc overflow check builtins.
See https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html, they
use hardware instructions when possible, like x86's Jump on Overflow.

The idea would be to use __builtin_mul_overflow_p(). For example (untested):

#if AV_GCC_VERSION_AT_LEAST(5,1)
#define av_builtin_mul_overflow_p(a, b) __builtin_mul_overflow_p(a, b, (int) 0)
#else
#define av_builtin_mul_overflow_p(a, b) ((a) > INT_MAX / (b)))
#endif

It can also be used all across the codebase, not just for these checks.

> 
> ---
>  libavutil/common.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libavutil/common.h b/libavutil/common.h
> index 8142b31fdb..6d795a353a 100644
> --- a/libavutil/common.h
> +++ b/libavutil/common.h
> @@ -99,6 +99,8 @@
>  #define FFSWAP(type,a,b) do{type SWAP_tmp= b; b= a; a= SWAP_tmp;}while(0)
>  #define FF_ARRAY_ELEMS(a) (sizeof(a) / sizeof((a)[0]))
>  
> +#define FF_RETURN_ON_OVERFLOW(ctx, x) if (x) {av_log(ctx, AV_LOG_ERROR, 
> "Overflow check failed: " #x"\n"); return AVERROR(ERANGE);}

Printing an error unconditionally seems like log bloat. We do all kinds of
sanity checks on demuxers and simply return INVALIDDATA without printing
anything if they fail.
Maybe we should do the same here and let the demuxer choose to print an
error or not.

> +
>  /* misc math functions */
>  
>  #ifdef HAVE_AV_CONFIG_H
> 

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 1/9] avutil: add FF_RETURN_ON_OVERFLOW

[Andreas Cadhalpun]

Suggested-by: Rodger Combs 
Signed-off-by: Andreas Cadhalpun 
---

Changed the name as suggested by wm4 and the return value as suggested
by Muhammad Faiz.
There are also two new overflow checks at the end of the series.

---
 libavutil/common.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libavutil/common.h b/libavutil/common.h
index 8142b31fdb..6d795a353a 100644
--- a/libavutil/common.h
+++ b/libavutil/common.h
@@ -99,6 +99,8 @@
 #define FFSWAP(type,a,b) do{type SWAP_tmp= b; b= a; a= SWAP_tmp;}while(0)
 #define FF_ARRAY_ELEMS(a) (sizeof(a) / sizeof((a)[0]))
 
+#define FF_RETURN_ON_OVERFLOW(ctx, x) if (x) {av_log(ctx, AV_LOG_ERROR, 
"Overflow check failed: " #x"\n"); return AVERROR(ERANGE);}
+
 /* misc math functions */
 
 #ifdef HAVE_AV_CONFIG_H
-- 
2.11.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel