Re: [PATCH 3/5] XML writer: track emitted types by bare pointer

[Matthias Maennich <maennich@google.com>]

Hi Dodji!

Thanks for the review and thanks for your thoughts!

On Fri, Dec 10, 2021 at 11:50:49AM +0100, Dodji Seketeli wrote:
>Hello,
>
>Matthias Maennich <maennich@google.com> a écrit:
>
>[...]
>
>> This is a performance and safety improvement made possible by the
>> previous changes which ensure that the same pointers are inserted and
>> looked up.
>>
>> This essentially removes the now unnecessary deep comparison.
>
>[...]
>
>> +++ b/src/abg-writer.cc
>> @@ -123,14 +123,10 @@ typedef unordered_map<type_base*,
>>  		      abigail::diff_utils::deep_ptr_eq_functor> type_ptr_map;
>>
>>  // A convenience typedef for a set of type_base*.
>> -typedef unordered_set<const type_base*, type_hasher,
>> -		      abigail::diff_utils::deep_ptr_eq_functor>
>> -type_ptr_set_type;
>> +typedef std::unordered_set<const type_base*> type_ptr_set_type;
>>
>>  /// A convenience typedef for a set of function type*.
>> -typedef unordered_set<function_type*, type_hasher,
>> -		      abigail::diff_utils::deep_ptr_eq_functor>
>> -fn_type_ptr_set_type;
>> +typedef std::unordered_set<function_type*> fn_type_ptr_set_type;
>
>The problem I see with doing this is that it's possible that two
>declaration-only classes, that are equivalent but that have different
>pointer values get into these sets.
>
>In that case, they would be considered different even though they are
>not.

If the XML writer considers two equivalent declaration-only types to be
different, one question to ask is: what is the real difference, that is,
how will this affect the outcome of abidiff? If the types never change
(kind, name or declaration/definition status), nothing should ever be
reported. If a type does change... there are two possibilities: either
the types were really one type and now perhaps abidiff reports diffs for
the same name in two different ways; or the types were really two
different ones and abidiff has a simpler job. In my experience, abidiff
doesn't always report declaration-only/defined transitions. It doesn't
sound like there will be any really bad impact on diffs from having this
kind of duplication. However, if someone can come up with a test case of
the kind you mention, that would give some extra reassurance.

>
>So maybe it would be better have an equality operator that uses
>is_non_canonicalized_type() to detect those rare cases and use
>structural comparison in those cases?

That might come at higher cost than it is beneficial.

>
>What do you think?

For us specifically - building with clang and for our use cases - if we
keep structural equality of any kind then we need a hash function to go
along with this and, as we've sadly found out, this isn't working well
at the moment. We are currently on a bit dated version of libabigail for
our production use, but would like to close that gap again to come
closer to master.

The risk of infinite loops and the reality of 30x slowdowns for certain
workloads mean we would need to apply these changes to remove structural
equality testing from the XML writer and then maintain an Android
version of libabigail as a more heavily-patched fork, to whatever extent
is feasible. I would rather we find a good solution that works for all
to get again close to upstream and not having to maintain such a fork.

Yet, as an additional piece of assurance: the testing we have done does
not only include kernels, but of course we heavily examined the
libabigail test suite. Additionally, we maintain a large set of small
test cases specifically created for ABI stability testing and to cover
corner cases of all sorts. We are in the process of publishing those as
well. So far, this has served as great input for this patch series as
well.

Does this make sense? What do you think?

Cheers,
Matthias

>
>-- 
>		Dodji