genbindings/subclassing: accurate pointer type management for subclasses

cmd/genbindings/config-allowlist.go

@@ -187,6 +187,11 @@ func AllowSignal(mm CppMethod) bool {
 }
 
 func AllowVirtual(mm CppMethod) bool {
+
+	if mm.MethodName == "metaObject" || mm.MethodName == "qt_metacast" {
+		return false
+	}
+
 	return true // AllowSignal(mm)
 }
 

cmd/genbindings/emitcabi.go

@@ -858,18 +858,11 @@ func emitBindingCpp(src *CppParsedHeader, filename string) (string, error) {
 						returnTransformP, returnTransformF = emitCABI2CppForwarding(returnParam, "\t\t")
 					}
 
-					paramArgs := []string{"handle__" + m.SafeMethodName()}
-
-					var signalCode string
-
-					for i, p := range m.Parameters {
-						signalCode += emitAssignCppToCabi(fmt.Sprintf("\t\t%s sigval%d = ", p.RenderTypeCabi(), i+1), p, p.ParameterName)
-						paramArgs = append(paramArgs, fmt.Sprintf("sigval%d", i+1))
-					}
+					handleVarname := "handle__" + m.SafeMethodName()
 
 					ret.WriteString(
 						"\t// cgo.Handle value for overwritten implementation\n" +
-							"\tintptr_t handle__" + m.SafeMethodName() + " = 0;\n" +
+							"\tintptr_t " + handleVarname + " = 0;\n" +
 							"\n",
 					)
 
@@ -878,9 +871,10 @@ func emitBindingCpp(src *CppParsedHeader, filename string) (string, error) {
 
 					ret.WriteString(
 						"\t// Subclass to allow providing a Go implementation\n" +
-							"\tvirtual " + m.ReturnType.RenderTypeQtCpp() + " " + m.CppCallTarget() + "(" + emitParametersCpp(m) + ") " + ifv(m.IsConst, "const ", "") + "override {\n" +
-							"\t\tif (handle__" + m.SafeMethodName() + " == 0) {\n",
+							"\tvirtual " + m.ReturnType.RenderTypeQtCpp() + " " + m.CppCallTarget() + "(" + emitParametersCpp(m) + ") " + ifv(m.IsConst, "const ", "") + "override {\n",
 					)
+
+					ret.WriteString("\t\tif (" + handleVarname + " == 0) {\n")
 					if m.IsPureVirtual {
 						if m.ReturnType.Void() {
 							ret.WriteString("\t\t\treturn; // Pure virtual, there is no base we can call\n")
@@ -894,9 +888,29 @@ func emitBindingCpp(src *CppParsedHeader, filename string) (string, error) {
 							ret.WriteString("\t\t\treturn;\n")
 						}
 					}
+					ret.WriteString("\t\t}\n")
+
+					paramArgs := []string{}
+					if m.IsConst {
+						// We're calling a Cgo-exported function, but Cgo can't
+						// describe a const pointer to a custom class, unless
+						// it's a primitive or wrapped in a typedef.
+						// Just strip the const_cast away
+						paramArgs = append(paramArgs, "const_cast<"+overriddenClassName+"*>(this)")
+					} else {
+						paramArgs = append(paramArgs, "this")
+					}
+					paramArgs = append(paramArgs, handleVarname)
+
+					var signalCode string
+
+					for i, p := range m.Parameters {
+						signalCode += emitAssignCppToCabi(fmt.Sprintf("\t\t%s sigval%d = ", p.RenderTypeCabi(), i+1), p, p.ParameterName)
+						paramArgs = append(paramArgs, fmt.Sprintf("sigval%d", i+1))
+					}
+
 					ret.WriteString(
-						"\t\t}\n" +
-							"\t\t\n" +
+						"\t\t\n" +
 							signalCode + "\n" +
 							"\t\t" + maybeReturn2 + "miqt_exec_callback_" + methodPrefixName + "_" + m.SafeMethodName() + "(" + strings.Join(paramArgs, `, `) + ");\n" +
 							returnTransformP + "\n" +
@@ -946,39 +960,40 @@ func emitBindingCpp(src *CppParsedHeader, filename string) (string, error) {
 
 		for i, ctor := range c.Ctors {
 
+			// The returned ctor needs to return a C++ pointer for not just the
+			// class itself, but also all of the inherited base classes
+			// That's because C++ virtual inheritance shifts the pointer; we
+			// need all the base pointers to call base methods from CGO
+			// Supply them all as out-parameters so we only need one roundtrip
+
 			preamble, forwarding := emitParametersCABI2CppForwarding(ctor.Parameters, "\t")
 
+			ret.WriteString(
+				"void " + methodPrefixName + "_new" + maybeSuffix(i) + "(" + emitParametersCabiConstructor(&c, &ctor) + ") {\n",
+			)
+
 			if ctor.LinuxOnly {
-
-				ret.WriteString(fmt.Sprintf(
-					"%s* %s_new%s(%s) {\n"+
-						"#ifdef Q_OS_LINUX\n"+
-						"%s"+
-						"\treturn new %s(%s);\n"+
-						"#else\n"+
-						"\treturn nullptr;\n"+
-						"#endif\n"+
-						"}\n"+
-						"\n",
-					methodPrefixName, methodPrefixName, maybeSuffix(i), emitParametersCabi(ctor, ""),
-					preamble,
-					cppClassName, forwarding,
-				))
-
-			} else {
-				ret.WriteString(fmt.Sprintf(
-					"%s* %s_new%s(%s) {\n"+
-						"%s"+
-						"\treturn new %s(%s);\n"+
-						"}\n"+
-						"\n",
-					methodPrefixName, methodPrefixName, maybeSuffix(i), emitParametersCabi(ctor, ""),
-					preamble,
-					cppClassName, forwarding,
-				))
-
+				ret.WriteString(
+					"#ifndef Q_OS_LINUX\n" +
+						"\treturn;\n" +
+						"#endif\n",
+				)
 			}
 
+			ret.WriteString(
+				preamble +
+					"\t" + cppClassName + "* ret = new " + cppClassName + "(" + forwarding + ");\n" + // Subclass class name
+					"\t*outptr_" + cabiClassName(c.ClassName) + " = ret;\n", // Original class name
+			)
+			for _, baseClass := range c.AllInherits() {
+				ret.WriteString("\t*outptr_" + baseClass + " = static_cast<" + baseClass + "*>(ret);\n")
+			}
+
+			ret.WriteString(
+				"}\n" +
+					"\n",
+			)
+
 		}
 
 		for _, m := range c.Methods {
@@ -1088,13 +1103,15 @@ func emitBindingCpp(src *CppParsedHeader, filename string) (string, error) {
 		// Virtual override helpers
 		for _, m := range virtualMethods {
 
-			// Virtual methods:
-			// 1. Allow overriding
+			// Virtual methods: Allow overriding
 			// (Never use a const self*)
+			// The pointer that we are passed is the base type, not the subclassed
+			// type. First cast the void* to the base type, and only then,
+			// upclass it
 
 			ret.WriteString(
 				`void ` + methodPrefixName + `_override_virtual_` + m.SafeMethodName() + `(void* self, intptr_t slot) {` + "\n" +
-					"\t( (" + cppClassName + "*)(self) )->handle__" + m.SafeMethodName() + " = slot;\n" +
+					"\tdynamic_cast<" + cppClassName + "*>( (" + cabiClassName(c.ClassName) + "*)(self) )->handle__" + m.SafeMethodName() + " = slot;\n" +
 					"}\n" +
 					"\n",
 			)

cmd/genbindings/emitgo.go

@@ -892,13 +892,7 @@ import "C"
 
 			preamble, forwarding := gfs.emitParametersGo2CABIForwarding(m)
 
-			returnTypeDecl := m.ReturnType.RenderTypeGo(&gfs)
-			if returnTypeDecl == "void" {
-				returnTypeDecl = ""
-			}
-			if m.ReturnType.QtClassType() && m.ReturnType.ParameterType != "QString" && m.ReturnType.ParameterType != "QByteArray" && !(m.ReturnType.Pointer || m.ReturnType.ByRef) {
-				returnTypeDecl = "*" + returnTypeDecl
-			}
+			returnTypeDecl := m.ReturnType.renderReturnTypeGo(&gfs)
 
 			rvalue := `C.` + goClassName + `_` + m.SafeMethodName() + `(` + forwarding + `)`
 
@@ -969,58 +963,87 @@ import "C"
 			gfs.imports["unsafe"] = struct{}{}
 			gfs.imports["runtime/cgo"] = struct{}{}
 
-			var cgoNamedParams []string
-			var paramNames []string
-			conversion := ""
+			// Add a package-private function to call the C++ base class method
+			// QWidget_virtualbase_PaintEvent
 
-			if len(m.Parameters) > 0 {
-				conversion = "// Convert all CABI parameters to Go parameters\n"
-			}
-			for i, pp := range m.Parameters {
-				cgoNamedParams = append(cgoNamedParams, pp.ParameterName+" "+pp.parameterTypeCgo())
+			{
+				preamble, forwarding := gfs.emitParametersGo2CABIForwarding(m)
+
+				forwarding = "unsafe.Pointer(this.h)" + strings.TrimPrefix(forwarding, `this.h`) // TODO integrate properly
+
+				returnTypeDecl := m.ReturnType.renderReturnTypeGo(&gfs)
+
+				ret.WriteString(`
+				func (this *` + goClassName + `) callVirtualBase_` + m.SafeMethodName() + `(` + gfs.emitParametersGo(m.Parameters) + `) ` + returnTypeDecl + ` {
+					` + preamble + `
+					` + gfs.emitCabiToGo("return ", m.ReturnType, `C.`+goClassName+`_virtualbase_`+m.SafeMethodName()+`(`+forwarding+`)`) + `
+				}
+			`)
 
-				paramNames = append(paramNames, fmt.Sprintf("slotval%d", i+1))
-				conversion += gfs.emitCabiToGo(fmt.Sprintf("slotval%d := ", i+1), pp, pp.ParameterName) + "\n"
 			}
 
-			cabiReturnType := m.ReturnType.parameterTypeCgo()
-			if cabiReturnType == "C.void" {
-				cabiReturnType = ""
-			}
+			// Add a function to set the virtual override handle
+			// It must be possible to call the base class version, so pass
+			// that a as a 'super' callback as an extra parameter
 
-			goCbType := `func(` + gfs.emitParametersGo(m.Parameters) + `)`
-			if !m.ReturnType.Void() {
-				goCbType += " " + m.ReturnType.RenderTypeGo(&gfs)
-			}
+			{
 
-			ret.WriteString(`func (this *` + goClassName + `) On` + m.SafeMethodName() + `(slot ` + goCbType + `) {
+				var cgoNamedParams []string
+				var paramNames []string = []string{"(&" + goClassName + "{h: self}).callVirtualBase_" + m.SafeMethodName()}
+				conversion := ""
+
+				if len(m.Parameters) > 0 {
+					conversion = "// Convert all CABI parameters to Go parameters\n"
+				}
+				for i, pp := range m.Parameters {
+					cgoNamedParams = append(cgoNamedParams, pp.ParameterName+" "+pp.parameterTypeCgo())
+
+					paramNames = append(paramNames, fmt.Sprintf("slotval%d", i+1))
+					conversion += gfs.emitCabiToGo(fmt.Sprintf("slotval%d := ", i+1), pp, pp.ParameterName) + "\n"
+				}
+
+				cabiReturnType := m.ReturnType.parameterTypeCgo()
+				if cabiReturnType == "C.void" {
+					cabiReturnType = ""
+				}
+
+				superCbType := `func(` + gfs.emitParametersGo(m.Parameters) + `) ` + m.ReturnType.renderReturnTypeGo(&gfs)
+
+				goCbType := `func(super ` + superCbType
+				if len(m.Parameters) > 0 {
+					goCbType += `, ` + gfs.emitParametersGo(m.Parameters)
+				}
+				goCbType += `) ` + m.ReturnType.renderReturnTypeGo(&gfs)
+
+				ret.WriteString(`func (this *` + goClassName + `) On` + m.SafeMethodName() + `(slot ` + goCbType + `) {
 					C.` + goClassName + `_override_virtual_` + m.SafeMethodName() + `(unsafe.Pointer(this.h), C.intptr_t(cgo.NewHandle(slot)) )
 				}
 				
 				//export miqt_exec_callback_` + goClassName + `_` + m.SafeMethodName() + `
-				func miqt_exec_callback_` + goClassName + `_` + m.SafeMethodName() + `(cb C.intptr_t` + ifv(len(m.Parameters) > 0, ", ", "") + strings.Join(cgoNamedParams, `, `) + `) ` + cabiReturnType + `{
+				func miqt_exec_callback_` + goClassName + `_` + m.SafeMethodName() + `(self *C.` + goClassName + `, cb C.intptr_t` + ifv(len(m.Parameters) > 0, ", ", "") + strings.Join(cgoNamedParams, `, `) + `) ` + cabiReturnType + `{
 					gofunc, ok := cgo.Handle(cb).Value().(` + goCbType + `)
 					if !ok {
 						panic("miqt: callback of non-callback type (heap corruption?)")
 					}
 				
 			`)
-			ret.WriteString(conversion + "\n")
-			if cabiReturnType == "" {
-				ret.WriteString(`gofunc(` + strings.Join(paramNames, `, `) + " )\n")
-			} else {
-				ret.WriteString(`virtualReturn := gofunc(` + strings.Join(paramNames, `, `) + " )\n")
-				virtualRetP := m.ReturnType // copy
-				virtualRetP.ParameterName = "virtualReturn"
-				binding, rvalue := gfs.emitParameterGo2CABIForwarding(virtualRetP)
-				ret.WriteString(binding + "\n")
-				ret.WriteString("return " + rvalue + "\n")
-			}
-			ret.WriteString(`
+				ret.WriteString(conversion + "\n")
+				if cabiReturnType == "" {
+					ret.WriteString(`gofunc(` + strings.Join(paramNames, `, `) + " )\n")
+				} else {
+					ret.WriteString(`virtualReturn := gofunc(` + strings.Join(paramNames, `, `) + " )\n")
+					virtualRetP := m.ReturnType // copy
+					virtualRetP.ParameterName = "virtualReturn"
+					binding, rvalue := gfs.emitParameterGo2CABIForwarding(virtualRetP)
+					ret.WriteString(binding + "\n")
+					ret.WriteString("return " + rvalue + "\n")
+				}
+				ret.WriteString(`
 				}
 			`)
 
-			// TODO add package-private function to call the C++ base class method
+			}
+
 		}
 
 		if c.CanDelete {

cmd/genbindings/statetracker.go

@@ -2,7 +2,7 @@ package main
 
 type lookupResultClass struct {
 	PackageName string
-	Class       *CppClass
+	Class       CppClass
 }
 
 type lookupResultTypedef struct {
@@ -28,8 +28,8 @@ func flushKnownTypes() {
 }
 
 func addKnownTypes(packageName string, parsed *CppParsedHeader) {
-	for i, c := range parsed.Classes {
-		KnownClassnames[c.ClassName] = lookupResultClass{packageName, &parsed.Classes[i] /* reference */}
+	for _, c := range parsed.Classes {
+		KnownClassnames[c.ClassName] = lookupResultClass{packageName, c /* copy */}
 	}
 	for _, td := range parsed.Typedefs {
 		KnownTypedefs[td.Alias] = lookupResultTypedef{packageName, td /* copy */}