[Bug default/31513] New: abidiff differences due to change in compiler version

[Bug default/31513] New: abidiff differences due to change in compiler version

[quic_ashudas at quicinc dot com]

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

            Bug ID: 31513
           Summary: abidiff differences due to change in compiler version
           Product: libabigail
           Version: unspecified
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: default
          Assignee: dodji at redhat dot com
          Reporter: quic_ashudas at quicinc dot com
                CC: libabigail at sourceware dot org
  Target Milestone: ---

We have created a shared library using the below source file, We are observing
abi differences as when we run abidiff on the library when compiled with
different compiler version GCC 7.5.0 and GCC 11.4.0

Ques 1: Is this behavior expected as we have not made any change in the source
files of the library the highlighted changes are in compiler source files.

Ques 2: If yes, can you share a suppression file to ignore all the changes
which  are not from the elf source file.


#include <iostream>
#include <string>
#include <vector>

namespace qiifa{

       class class1{
                public:
                        class1();
                        std::string generate_lorem_ipsum(unsigned int length);
                        int addnums(int x, int y);
                        int subnums(int x, int y);
                private:
                        std::vector<char> letters;

        };

}

Abidiff log

"Functions changes summary: 0 Removed, 1 Changed (3 filtered out), 0 Added
functions
 Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

 1 function with some indirect sub-type change:

   [C] 'method int qiifa::class1::addnums(int, int)' has some indirect sub-type
changes:
     implicit parameter 0 of type 'qiifa::class1* const' has sub-type changes:
       in unqualified underlying type 'qiifa::class1*':
         in pointed to type 'class qiifa::class1':
           type size hasn't changed
           1 data member change:
             type of 'std::vector<char, std::allocator<char> > letters'
changed:
               type size hasn't changed
               1 base class change:
                 'struct std::_Vector_base<char, std::allocator<char> >'
changed:
                   type size hasn't changed
                   1 data member change:
                     type of 'std::_Vector_base<char, std::allocator<char>
>::_Vector_impl _M_impl' changed:
                       type size hasn't changed
                       1 base class insertion:
                         struct std::_Vector_base<char, std::allocator<char>
>::_Vector_impl_data
                       3 data member deletions:
                         'std::_Vector_base<char, std::allocator<char>
>::pointer _M_start', at offset 0 (in bits)
                         'std::_Vector_base<char, std::allocator<char>
>::pointer _M_finish', at offset 64 (in bits)
                         'std::_Vector_base<char, std::allocator<char>
>::pointer _M_end_of_storage', at offset 128 (in bits)

 "

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

[Bug default/31513] abidiff differences due to change in compiler version

[dodji at redhat dot com]

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

dodji at redhat dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |WAITING
   Last reconfirmed|                            |2024-03-20

--- Comment #1 from dodji at redhat dot com ---
Hello,

Thank you for reporting this problem.

Would it be possible that you provide the exact binaries?  That would make the
reproduction and debugging much more precise.

Thanks.

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

[Bug default/31513] abidiff differences due to change in compiler version

[quic_ashudas at quicinc dot com]

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

--- Comment #2 from Ashutosh Das (QUIC) <quic_ashudas at quicinc dot com> ---
Created attachment 15418
  --> https://sourceware.org/bugzilla/attachment.cgi?id=15418&action=edit
Compiled shared libraries from different compiler version gcc7 and gcc11

Shared library compared on gcc11

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

[Bug default/31513] abidiff differences due to change in compiler version

[quic_ashudas at quicinc dot com]

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

--- Comment #3 from Ashutosh Das (QUIC) <quic_ashudas at quicinc dot com> ---
Created attachment 15419
  --> https://sourceware.org/bugzilla/attachment.cgi?id=15419&action=edit
Compiled shared libraries from different compiler version gcc7

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

[Bug default/31513] abidiff differences due to change in compiler version

[quic_ashudas at quicinc dot com]

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

--- Comment #4 from Ashutosh Das (QUIC) <quic_ashudas at quicinc dot com> ---
(In reply to dodji from comment #1)
> Hello,
> 
> Thank you for reporting this problem.
> 
> Would it be possible that you provide the exact binaries?  That would make
> the reproduction and debugging much more precise.
> 
> Thanks.

Attached the required libraries.

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

Re: [Bug default/31513] abidiff differences due to change in compiler version

[Dodji Seketeli]

Hello,

> Attached the required libraries.

Thanks!  These have been really useful.

So here is my analysis, looking at the debug info.

I think it all boils down to a change in the implementation details of
the vector type of the standard library.  Namely, the type:
    _Vector_base<char, std::allocator<char> >::_Vector_impl.

Let's look at a dump of the DWARF debuginfo for that type in the binary
resulting from the libstdc++ implementation for gcc7.xx:


     [  2c03]        structure_type       abbrev: 17
                     name                 (strp) "_Vector_impl"
                     byte_size            (data1) 24
                     decl_file            (data1) stl_vector.h (3)
                     decl_line            (data1) 81
                     sibling              (ref4) [  2cc5]
     [  2c0f]          inheritance          abbrev: 59
                       type                 (ref4) [  2540]
                       data_member_location (data1) 0
     [  2c15]          member               abbrev: 7
                       name                 (strp) "_M_start"
                       decl_file            (data1) stl_vector.h (3)
                       decl_line            (data1) 84
                       type                 (ref4) [  2cc5]
                       data_member_location (data1) 0
     [  2c21]          member               abbrev: 7
                       name                 (strp) "_M_finish"
                       decl_file            (data1) stl_vector.h (3)
                       decl_line            (data1) 85
                       type                 (ref4) [  2cc5]
                       data_member_location (data1) 8
     [  2c2d]          member               abbrev: 7
                       name                 (strp) "_M_end_of_storage"
                       decl_file            (data1) stl_vector.h (3)
                       decl_line            (data1) 86
                       type                 (ref4) [  2cc5]
                       data_member_location (data1) 16
     [...]


So we see that the _Vector_impl type inherits a type identified by the
DIE (debug information entry) at offset 0x2540 (which happens to be the
type allocator<char>) and has three data members: _M_start, _M_finish,
and _M_end_of_storage.


Now, let's look at the similar dump of the DWARF debuginfo for that type
in the binary resulting from the libstdc++ implementation for gcc11.xx:


     [  36b3]        structure_type       abbrev: 23
                     name                 (strp) "_Vector_impl"
                     byte_size            (data1) 24
                     decl_file            (data1) stl_vector.h (3)
                     decl_line            (data1) 128
                     decl_column          (data1) 14
                     sibling              (ref4) [  377e]
     [  36c0]          inheritance          abbrev: 50
                       type                 (ref4) [   bb5]
                       data_member_location (implicit_const) 0
     [  36c5]          inheritance          abbrev: 50
                       type                 (ref4) [  35f9]
                       data_member_location (implicit_const) 0
     [  36ca]          subprogram           abbrev: 24
                       external             (flag_present) yes
                       name                 (strp) "_Vector_impl"
                       decl_file            (data1) stl_vector.h (3)
                       decl_line            (data1) 131
                       decl_column          (data1) 2
                       linkage_name         (strp) "_ZNSt12_Vector_baseIcSaIcEE12_Vector_implC4Ev"
                       declaration          (flag_present) yes
                       object_pointer       (ref4) [  36de]
                       sibling              (ref4) [  36e4]
     [  36de]            formal_parameter     abbrev: 2
                         type                 (ref4) [  6bce]
                         artificial           (flag_present) yes
     [  36e4]          subprogram           abbrev: 24

     [...]

Here, we see that the _Vector_impl inherits *TWO* types.  One is
identified by the DIE at offset 0xbb5 (which is the type
allocator<char>) and the other is the type identified by the DIE at
offset 0x35f9 which the (newly introduced) type:

    "_Vector_base<char, std::allocator<char> >::_Vector_impl_data"

And we see that this new implementation of _Vector_impl doesn't have any
of the data members _M_start, _M_finish,  _M_end_of_storage.  Now let's
look at the DIE for the type:

    "_Vector_base<char, std::allocator<char> >::_Vector_impl_data"


 [  35f9]        structure_type       abbrev: 23
                 name                 (strp) "_Vector_impl_data"
                 byte_size            (data1) 24
                 decl_file            (data1) stl_vector.h (3)
                 decl_line            (data1) 91
                 decl_column          (data1) 14
                 sibling              (ref4) [  36a2]
 [  3606]          member               abbrev: 6
                   name                 (strp) "_M_start"
                   decl_file            (data1) stl_vector.h (3)
                   decl_line            (data1) 93
                   decl_column          (data1) 10
                   type                 (ref4) [  36a7]
                   data_member_location (data1) 0
 [  3613]          member               abbrev: 6
                   name                 (strp) "_M_finish"
                   decl_file            (data1) stl_vector.h (3)
                   decl_line            (data1) 94
                   decl_column          (data1) 10
                   type                 (ref4) [  36a7]
                   data_member_location (data1) 8
 [  3620]          member               abbrev: 6
                   name                 (strp) "_M_end_of_storage"
                   decl_file            (data1) stl_vector.h (3)
                   decl_line            (data1) 95
                   decl_column          (data1) 10
                   type                 (ref4) [  36a7]
                   data_member_location (data1) 16
 [...]
 

We see that this new type has the three data members _M_start,
_M_finish,  _M_end_of_storage that are missing from _Vector_impl.

In other words, the data members of the class _Vector_impl in gcc7 were moved to
the class _Vector_impl_data in gcc11 and _Vector_impl now inherits
_V_Vector_impl_data.

Note that the layout of the _Vector_impl hasn't changed.  Its size
hasn't changed either.  This is coherent with what abidiff says, when it
says that the size hasn't changed (and when it doesn't report any offset change):

                  type of 'std::_Vector_base<char, std::allocator<char> >::_Vector_impl _M_impl' changed:
                    type size hasn't changed
                    1 base class insertion:
                      struct std::_Vector_base<char, std::allocator<char> >::_Vector_impl_data at stl_vector.h:91:1
                    3 data member deletions:
                      'std::_Vector_base<char, std::allocator<char> >::pointer _M_start', at offset 0 (in bits) at stl_vector.h:84:1
                      'std::_Vector_base<char, std::allocator<char> >::pointer _M_finish', at offset 64 (in bits) at stl_vector.h:85:1
                      'std::_Vector_base<char, std::allocator<char> >::pointer _M_end_of_storage', at offset 128 (in bits) at stl_vector.h:86:1


In other words, this is not an incompatible ABI change.  I'd argue that
it's not even an ABI change.

Now, I am thinking that *maybe* we should make libabigail detect that
the data members _M_start, _M_finish and _M_end_of_storage were moved to
the new base class _Vector_impl_data with no observable change to the
layout of the _Vector_impl class.  This would have made the analysis
much simpler for the user.  What do you think?  If you agree, then I
think we should add a new enhancement request for that feature.

> Ques 1: Is this behavior expected as we have not made any change in the source
> files of the library the highlighted changes are in compiler source files.

I guess I would answer "it depends".  Sorry.

This is a real change that happened in the implementation details of the
libstdc++ standard library.  If, for a reason, you are interested in
reviewing this kind of changes you might be interested in *seeing* them
in the first place.

If, on the other hand, you are not interested in the changes about types
(that are implementation details) of the libstdc++ standard library,
then yes, these changes should be suppressed from the output of
abidiff.  You should be able to control this behavior, I think.


> Ques 2: If yes, can you share a suppression file to ignore all the
> changes which are not from the elf source file.

So, here is an example of suppression file that you could use, if you
want to suppress all the changes to the types of the "std" namespace:

---------->8<---------------------
[suppress_type]
  # Suppress changes in types of the std:: namespace
  name_regexp = ^std::.*
--------->8<----------------------

For the record, here is what I am getting when using a suppression file
with that content:


        $ cat default.suppr
        [suppress_type]
          # Suppress changes in types of the std:: namespace
          name_regexp = ^std::.*

        $ abidiff --suppr default.suppr bin/libloremipsum_gcc7.so bin/libloremipsum_gcc11.so
        Functions changes summary: 16 Removed, 0 Changed (20 filtered out), 21 Added functions
        Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
        Function symbols changes summary: 2 Removed, 2 Added function symbols not referenced by debug info
        Variable symbols changes summary: 0 Removed, 0 Added variable symbol not referenced by debug info

        [...]
        $

Note how all the *type changes* got filtered out, as shown in the summary.

I hope this helps.

-- 
		Dodji

[Bug default/31513] abidiff differences due to change in compiler version

[woodard at redhat dot com]

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

Ben Woodard <woodard at redhat dot com> changed:

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

--- Comment #5 from Ben Woodard <woodard at redhat dot com> ---
As a person who has done a lot of testing like this:

Ques 1: Is this behavior expected as we have not made any change in the source
files of the library the highlighted changes are in compiler source files.

"is this behavior expected" is a tricky question. As a person who has done a
lot of this testing. I would say that that this behavior is "expected" in the
sense that I do not at all find it surprising.  On the other hand, is this
behavior "desired", no not really. Here is the thing, libabigail pulls its
information from a combination of DWARF and ELF and while ELF pretty much is
needed to make things work and if it isn't "right" the program doesn't run.
DWARF on the other hand has been more or less an afterthought and if things
were not correct then *shrug*. The feeling was it wasn't a critical problem if
debuggers didn't work perfectly in all cases. Tools such as libabigail, stress
the DWARF in new ways and have uncovered quite a few compiler bugs over the
years. We work closely with the compiler people and have resolved many issues
as they have come up. 

So with compilers as far apart as 7.5.0 and 11.4.0 I think it is pretty fair to
assume that we found and fixed quite a few problems between those two releases
and some of the differences are false errors due to problems with the DWARF
emitted by the compiler. The fact that the problems end up showing up in part
of the stdlib also is suggestive that they are more a problem with DWARF than a
problem with your library's ABI changing. 

That being said, there may be some changes in how the templates in the headers
that make up that part of the standard library are instantiated based on the
compiler version or supported language version and this could lead to changes
like you are seeing. If that bit of code has a conditional compilation that
adds those members or deletes those other members based on the compiler's
language version then you could see that.

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

[Bug default/31513] abidiff differences due to change in compiler version

[quic_ashudas at quicinc dot com]

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

--- Comment #6 from Ashutosh Das (QUIC) <quic_ashudas at quicinc dot com> ---
Ques .1 Are we planning to address these undesired behaviors by working with
the compiler team?


Ques .2 Is there a way to identify the changes are made in compiler files using
namespace or other identifier and suppress these changes using suppression
files.

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

[Bug default/31513] abidiff differences due to change in compiler version

[dodji at seketeli dot org]

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

--- Comment #7 from dodji at seketeli dot org ---
Hello,

> Attached the required libraries.

Thanks!  These have been really useful.

So here is my analysis, looking at the debug info.

I think it all boils down to a change in the implementation details of
the vector type of the standard library.  Namely, the type:
    _Vector_base<char, std::allocator<char> >::_Vector_impl.

Let's look at a dump of the DWARF debuginfo for that type in the binary
resulting from the libstdc++ implementation for gcc7.xx:


     [  2c03]        structure_type       abbrev: 17
                     name                 (strp) "_Vector_impl"
                     byte_size            (data1) 24
                     decl_file            (data1) stl_vector.h (3)
                     decl_line            (data1) 81
                     sibling              (ref4) [  2cc5]
     [  2c0f]          inheritance          abbrev: 59
                       type                 (ref4) [  2540]
                       data_member_location (data1) 0
     [  2c15]          member               abbrev: 7
                       name                 (strp) "_M_start"
                       decl_file            (data1) stl_vector.h (3)
                       decl_line            (data1) 84
                       type                 (ref4) [  2cc5]
                       data_member_location (data1) 0
     [  2c21]          member               abbrev: 7
                       name                 (strp) "_M_finish"
                       decl_file            (data1) stl_vector.h (3)
                       decl_line            (data1) 85
                       type                 (ref4) [  2cc5]
                       data_member_location (data1) 8
     [  2c2d]          member               abbrev: 7
                       name                 (strp) "_M_end_of_storage"
                       decl_file            (data1) stl_vector.h (3)
                       decl_line            (data1) 86
                       type                 (ref4) [  2cc5]
                       data_member_location (data1) 16
     [...]


So we see that the _Vector_impl type inherits a type identified by the
DIE (debug information entry) at offset 0x2540 (which happens to be the
type allocator<char>) and has three data members: _M_start, _M_finish,
and _M_end_of_storage.


Now, let's look at the similar dump of the DWARF debuginfo for that type
in the binary resulting from the libstdc++ implementation for gcc11.xx:


     [  36b3]        structure_type       abbrev: 23
                     name                 (strp) "_Vector_impl"
                     byte_size            (data1) 24
                     decl_file            (data1) stl_vector.h (3)
                     decl_line            (data1) 128
                     decl_column          (data1) 14
                     sibling              (ref4) [  377e]
     [  36c0]          inheritance          abbrev: 50
                       type                 (ref4) [   bb5]
                       data_member_location (implicit_const) 0
     [  36c5]          inheritance          abbrev: 50
                       type                 (ref4) [  35f9]
                       data_member_location (implicit_const) 0
     [  36ca]          subprogram           abbrev: 24
                       external             (flag_present) yes
                       name                 (strp) "_Vector_impl"
                       decl_file            (data1) stl_vector.h (3)
                       decl_line            (data1) 131
                       decl_column          (data1) 2
                       linkage_name         (strp)
"_ZNSt12_Vector_baseIcSaIcEE12_Vector_implC4Ev"
                       declaration          (flag_present) yes
                       object_pointer       (ref4) [  36de]
                       sibling              (ref4) [  36e4]
     [  36de]            formal_parameter     abbrev: 2
                         type                 (ref4) [  6bce]
                         artificial           (flag_present) yes
     [  36e4]          subprogram           abbrev: 24

     [...]

Here, we see that the _Vector_impl inherits *TWO* types.  One is
identified by the DIE at offset 0xbb5 (which is the type
allocator<char>) and the other is the type identified by the DIE at
offset 0x35f9 which the (newly introduced) type:

    "_Vector_base<char, std::allocator<char> >::_Vector_impl_data"

And we see that this new implementation of _Vector_impl doesn't have any
of the data members _M_start, _M_finish,  _M_end_of_storage.  Now let's
look at the DIE for the type:

    "_Vector_base<char, std::allocator<char> >::_Vector_impl_data"


 [  35f9]        structure_type       abbrev: 23
                 name                 (strp) "_Vector_impl_data"
                 byte_size            (data1) 24
                 decl_file            (data1) stl_vector.h (3)
                 decl_line            (data1) 91
                 decl_column          (data1) 14
                 sibling              (ref4) [  36a2]
 [  3606]          member               abbrev: 6
                   name                 (strp) "_M_start"
                   decl_file            (data1) stl_vector.h (3)
                   decl_line            (data1) 93
                   decl_column          (data1) 10
                   type                 (ref4) [  36a7]
                   data_member_location (data1) 0
 [  3613]          member               abbrev: 6
                   name                 (strp) "_M_finish"
                   decl_file            (data1) stl_vector.h (3)
                   decl_line            (data1) 94
                   decl_column          (data1) 10
                   type                 (ref4) [  36a7]
                   data_member_location (data1) 8
 [  3620]          member               abbrev: 6
                   name                 (strp) "_M_end_of_storage"
                   decl_file            (data1) stl_vector.h (3)
                   decl_line            (data1) 95
                   decl_column          (data1) 10
                   type                 (ref4) [  36a7]
                   data_member_location (data1) 16
 [...]


We see that this new type has the three data members _M_start,
_M_finish,  _M_end_of_storage that are missing from _Vector_impl.

In other words, the data members of the class _Vector_impl in gcc7 were moved
to
the class _Vector_impl_data in gcc11 and _Vector_impl now inherits
_V_Vector_impl_data.

Note that the layout of the _Vector_impl hasn't changed.  Its size
hasn't changed either.  This is coherent with what abidiff says, when it
says that the size hasn't changed (and when it doesn't report any offset
change):

                  type of 'std::_Vector_base<char, std::allocator<char>
>::_Vector_impl _M_impl' changed:
                    type size hasn't changed
                    1 base class insertion:
                      struct std::_Vector_base<char, std::allocator<char>
>::_Vector_impl_data at stl_vector.h:91:1
                    3 data member deletions:
                      'std::_Vector_base<char, std::allocator<char> >::pointer
_M_start', at offset 0 (in bits) at stl_vector.h:84:1
                      'std::_Vector_base<char, std::allocator<char> >::pointer
_M_finish', at offset 64 (in bits) at stl_vector.h:85:1
                      'std::_Vector_base<char, std::allocator<char> >::pointer
_M_end_of_storage', at offset 128 (in bits) at stl_vector.h:86:1


In other words, this is not an incompatible ABI change.  I'd argue that
it's not even an ABI change.

Now, I am thinking that *maybe* we should make libabigail detect that
the data members _M_start, _M_finish and _M_end_of_storage were moved to
the new base class _Vector_impl_data with no observable change to the
layout of the _Vector_impl class.  This would have made the analysis
much simpler for the user.  What do you think?  If you agree, then I
think we should add a new enhancement request for that feature.

> Ques 1: Is this behavior expected as we have not made any change in the source
> files of the library the highlighted changes are in compiler source files.

I guess I would answer "it depends".  Sorry.

This is a real change that happened in the implementation details of the
libstdc++ standard library.  If, for a reason, you are interested in
reviewing this kind of changes you might be interested in *seeing* them
in the first place.

If, on the other hand, you are not interested in the changes about types
(that are implementation details) of the libstdc++ standard library,
then yes, these changes should be suppressed from the output of
abidiff.  You should be able to control this behavior, I think.


> Ques 2: If yes, can you share a suppression file to ignore all the
> changes which are not from the elf source file.

So, here is an example of suppression file that you could use, if you
want to suppress all the changes to the types of the "std" namespace:

---------->8<---------------------
[suppress_type]
  # Suppress changes in types of the std:: namespace
  name_regexp = ^std::.*
--------->8<----------------------

For the record, here is what I am getting when using a suppression file
with that content:


        $ cat default.suppr
        [suppress_type]
          # Suppress changes in types of the std:: namespace
          name_regexp = ^std::.*

        $ abidiff --suppr default.suppr bin/libloremipsum_gcc7.so
bin/libloremipsum_gcc11.so
        Functions changes summary: 16 Removed, 0 Changed (20 filtered out), 21
Added functions
        Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
        Function symbols changes summary: 2 Removed, 2 Added function symbols
not referenced by debug info
        Variable symbols changes summary: 0 Removed, 0 Added variable symbol
not referenced by debug info

        [...]
        $

Note how all the *type changes* got filtered out, as shown in the summary.

I hope this helps.

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

[Bug default/31513] abidiff differences due to change in compiler version

[quic_ashudas at quicinc dot com]

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

--- Comment #8 from Ashutosh Das (QUIC) <quic_ashudas at quicinc dot com> ---
Thanks for sharing the detailed analysis.

I understand that this some developers might be interested in knowing more
about such changes, however as we agree this are compatible changes in that
case the abidiff return code should not return value 12 (suggests incompatible
change).

Can we make this error messages as warning and return a compatible return code.

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

[Bug default/31513] abidiff differences due to change in compiler version

[quic_ashudas at quicinc dot com]

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

--- Comment #9 from Ashutosh Das (QUIC) <quic_ashudas at quicinc dot com> ---
Thanks for sharing the detailed analysis.

I understand that this some developers might be interested in knowing more
about such changes, however as we agree this are compatible changes in that
case the abidiff return code should not return value 12 (suggests incompatible
change).

Can we make this error messages as warning and return a compatible return code.

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

[Bug default/31513] abidiff differences due to change in compiler version

[dodji at redhat dot com]

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

dodji at redhat dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |ASSIGNED

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

[Bug default/31513] abidiff differences due to change in compiler version

[dodji at redhat dot com]

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

--- Comment #10 from dodji at redhat dot com ---
"quic_ashudas at quicinc dot com" <sourceware-bugzilla@sourceware.org>
writes:

> https://sourceware.org/bugzilla/show_bug.cgi?id=31513
>
> --- Comment #8 from Ashutosh Das (QUIC) <quic_ashudas at quicinc dot com> ---
> Thanks for sharing the detailed analysis.

You are welcome.

> I understand that this some developers might be interested in knowing more
> about such changes, however as we agree this are compatible changes in that
> case the abidiff return code should not return value 12 (suggests incompatible
> change).

Oh, I didn't realize that the return code was:

    (ABIDIFF_ABI_CHANGE | ABIDIFF_ABI_INCOMPATIBLE_CHANGE).

It should indeed be ABIDIFF_ABI_CHANGE.

Now I think that is the real bug.

> Can we make this error messages as warning and return a compatible return code.

Sure.  The return code should indeed be ABIDIFF_ABI_CHANGE (suggesting a
review from the user).

That is the real bug.

Thinking more about that, I am even thinking that we might go further
and filter that change out by default.

Thanks again for reporting this issue.

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

[Bug default/31513] abidiff wrongly considers data members moved to base class as harmful

[dodji at seketeli dot org]

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

Dodji Seketeli <dodji at seketeli dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|abidiff differences due to  |abidiff wrongly considers
                   |change in compiler version  |data members moved to base
                   |                            |class as harmful

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

[Bug default/31513] abidiff wrongly considers data members moved to base class as harmful

[dodji at seketeli dot org]

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

--- Comment #11 from Dodji Seketeli <dodji at seketeli dot org> ---
Hello,

I have posted a candidate patch for this issue in commit
https://sourceware.org/git/?p=libabigail.git;a=commit;h=338394f5454990c715b52bb4bc2ed47b39d6528b.
 You can get it from the Git repository of the project in the branch
"users/dodji/fixes" that can be browsed at
https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/fixes.

You can check it out with the command: git clone -b users/dodji/fixes
git://sourceware.org/git/libabigail.git.  You can build the source code by
following the instructions at
https://sourceware.org/git/?p=libabigail.git;a=blob;f=COMPILING.

Please note that running abidiff on the two binaries that you provided shows
that several functions (instantiations of libstdc++ function templates) were
removed from the first binary.  That is why abidiff sets the bit
ABIDIFF_ABI_INCOMPATIBLE_CHANGE in the exit value.

To suppress the noise coming from these libstdc++ function template
instantiations (which could be avoided by using the symbol visibility options
of g++, by the way), I provided a suppression file in the non-regression tests
accompanying the patch at
https://sourceware.org/git/?p=libabigail.git;a=blob;f=tests/data/test-abidiff-exit/PR31513/reported/libstdcpp.suppr;h=6c3761006a7c304c8a98dfd2a74097ef9e3282e2;hb=338394f5454990c715b52bb4bc2ed47b39d6528b.

You might want to test this branch on your system and tell me if that fixes the
issue for you.  I hope this helps.

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

[Bug default/31513] abidiff wrongly considers data members moved to base class as harmful

[quic_ashudas at quicinc dot com]

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

--- Comment #12 from Ashutosh Das (QUIC) <quic_ashudas at quicinc dot com> ---

> You might want to test this branch on your system and tell me if that fixes
> the issue for you.  I hope this helps.

I was able to test it and result are as expected it does return compatible
source code for the discussed case.

Thanks for the providing the fix.

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

[Bug default/31513] abidiff wrongly considers data members moved to base class as harmful

[dodji at seketeli dot org]

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

Dodji Seketeli <dodji at seketeli dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #13 from Dodji Seketeli <dodji at seketeli dot org> ---
(In reply to Ashutosh Das (QUIC) from comment #12)

[...]

> I was able to test it and result are as expected it does return compatible
> source code for the discussed case.

Thanks for taking time to test the patch!

I have thus merged the users/dodji/fixes branch into the mainline.  The merged
commit is
https://sourceware.org/git/?p=libabigail.git;a=commit;h=338394f5454990c715b52bb4bc2ed47b39d6528b
and going to be available in the next libabigail 2.5 version.

> Thanks for the providing the fix.

Thank you for reporting this problem and sorry for the inconvenience.

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

[Bug default/31513] abidiff wrongly considers data members moved to base class as harmful

[quic_jiafan at quicinc dot com]

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

Jianfeng Fan <quic_jiafan at quicinc dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |quic_jiafan at quicinc dot com

--- Comment #14 from Jianfeng Fan <quic_jiafan at quicinc dot com> ---
Hi,
There may be an issue in the commit
https://sourceware.org/git/?p=libabigail.git;a=commit;h=338394f5454990c715b52bb4bc2ed47b39d6528b

in abg-comp-filter.cc
https://sourceware.org/git/?p=libabigail.git;a=blobdiff;f=src/abg-comp-filter.cc;h=caea9d0c5308264b40f9b07f5d1bdfebef5caae4;hp=63716775ee8ebe482a7b2f0c504e42372979d77e;hb=338394f5454990c715b52bb4bc2ed47b39d6528b;hpb=a917072c12449cfea03e79a6e82e286e0f54f57b

Added inline comments
static bool
has_subtype_changes(const string_decl_base_sptr_map& f_data_members,
                    const string_decl_base_sptr_map& s_data_members,
                    diff_context_sptr ctxt)
{
  // Now compare the offsets of the data members collected.
  var_decl_sptr s_member;
  for (auto entry : f_data_members)
    {
      var_decl_sptr f_member = is_var_decl(entry.second);
      ABG_ASSERT(f_member);

      auto i = s_data_members.find(entry.first);
      if (i == s_data_members.end())
        {
          unsigned offset = get_data_member_offset(f_member);
          s_member = find_data_member_at_offset(s_data_members, offset);
          if (!s_member)
            // A data member was suppressed; that's bad; let's consider
            // that as a sub-type change.
            return true;
        }

      if (!s_member) <<<<<<<<< we never reset s_member, so it will only be
assigned once, each f_member compare to the first s_member.
        s_member = is_var_decl(i->second);
      ABG_ASSERT(s_member);
      diff_sptr d =compute_diff(f_member->get_type(), s_member->get_type(),
ctxt);
      <<<<<<<<< we can add s_member.reset(); here, s_member can be updated next
time.
      if (d->has_changes())
        return true;
    }
  return false;
}

With this change, the result for test case report1.txt can be changed to
Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added
function
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

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

Re: [Bug default/31513] abidiff wrongly considers data members moved to base class as harmful

[Dodji Seketeli]

Hello Jianfeng,

[...]

> Added inline comments
> static bool
> has_subtype_changes(const string_decl_base_sptr_map& f_data_members,
>                     const string_decl_base_sptr_map& s_data_members,
>                     diff_context_sptr ctxt)
> {
>   // Now compare the offsets of the data members collected.
>   var_decl_sptr s_member;
>   for (auto entry : f_data_members)
>     {
>       var_decl_sptr f_member = is_var_decl(entry.second);
>       ABG_ASSERT(f_member);
>
>       auto i = s_data_members.find(entry.first);
>       if (i == s_data_members.end())
>         {
>           unsigned offset = get_data_member_offset(f_member);
>           s_member = find_data_member_at_offset(s_data_members, offset);
>           if (!s_member)
>             // A data member was suppressed; that's bad; let's consider
>             // that as a sub-type change.
>             return true;
>         }
>
>       if (!s_member) <<<<<<<<< we never reset s_member, so it will only be assigned once, each f_member compare to the first s_member.
>         s_member = is_var_decl(i->second);

Ahah! Good catch.  You are right!

s_member should have been declared in the same scope as f_member.

Please find below the patch that I am proposing to fix this, just like
what is done in the has_offset_changes function. Note that you can get
that patch from the branch
https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/try-PR31513.


Here is the patch.  What do you think about it?

From 1d61a563334a4f15cd834c1de7a3041cbf9e3ac8 Mon Sep 17 00:00:00 2001
From: Dodji Seketeli <dodji@redhat.com>
Date: Wed, 3 Apr 2024 11:10:38 +0200
Subject: [PATCH] Bug 31513 - Fix fallout of initial patch

As Jianfeng Fan pointed out in a comment at
https://sourceware.org/bugzilla/show_bug.cgi?id=31513#c14, there is a
thinko in commit
https://sourceware.org/git/?p=libabigail.git;a=commit;h=338394f5454990c715b52bb4bc2ed47b39d6528b.

has_subtype_changes forgets to reset the s_member when f_member is
reset.  Both variables should be reset in tandem, just like what is
done in has_offset_changes.

Fixed thus.

	* src/abg-comp-filter.cc (has_subtype_changes): Reset s_member in
	the loop, just like f_member.
	* tests/data/test-abidiff-exit/PR31513/non-regr/report1.txt: Adjust.
	* tests/data/test-abidiff-exit/PR31513/non-regr/report2.txt: Adjust.
	* tests/test-abidiff-exit.cc (in_out_specs): Adjust.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 src/abg-comp-filter.cc                                |  2 +-
 .../test-abidiff-exit/PR31513/non-regr/report1.txt    | 11 +----------
 .../test-abidiff-exit/PR31513/non-regr/report2.txt    | 11 +----------
 tests/test-abidiff-exit.cc                            |  4 ++--
 4 files changed, 5 insertions(+), 23 deletions(-)

diff --git a/src/abg-comp-filter.cc b/src/abg-comp-filter.cc
index caea9d0c..2156fad0 100644
--- a/src/abg-comp-filter.cc
+++ b/src/abg-comp-filter.cc
@@ -331,12 +331,12 @@ has_subtype_changes(const string_decl_base_sptr_map& f_data_members,
 		    diff_context_sptr ctxt)
 {
   // Now compare the offsets of the data members collected.
-  var_decl_sptr s_member;
   for (auto entry : f_data_members)
     {
       var_decl_sptr f_member = is_var_decl(entry.second);
       ABG_ASSERT(f_member);
 
+      var_decl_sptr s_member;
       auto i = s_data_members.find(entry.first);
       if (i == s_data_members.end())
 	{
diff --git a/tests/data/test-abidiff-exit/PR31513/non-regr/report1.txt b/tests/data/test-abidiff-exit/PR31513/non-regr/report1.txt
index dff216f8..9666a8fd 100644
--- a/tests/data/test-abidiff-exit/PR31513/non-regr/report1.txt
+++ b/tests/data/test-abidiff-exit/PR31513/non-regr/report1.txt
@@ -1,12 +1,3 @@
-Functions changes summary: 0 Removed, 1 Changed, 0 Added function
+Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added function
 Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
 
-1 function with some indirect sub-type change:
-
-  [C] 'function int foo(type&)' at test1-v0.cc:10:1 has some indirect sub-type changes:
-    parameter 1 of type 'type&' has sub-type changes:
-      in referenced type 'struct type' at test1-v1.cc:8:1:
-        type size hasn't changed
-        1 base class insertion:
-          struct base at test1-v1.cc:2:1
-
diff --git a/tests/data/test-abidiff-exit/PR31513/non-regr/report2.txt b/tests/data/test-abidiff-exit/PR31513/non-regr/report2.txt
index 25d39e9b..9666a8fd 100644
--- a/tests/data/test-abidiff-exit/PR31513/non-regr/report2.txt
+++ b/tests/data/test-abidiff-exit/PR31513/non-regr/report2.txt
@@ -1,12 +1,3 @@
-Functions changes summary: 0 Removed, 1 Changed, 0 Added function
+Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added function
 Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
 
-1 function with some indirect sub-type change:
-
-  [C] 'function int foo(type&)' at test2-v0.cc:10:1 has some indirect sub-type changes:
-    parameter 1 of type 'type&' has sub-type changes:
-      in referenced type 'struct type' at test2-v1.cc:9:1:
-        type size hasn't changed
-        1 base class insertion:
-          struct base at test2-v1.cc:2:1
-
diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc
index 104d0580..29df74ff 100644
--- a/tests/test-abidiff-exit.cc
+++ b/tests/test-abidiff-exit.cc
@@ -1331,7 +1331,7 @@ InOutSpec in_out_specs[] =
     "",
     "",
     "--no-default-suppression",
-    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
+    abigail::tools_utils::ABIDIFF_OK,
     "data/test-abidiff-exit/PR31513/non-regr/report1.txt",
     "output/test-abidiff-exit/PR31513/non-regr/report1.txt"
   },
@@ -1346,7 +1346,7 @@ InOutSpec in_out_specs[] =
     "",
     "",
     "--no-default-suppression",
-    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
+    abigail::tools_utils::ABIDIFF_OK,
     "data/test-abidiff-exit/PR31513/non-regr/report2.txt",
     "output/test-abidiff-exit/PR31513/non-regr/report2.txt"
   },
-- 
2.39.3



-- 
		Dodji

[Bug default/31513] abidiff wrongly considers data members moved to base class as harmful

[dodji at seketeli dot org]

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

--- Comment #15 from Dodji Seketeli <dodji at seketeli dot org> ---
Hello Jianfeng,

[...]

> Added inline comments
> static bool
> has_subtype_changes(const string_decl_base_sptr_map& f_data_members,
>                     const string_decl_base_sptr_map& s_data_members,
>                     diff_context_sptr ctxt)
> {
>   // Now compare the offsets of the data members collected.
>   var_decl_sptr s_member;
>   for (auto entry : f_data_members)
>     {
>       var_decl_sptr f_member = is_var_decl(entry.second);
>       ABG_ASSERT(f_member);
>
>       auto i = s_data_members.find(entry.first);
>       if (i == s_data_members.end())
>         {
>           unsigned offset = get_data_member_offset(f_member);
>           s_member = find_data_member_at_offset(s_data_members, offset);
>           if (!s_member)
>             // A data member was suppressed; that's bad; let's consider
>             // that as a sub-type change.
>             return true;
>         }
>
>       if (!s_member) <<<<<<<<< we never reset s_member, so it will only be assigned once, each f_member compare to the first s_member.
>         s_member = is_var_decl(i->second);

Ahah! Good catch.  You are right!

s_member should have been declared in the same scope as f_member.

Please find below the patch that I am proposing to fix this, just like
what is done in the has_offset_changes function. Note that you can get
that patch from the branch
https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/try-PR31513.


Here is the patch.  What do you think about it?

From 1d61a563334a4f15cd834c1de7a3041cbf9e3ac8 Mon Sep 17 00:00:00 2001
From: Dodji Seketeli <dodji@redhat.com>
Date: Wed, 3 Apr 2024 11:10:38 +0200
Subject: [PATCH] Bug 31513 - Fix fallout of initial patch

As Jianfeng Fan pointed out in a comment at
https://sourceware.org/bugzilla/show_bug.cgi?id=31513#c14, there is a
thinko in commit
https://sourceware.org/git/?p=libabigail.git;a=commit;h=338394f5454990c715b52bb4bc2ed47b39d6528b.

has_subtype_changes forgets to reset the s_member when f_member is
reset.  Both variables should be reset in tandem, just like what is
done in has_offset_changes.

Fixed thus.

        * src/abg-comp-filter.cc (has_subtype_changes): Reset s_member in
        the loop, just like f_member.
        * tests/data/test-abidiff-exit/PR31513/non-regr/report1.txt: Adjust.
        * tests/data/test-abidiff-exit/PR31513/non-regr/report2.txt: Adjust.
        * tests/test-abidiff-exit.cc (in_out_specs): Adjust.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 src/abg-comp-filter.cc                                |  2 +-
 .../test-abidiff-exit/PR31513/non-regr/report1.txt    | 11 +----------
 .../test-abidiff-exit/PR31513/non-regr/report2.txt    | 11 +----------
 tests/test-abidiff-exit.cc                            |  4 ++--
 4 files changed, 5 insertions(+), 23 deletions(-)

diff --git a/src/abg-comp-filter.cc b/src/abg-comp-filter.cc
index caea9d0c..2156fad0 100644
--- a/src/abg-comp-filter.cc
+++ b/src/abg-comp-filter.cc
@@ -331,12 +331,12 @@ has_subtype_changes(const string_decl_base_sptr_map&
f_data_members,
                    diff_context_sptr ctxt)
 {
   // Now compare the offsets of the data members collected.
-  var_decl_sptr s_member;
   for (auto entry : f_data_members)
     {
       var_decl_sptr f_member = is_var_decl(entry.second);
       ABG_ASSERT(f_member);

+      var_decl_sptr s_member;
       auto i = s_data_members.find(entry.first);
       if (i == s_data_members.end())
        {
diff --git a/tests/data/test-abidiff-exit/PR31513/non-regr/report1.txt
b/tests/data/test-abidiff-exit/PR31513/non-regr/report1.txt
index dff216f8..9666a8fd 100644
--- a/tests/data/test-abidiff-exit/PR31513/non-regr/report1.txt
+++ b/tests/data/test-abidiff-exit/PR31513/non-regr/report1.txt
@@ -1,12 +1,3 @@
-Functions changes summary: 0 Removed, 1 Changed, 0 Added function
+Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added
function
 Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

-1 function with some indirect sub-type change:
-
-  [C] 'function int foo(type&)' at test1-v0.cc:10:1 has some indirect sub-type
changes:
-    parameter 1 of type 'type&' has sub-type changes:
-      in referenced type 'struct type' at test1-v1.cc:8:1:
-        type size hasn't changed
-        1 base class insertion:
-          struct base at test1-v1.cc:2:1
-
diff --git a/tests/data/test-abidiff-exit/PR31513/non-regr/report2.txt
b/tests/data/test-abidiff-exit/PR31513/non-regr/report2.txt
index 25d39e9b..9666a8fd 100644
--- a/tests/data/test-abidiff-exit/PR31513/non-regr/report2.txt
+++ b/tests/data/test-abidiff-exit/PR31513/non-regr/report2.txt
@@ -1,12 +1,3 @@
-Functions changes summary: 0 Removed, 1 Changed, 0 Added function
+Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added
function
 Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

-1 function with some indirect sub-type change:
-
-  [C] 'function int foo(type&)' at test2-v0.cc:10:1 has some indirect sub-type
changes:
-    parameter 1 of type 'type&' has sub-type changes:
-      in referenced type 'struct type' at test2-v1.cc:9:1:
-        type size hasn't changed
-        1 base class insertion:
-          struct base at test2-v1.cc:2:1
-
diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc
index 104d0580..29df74ff 100644
--- a/tests/test-abidiff-exit.cc
+++ b/tests/test-abidiff-exit.cc
@@ -1331,7 +1331,7 @@ InOutSpec in_out_specs[] =
     "",
     "",
     "--no-default-suppression",
-    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
+    abigail::tools_utils::ABIDIFF_OK,
     "data/test-abidiff-exit/PR31513/non-regr/report1.txt",
     "output/test-abidiff-exit/PR31513/non-regr/report1.txt"
   },
@@ -1346,7 +1346,7 @@ InOutSpec in_out_specs[] =
     "",
     "",
     "--no-default-suppression",
-    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
+    abigail::tools_utils::ABIDIFF_OK,
     "data/test-abidiff-exit/PR31513/non-regr/report2.txt",
     "output/test-abidiff-exit/PR31513/non-regr/report2.txt"
   },

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

[Bug default/31513] abidiff wrongly considers data members moved to base class as harmful

[quic_jiafan at quicinc dot com]

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

--- Comment #16 from Jianfeng Fan <quic_jiafan at quicinc dot com> ---
Thanks for providing the patch, it looks good to me.

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

[Bug default/31513] abidiff wrongly considers data members moved to base class as harmful

[dodji at seketeli dot org]

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

Dodji Seketeli <dodji at seketeli dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #17 from Dodji Seketeli <dodji at seketeli dot org> ---
(In reply to Jianfeng Fan from comment #16)
> Thanks for providing the patch, it looks good to me.

Thanks, applied to the master branch at
https://sourceware.org/git/?p=libabigail.git;a=commit;h=29719218eea567c6fef9024269ad529684f76c4d.

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