[Bug default/26672] New: detection of changes to an abstract base class

[Bug default/26672] New: detection of changes to an abstract base class

[gprocida+abigail at google dot com]

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

            Bug ID: 26672
           Summary: detection of changes to an abstract base class
           Product: libabigail
           Version: unspecified
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: default
          Assignee: dodji at redhat dot com
          Reporter: gprocida+abigail at google dot com
                CC: libabigail at sourceware dot org
  Target Milestone: ---

Related bug:

Bug 20476: abidiff doesn't report incompatibility for abstract class when
implementation has hidden visibility

There was some discussion about libabigail's handling of interface classes have
only pure virtual methods.

I did some investigation around the example that was given to me.

$ cat virtual00_cxx.0.cc
class MyCallback {
public:
  virtual void event1() = 0;
  virtual void event2() = 0;
};

class MyClass {
public:
  void doSomething() {cb->event1(); cb->event2();}
  void registerCallback(MyCallback* cb) {this->cb = cb;}
private:
  MyCallback* cb;
};

static MyClass a;

$ cat virtual00_cxx.1.cc
class MyCallback {
public:
  virtual void event1() = 0;
  virtual void event2() = 0;
  virtual void event3() = 0; // added
};

class MyClass {
public:
  void doSomething() {cb->event1(); cb->event2(); cb->event3();}
  void registerCallback(MyCallback* cb) {this->cb = cb;}
private:
  MyCallback* cb;
};

static MyClass a;

$ for x in 0 1; do g++ -Wall -Wextra -g -ggdb -fno-eliminate-unused-debug-types
-g3 -c virtual00_cxx.$x.cc; dwarfdump virtual00_cxx.$x.o >
virtual00_cxx.$x.dwarf; done

Rather as expected, the DWARF dumps have no information about the event
functions. I couldn't find an option to get GCC to emit this.

I then defined virtual destructors for MyCallback. Now the event functions
appear in the DWARF.

Less expectedly, abidiff --harmless still sees no difference. However, that's
because the variables "a" are static.

Assuming this is not completely artificial, what do you the best approach to
picking up this kind of ABI difference?

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

[Bug default/26672] detection of changes to an abstract base class

[dodji at redhat dot com]

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

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

From what I understand, this is rather related to showing changes to an
abstract class that contains only pure virtual functions (i.e, a pure
interface class).

yes, GCC doesn't generate any debug info for the pure interface class as
it is.  What does LLVM do in that case btw?

I think, though, that if there was a concrete implementation of that
interface (a class deriving from that interface and implementing its
virtual functions) that could be instantiated, then GCC emits debug info
for that implementation, including debug info describing the pure
interface class).  In that case, Libabigail should detect the interface
change (addition of a virtual function) on that concrete implementation
of the interface.  If there is no such code (instantiating a concrete
implementation of that interface) anywhere in the source code, then I am
genuiously curious to know how the code in its whole is supposed to
work.  I mean, surely, something needs to instantiate a concrete
implementation of that interface at some point, right?  In any case, by
knowing more about that use case, maybe we can come up with a scheme to
catch the change.

So, here is the change I made to the your example to try to catch the
addition and illustrate what I mean by "concrete implementation of the
interface":

-------------------------------->8<----------------------------------
$ cat v0.cc
class MyCallback {
public:
  virtual void event1() = 0;
  virtual void event2() = 0;
};

class MyClass {
public:
  void doSomething() {cb->event1(); cb->event2();}
  void registerCallback(MyCallback* cb) {this->cb = cb;}
private:
  MyCallback* cb;
};

static MyClass a;

class Event : public MyCallback
{
  void event1() {}
  void event2() {}
};

MyCallback*
make_new_callback()
{
  MyCallback *result = new Event;
  return result;
}
$ 
$ 
$ cat v1.cc
class MyCallback {
public:
  virtual void event1() = 0;
  virtual void event2() = 0;
  virtual void event3() = 0; // added
};

class MyClass {
public:
  void doSomething() {cb->event1(); cb->event2(); cb->event3();}
  void registerCallback(MyCallback* cb) {this->cb = cb;}
private:
  MyCallback* cb;
};

static MyClass a;

class Event : public MyCallback
{
  void event1() {}
  void event2() {}
  void event3() {}
};

MyCallback*
make_new_callback()
{
  MyCallback *result = new Event;
  return result;
}

$ 
$ g++ -g -c v0.cc
$ g++ -g -c v1.cc
$
$ abidiff v0.o v1.o
Functions changes summary: 0 Removed, 1 Changed, 1 Added functions
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

1 Added function:

  [A] 'method virtual void Event::event3()'    {_ZN5Event6event3Ev}
    note that this adds a new entry to the vtable of class Event

1 function with some indirect sub-type change:

  [C] 'function MyCallback* make_new_callback()' at v1.cc:26:1 has some
indirect sub-type changes:
    return type changed:
      in pointed to type 'class MyCallback' at v1.cc:1:1:
        type size hasn't changed
        1 member function insertion:
          'method virtual void MyCallback::event3()' at v1.cc:5:1, virtual at
voffset 2/2

$ 

-------------------------------->8<----------------------------------

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

[Bug default/26672] detection of changes to an abstract base class

[jiyong at google dot com]

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

--- Comment #2 from jiyong at google dot com ---
>  I am genuiously curious to know how the code in its whole is supposed to
work

The abstract class `MyCallback` is expected to be implemented by a client, not
by the library that is implementing `MyClass`.

I see that abidiff works as expected for the C-equivalent of the example.

~/temp $ cat foo1.c

struct MyCallback {
  void (*event1)();
  void (*event2)();
};

static struct MyCallback* saved_cb;

void doSomething() { saved_cb->event1(); saved_cb->event2(); }
void registerCallback(struct MyCallback *cb) { saved_cb = cb;}

~/temp $ cat foo2.c

struct MyCallback {
  void (*event1)();
  void (*event2)();
  void (*event3)();
};

static struct MyCallback* saved_cb;

void doSomething() { saved_cb->event1(); saved_cb->event2();
saved_cb->event3(); }
void registerCallback(struct MyCallback *cb) { saved_cb = cb;}

~/temp $ gcc -c -g foo1.c -o out1.o
~/temp $ gcc -c -g foo2.c -o out2.o
~/temp $ abidiff out1.o out2.o
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] 'function void registerCallback(MyCallback*)' at foo2.c:11:1 has some
indirect sub-type changes:
    parameter 1 of type 'MyCallback*' has sub-type changes:
      in pointed to type 'struct MyCallback' at foo2.c:2:1:
        type size changed from 128 to 192 (in bits)
        1 data member insertion:
          'void (variadic parameter type)* MyCallback::event3', at offset 128
(in bits) at foo2.c:5:1

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

[Bug default/26672] detection of changes to an abstract base class

[dodji at redhat dot com]

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

--- Comment #3 from dodji at redhat dot com ---
"jiyong at google dot com" <sourceware-bugzilla@sourceware.org> writes:

> https://sourceware.org/bugzilla/show_bug.cgi?id=26672
>
> --- Comment #2 from jiyong at google dot com ---
>>  I am genuiously curious to know how the code in its whole is supposed to
> work
>
> The abstract class `MyCallback` is expected to be implemented by a client, not
> by the library that is implementing `MyClass`.

Ahh, okay, I see.

So, maybe in that case, you might want to try to use the GCC option
-femit-class-debug-always.

The problem with that option is that it duplicates debug info for
classes, making the whole thing potentially much bigger.

Otherwise, the debug info for a vtable would be emitted only in files
where the virtual method are emitted.  Look for "key method" at
https://gcc.gnu.org/onlinedocs/gcc-10.2.0/gcc/Vague-Linkage.html#Vague-Linkage
to understand the details.

Here is what I am getting with -femit-class-debug-always:

$ cat v0.cc
class MyCallback {
public:
  virtual void event1() = 0;
  virtual void event2() = 0;
};

class MyClass {
public:
  void doSomething() {cb->event1(); cb->event2();}
  void registerCallback(MyCallback* cb) {this->cb = cb;}
private:
  MyCallback* cb;
};

static MyClass a;

void
foo(MyClass&)
{
}
dodji@ayevide:PR26672$ cat v1.cc
class MyCallback {
public:
  virtual void event1() = 0;
  virtual void event2() = 0;
  virtual void event3() = 0; // added
};

class MyClass {
public:
  void doSomething() {cb->event1(); cb->event2(); cb->event3();}
  void registerCallback(MyCallback* cb) {this->cb = cb;}
private:
  MyCallback* cb;
};

static MyClass a;

void
foo(MyClass&)
{
}
$ 
$ 
$ g++ -g -femit-class-debug-always -c v0.cc
$ g++ -g -femit-class-debug-always -c v1.cc
$ 
$ 
$ abidiff v0.o v1.o
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] 'function void foo(MyClass&)' at v1.cc:19:1 has some indirect sub-type
changes:
    parameter 1 of type 'MyClass&' has sub-type changes:
      in referenced type 'class MyClass' at v1.cc:8:1:
        type size hasn't changed
        1 data member change:
          type of 'MyCallback* MyClass::cb' changed:
            in pointed to type 'class MyCallback' at v1.cc:1:1:
              type size hasn't changed
              1 member function insertion:
                'method virtual void MyCallback::event3()' at v1.cc:5:1,
virtual at voffset 2/2

$ 

Would that help you?

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

[Bug default/26672] detection of changes to an abstract base class

[jiyong at google dot com]

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

--- Comment #4 from jiyong at google dot com ---
Thanks a lot! That worked.

I have one complaint though (sorry!). In your modified example, it looks like
the existence of `void foo(MyClass&){}` is the critical part. When I had that
in the source files, `-femit-class-debug-always` wasn't actually needed. When I
omit the function definition, the flag was useless.

It looks like I need at least one function or a variable that ...

1) is defined outside of the class def and
2) has MyClass in its signature

A few experiments:

-------- DIDN'T WORK---------------
class MyClass {
   ...
    void foo(){} // doesn't have MyClass in the signature
}; 

-------- DIDN'T WORK---------------
class MyClass {
    ...
     void foo(MyClass*){} // inline definition
};

-------- WORKED ---------------
class MyClass {
     ...
     void foo(MyClass*);
};
void MyClass::foo(MyClass*){} // has MyClass as signature, and defined outside
of the class


-------- DIDN'T WORK---------------
class MyClass {
     ...
     void foo();
};
void MyClass::foo() {} // defined outside of the class, but doesn't have
MyClass as signature

-------- WORKED ---------------
class MyClass {
     ...
};
void foo(MyClass*){} // your example

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

[Bug default/26672] detection of changes to an abstract base class

[dodji at redhat dot com]

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

--- Comment #5 from dodji at redhat dot com ---
"jiyong at google dot com" <sourceware-bugzilla@sourceware.org> writes:

> Thanks a lot! That worked.

Cool!

> I have one complaint though (sorry!).

No problem :-)

> In your modified example, it looks like
> the existence of `void foo(MyClass&){}` is the critical part. When I had that
> in the source files, `-femit-class-debug-always` wasn't actually needed. When I
> omit the function definition, the flag was useless.

Right.

> It looks like I need at least one function or a variable that ...
>
> 1) is defined outside of the class def and
> 2) has MyClass in its signature

In summary, it must be a function or a variable which ELF symbol is
publicly exported by the binary, and which uses the type (class) in its
signature, indeed.

In other words, the type must be part of the ABI of the binary, for it
to be taken into account in the analysis.  By default, we try to make it
so that the analysis doesn't take into account types that are 'internal'
to the binary, from an ABI standpoint.

>
> A few experiments:
>
> -------- DIDN'T WORK---------------
> class MyClass {
>    ...
>     void foo(){} // doesn't have MyClass in the signature
> }; 
>
> -------- DIDN'T WORK---------------
> class MyClass {
>     ...
>      void foo(MyClass*){} // inline definition
> };
>
> -------- WORKED ---------------
> class MyClass {
>      ...
>      void foo(MyClass*);
> };
> void MyClass::foo(MyClass*){} // has MyClass as signature, and defined outside
> of the class
>
>
> -------- DIDN'T WORK---------------
> class MyClass {
>      ...
>      void foo();
> };
> void MyClass::foo() {} // defined outside of the class, but doesn't have
> MyClass as signature
>
> -------- WORKED ---------------
> class MyClass {
>      ...
> };
> void foo(MyClass*){} // your example

These examples does seem to make sense to me.

Is this suprising to you?  How would you want it to be otherwise?

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

[Bug default/26672] detection of changes to an abstract base class

[jiyong at google dot com]

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

--- Comment #6 from jiyong at google dot com ---
> 1) is defined outside of the class def and
> 2) has MyClass in its signature

Among these two conditions, #1 is understandable because otherwise the function
will be part of the clients. However, #2 doesn't seem to make sense (at least
to me) because member functions of a class have `MyClass*` as the implicit
argument, but that doesn't seem to be not considered as part of the ABI by
libabigail.

> Is this suprising to you?  How would you want it to be otherwise?

The below case is against my expectation.

-------- DIDN'T WORK---------------
class MyCallback {
public:
  virtual void event1()=0;
  virtual void event2()=0;
  virtual void event3()=0;
};
class MyClass {

     void foo();
};
void MyClass::foo() {} // defined outside of the class, but doesn't have
MyClass as signature

As a naive user, I expect that libabigail will warn me when there is a change
that might break my existing C++ clients which were built before the change.
Let me give you a more concrete example.

~/temp $ cat old/test.h                                                         
class MyCallback {                                                              
public:
 virtual void event1()=0;
 virtual void event2()=0;
};

class MyClass {
public:
  void registerCallback(MyCallback* cb) {this->cb = cb;}
  void doSomething();
private:
  MyCallback* cb;
};

~/temp $ cat old/test.cpp
#include "test.h"
void MyClass::doSomething() {cb->event1(); cb->event2(); }

// Let's build the old library and link our client with it.
~/temp $ cd old
~/temp/old $ g++ -c -g -femit-class-debug-always test.cpp
~/temp/old $ g++ test.o -shared -o libtest.so 
~/temp/old $ cd ..
~/temp $ g++ client.cpp -I old -L old -ltest
~/temp $ LD_LIBRARY_PATH=./old ./a.out                                          
event1
event2

// Works as expected. 
// Then let's modify the library by adding event3().
~/temp $ cat new/test.h
class MyCallback {
public:
 virtual void event1()=0;
 virtual void event2()=0;
 virtual void event3()=0; // added
};

class MyClass {
public:
  void registerCallback(MyCallback* cb) {this->cb = cb;}
  void doSomething();
private:
  MyCallback* cb;
};

~/temp $ cat new/test.cpp
#include "test.h"
void MyClass::doSomething() {cb->event1(); cb->event2(); cb->event3();} // call
event3() as well

// Build the new library and run the "old" client with the new library.
~/temp $ cd new
~/temp/new $ g++ -c -g -femit-class-debug-always test.cpp
~/temp/new $ g++ test.o -shared -o libtest.so 
~/temp/new $ cd ..
~/temp $ LD_LIBRARY_PATH=./new ./a.out                                          
event1
event2
Segmentation fault

// The failure is as expected because event3() is not implemented by the old
client. This means that the change from old to new is indeed an ABI breaking
change anyway.

~/temp $ abidiff old/test.o new/test.o

// But abidiff shows nothing. It starts to report the diff when I explicitly
add an unused `MyClass*` argument to doSomething.

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

[Bug default/26672] detection of changes to an abstract base class

[dodji at redhat dot com]

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

--- Comment #7 from dodji at redhat dot com ---
"jiyong at google dot com" <sourceware-bugzilla@sourceware.org> writes:

> https://sourceware.org/bugzilla/show_bug.cgi?id=26672
>
> --- Comment #6 from jiyong at google dot com ---
>> 1) is defined outside of the class def and
>> 2) has MyClass in its signature

[...]

> #2 doesn't seem to make sense (at least to me) because member
> functions of a class have `MyClass*` as the implicit argument, but
> that doesn't seem to be not considered as part of the ABI by
> libabigail.

Ahh, I see.  You are definitely right.  Yes, we've been consciously
avoiding the implicit 'this' argument of methods for a while now due to
some internal difficulties.

I'll revisit this issue now.  I think the proper infrastructure should
be in place now to support that implicit argument now.

I'll be working on a patch for this then and I'll let you know.

[...]

Thanks.

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

[Bug default/26672] detection of changes to an abstract base class

[jiyong at google dot com]

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

--- Comment #8 from jiyong at google dot com ---
That's great. Thanks a lot!

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

[Bug default/26672] detection of changes to an abstract base class

[dodji at redhat dot com]

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

dodji at redhat dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2020-10-06
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |ASSIGNED

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

[Bug default/26672] Implicit 'this' parameter not taken in account during member function comparison

[dodji at redhat dot com]

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

dodji at redhat dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|detection of changes to an  |Implicit 'this' parameter
                   |abstract base class         |not taken in account during
                   |                            |member function comparison

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

[Bug default/26672] Implicit 'this' parameter not taken in account during member function comparison

[dodji at redhat dot com]

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

dodji at redhat dot com changed:

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

--- Comment #9 from dodji at redhat dot com ---
The commit
https://sourceware.org/git/?p=libabigail.git;a=commit;h=2f92777dc8c34a7f84011fbd62811b3c5d076a1a
in the master branch should hopefully address the issue of ignoring the type of
the implicit "this" parameter of a member function.

So, it should now be possible to detect a change to a class by analyzing the
member functions.

Thanks for taking the time to report this issue and sorry for the
inconvenience.

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

[Bug default/26672] Implicit 'this' parameter not taken in account during member function comparison

[jiyong at google dot com]

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

--- Comment #10 from jiyong at google dot com ---
This works! I could verify that the ToT libabigail correctly fails with my last
example. 

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 void MyClass::doSomething()' at test.h:11:1 has some indirect
sub-type changes:
    implicit parameter 0 of type 'MyClass*' has sub-type changes:
      in pointed to type 'class MyClass' at test.h:8:1:
        type size hasn't changed
        1 data member change:
          type of 'MyCallback* MyClass::cb' changed:
            in pointed to type 'class MyCallback' at test.h:1:1:
              type size hasn't changed
              1 member function insertion:
                'method virtual void MyCallback::event3()' at test.h:5:1,
virtual at voffset 2/2
              no member function changes (2 filtered);

Thank you Dogji for your quick fix and also for being very open to the report.

> sorry for the inconvenience

Not at all. There was zero inconvenience. 
Again, thank you for your support. Good bye!

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