[PATCH v4 05/15] diff suppression: Fix handling of change kinds.

[Giuliano Procida]

Hi Dodji.

On Mon, 11 May 2020 at 15:15, Dodji Seketeli <dodji@seketeli.org> wrote:
>
> Giuliano Procida <gprocida@google.com> a écrit:
>
> > When parsing suppression specifications, libabigail attempts to detect
> > and ignore suppressions that are empty and so would match everything
> > in their category by default.
>
> No.  Libabigail doesn't try to detect suppressions that are empty.
> Rather, it requires *some* properties to be present in the suppression.
>
> Those mandatory properties are documented for each kind of suppression
> at
>
https://sourceware.org/libabigail/manual/libabigail-concepts.html#suppress-type
,
> for instance.
>
> The reason why there are mandatory properties is to somehow, to give
> incentive to the the writer of the suppression to think and to minimise
> the risk of having supprising false negatives due to suppression
> specifications that are too "greedy" in a sense.
>
> In other words, when in doubt, we choose to not suppress anything,
> rather than suppress too much.
>
> I agree that this is a "balanced" act.

The code and documentation should agree, certainly! If we stick to current
implementation and documentation, I would feel better if the code noisily
ignored rather than silently ignored such suppressions.

> > Unfortunately, with the way the parser currently works, these checks
> > have to be against an exhaustive list of fields that matter, rather
> > than by listing the fields that don't (like label).
>
> What I said above is the reason why it works against an exhaustive
> list.  So that is by design.
>
> > They are fragile in the face of changes that add new fields.
>
> Well, new fields are not a problem because they won't change the set of
> mandatory fields that have been chosen already.

Well, once the dust has settled, it should be very cheap to add
source_location_regexp, for example. But, the way I see the code working,
my fragility argument doesn't stand.

> > Two of the short-cut checks are in fact buggy, missing out the
> > change_kind field.
>
> That's by design.  You don't need to specify the change_kind property
> for the suppress_type directive to work because that property it's
> considered advanced.

It's not that change_kind should be mandatory but that it should be one of
the fields considered sufficient for the specification not to be ignored.

The tests as written are very confusing then. A comment in the .suppr files
that only specify change_kind to say "this suppression will be ignored"
would be useful. It was these tests that led me to believe the code should
have included change_kind as one of the sufficient fields.

> > One of the checks also risks a null pointer
> > dereference as it doesn't actually trigger a return from the function.
>
> That ought to be fixed, I guess.
>
> > This patch eliminates (rather than fixes up) this short-cutting on the
> > grounds that it is a maintenance burden and inconsistent behaviour.
>
> > Users should be able to do this:
> >
> > [suppress_variable]
> >   label = Suppress all changes to variables
>
> That's precisely what I don't want to allow, by design.
>
> I'd rather have the user specify a regular expression that matches
> everything rather than that.

To give you my perspective...

I have no idea what suppression specifications are being used in reality,
so I am looking at things fairly abstractly. I see things through a
mathematical viewpoint: a suppression is simply a predicate (function
returning a truth value) on a type of object.

libabigail supports multiple suppression specifications and ORs them
together. Within each suppression specification, things are (mostly) ANDed
together (and within ANDs there is some support for NOT as well via
_not_regex properties). [c.f. DNF
<https://en.wikipedia.org/wiki/Disjunctive_normal_form>]

So the user can construct certain types of predicates. More generally, the
predicates which always return true or always return false are perfectly
valid mathematically. The always false one (never suppress) is
constructible by supplying an empty OR - no predicates - and the always
true one would logically be constructible by supplying an empty AND - a
predicate with no properties. Treating empty AND as false is what I meant
by "inconsistent".

More practically though, what do we want to happen when the user does
something bad? The current behaviour is to silently ignore.

I think it would be more useful to generate warning messages. Then there is
no surprise to the user if their suppression is either ignored or
suppresses too much. I don't mind so much which behaviour is implemented
so long as there is this coverage. It would be nice if the user didn't have
to do regex matches to implement "true" or "only additions" as predicates,
but that's a smaller issue.

> So, in light of my explanations above, I'd prefer that we don't apply
> this patch.

Understood.

> Does that make sense?

I can preserve the existing behaviours, but I would really want to do both
of:

a) comment the tests that were surprising to me (or allow change_kind as a
sufficient property)
b) emit warnings to the user whenever a suppression is going to be ignored
because it doesn't have one of a list of properties

I'm happy to adapt the code and repost.

> Cheers,
>
> --
>                 Dodji

Regards,
Giuliano.