[FFmpeg-devel] [PATCH] "assert(a && b)" --> "assert(a); assert(b)" for more precise diagnostics, except for libformat

[Adam Richter]

On Tue, May 14, 2019 at 6:48 PM mypopy at gmail.com <mypopy at gmail.com> wrote:
>
> On Wed, May 15, 2019 at 7:01 AM Hendrik Leppkes <h.leppkes at gmail.com> wrote:
> >
> > On Tue, May 14, 2019 at 11:25 PM Adam Richter <adamrichter4 at gmail.com> wrote:
> > >
> > > Consider, for example, if you agree that columnization makes this range check
> > > more recognizable in a glance and makes it easier to spot what the bounds are
> > > (the sixteen space indentation is taken from the code in which it appeared):
> > >
> > >                 av_assert0(par->bits_per_coded_sample >= 0 &&
> > > par->bits_per_coded_sample <= 8);
> > >
> > >                             ...vs...
> > >
> > >                 av_assert0(par->bits_per_coded_sample >= 0);
> > >                 av_assert0(par->bits_per_coded_sample <= 8);
> > >
> > > A possible counter-argument to this might be that, in a long sequence
> > > of assertions, can be informative to group related assertions
> > > together, which I think is true, but it is possible to get this other
> > > means, such as by using blank lines to separate express such grouping.
> > >
> > > So, Mark, if you decide you are OK with my complete patches, I encourage
> > > you to let me know.  Otherwise, if there are any of those changes which you
> > > are OK with, I would like to just to to get those merged.  Finally, if you would
> > > rather see none of those changes merged (one one to split the assertions in
> > > libavformat and one was for everywhere else in ffmpeg), please let me know
> > > about that too, in which case, if no one advocates for their
> > > inclusion, I'll drop
> > > my proposal to merge these changes.
> > >
> >
> > Unfortunately I have to agree with Mark. asserst that check the same
> > value or extremely closely related values should stay combined.
> >
> I agree this part

I am trying to understand what problem you see with this.

It occurs to me that you might be concerned about generating less
efficient code for the common assert success case, in particular,
in the example I showed for readability, potentially dereferencing
"par" twice, but I made a test program (attached) and determined
from reading the generated assembly that at least for gcc with
optimization -O2 on x86_64, x86_32, aarch64 and arm32, as
long as the abort function has "__attribute__ ((noreturn))", the
compiler seems to be able to avoid repeating the memory fetch
for the second assertion.  I also check this for clang -O2 on
on x86_64.

Of course, without the noreturn attribute on the abort function,
the compiler realizes that the abort function could have written
to memory, so it refetches the value in the split assertion case,
which I think might have been your concern, but as long as
the abort function is declared with an attribute noreturn, we
should be OK for gcc.

On the other hand, I'm not sure what compilers people use
for other platforms such as Microsoft Windows and if you tell
me that it is a known problem on a specific platform that is
potentially relevant to ffmpeg, that would probably change my
mind.

Of course, it's not necessary for you change my mind or for
you to invest further time in this discussion, as I imagine you
and other participants have other things to do.  So, if I don't
get a further explanation, I may still believe that you're wrong,
but I'll respect your need to prioritize tasks other than continuing
this discussion, and will not expect to see my proposed change
merged unless the predominant opinion of others in this discussion
changes to being in favor it, which, so far, I acknowledge, it is not.

> > >
> > > Also after this, I may take a look at adding a branch hint to the av_assertN
> > > macros if nobody objects.
> > >
> >
> > Please don't, we don't do branch hints anywhere, and this is a bad
> > place to start.
> > If an assert is truely performance sensitive (as in, it makes a
> > measurable difference), it should probably be moved from assert0 to
> > assert1, and as such is only enabled in testing builds.
> >
> If assert0 or assert1 lead to performance drop, we need some profiling
> data, then try to fix it.

The above comments by Hendrick and you does not identify a cost to
adding a branch hint to assert.  There would be a downside in the form of
a few lines of code complexity in libavutil/avassert.h.  If that is
your concern,
I guess our disagreement is that I see reducing the cost of assert so that
perhaps CPU usage with and without would be a tiny bit more similar for
reproducing problems and maybe (I'm not saying it's likely) it might tip a
trade-off in favor of keeping an assert enabled in some borderline
circumstance.  I'd take that trade (add the branch prediction).

If you want to educate me on some other reason why I may be wrong,
about adding the branch prediction for assertion checks, I'd been keen
to know, but I realize everyone's time is limited, and if I haven't
convinced you and also created consensus in favor of adding the
branch prediction to assertion checking, then I don't expect to advocate
further on this list for merging such a change into your tree at this time.

Thanks to everyone who has participated in this discussion for your input.

Adam
-------------- next part --------------
A non-text attachment was scrubbed...
Name: abort-test.c
Type: text/x-csrc
Size: 581 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190515/2369cb83/attachment.c>