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

[Adam Richter]

On Sun, May 12, 2019 at 11:16 AM Michael Niedermayer
<michael at niedermayer.cc> wrote:
>
> On Sun, May 12, 2019 at 05:58:50PM +0100, Mark Thompson wrote:
> > On 12/05/2019 16:24, Adam Richter wrote:
> > > This patch separates statements of the form "assert(a && b);" into
> > > "assert(a);" and "assert(b);", typically involving an assertion
> > > function like av_assert0.
> > >
> > > This patch covers all of ffmpeg, except for the libavformat, which I
> > > have already submitted separately.
> > >
> > > I have not tested this patch other than observing that ffmpeg still
> > > builds without any apparent new complaints, that no complaints in the
> > > build contain "assert", and that "make fate" seems to succeed.
> > >
> > > Thanks in advance for considering the attached patch.
> > >
> > > Adam
> > >
> > >
> > > From f815a2481a19cfd191b9f97e246b307b71d6c790 Mon Sep 17 00:00:00 2001
> > > From: Adam Richter <adamrichter4 at gmail.com>
> > > Date: Sun, 12 May 2019 08:02:51 -0700
> > > Subject: [PATCH] "assert(a && b)" --> "assert(a); assert(b)" for more
> > >  precise diagnostics, except for libformat.
> > >
> > > This patch separates statements of the form "assert(a && b);" into
> > > "assert(a);" and "assert(b);", for more precise diagnostics when
> > > an assertion fails, which can be especially important in cases where
> > > the problem may not be easily reproducible and save developer time.
> > > Usually, this involves functions like av_assert0.
> >
> > I don't feel very convinced by the general case of this argument.  Assertions are primarily present to assist a developer reading/writing the code; they should never be triggering at runtime in non-development contexts.
> >
> > Where the statements a and b are not related then yes, splitting them is a good idea.  But when it's something like a bounds check on one variable ("av_assert0(A < n && n < B)", as appears quite a few times below) then I think keeping it as one statement feels clearer.  Similarly for highly related conditions ("av_assert0(p && p->f)" or "av_assert0(x < width && y < height)").
> >
> > > There are a couple of cases that this patch does not change:
> > > (1) assert conjunctions of the form assert(condition && "string literal
> > >     to pass to the user as a helpful tip."), and
> > > (2) assert condjunctions where the first part contained a variable
> > >     assignment that was used in the second part of the assertion and
> > >     not outside the assert (so that the variable assignment be elided
> > >     if the assertion checking omitted).
> > >
> > > This patch covers all of ffmpeg except for libavformat, which was
> > > covered in a patch that I previously submitted separately.
> > >
> > > These changes build without any new complaints that I noticed, and
> > > "make fate" succeeds, but I have not otherwise tested them.
> > >
> > > Signed-off-by: Adam Richter <adamrichter4 at gmail.com>
> > > ...
> > > diff --git a/libavcodec/aacpsy.c b/libavcodec/aacpsy.c
> > > index fca692cb15..bef52e8b02 100644
> > > --- a/libavcodec/aacpsy.c
> > > +++ b/libavcodec/aacpsy.c
> > > @@ -504,9 +504,11 @@ static int calc_bit_demand(AacPsyContext *ctx, float pe, int bits, int size,
> > >      fill_level = av_clipf((float)ctx->fill_level / size, clip_low, clip_high);
> > >      clipped_pe = av_clipf(pe, ctx->pe.min, ctx->pe.max);
> > >      bit_save   = (fill_level + bitsave_add) * bitsave_slope;
> > > -    assert(bit_save <= 0.3f && bit_save >= -0.05000001f);
> > > +    assert(bit_save <= 0.3f);
> > > +    assert(bit_save >= -0.05000001f);
> > >      bit_spend  = (fill_level + bitspend_add) * bitspend_slope;
> > > -    assert(bit_spend <= 0.5f && bit_spend >= -0.1f);
> > > +    assert(bit_spend <= 0.5f);
> > > +    assert(bit_spend >= -0.1f);
> >
> > While you're touching calls to traditional assert() please consider turning them into suitable av_assertN().
>
> I agree that they should be changed to av_assertN() but it should be
> in a seperate commit/patch
>
> These assert -> av_assertN changes will likely in some cases "activate"
> "dormant" checks which fail. Managing / Testing / Reviewing and correcting
> these should be easier if they are not in a large change splitting asserts
>
> Thanks
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
[...]

Hello, Michael and Mark.

Thank you for your reviews of my proposed changes that would split
most assertions
of the form "a && b" into separate assertions.

Michael, if I understand correctly, you are OK with the patches after
having looked at
them to spot problems, but are not advocating for or against their
inclusion.  Mark, if I
understand correctly, you would rather see assertions split only where "a and
b are not related."  I don't think anyone else has chimed in, but I
would welcome being
told if I missed anyone else's expressed opinion or if anyone wants to
add anything now.

To make the most efficient use of your time, I would like to proceed
as follows.  In the rest
of this message, I will include an argument, primarily in the hopes of
persuading you,
Mark.  After that, if you're still not on board with merging all of
the patches, and if there
are no other replies in the next day or two, then I encourage you to
post a subset of my
patches which you think should be merged (if any, and if you have the
time).  Otherwise,
probably around this weekend, I will try to make a new subset of these
patches that I
think you might be OK with and post that for comments.

However, first, in the hopes that we can avoid picking out which part
of the patches to
merge, here is a response, Mark, to your previously stated objections.

As a preliminary, regarding your remark "Assertions are primarily
present to assist a
developer reading/writing the code", I just want to clarify something
for other readers
that, av_assert0 statements are always compiled in, and, as for the
other assertions,
there may be some small value in reducing their costs so that they
result in timing
behavior that is a bit closer to the behavior without the assertions,
which might be
where a bug occurred.  So, there may still be some small value in
being able to pass
branch predictions to the compiler in the assertion statement, although I was
surprised to see that neither glibc nor ffmpeg appear currently to do this.

However, I recognize that most of your argument, Mark, is about readability.

At least in my limited experience, I think that, separated assertions
are actually
almost always more readable, sometimes eliminating parentheses, often
making references to the same values line up on the same columns, which
can make range checks more obvious, and sometimes changing
multi-line conditions to separate single line conditions, which can
be recognized separately.

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.

By the way, after this, regarding the your (Mark and Michael's) discussion of
changing the few assert() call that my patches effected to av_assertN, I have
not looked in much depth, but I think that all such cases may be in pretty
busy paths, so it might be most appropriate to replace them with av_assert2,
which normally is not compiled in.

Also after this, I may take a look at adding a branch hint to the av_assertN
macros if nobody objects.

In any case, thanks for your thoughtful consideration of these proposed
edits.

Adam