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

[Dodji Seketeli <dodji@seketeli.org>]

Matthias Maennich <maennich@google.com> a écrit:

[...]

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

The problem is not necessarily at the abidiff level per say.

The problem would be duplication of decl-only types in the abixml
output, I think, and maybe infinite loops in those cases.  The infinite
loops are easy to debug, though.  So I am not concerned about them.

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

The reason why I was pointing to this "general" issue is to make sure
you are aware of this.  As type duplications in abixml was something you
guys were tracking (and rightly so) I thought I'd point out that we
still have the risk here.

But because the type id map (writer_context::m_type_id_map) is not
affected, the duplicated types will correctly be identified as such by
the reader; thus I don't think abidiff is going to be affected.

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

I could not tell, as I don't necessarily have the right binaries at
hand.  I trust you.

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

If you don't really care about the potential type duplication in the
abixml as stated above, frankly, let's just get this patch in.

Are you okay with that?

Cheers,

-- 
		Dodji