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

[Hendrik Leppkes]

On Wed, May 15, 2019 at 9:21 PM Adam Richter <adamrichter4 at gmail.com> wrote:
>
> 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.

You seem to be totally overthinking this. I'm not concerned about code
generation or anything like that, just the simple fact that I believe
that the checks are more logical and actually easier to understand if
they are logically grouped and combined. And I really don't see the
advantage in any of these changes, personally.

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

Check the mailing list archives for previous discussion of using
"likely"/"unlikely" and the likes. There is serious contention to
their usefulness in general, so we should rather avoid making a
precedent for them in some debug statements.

- Hendrik