Stack overflow due to calling superclass method inside virtual override

[HertzDevil]

I was doing work that required an overriding method to call the overridden method in the superclass (Qt::Object::eventFilter from qt5.cr to be exact), but to no avail. Suppose this class is in spec/integration/virtual_override_spec.cr:

class OverrideThing < Test::Base
  def calc(a, b) # (1)
    super * (a - b)
  end
end

# ...

OverrideThing.new.calc(10, 4).should eq(84) # (0)

This leads to a stack overflow:

...
from virtual_override_spec.cr:30:11 in 'calc'     # (1)
from tmp/virtual_override.cr:145:5 in '->'        # (6)
from _ZNK11CrystalProcIiJiiEEclEii                # (5)
from _ZN14BgInherit_Base4calcEii                  # (4)
from bg_Base_calc_int_int                         # (3)
from tmp/virtual_override.cr:131:7 in 'calc'      # (2)
from virtual_override_spec.cr:30:11 in 'calc'     # (1)
from tmp/virtual_override.cr:145:5 in '->'        # (6)
from _ZNK11CrystalProcIiJiiEEclEii                # (5)
from _ZN14BgInherit_Base4calcEii                  # (4)
from bg_Base_calc_int_int                         # (3)
from tmp/virtual_override.cr:131:7 in 'calc'      # (2)
from virtual_override_spec.cr:30:11 in 'calc'     # (1)
from virtual_override_spec.cr:78:11 in '->'       # (0)

The bodies of the relevant generated methods are:

module Test
  @[Link(ldflags: "#{__DIR__}/../tmp/virtual_override.o -lstdc++")]
  lib Binding
    fun bg_Base_calc_int_int(_self_ : Base*, a : Int32, b : Int32) : Int32 # (3)
  end

  class Base
    def initialize()
      # ...
      jump_table = Binding::BgJumptable_Base.new(
        bg_Base_calc_int_int: BindgenHelper.wrap_proc(
          Proc(Int32, Int32, Int32).new{|a, b| self.calc(a, b) } # (6)
        ),
        # ...
      )
      Binding.bg_BgInherit_Base_JUMPTABLE_BgJumptable_Base_R(result, pointerof(jump_table))
    end

    def calc(a : Int32, b : Int32) : Int32 # (2)
      Binding.bg_Base_calc_int_int(self, a, b)
    end
  end
end

struct BgInherit_Base : public Base {
  using Base::Base;
  BgJumptable_Base bgJump;
  int calc(int a, int b) override { // (4)
    BgInherit_Base *_self_ = this;
    if (_self_->bgJump.bg_Base_calc_int_int.isValid()) {
      return _self_->bgJump.bg_Base_calc_int_int(a, b);
    } else {
      return Base::calc(a, b);
    }
  }
};

extern "C" int bg_Base_calc_int_int(Base * _self_, int a, int b) { // (3)
  return _self_->calc(a, b);
}

template<typename T, typename ... Args>
struct CrystalProc {
  T operator()(Args ... arguments) const { // (5)
    if (this->self) {
      return this->withSelf(this->self, arguments...);
    } else {
      return this->withoutSelf(arguments...);
    }
  }
};

If the method is simply overridden, calls should stop at (1) immediately; if an override isn't provided, the jump table entry for calc would be invalid, so C++'s Base::calc would be invoked after (4). It looks like BgInherit_Base::calc always invokes the most derived calc method, so it cannot distinguish between super and a regular call.

[HertzDevil]

If the solution is to simply expose the superclass methods, this is what I came up with:

extern "C" int bg_Base_SUPER_calc_int_int(Base * _self_, int a, int b) {
  return _self_->Base::calc(a, b);
}

extern "C" int bg_Base_calc_int_int(Base * _self_, int a, int b) { // keep
  return _self_->calc(a, b);
}

module Test
  lib Binding
    fun bg_Base_SUPER_calc_int_int(_self_ : Base*, a : Int32, b : Int32) : Int32
    fun bg_Base_calc_int_int(_self_ : Base*, a : Int32, b : Int32) : Int32 # keep
  end

  class Base
    struct Superclass
      def initialize(@myself : Base)
      end

      def calc(a : Int32, b : Int32) : Int32
        Binding.bg_Base_SUPER_calc_int_int(@myself, a, b)
      end
    end

    def superclass
      Superclass.new(self)
    end

    def superclass(_type : Base.class)
      Superclass.new(self)
    end

    def calc(a : Int32, b : Int32) : Int32 # keep
      Binding.bg_Base_calc_int_int(self, a, b)
    end
  end
end

class OverrideThing < Test::Base
  def calc(a, b)
    superclass.calc(a, b) * (a - b)
    # following also works in Test::Subclass
    # superclass(Test::Base).calc(a, b) * (a - b)
  end
end

That is, for every non-pure abstract class, on the Crystal platform,

• define an inner struct called Superclass, which is simply a wrapper of the calling instance;
• define a method called superclass, which wraps the receiver instance (usually the caller itself);
• define an overload taking the class's type tag, so that generated subclasses of this class may still refer to this wrapper.

For every non-pure abstract method,

• on the Cpp platform, generate a clone that contains SUPER in the function name and always invokes the method from the class itself, preventing dynamic lookup or invoking the CrystalProc from the jump table;
• on the Binding platform, generate a fun clone that contains SUPER;
• on the Crystal platform, clone the method inside the Superclass struct, using the SUPER fun instead of the original one.

Then in the actual overriding method, rather than using super, the superclass method will provide direct access to those methods. It looks reasonably close to super, supports overloaded methods, and should work with multiple inheritance on both C++ and Crystal. super doesn't break inside initialize, by the way.

The main drawback is that generating all this extra boilerplate for every single abstract method might be overkill in very large codebases like qt5.cr, so classes might have to opt in for this feature using the YAML config files.

[Papierkorb]

Hello,

Oh dear, the virtual-override stuff was fun (for some good and some wrong) reasons to write back then :stuck_out_tongue:

Your solution sounds good to me. I'm wondering however if #superclass should be marked as protected as measure to avoid abuse. superclass is also a lot like Javas super giving out a "wrapper object" much like in your proposal.

Also, I see where you're coming from in terms of code bloat. I'd be interested in simply measuring how bad it is on Qt5, as I guess that it'll be the largest library we have for quite a while.

All in all your proposal is sound to me :+1: Cheers!

[HertzDevil]

Got it to work in 5f9e545. On my local qt5.cr installation, this generated 82 Superclass classes, 1999 SUPER lib funs, and 971 C++ SUPER wrappers when everything was enabled. The file size changes are as follows:

• src/binding/binding_$(BINDING_PLATFORM).cr: 3.2 MB -> 3.7 MB (+16.6%)
• ext/qt_binding_$(BINDING_PLATFORM).cpp: 1.8 MB -> 1.9 MB (+8.8%)
• ext/qt_binding_$(BINDING_PLATFORM).o: 5.8 MB -> 6.0 MB (+3.3%)
• ext/binding_$(BINDING_PLATFORM).a: 6.4 MB -> 6.6 MB (+3.7%)

Every class that derives from Qt::Object must include #meta_object, #qt_metacast, #qt_metacall, #event, #event_filter, #timer_event, #child_event, #custom_event, #connect_notify, and #disconnect_notify. The first 3 are internals for the Q_OBJECT macro and are presumably generated by moc, so users are not supposed to override them manually, and it wouldn't hurt if wrappers for these methods are disabled.

The virtual classes that don't derive from Qt::Object are PaintDevice, LayoutItem, SpacerItem, WidgetItem, GraphicsItem and subclasses, GraphicsItemGroup, and Surface (which has no concrete abstract methods).

I don't think the impact is so large that anything needs to be disabled, but if that's the case, my current plan is that types should be able to opt out of the superclass wrapper scheme by specifying the following in the YAML config files:

types:
  QObject:
    generate_superclass: false # default true
  QWidget:
    superclass_ignore_methods: [ "metaObject", "qt_metacast", "qt_metacall" ] # default [ ]

Then the Qt processor, which runs before VirtualOverride, automatically adds those 3 methods to the superclass_ignore_methods field of every class if necessary.

As mentioned in the PR, those Superclasses are currently still classes instead of structs. Also, the #superclass overloads that take a type argument are not added, because upon further thought this is now possible:

class MyWidget < Qt::Widget
  def event(_evt : Qt::Event) : Bool
    true # recognize every event, but do nothing
  end

  private def old_event(evt)
    superclass.event(evt)
  end
end

class SubWidget < MyWidget
  def event(evt : Qt::Event) : Bool
    old_event(evt) # calls Qt::Widget's version despite MyWidget overriding #event
  end
end

[Papierkorb]

Really good progress you have in #70, 👍 to that! My notes below:

On my local qt5.cr installation, this generated 82 Superclass classes, 1999 SUPER lib funs, and 971 C++ SUPER wrappers when everything was enabled. The file size changes are as follows:

src/binding/binding_$(BINDING_PLATFORM).cr: 3.2 MB -> 3.7 MB (+16.6%) ext/qtbinding$(BINDING_PLATFORM).cpp: 1.8 MB -> 1.9 MB (+8.8%) ext/qtbinding$(BINDINGPLATFORM).o: 5.8 MB -> 6.0 MB (+3.3%) ext/binding$(BINDING_PLATFORM).a: 6.4 MB -> 6.6 MB (+3.7%)

I think that's fine. The Crystal compiler should discard unused Crystal methods on its side, and the linker discard unused symbols from C++ land. So my only concern would be compile time implosion, is it much worse for you? Considering the opt-out possibility there's more to win than to lose I think.

types:
  QObject:
    generate_superclass: false # default true
  QWidget:
    superclass_ignore_methods: [ "metaObject", "qt_metacast", "qt_metacall" ] # default [ ]

These fields should be regexp's. They're not much more expensive but are easier to deal with in anticipation of future changes of a library unknown to us yet.

As mentioned in the PR, those Superclasses are currently still classes instead of structs. Also, the #superclass overloads that take a type argument are not added [...]

I see this as optional additional feature for now. It's required at some point to wrap stuff like this:

struct A { virtual void foo(); };
struct B { virtual void foo(); };
struct C : public A, virtual B { };

iirc, in terms of Qt, there was a single class making use of this, which is so rarely used I forgot what it's called. So, for Qt, it's not that interesting. No idea if some popular C++ lib out there does.

All in all this looks really good, thank you for working on this! 👍