[FFmpeg-devel] [PATCH] avcodec/apedec: fix undefined left shifts of negative numbers

[Michael Niedermayer]

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 <onemda at gmail.com> wrote:
> > On 9/25/15, Ganesh Ajjanagadde <gajjanagadde at gmail.com> wrote:
> >> On Sat, Sep 19, 2015 at 10:18 PM, Ganesh Ajjanagadde
> >> <gajjanagadde at gmail.com> 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 <gajjanagadde at gmail.com>
> >>> ---
> >>>  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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150929/e99f6600/attachment.sig>