On Sat, Oct 17, 2015 at 3:57 PM, Mark Harris <mark....@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 <gajja...@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@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
