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

[Dodji Seketeli]

Giuliano Procida <gprocida@google.com> a écrit:


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

In general, I chose to avoid emitting user messages unless those are
emitted in well defined places like special "passes" that are meant for
analysis and reporting.

So rather than scathering user messages left and right everywhere, I'd
prefer we properly fix this issue by adding a (set of) pass(es) that
would walk the parsed ini file, analyse it and emit warnings or errors
as necessary.

I understand that you've proposed something in that regard at
https://sourceware.org/pipermail/libabigail/2020q2/002160.html, so I
think the discussion will happen there.

So the more I think about this, the less I am in favour of handling that
here.

[...]

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

When I was assessing what property should be part of the sufficient set,
it didn't seem to me that change_kind should be in there (because it did
sound too easy to make it match a super-broad set of things to
suppress).  The goal, in my mind is *NOT* to strive for minimalism or
orthogonality.  To the contrary.  I'd rather require some redundancy to
be sure user think about this twice.  Because, again, it's super easy to
come up with false negatives otherwise.

Nevertheless if you think it should, then it's just a discussion to be
held.  As a matter of fact, I am not dead opposed to it.  So in the
grand scheme of things, I don't see this as bug.


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

Fair enough.

[...]

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

Yes.  The "silent" part of the behaviour was not meant to be engraved in
stone forever :-)  I still want to add a semantic pass to handle that
separately.


> I think it would be more useful to generate warning messages.

Agreed.

[...]

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

Agreed.

> 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 am not opposed to this, but as I said above, I'd rather see this
happen as part of the separate discussion/project I mentioned earlier.

> I'm happy to adapt the code and repost.

I'd happily accept a patch that does a/ in this context.

I hope this helps.

Cheers,

-- 
		Dodji