Re: [PATCH 2/2] writer: Avoid using dynamic hashing in type maps

[Dodji Seketeli <dodji@redhat.com>]

Hello,

Dodji Seketeli <dodji@redhat.com> writes:

> Hey,
>
> Giuliano Procida <gprocida@google.com> writes:
>
>> This probably fixes the hash/equality issue. At least, it makes no
>> difference (for a simple stock kernel) on top of, or without, my change
>> that switched to using plain pointers.
>
> Okay, that is good to know, thanks.
>
>>
>> It does cause the XML writer to emit (more) XML elements with duplicate
>> ids, perhaps where anonymous types are being identified very late in the
>> process. You can see this with the following, before and after the change.
>> Admittedly, there are already about 400 ids that appear to be duplicated!
>>
>> find tests -name '*abi' | while read x; do sed -En "s;.*
>> id='([^']*)'.*;\1;p" $x | sort | uniq -c | grep -vw 1 | sort -nr | sed
>> "s;^;$x: ;"; done
>
>
> This is interesting.  I went through most of the duplicated IDs reported
> by your script.  Thank you for putting this together.
>
> I have just filled an enhancement request for a potential tool à la
> abilint (probably reusing abilint infrastructure) that would
> analyze/sanitize abixml files to detect potential pathological cases for
> these: https://sourceware.org/bugzilla/show_bug.cgi?id=26591.
>
> Most of the current redundant IDs in the tests/data/test-read-dwarf are
> for array sub-ranges.  They are are always sub-types of arrays in C and
> C++.  They have their own ID but we don't de-duplicate these at the
> moment.  So this is not an symptom of a problem per se.
>
> There are other cases that are mostly related to C++.  I have listed
> these in the enhancement request above.
>
> From manual inspection of the cases reported by your script, I haven't
> noticed a case resulting from a type equality problem.  If you do, I'd
> be glad to look into.
>
>
>> I'd still prefer less code to more code and to switch to making the XML
>> writer not care about equality at all.
>
> I still think that the writer should try (as much as possible, but no
> more) to ensure that types are emitted just once.  In the past, it was
> the first point where this was done.  Then slowly, we built
> de-duplication features throughout the stack.  They are not perfect yet,
> so I think we still need to ensuring that at the writer level.  Much
> less now, but we still do.
>
> Note that we do much less de-duplication at the level of the writer than
> we used to do in the past.  Hopefully at some point we won't need it
> that anymore, and that point I'll be happy to remove that from the
> writer.  But I don't think that time has come yet.
>
> [...]
>
>>> --- a/src/abg-ir.cc
>>> +++ b/src/abg-ir.cc
>>> @@ -23085,6 +23085,45 @@ size_t
>>>  hash_type_or_decl(const type_or_decl_base_sptr& tod)
>>>  {return hash_type_or_decl(tod.get());}
>>>
>>> +/// Hash a type by either returning the pointer value of its canonical
>>> +/// type or by returning a constant if the type doesn't have a
>>> +/// canonical type.
>>> +///
>>> +/// @param t the type to consider.
>>> +///
>>> +/// @return the hash value.
>>> +size_t
>>> +hash_as_canonical_type_or_constant(const type_base *t)
>>> +{
>>> +  type_base *canonical_type = 0;
>>> +
>>> +  if (t)
>>> +    canonical_type = t->get_naked_canonical_type();
>>> +
>>> +  if (!canonical_type)
>>> +    {
>>>
>>
>> Is the following code definitely compatible with the definition of equality
>> or is there a chance you'll determine two different canonical types of
>> definitions of declarations but equality would actually succeed? If you
>> have any doubts, then it's safer to remove this.
>
> If two decl-only classes have definitions, then their definitions are
> compared.
>
> So I do expect the following code to comply with the equality code, yes.
>
>>
>>
>>> +      // If the type doesn't have a canonical type, maybe it's because
>>> +      // it's a declaration-only type?  If that's the case, let's try
>>> +      // to get the canonical type of the definition of this
>>> +      // declaration.
>>> +      decl_base *decl = is_decl(t);
>>> +      if (decl
>>> +         && decl->get_is_declaration_only()
>>> +         && decl->get_naked_definition_of_declaration())
>>> +       {
>>> +         type_base *definition =
>>> +           is_type(decl->get_naked_definition_of_declaration());
>>> +         ABG_ASSERT(definition);
>>> +         canonical_type = definition->get_naked_canonical_type();
>>> +       }
>>> +    }
>>> +
>>> +  if (canonical_type)
>>> +    return reinterpret_cast<size_t>(canonical_type);
>>> +
>>> +  return 0xDEADBABE;
>>> +}
>>> +
>>>  /// Test if the pretty representation of a given @ref function_decl is
>>>  /// lexicographically less then the pretty representation of another
>>>  /// @ref function_decl.
>>> diff --git a/src/abg-writer.cc b/src/abg-writer.cc
>>> index 4c751c2..59060e1 100644
>>> --- a/src/abg-writer.cc
>>> +++ b/src/abg-writer.cc
>>> @@ -136,7 +136,7 @@ struct type_hasher
>>>  {
>>>    size_t
>>>    operator()(const type_base* t) const
>>> -  {return hash_type(t);}
>>> +  {return hash_as_canonical_type_or_constant(t);}
>>>  }; // end struct type_hasher
>>>
>>>  /// A convenience typedef for a map that associates a pointer to type
>>> diff --git a/tests/data/test-read-dwarf/test16-pr18904.so.abi
>>> b/tests/data/test-read-dwarf/test16-pr18904.so.abi
>>> index 0075a6e..377d322 100644
>>>
>>
>> Modulo my comments above,
>>
>> Signed-off-by: Giuliano Procida <gprocida@google.com>
>
> Thanks! 

Applied to master at https://sourceware.org/git/?p=libabigail.git;a=commit;h=5cf2473d3ca3acdb5664313e33c4ced618cf023a.

Cheers,

-- 
		Dodji