[Bug default/19734] New: Shifted exported fields in c++ classes are not detected as abi breakage

[Bug default/19734] New: Shifted exported fields in c++ classes are not detected as abi breakage

[dank at kegel dot com]

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

            Bug ID: 19734
           Summary: Shifted exported fields in c++ classes are not
                    detected as abi breakage
           Product: libabigail
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: default
          Assignee: dodji at redhat dot com
          Reporter: dank at kegel dot com
                CC: libabigail at sourceware dot org
  Target Milestone: ---

abidiff fails to detect abi breakage from added structure members.
To demonstrate, create these files, then run foo.sh:

::::::::::::::
foo_h
::::::::::::::
struct __attribute__ ((visibility ("default"))) lego {
    #ifdef VERSION2
    char buggy_symbol[10];
    #else
    char buggy_symbol[5];
    #endif
    int payload;

    lego();

    int get() { return payload; }
    int set(int x) { payload = x; }
};
::::::::::::::
foo.cpp
::::::::::::::
#include "foo.h"

lego::lego() : payload(42) {}
::::::::::::::
main.cpp
::::::::::::::
#include "foo.h"
#include <assert.h>

int main(int argc, char **argv)
{
    lego x;
    assert(x.get() == 42);
    return (x.get() == 42) ? 0 : 1;
}
::::::::::::::
foo.sh
::::::::::::::
#!/bin/sh
set -e
set -x
rm -rf v1 v2
mkdir v1 v2
cp foo_h v1/foo.h
echo "#define VERSION2 1" > v2/foo.h
cat foo_h >> v2/foo.h

# Build library version 1
g++ -g -I v1 foo.cpp -shared -fpic -o libfoo1.so -g
# Build library version 2
g++ -g -I v2 foo.cpp -shared -fpic -o libfoo2.so -g
# abidiff reports no ABI breakage between version 1 and version 2
abidiff libfoo1.so libfoo2.so
# but abidw shows the abi breakage is potentially detectable:
abidw libfoo1.so > abi1.xml
abidw libfoo2.so > abi2.xml
diff -u abi1.xml abi2.xml

# App built against version 1 runs fine against version 1
g++ -g -I v1 main.cpp ./libfoo1.so -o main1
./main1 || echo failed
# App built against version 2 crashes when run against version 1 despite
all-clear from abidiff
g++ -g -I v2 main.cpp ./libfoo1.so -o main2
./main2 || echo failed

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

[Bug default/19734] Libabigail skips the 'this' pointer when comparing two member functions

[dodji at redhat dot com]

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

dodji at redhat dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
            Summary|Shifted exported fields in  |Libabigail skips the 'this'
                   |c++ classes are not         |pointer when comparing two
                   |detected as abi breakage    |member functions

--- Comment #1 from dodji at redhat dot com ---
Right, this is a real issue.

The problem is that today, when libabigail compares two functions (two
constructors, for instance) it doesn't compare their implicit 'this' parameter.

In the case of the example, lego::lego() is the only function that has the
"lego" type as a sub-type.  But that lego type, is reachable only through the
implicit 'this' parameter of that function.  As libabigail skips that implicit
this pointer during its comparison, it doesn't see the changes that were made
to the lego type.

Just to illustrate my point, I added another member function lego.  That member
explicitly takes a parameter which type "contains" the lego type.

Here is the diff of my change to your example:
----------------------------------->8<--------------------
$ diff -u foo.h.orig foo_h
--- foo.h.orig  2016-02-26 16:02:43.122804515 +0100
+++ foo_h       2016-02-26 15:10:03.473190139 +0100
@@ -7,6 +7,7 @@
   int payload;

   lego();
+  lego(const lego&);

   int get() { return payload; }
   int set(int x) { payload = x; }
dodji@tintin:PR19734$ diff -u foo.cpp.orig foo.cpp
--- foo.cpp.orig        2016-02-26 16:02:30.621655698 +0100
+++ foo.cpp     2016-02-26 16:00:01.296878100 +0100
@@ -1,3 +1,10 @@
+#include <string.h>
 #include "foo.h"

 lego::lego() : payload(42) {}
+
+lego::lego(const lego& l)
+{
+  memcpy(buggy_symbol, l.buggy_symbol, sizeof(buggy_symbol));
+  payload = l.payload;
+}
$


Now abidiff does see the change.  This is what running foo.sh now yields:

[...]

+ abidiff libfoo1.so libfoo2.so
Functions changes summary: 0 Removed, 1 Changed, 0 Added function
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

1 function with some indirect sub-type change:

  [C]'method lego::lego(const lego&)' has some indirect sub-type changes:
    parameter 1 of type 'const lego&' has sub-type changes:
      in referenced type 'const lego':
        in unqualified underlying type 'struct lego':
          type size changed from 96 to 128 bits
          2 data member changes:
           type of 'char lego::buggy_symbol[5]' changed:
             type name changed from 'char[5]' to 'char[10]'
             array type size changed from 40 to 80 bits:
             array type subrange 1 changed length from 5 to 10

           'int lego::payload' offset changed from 64 to 96 (in bits)
[...]


Libabigail skips the this parameter because otherwise, too much noise is
emitted in the report.  In this small test case, there won't be any noise, of
course.  But on big c++ code bases with lots classes that have lots of member
functions, there can be lots of duplicated change reports that seriously
clutter the output.  I made that decision back when the library didn't have a
strong framework for filtering out redundant change reports.  It thought that
in general, there would be other functions that would use the class type
explicitly, so skipping the this parameter wouldn't necessarily make user miss
changes on the class type.

And you are with a real life case that rightfully argues otherwise :-)

I guess now that the library has stronger filtering framework, the this
parameter can be taking in account again when comparing methods.  We'll then
need to go through the resulting change reports for various interesting
binaries, spot the cases where unnecessary duplicated change reports are
emitted due to this change, and devise automatic redundancy detection and
filtering for those.

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

[Bug default/19734] Libabigail skips the 'this' pointer when comparing two member functions

[maennich at android dot com]

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

Matthias Maennich <maennich at android dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |maennich at android dot com
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #2 from Matthias Maennich <maennich at android dot com> ---
That appears to be fixed by a recent patch 

2f92777dc8c3 ("Consider the implicit 'this' parameter when comparing methods")
https://sourceware.org/git/?p=libabigail.git;a=commit;h=2f92777dc8c34a7f84011fbd62811b3c5d076a1a

This is now reported as

 | Functions changes summary: 0 Removed, 1 Changed, 0 Added function
 | Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
 |
 | 1 function with some indirect sub-type change:
 |
 |   [C] 'method lego::lego()' at foo.h:10:1 has some indirect sub-type
changes:
 |     implicit parameter 0 of type 'lego*' has sub-type changes:
 |       in pointed to type 'struct lego' at foo.h:2:1:
 |         type size changed from 96 to 128 (in bits)
 |         2 data member changes:
 |           type of 'char lego::buggy_symbol[5]' changed:
 |             type name changed from 'char[5]' to 'char[10]'
 |             array type size changed from 40 to 80
 |             array type subrange 1 changed length from 5 to 10
 |           'int lego::payload' offset changed from 64 to 96 (in bits) (by +32
bits)


Hence closing.

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