After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 706712 - [ABI] Vala Interface+Abstract class pair creates double indirection
[ABI] Vala Interface+Abstract class pair creates double indirection
Status: RESOLVED OBSOLETE
Product: vala
Classification: Core
Component: Code Generator
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Vala maintainers
Vala maintainers
Depends on: 706998
Blocks:
 
 
Reported: 2013-08-24 14:53 UTC by Maciej (Matthew) Piechotka
Modified: 2018-05-22 14:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Vala test.c (11.57 KB, text/x-csrc)
2013-08-28 15:23 UTC, Maciej (Matthew) Piechotka
Details

Description Maciej (Matthew) Piechotka 2013-08-24 14:53:44 UTC
Vala creates double indirection when implementing interface by virtual methods. This was discussed on IRC and by blog post[1] however the original solution is incorrect - I put the discussion here to put the arguments etc. in one place (sorry for any disorganization it might cause but it is good to also have the invalid arguments documented).

The proposed solution was to update the both abstract method as well as interface in class but this changes both ABI and C API as the subclasses need to do the same. Let say we have interface I with method f, abstract class A implementing it and classes B and C (where C extends B, which extends A) where only B is compiled using the new method. it will override the I.f and A.f. C.f will override A.f but not I.f. Therefore calling method f on C will call B.f instead of C.f.

Possible solution would be to have a dynamic fix in abstract class. I.e. instead of adding simply:

static void gee_abstract_list_gee_list_interface_init (GeeListIface * iface) {
	(...)
	iface->set = (void (*)(GeeList*, gint, gconstpointer)) gee_abstract_list_set;
	(...)
}

void gee_abstract_list_set (GeeAbstractList* self, gint index, gconstpointer item) {
	g_return_if_fail (self != NULL);
	GEE_ABSTRACT_LIST_GET_CLASS (self)->set (self, index, item);
}

Implement it in following way:

static void gee_abstract_list_gee_list_interface_init (GeeListIface * iface) {
	(...)
	iface->set = (void (*)(GeeList*, gint, gconstpointer)) gee_abstract_list_impl_set;
	(...)
}

static void
gee_abstract_list_impl_set (GeeAbstractList* self, gint index, gconstpointer item) {
	void (*real)(GeeList*, gint, gconstpointer);
	real = GEE_ABSTRACT_LIST_GET_CLASS (self)->set;
	GEE_LIST_GET_INTERFACE (self)->set = real;
	real (self, index, item);
}

void gee_abstract_list_set (GeeAbstractList* self, gint index, gconstpointer item) {
	g_return_if_fail (self != NULL);
	GEE_ABSTRACT_LIST_GET_CLASS (self)->set (self, index, item);
}

This however cause other problem - it would be yet another violation of the strict C11 spec[2] for multi-thread access. While it should not cause problem on major architectures (x86, arm, ...) it might on architectures where word access is not atomic[3] (DEC Alpha for example). Using traditional atomics is too expensive and likely they would overcome any potential benefits due to cost of memory barrier. However the C11 primitives allows to avoid the barrier entirely:

// list.c
void gee_list_set (GeeList* self, gint index, gconstpointer item) {
	void (*real)(GeeList*, gint, gconstpointer);
	g_return_if_fail (self != NULL);
	real = atomic_load_explicit (&GEE_LIST_GET_INTERFACE (self)->set, memory_order_relaxed);
	&GEE_LIST_GET_INTERFACE (self)->set (self, index, item);
}

// abstractlist.c
static void gee_abstract_list_gee_list_interface_init (GeeListIface * iface) {
	(...)
	iface->set = ATOMIC_INIT ((void (*)(GeeList*, gint, gconstpointer)) gee_abstract_list_impl_set);
	(...)
}

static void
gee_abstract_list_impl_set (GeeAbstractList* self, gint index, gconstpointer item) {
	void (*real)(GeeList*, gint, gconstpointer);
	real = GEE_ABSTRACT_LIST_GET_CLASS (self)->set;
	atomic_store_explicit (GEE_LIST_GET_INTERFACE (self)->set = real, real, memory_order_relaxed);
	real (self, index, item);
}

void gee_abstract_list_set (GeeAbstractList* self, gint index, gconstpointer item) {
	g_return_if_fail (self != NULL);
	GEE_ABSTRACT_LIST_GET_CLASS (self)->set (self, index, item);
}

This on the other hand requires changing the interface both in terms of implementation of gee_list_set as well as the C API and possibly ABI (depending on implementation of compiler). Furthermore the standard-compilant stdthread.h is not present on current systems and relevant optimizations have been introduced only in newest compilers.

The possible solution is to extend GObject. It would be similar to g_type_add_interface_check[4] only called after a abstract class implementation instead of interface.

[1] http://blog.piechotka.com.pl/2013/05/18/vala-abi-and-branch-prediction/
[2] Other problems include, for example, storing integer in pointer.
[3] Here atomic does not mean synchronized. It corresponds to relaxed memory ordering in C11 standard - i.e. it is guaranteed that the value read is one of values written (so there is no half-written words). It is often hard to find this information as usually 'atomic' refers to more old-fashioned 
[4] https://developer.gnome.org/gobject/2.34/gobject-Type-Information.html#g-type-add-interface-check
Comment 1 Maciej (Matthew) Piechotka 2013-08-28 15:23:40 UTC
Created attachment 253403 [details]
Vala test.c

After investigation it looks like the vtable is per interface implementation not per instance so the above proposal won't work. I manage to work a trick implementation which will not work with glib 2.36+ however (bug #687659). It might be good to talk with glib devs about it).

The attached code is solution when to abstract class a base_init is added and adds the interface (it should be more sophisticated and check for example if the interface is not implemented, handle plugins properly etc. but it should be doable without breaking API/ABI and possibly extending it).

static void acl_child_iface_interface_init (IFaceIface * iface) {
        AClClass *class;
        class = g_type_class_ref (((GTypeInterface *)iface)->g_instance_type);
        iface->f = class->f;
        g_type_class_unref (class);
}

static void acl_base_init (AClClass * klass) {
        if (G_TYPE_FROM_CLASS (klass) != TYPE_ACL) {
                static const GInterfaceInfo iface_info = { (GInterfaceInitFunc) acl_child_iface_interface_init, (GInterfaceFinalizeFunc) NULL, NULL};
                g_type_add_interface_static (G_TYPE_FROM_CLASS (klass),
                                             TYPE_IFACE,
                                             &iface_info);
        }
}

Test program:

interface IFace : Object {
    public abstract void f ();
}

abstract class ACl : Object, IFace {
    public abstract void f ();
}

class BCl : ACl {
    public override void f () {
       stderr.printf ("BCl.f ()\n");
    }
}

class CCl : BCl {
    public override void f () {
       stderr.printf ("CCl.f ()\n");
    }
}

class DCl : CCl {
    public override void f () {
       stderr.printf ("DCl.f ()\n");
    }
}

int main () {
    stderr.printf (">>> BCl\n");
    IFace b = new BCl ();
    b.f ();
    stderr.printf (">>> CCl\n");
    IFace c = new CCl ();
    c.f ();
    stderr.printf (">>> DCl\n");
    IFace d = new DCl ();
    d.f ();
    stderr.printf (">>> DCl\n");
    b = new BCl ();
    b.f ();
    return 0;
}

The output from (modified) test program:

>>> BCl
acl_class_init: BCl
acl_child_iface_interface_init: IFace - BCl
    >>> 0x238ae00->0x400e60
iface(0x238ae00)->f
BCl.f ()
>>> CCl
acl_class_init: CCl
acl_child_iface_interface_init: IFace - CCl
    >>> 0x238b140->0x4010f4
iface(0x238b140)->f
CCl.f ()
>>> DCl
acl_class_init: DCl
acl_child_iface_interface_init: IFace - DCl
    >>> 0x2385d20->0x401231
iface(0x2385d20)->f
DCl.f ()
>>> BCl
iface(0x238ae00)->f
BCl.f ()
Comment 2 Maciej (Matthew) Piechotka 2014-07-26 14:44:20 UTC
After talking with Ryan I found possible workaround (it looks like GLib folks don't want to 'fix' bug 706998 for obvious reasons). It looks like since 0.25.x it can be nearly entirely implementable on the side of library. It would not fully work with virtual methods in interfaces in automatic way.

The idea is to leverage an explicit interface method implementation. Consider:

interface IFace : Object {
    public abstract void f ();
    public virtual void g () {
        stdout.printf("IFace.g ()\n");
    }
}

abstract class ACl : Object, IFace {
    public abstract void f ();
    public virtual void g () {
        stdout.printf("ACl.g ()\n");
    }
}

class BCl : ACl {
    public override void f () {
         stderr.printf ("BCl.f ()\n");
    }
}

class CCl : BCl, IFace {
    public override void f () {
       stderr.printf ("CCl.f ()\n");
    }
    public override void IFace.f () {
       if (this.get_type () != typeof(CCl)) {
           stderr.printf ("CCl.Iface.f () [fake]\n");
           ((ACl)this).f ();
           return;
       }
       stderr.printf ("CCl.Iface.f ()\n");
    }
}

class DCl : CCl {
    public override void f () {
       stderr.printf ("DCl.f ()\n");
    }
    public override void g () {
       stdout.printf ("DCl.g ()\n");
    }
}

int main () {
    stderr.printf (">>> BCl\n");
    IFace b = new BCl ();
    b.f ();
    b.g ();
    stderr.printf (">>> CCl\n");
    IFace c = new CCl ();
    c.f ();
    c.g ();
    stderr.printf (">>> DCl\n");
    IFace d = new DCl ();
    d.f ();
    d.g ();
    stderr.printf (">>> BCl\n");
    b = new BCl ();
    b.f ();
    b.g ();
    return 0;
}

Output:
>>> BCl
BCl.f ()
ACl.g ()
>>> CCl
CCl.Iface.f ()
ACl.g ()
>>> DCl
CCl.Iface.f () [fake]
DCl.f ()
DCl.g ()
>>> BCl
BCl.f ()
ACl.g ()

The problem with ACl.g () appearing as last element in output is the mentioned before problem as BCl misses the implementation - BCL needs to have an overload for all possible methods in parent (including virtuals which might be extended in future).

if:
 - A is subclass of B
 - B implements interface I and have abstract/virtual i
 - A overloads function f from interface I
 - A, B and I are in the same library (this condition can be relaxed but not in a straightforward way)
then it might make sense to make A implement I in a way described above automatically by Vala. This would not break API/ABI, did not involve modification of glib, allow subclassing of A and avoid problem of 'missing virtual functions' as A and B are compiled at the same time so it's not possible to 'add' the virtual function to I without recompiling A and B.
Comment 3 Maciej (Matthew) Piechotka 2014-07-28 14:06:41 UTC
Adding to see also related bug in interface overloading.
Comment 4 GNOME Infrastructure Team 2018-05-22 14:55:48 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/vala/issues/401.