Re: [Bug default/21296] New: abidiff reports possibly bogus differences and crashes

[Dodji Seketeli <dodji@seketeli.org>]

I have applied the patch
https://sourceware.org/git/gitweb.cgi?p=libabigail.git;a=commit;h=0c820488d4a161f1bd0d1fc3b70bb2518130512b
to the master branch to fix the abort.

Below are my comments for the other issues.

> the second changed function seems to only be different with regards to
> whitespace. We think that whitespace differences should be
> ignored. Can you make the evaluative function ignore the change in the
> whitespace.

[...]

>   [C]'function std::tuple<STR&&> my_forward_as_tuple<STR>(STR&&)' at
> clanggcc.cxx:225:1 has some indirect sub-type changes:
>     return type changed:
>       type name changed from 'std::tuple<STR&&>' to 'std::tuple<STR &&>'

It's true that here, the name of the type changed from 'tuple<STR&&>' to
'tuple<STR &&>', due to the white space addition.

Today, the type comparison engine considers two types with different
names as being different.

Ideally, clang and gcc should emit type names that are normalized
somehow, so that we can compare them.

I guess libabigail could indeed normalize the type names, but thas is
more involved than just stripping white spaces.

For instance:

>
>       1 base class deletion:
>         struct std::_Tuple_impl<0ul, STR&&> at clanggcc.cxx:119:1
>       1 base class insertion:
>         struct std::_Tuple_impl<0, STR &&> at clanggcc.cxx:119:1

Here, in the type name "_Tuple_impl<0ul, STR&&>", the '0ul' constant
literal changed to become the constant literal '0'.  Both are
equivalent.  But recognizing this is different from the whitespace case
above.

I think we can (and should) do better than what we are doing.

That is, we should try harder to normalize type names before comparing
them.  I guess that improvement could even use its own tracking bug.

[...]

> 3) libabigail seems to miss the true nature of the ABI change which is that one
> of the parameters is passed on the stack while the other is passed on a
> register. We believe that GCC implements the correct calling convention
> according to the Itanium C++ ABI.

To date, Libabigail doesn't look at the location of function parameters.
So changes in argument passing conventions are not detected at the
moment.

I think we could add that feature, but that would be a project in its
own.  Actually, there is an opened enhancement request for this
https://sourceware.org/bugzilla/show_bug.cgi?id=19949.


> See: https://bugs.llvm.org//show_bug.cgi?id=23034 I haven't looked at the DWARF
> yet but jwakely seemed to say that he wasn't surprised that libabigail missed
> detecting this ABI change because the DWARF was insufficient.

The reason why libabigail missed it is that it's not yet trying to see
it :-)

But Jonathan is right that just looking at the DWARF as it is today will
not necessarily be enough.

For instance, let's look at the DWARF of the function
"my_forward_as_tuple<STR>" by generated libclang.so.  I think it's that
one you are talking about.

It says:

 [   498]    subprogram
             low_pc               (addr) +0x00000000000010b0 <_Z19my_forward_as_tupleIJ3STREESt5tupleIJDpOT_EES4_>
             high_pc              (data4) 57 (+0x00000000000010e9)
             frame_base           (exprloc) 
              [   0] reg6
             linkage_name         (strp) "_Z19my_forward_as_tupleIJ3STREESt5tupleIJDpOT_EES4_"
             name                 (strp) "my_forward_as_tuple<STR>"
[...]
 [   4b5]      formal_parameter
               location             (exprloc) 
                [   0] fbreg -16
               name                 (strp) "__args"

If I read that correctly, it says that the parameter of the
my_forward_as_tuple<STR> function is to be found 16 bytes after the
"Frame Base" pointer.  That's the meaning of the "fbreg -16" value of
the DW_AT_location attribute of the formal parameter.

And the frame base register is the register number "6", if I believe the
value of the DW_AT_frame attribute which is "reg6".

The mapping of DWARF register for the x86_64 abi says that register 6 is
%rbp.

I think that means that clang++ is saying that the argument of the
formal_parameter is being passed on the stack.

I haven't yet looked at what the assembly code is doing.

Looking at the DWARF emitted by GCC, it's saying something similar.  The
argument is being passed on the stack.

I'll look at what we have when libclang.so is compiled with
optimizations, and I'll look at the assembly too, before we jump to
conclusions.


-- 
		Dodji