Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Working record finalizer #17

Closed
diamondburned opened this issue Jul 15, 2021 · 1 comment
Closed

Working record finalizer #17

diamondburned opened this issue Jul 15, 2021 · 1 comment
Labels
broken Changes that require refactoring

Comments

@diamondburned
Copy link
Owner

Right now, record finalizers are implemented incorrectly. An example code from ./pkg/gsk/v4/gsktypes.go shows this issue:

	_cret = C.gsk_transform_rotate_3d(_arg0, _arg1, _arg2)

	var _transform *Transform // out

	_transform = (*Transform)(unsafe.Pointer(_cret))
	C.gsk_transform_ref(_cret)
	runtime.SetFinalizer(_transform, func(v *Transform) {
		C.gsk_transform_unref((*C.GskTransform)(unsafe.Pointer(v)))
	})

Here, the C-heap-allocated (malloced) _cret pointer is converted into a pointer to a Go type, and a finalizer is set on the converted value. This doesn't work, however. Running this code will trigger a runtime panic:

fatal error: runtime.SetFinalizer: pointer not in allocated block

It seems like the most straightforward solution would be to forbid Go from allocating record types at all by generating all records like so:

type Transform struct {
    nocopy [0]sync.Mutex
	native *C.GskTransform
}

This way, zero-values of Transform are always invalid, and the caller must use an appropriate constructor that mallocs the value. The nocopy field ensures that the struct value is never copied, because copying the struct value will ruin the finalizer. The alignment of that field means that nocopy does not grow the Transform type to be more than a pointer.

It might also be worth it to consider making native a uintptr type instead of an actual pointer type to reduce possible GC overhead, but this is likely already handled trivially in the Go runtime.

@diamondburned
Copy link
Owner Author

An external package could cast the type without needing to access the unexported fields by unsafe casting the types, like how the code generator currently handles unexported fields from some cairo types:

	{
		_p := &struct{ p unsafe.Pointer }{unsafe.Pointer(_cret)}
		_pattern = (*cairo.Pattern)(unsafe.Pointer(_p))
	}
	C.cairo_pattern_reference(_cret)
	runtime.SetFinalizer(_pattern, func(v *cairo.Pattern) {
		C.cairo_pattern_destroy((*C.cairo_pattern_t)(unsafe.Pointer(v.Native())))
	})

Only now, it would look something like this:

	p := &struct {
		_ [0]sync.Mutex
		p unsafe.Pointer
	}{
		[0]sync.Mutex{},
		unsafe.Pointer(new(string)),
	}

It would probably be tedious to inline something like that, but it's probably better than generating helper functions.

@diamondburned diamondburned added the broken Changes that require refactoring label Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
broken Changes that require refactoring
Projects
None yet
Development

No branches or pull requests

1 participant