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

[Ganesh Ajjanagadde]

On Tue, Sep 29, 2015 at 9:08 AM, Michael Niedermayer <michaelni at gmx.at> 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 <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

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 at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>