[PATCH 1/1] abg-writer.cc: Track types by plain address

[Dodji Seketeli]

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

[...]

> We unfortunately cannot send you an internal (built with Clang and
> linked to libc++) binary. :-/

OK, we'll have to find a way to debug this in this setting then.

>  We have seen bad XML output with both proprietary and AOSP kernel
> builds, but not as yet with a plain GCC/libstdc++ abidw.

OK.  Note that bugs happened in the canonicalization code many times.
Sometimes those were even related to the way the toolchain generates
debug info etc.  They've been fixed so far.  That's maybe why you don't
see issue with GCC.  My take is we just need to fix them when they are
uncovered by other toolchains.

> Here are the debugging details.
>
> record_type_as_referenced is passed a type
> it's an instance of "union { }" nested in a struct - these can appear
> in the kernel if their content is #if optional
> m_referenced_non_canonical_types_set.insert is called

If m_referenced_non_canonical_types_set.insert called (rather than
m_referenced_non_canonical_types_set.insert), then it means the union
type doesn't have a canonical type.  Why is that?

>and fails, b'cos:
> * the type has hash code 4146663795 (and has an internal name of
> "__anonymous_union__18")

I have some thoughts about how we should handle anonymous types and
canoncalization.  I haven't had time to dig into that, but there might
be some improvements to be made there.  This might be the subject of a
separate dedicated discussion.  But first I think the anwser to my
question above can shed some light about what is going on.

> * another type already in the set has hash code 3049540639 (and has an
> internal name of "__anonymous_union__5")
> * the hash codes modulo the hash table bucket count 97 are both 28
> * the two types compare as equal

Right.  If the two types are equal then their hash should be equal.  I
suspect this is a bug in the type_base::dynamic_hash code.  More
precisely, it might be because decl_base::hash::operator() in
abg-hash.cc always takes into account the name of the union, even if
it's anonymous.  The internal name should not be taken into account when
dealing with anonymous types.  This code predates support for anonymous
types so it might need some updating.  I can't say for sure as I can't
debug the thing myself, but that would be my guess.

> At a surface level the equals/hash contract is broken and this should
> be fixed.

It sounds like it, yes.

> Some extensive checking could be done to verify it's not broken in
> other places, but given the complexity of "equals", that may be some
> effort. It's possible that fixing this would make more anonymous types
> be treated as equal (but see my arguments above).

I am not sure we need to dive that far in practise.  This fix might not
be that complicated.  Let's follow the debugging path first.

If you are available on IRC we can do a remote/cooperative debugging
session.  That might be more high bandwidth than this.

Cheers,

[...]

-- 
		Dodji