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

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

[jwakely.gcc at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=21296

            Bug ID: 21296
           Summary: abidiff reports possibly bogus differences and crashes
           Product: libabigail
           Version: unspecified
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: default
          Assignee: dodji at redhat dot com
          Reporter: jwakely.gcc at gmail dot com
                CC: libabigail at sourceware dot org
  Target Milestone: ---

Created attachment 9935
  --> https://sourceware.org/bugzilla/attachment.cgi?id=9935&action=edit
testcase

Using GCC 6 and Clang (any version) to compile the attached code into two
shared libs produces libraries that crash abigail:

clang++ -std=c++14  -o libclang.so -shared -fPIC  clanggcc.cxx  -g
g++   -o libgcc.so -shared -fPIC  clanggcc.cxx  -g

abidiff libgcc.so libclang.so  
Functions changes summary: 0 Removed (1 filtered out), 2 Changed (9 filtered
out), 0 Added functions
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

2 functions with some indirect sub-type change:

  [C]'method STR::~STR(int)' at clanggcc.cxx:6:1 has some indirect sub-type
changes:
    linkage names of method STR::~STR(int)
    changed from '_ZN3STRD1Ev, _ZN3STRD2Ev' to '_ZN3STRD2Ev'

    name of symbol changed from _ZN3STRD1Ev to _ZN3STRD2Ev
    parameter 1 of type 'int' was removed


  [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 &&>'
      type size hasn't changed

      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
abidiff: ../../src/abg-comparison.cc:8205: virtual void
abigail::comparison::fn_parm_diff::report(std::ostream&, const string&) const:
Assertion `get_type_diff() && get_type_diff()->to_be_reported()' failed.
Aborted (core dumped)


If GCC trunk is used instead of GCC 6 then abigail doesn't crash, but still
reports the same diferences, which appear to be bogus.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug default/21296] naive comparison of names of template instantiations leads to false postives

[dodji at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=21296

--- Comment #3 from dodji at redhat dot com ---
*** Bug 21772 has been marked as a duplicate of this bug. ***

-- 
You are receiving this mail because:
You are on the CC list for the bug.

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

[Dodji Seketeli]

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

[Bug default/21296] naive comparison of names of template instantiations leads to false postives

[dodji at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=21296

dodji at redhat dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2017-07-17
            Summary|abidiff reports possibly    |naive comparison of names
                   |bogus differences and       |of template instantiations
                   |crashes                     |leads to false postives
     Ever confirmed|0                           |1

-- 
You are receiving this mail because:
You are on the CC list for the bug.

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

[woodard at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=21296

Ben Woodard <woodard at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |woodard at redhat dot com

--- Comment #1 from Ben Woodard <woodard at redhat dot com> ---
jwakely and I were discussing this, this morning.

We think that there are actually three problems:
1) the abidiff crash
2) 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.
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.
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. If that is in
fact the case, then we probably need to file a couple of BZ's to improve the
DWARF on GCC and CLAMG so that libabigail has the information needed to address
this.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

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

[dodji at seketeli dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=21296

--- Comment #2 from dodji at seketeli dot 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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug default/21296] naive comparison of names of template instantiations leads to false postives

[maennich at android dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=21296

Matthias Maennich <maennich at android dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement
                 CC|                            |maennich at android dot com

--- Comment #4 from Matthias Maennich <maennich at android dot com> ---
This is somewhat a duplicate of 21492, still I keep it open separately. I move
it to be an enhancement, though.

-- 
You are receiving this mail because:
You are on the CC list for the bug.