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

[Mark Harris]

> -    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.

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.

 - Mark