[FFmpeg-devel] [PATCHv2] avformat/movenc: suppress -Wstrict-overflow warnings

Ganesh Ajjanagadde gajjanag at mit.edu
Sat Oct 17 23:35:38 CEST 2015


On Sat, Oct 17, 2015 at 3:57 PM, Mark Harris <mark.hsj at gmail.com> wrote:
>> -    if (cluster_idx >= track->entry)
>> +    /* GCC 5.2 wants to "optimize" cluster_idx >= track->entry to the below
>> +     * expression. We actually mean cluster_idx >= track->entry. */
>> +    if (cluster_idx - track->entry >= 0)
>>          return 0;
>
> On Sat, Oct 17, 2015 at 11:04 AM, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
>> Yes, it is really a questions of where on the ROC curve you want to
>> land, and as such has no technical solution. Since there seems to be
>> almost universal consensus against my proposal in this case, consider
>> the patch dropped. I may return to it in a few years :).
>
> Just a suggestion but I think the key thing to avoid in future patches
> is changing an obviously correct check to something else that has the
> potential to introduce new undefined behavior or subtle bugs.
>
> For example the original check here is the obvious check to ensure
> that access to the cluster array cannot occur at or beyond index
> track->entry.  The proposed replacement cannot ensure that because the
> arithmetic has the potential to overflow and produce the wrong result,
> even without any compiler optimizations that eliminate the check.  So
> this is simply replacing a potential for undefined behavior that is
> impossible (provable by looking only at this source file) but gcc (and
> not clang) falsely warn about, with a potential for new undefined
> behavior that gcc doesn't warn about even though it cannot be ruled
> out, at least not without analyzing other source files.
>
> As an example, suppose it was possible for some other source file to
> set track->entry to INT_MIN.  Previously the comparison would be true
> and it would safely return 0, but with the proposed change it would
> result in undefined behavior and may attempt to access entries in the
> cluster array that do not exist.

No it won't: please read the thread, GCC is already doing some
expression massaging as an "optimization", and then warning us
afterwards.

>
> This is different than quieting a warning by unnecessarily
> initializing a local variable to 0, because changing a local variable
> from uninitialized to initialized does not introduce any new undefined
> behavior.

The commit message said that I have manually audited it. By the way,
that analysis was useful, simply because GCC is already doing it and
we need to know that it is safe :).

>
>  - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


More information about the ffmpeg-devel mailing list