Re: [PATCH v2] Add regression tests for ctf reading

[Guillermo Martinez <guillermo.e.martinez@oracle.com>]

On Friday, November 26, 2021 5:23:28 AM CST Dodji Seketeli wrote:
> Hello,
> 
> [...]
> 
> On Nov 24, 2021, at 8:36 AM, Dodji Seketeli <dodji@seketeli.org> wrote
> 
> >> So where exactly is the CTF test disabled?
> 
> Guillermo Martinez <guillermo.e.martinez@oracle.com> a écrit:
> 
> > I remove those test entries, so the possible problem detected using external diff
> > command in CTF is, for instance, in: test-read-common/test-PR26568-1-ctf.o.abi,
> > the first time that runtestreadctf is executed an abixml file  is generated:
> >
> > ...
> >     <class-decl name='' size-in-bits='32' alignment-in-bits='32' is-struct='yes' is-anonymous='yes' visibility='default' id='type-id-3'>
> >       <data-member access='public' layout-offset-in-bits='0'>
> >         <var-decl name='x' type-id='type-id-4' visibility='default'/>
> >       </data-member>
> >     </class-decl>
> >     <class-decl name='' size-in-bits='64' alignment-in-bits='64' is-struct='yes' is-anonymous='yes' visibility='default' id='type-id-5'>
> >       <data-member access='public' layout-offset-in-bits='0'>
> >         <var-decl name='y' type-id='type-id-6' visibility='default'/>
> >       </data-member>
> >     </class-decl>
> >
> > and this is second execution:
> > ...
> >     <class-decl name='' size-in-bits='64' alignment-in-bits='64' is-struct='yes' is-anonymous='yes' visibility='default' id='type-id-5'>
> >       <data-member access='public' layout-offset-in-bits='0'>
> >         <var-decl name='y' type-id='type-id-6' visibility='default'/>
> >       </data-member>
> >     </class-decl>
> >     <class-decl name='' size-in-bits='32' alignment-in-bits='32' is-struct='yes' is-anonymous='yes' visibility='default' id='type-id-3'>
> >       <data-member access='public' layout-offset-in-bits='0'>
> >         <var-decl name='x' type-id='type-id-4' visibility='default'/>
> >       </data-member>
> >     </class-decl>
> >
> > As we can see both abixml files  have *same nodes information* but the nodes
> > are in a different order, so the test should be marked as passed, however using
> > the external diff command the test fails. so my question is should we rely in diff
> > command to compare the corpus of the ELF input files (abixml files)?
> 
> So, you see here, this is exactly why we use the diff tool IN ADDITION
> to abidiff (which only look for ABI changes).
> 
> Here, you see that two invocations of abidw is yielding possibly two
> *DIFFERENT* abixml files.  They are different because some struct type
> definition come out in different order.
OK, but they represent the same ABI, because the nodes order is not
relevant here .. Is that correct?

> Why?
> 
> This is because we have two struct types that have the same (empty) name
> and yet they are different.  The abixml writer sorts types using their
> names, somehow.  So some times, one type comes first; some times, it's
> the other.
OK.
> Let's look at the issue a bit deeper.
> 
> These are anonymous struct.
> 
> For various reasons, we need to designate types in the Libabigail
> system.  So every single type needs to be given a name.  Even anonymous
> types.
> 
> Most of the time, in C, anonymous types are named using typedefs.  E.g:
> 
> typedef struct
> {
>   int x;
> } struct_name;
> 
> "typedef struct_name" is the 'naming typedef' of the struct type.  It's
> used to name it.  That's how it's designated to be later used.
> 
> The libabigail IR has the decl_base::set_naming_typedef() function to
> set naming typedefs on 
> 
> If you look at the code of the dwarf reader, in the function
> build_typedef_type, near the end, you'll see that it uses the
> set_naming_typedef method to set the naming typedef of the underlying
> anonymous class (or struct) it sees.
> 
> Once you've done that, the struct will be represented in the abixml file
> as having the name "struct_name".  And your problem will be solved, I
> think.
> 
> There are other techniques used in the dwarf-reader to handle anonymous
> types that are not named by a typedef.  But those are super rare,
> especially in C.  So I hope that using naming typedef should be good
> enough for a start, in practise.
> 
> It's precisely to detect cases like this that the "diff" tool is used.
So, is correct mark this test as failed as the current ctf/dwarf test harness does?

I'll isolate the scenario using anonymous types as you explained above :-).
Thanks so much for your hints!!
  
> I hope this helps.
> 
> [...]
> 
> Cheers,