Re: [FFmpeg-devel] [PATCH] avcodec/apedec: fix undefined left shifts of negative numbers
On Tue, Sep 29, 2015 at 11:22 AM, Ronald S. Bultje wrote: > Hi, > > On Tue, Sep 29, 2015 at 11:04 AM, Ganesh Ajjanagadde > wrote: > >> On Tue, Sep 29, 2015 at 9:24 AM, Hendrik Leppkes >> wrote: >> > On Sun, Sep 20, 2015 at 4:18 AM, Ganesh Ajjanagadde >> > wrote: >> >> This fixes -Wshift-negative-value reported with clang 3.7+, e.g >> >> >> http://fate.ffmpeg.org/log.cgi?time=20150919172459&log=compile&slot=x86_64-darwin-clang-polly-notiling-3.7 >> . >> >> Note that the patch crucially depends on int >= 32 bits, >> >> an assumption made in many places in the codebase. >> >> >> >> Signed-off-by: Ganesh Ajjanagadde >> >> --- >> >> libavcodec/apedec.c | 2 +- >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c >> >> index 5536e0f..7b34d26 100644 >> >> --- a/libavcodec/apedec.c >> >> +++ b/libavcodec/apedec.c >> >> @@ -1281,7 +1281,7 @@ static void do_apply_filter(APEContext *ctx, int >> version, APEFilter *f, >> >> /* Update the adaption coefficients */ >> >> absres = FFABS(res); >> >> if (absres) >> >> -*f->adaptcoeffs = ((res & (-1<<31)) ^ (-1<<30)) >> >> >> +*f->adaptcoeffs = ((res & (-(1<<31))) ^ (-(1<<30))) >> >> >>(25 + (absres <= f->avg*3) + (absres >> <= f->avg*4/3)); >> >> else >> >> *f->adaptcoeffs = 0; >> >> -- >> >> 2.5.2 >> >> >> > >> > After this patch (GCC 5.2, x86) >> > >> > libavcodec/apedec.c: In function 'do_apply_filter': >> > libavcodec/apedec.c:1284:44: warning: integer overflow in expression >> > [-Woverflow] >> > *f->adaptcoeffs = ((res & (-(1<<31))) ^ (-(1<<30))) >> >> >> Good catch - made an off by one error in my assumptions. I don't see a >> nice, clean way of dealing with -(1 << 31). >> I propose one of the following: >> 1. use INT32_MIN. >> 2. use an explicit binary/hexadecimal mask. >> 3. use e.g (-2)*(1<<30). > > > 0x8000U is fine. This should be ok - res will be promoted to unsigned. The bit representation being identical is not guaranteed, but in the 2's complement world it should be fine. > > Ronald > ___ > 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] avcodec/apedec: fix undefined left shifts of negative numbers
Hi, On Tue, Sep 29, 2015 at 11:04 AM, Ganesh Ajjanagadde wrote: > On Tue, Sep 29, 2015 at 9:24 AM, Hendrik Leppkes > wrote: > > On Sun, Sep 20, 2015 at 4:18 AM, Ganesh Ajjanagadde > > wrote: > >> This fixes -Wshift-negative-value reported with clang 3.7+, e.g > >> > http://fate.ffmpeg.org/log.cgi?time=20150919172459&log=compile&slot=x86_64-darwin-clang-polly-notiling-3.7 > . > >> Note that the patch crucially depends on int >= 32 bits, > >> an assumption made in many places in the codebase. > >> > >> Signed-off-by: Ganesh Ajjanagadde > >> --- > >> libavcodec/apedec.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c > >> index 5536e0f..7b34d26 100644 > >> --- a/libavcodec/apedec.c > >> +++ b/libavcodec/apedec.c > >> @@ -1281,7 +1281,7 @@ static void do_apply_filter(APEContext *ctx, int > version, APEFilter *f, > >> /* Update the adaption coefficients */ > >> absres = FFABS(res); > >> if (absres) > >> -*f->adaptcoeffs = ((res & (-1<<31)) ^ (-1<<30)) >> > >> +*f->adaptcoeffs = ((res & (-(1<<31))) ^ (-(1<<30))) >> > >>(25 + (absres <= f->avg*3) + (absres > <= f->avg*4/3)); > >> else > >> *f->adaptcoeffs = 0; > >> -- > >> 2.5.2 > >> > > > > After this patch (GCC 5.2, x86) > > > > libavcodec/apedec.c: In function 'do_apply_filter': > > libavcodec/apedec.c:1284:44: warning: integer overflow in expression > > [-Woverflow] > > *f->adaptcoeffs = ((res & (-(1<<31))) ^ (-(1<<30))) >> > > Good catch - made an off by one error in my assumptions. I don't see a > nice, clean way of dealing with -(1 << 31). > I propose one of the following: > 1. use INT32_MIN. > 2. use an explicit binary/hexadecimal mask. > 3. use e.g (-2)*(1<<30). 0x8000U is fine. Ronald ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/apedec: fix undefined left shifts of negative numbers
On Tue, Sep 29, 2015 at 9:24 AM, Hendrik Leppkes wrote: > On Sun, Sep 20, 2015 at 4:18 AM, Ganesh Ajjanagadde > wrote: >> This fixes -Wshift-negative-value reported with clang 3.7+, e.g >> http://fate.ffmpeg.org/log.cgi?time=20150919172459&log=compile&slot=x86_64-darwin-clang-polly-notiling-3.7. >> Note that the patch crucially depends on int >= 32 bits, >> an assumption made in many places in the codebase. >> >> Signed-off-by: Ganesh Ajjanagadde >> --- >> libavcodec/apedec.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c >> index 5536e0f..7b34d26 100644 >> --- a/libavcodec/apedec.c >> +++ b/libavcodec/apedec.c >> @@ -1281,7 +1281,7 @@ static void do_apply_filter(APEContext *ctx, int >> version, APEFilter *f, >> /* Update the adaption coefficients */ >> absres = FFABS(res); >> if (absres) >> -*f->adaptcoeffs = ((res & (-1<<31)) ^ (-1<<30)) >> >> +*f->adaptcoeffs = ((res & (-(1<<31))) ^ (-(1<<30))) >> >>(25 + (absres <= f->avg*3) + (absres <= >> f->avg*4/3)); >> else >> *f->adaptcoeffs = 0; >> -- >> 2.5.2 >> > > After this patch (GCC 5.2, x86) > > libavcodec/apedec.c: In function 'do_apply_filter': > libavcodec/apedec.c:1284:44: warning: integer overflow in expression > [-Woverflow] > *f->adaptcoeffs = ((res & (-(1<<31))) ^ (-(1<<30))) >> Good catch - made an off by one error in my assumptions. I don't see a nice, clean way of dealing with -(1 << 31). I propose one of the following: 1. use INT32_MIN. 2. use an explicit binary/hexadecimal mask. 3. use e.g (-2)*(1<<30). I have no preference, and will post updated patch within the next few days, unless someone does the trivial fix and pushes before then. > ___ > 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] avcodec/apedec: fix undefined left shifts of negative numbers
On Sun, Sep 20, 2015 at 4:18 AM, Ganesh Ajjanagadde wrote: > This fixes -Wshift-negative-value reported with clang 3.7+, e.g > http://fate.ffmpeg.org/log.cgi?time=20150919172459&log=compile&slot=x86_64-darwin-clang-polly-notiling-3.7. > Note that the patch crucially depends on int >= 32 bits, > an assumption made in many places in the codebase. > > Signed-off-by: Ganesh Ajjanagadde > --- > libavcodec/apedec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c > index 5536e0f..7b34d26 100644 > --- a/libavcodec/apedec.c > +++ b/libavcodec/apedec.c > @@ -1281,7 +1281,7 @@ static void do_apply_filter(APEContext *ctx, int > version, APEFilter *f, > /* Update the adaption coefficients */ > absres = FFABS(res); > if (absres) > -*f->adaptcoeffs = ((res & (-1<<31)) ^ (-1<<30)) >> > +*f->adaptcoeffs = ((res & (-(1<<31))) ^ (-(1<<30))) >> >(25 + (absres <= f->avg*3) + (absres <= > f->avg*4/3)); > else > *f->adaptcoeffs = 0; > -- > 2.5.2 > After this patch (GCC 5.2, x86) libavcodec/apedec.c: In function 'do_apply_filter': libavcodec/apedec.c:1284:44: warning: integer overflow in expression [-Woverflow] *f->adaptcoeffs = ((res & (-(1<<31))) ^ (-(1<<30))) >> ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/apedec: fix undefined left shifts of negative numbers
On Tue, Sep 29, 2015 at 9:08 AM, Michael Niedermayer wrote: > On Tue, Sep 29, 2015 at 08:08:54AM -0400, Ganesh Ajjanagadde wrote: >> On Tue, Sep 29, 2015 at 4:11 AM, Paul B Mahol wrote: >> > On 9/25/15, Ganesh Ajjanagadde wrote: >> >> On Sat, Sep 19, 2015 at 10:18 PM, Ganesh Ajjanagadde >> >> wrote: >> >>> This fixes -Wshift-negative-value reported with clang 3.7+, e.g >> >>> http://fate.ffmpeg.org/log.cgi?time=20150919172459&log=compile&slot=x86_64-darwin-clang-polly-notiling-3.7. >> >>> Note that the patch crucially depends on int >= 32 bits, >> >>> an assumption made in many places in the codebase. >> >>> >> >>> Signed-off-by: Ganesh Ajjanagadde >> >>> --- >> >>> libavcodec/apedec.c | 2 +- >> >>> 1 file changed, 1 insertion(+), 1 deletion(-) >> >>> >> >>> diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c >> >>> index 5536e0f..7b34d26 100644 >> >>> --- a/libavcodec/apedec.c >> >>> +++ b/libavcodec/apedec.c >> >>> @@ -1281,7 +1281,7 @@ static void do_apply_filter(APEContext *ctx, int >> >>> version, APEFilter *f, >> >>> /* Update the adaption coefficients */ >> >>> absres = FFABS(res); >> >>> if (absres) >> >>> -*f->adaptcoeffs = ((res & (-1<<31)) ^ (-1<<30)) >> >> >>> +*f->adaptcoeffs = ((res & (-(1<<31))) ^ (-(1<<30))) >> >> >>>(25 + (absres <= f->avg*3) + (absres >> >>> <= >> >>> f->avg*4/3)); >> >>> else >> >>> *f->adaptcoeffs = 0; >> >>> -- >> >>> 2.5.2 >> >>> >> >> >> >> ping >> >> ___ >> >> ffmpeg-devel mailing list >> >> ffmpeg-devel@ffmpeg.org >> >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> >> > >> > I guess its fine if fate passes. >> >> Can't confirm that on my end, since I don't know if my fate runs a >> test for this. Can someone tell me an easy way to check if make fate >> tests a particular code or note for future reference? > > easy way: add abort() in the codepath that changes > and run fate, if it passes it was not tested thanks for the tip. > >> >> On the other hand, note that -(1 << n) and (-1 << n) are identical at >> least on GCC and clang, so I think this should be fine. > > yes > > applied > > thx > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > The bravest are surely those who have the clearest vision > of what is before them, glory and danger alike, and yet > notwithstanding go out to meet it. -- Thucydides > > ___ > 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] avcodec/apedec: fix undefined left shifts of negative numbers
On Tue, Sep 29, 2015 at 08:08:54AM -0400, Ganesh Ajjanagadde wrote: > On Tue, Sep 29, 2015 at 4:11 AM, Paul B Mahol wrote: > > On 9/25/15, Ganesh Ajjanagadde wrote: > >> On Sat, Sep 19, 2015 at 10:18 PM, Ganesh Ajjanagadde > >> wrote: > >>> This fixes -Wshift-negative-value reported with clang 3.7+, e.g > >>> http://fate.ffmpeg.org/log.cgi?time=20150919172459&log=compile&slot=x86_64-darwin-clang-polly-notiling-3.7. > >>> Note that the patch crucially depends on int >= 32 bits, > >>> an assumption made in many places in the codebase. > >>> > >>> Signed-off-by: Ganesh Ajjanagadde > >>> --- > >>> libavcodec/apedec.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c > >>> index 5536e0f..7b34d26 100644 > >>> --- a/libavcodec/apedec.c > >>> +++ b/libavcodec/apedec.c > >>> @@ -1281,7 +1281,7 @@ static void do_apply_filter(APEContext *ctx, int > >>> version, APEFilter *f, > >>> /* Update the adaption coefficients */ > >>> absres = FFABS(res); > >>> if (absres) > >>> -*f->adaptcoeffs = ((res & (-1<<31)) ^ (-1<<30)) >> > >>> +*f->adaptcoeffs = ((res & (-(1<<31))) ^ (-(1<<30))) >> > >>>(25 + (absres <= f->avg*3) + (absres <= > >>> f->avg*4/3)); > >>> else > >>> *f->adaptcoeffs = 0; > >>> -- > >>> 2.5.2 > >>> > >> > >> ping > >> ___ > >> ffmpeg-devel mailing list > >> ffmpeg-devel@ffmpeg.org > >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >> > > > > I guess its fine if fate passes. > > Can't confirm that on my end, since I don't know if my fate runs a > test for this. Can someone tell me an easy way to check if make fate > tests a particular code or note for future reference? easy way: add abort() in the codepath that changes and run fate, if it passes it was not tested > > On the other hand, note that -(1 << n) and (-1 << n) are identical at > least on GCC and clang, so I think this should be fine. yes applied thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The bravest are surely those who have the clearest vision of what is before them, glory and danger alike, and yet notwithstanding go out to meet it. -- Thucydides signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/apedec: fix undefined left shifts of negative numbers
On Tue, Sep 29, 2015 at 4:11 AM, Paul B Mahol wrote: > On 9/25/15, Ganesh Ajjanagadde wrote: >> On Sat, Sep 19, 2015 at 10:18 PM, Ganesh Ajjanagadde >> wrote: >>> This fixes -Wshift-negative-value reported with clang 3.7+, e.g >>> http://fate.ffmpeg.org/log.cgi?time=20150919172459&log=compile&slot=x86_64-darwin-clang-polly-notiling-3.7. >>> Note that the patch crucially depends on int >= 32 bits, >>> an assumption made in many places in the codebase. >>> >>> Signed-off-by: Ganesh Ajjanagadde >>> --- >>> libavcodec/apedec.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c >>> index 5536e0f..7b34d26 100644 >>> --- a/libavcodec/apedec.c >>> +++ b/libavcodec/apedec.c >>> @@ -1281,7 +1281,7 @@ static void do_apply_filter(APEContext *ctx, int >>> version, APEFilter *f, >>> /* Update the adaption coefficients */ >>> absres = FFABS(res); >>> if (absres) >>> -*f->adaptcoeffs = ((res & (-1<<31)) ^ (-1<<30)) >> >>> +*f->adaptcoeffs = ((res & (-(1<<31))) ^ (-(1<<30))) >> >>>(25 + (absres <= f->avg*3) + (absres <= >>> f->avg*4/3)); >>> else >>> *f->adaptcoeffs = 0; >>> -- >>> 2.5.2 >>> >> >> ping >> ___ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> > > I guess its fine if fate passes. Can't confirm that on my end, since I don't know if my fate runs a test for this. Can someone tell me an easy way to check if make fate tests a particular code or note for future reference? On the other hand, note that -(1 << n) and (-1 << n) are identical at least on GCC and clang, so I think this should be fine. > ___ > 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] avcodec/apedec: fix undefined left shifts of negative numbers
On 9/25/15, Ganesh Ajjanagadde wrote: > On Sat, Sep 19, 2015 at 10:18 PM, Ganesh Ajjanagadde > wrote: >> This fixes -Wshift-negative-value reported with clang 3.7+, e.g >> http://fate.ffmpeg.org/log.cgi?time=20150919172459&log=compile&slot=x86_64-darwin-clang-polly-notiling-3.7. >> Note that the patch crucially depends on int >= 32 bits, >> an assumption made in many places in the codebase. >> >> Signed-off-by: Ganesh Ajjanagadde >> --- >> libavcodec/apedec.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c >> index 5536e0f..7b34d26 100644 >> --- a/libavcodec/apedec.c >> +++ b/libavcodec/apedec.c >> @@ -1281,7 +1281,7 @@ static void do_apply_filter(APEContext *ctx, int >> version, APEFilter *f, >> /* Update the adaption coefficients */ >> absres = FFABS(res); >> if (absres) >> -*f->adaptcoeffs = ((res & (-1<<31)) ^ (-1<<30)) >> >> +*f->adaptcoeffs = ((res & (-(1<<31))) ^ (-(1<<30))) >> >>(25 + (absres <= f->avg*3) + (absres <= >> f->avg*4/3)); >> else >> *f->adaptcoeffs = 0; >> -- >> 2.5.2 >> > > ping > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > I guess its fine if fate passes. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/apedec: fix undefined left shifts of negative numbers
On Sat, Sep 19, 2015 at 10:18 PM, Ganesh Ajjanagadde wrote: > This fixes -Wshift-negative-value reported with clang 3.7+, e.g > http://fate.ffmpeg.org/log.cgi?time=20150919172459&log=compile&slot=x86_64-darwin-clang-polly-notiling-3.7. > Note that the patch crucially depends on int >= 32 bits, > an assumption made in many places in the codebase. > > Signed-off-by: Ganesh Ajjanagadde > --- > libavcodec/apedec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c > index 5536e0f..7b34d26 100644 > --- a/libavcodec/apedec.c > +++ b/libavcodec/apedec.c > @@ -1281,7 +1281,7 @@ static void do_apply_filter(APEContext *ctx, int > version, APEFilter *f, > /* Update the adaption coefficients */ > absres = FFABS(res); > if (absres) > -*f->adaptcoeffs = ((res & (-1<<31)) ^ (-1<<30)) >> > +*f->adaptcoeffs = ((res & (-(1<<31))) ^ (-(1<<30))) >> >(25 + (absres <= f->avg*3) + (absres <= > f->avg*4/3)); > else > *f->adaptcoeffs = 0; > -- > 2.5.2 > ping ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avcodec/apedec: fix undefined left shifts of negative numbers
This fixes -Wshift-negative-value reported with clang 3.7+, e.g http://fate.ffmpeg.org/log.cgi?time=20150919172459&log=compile&slot=x86_64-darwin-clang-polly-notiling-3.7. Note that the patch crucially depends on int >= 32 bits, an assumption made in many places in the codebase. Signed-off-by: Ganesh Ajjanagadde --- libavcodec/apedec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c index 5536e0f..7b34d26 100644 --- a/libavcodec/apedec.c +++ b/libavcodec/apedec.c @@ -1281,7 +1281,7 @@ static void do_apply_filter(APEContext *ctx, int version, APEFilter *f, /* Update the adaption coefficients */ absres = FFABS(res); if (absres) -*f->adaptcoeffs = ((res & (-1<<31)) ^ (-1<<30)) >> +*f->adaptcoeffs = ((res & (-(1<<31))) ^ (-(1<<30))) >> (25 + (absres <= f->avg*3) + (absres <= f->avg*4/3)); else *f->adaptcoeffs = 0; -- 2.5.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
