jwijenbergh / puregotk

Autogenerated GTK4+Adwaita bindings for Go leveraging ebitengine/purego
MIT License
12 stars 3 forks source link

GType data type mismatch #6

Open pdf opened 7 months ago

pdf commented 7 months ago

Currently there are roughly a couple of hundred APIs that are not usable due to the mismatch between the Go type generated for GType and the type in the C API.

In an earlier discussion the reason for retaining this mismatch in the short term was due to struct size mismatches in e.g. gobject.Value:

https://github.com/jwijenbergh/puregotk/pull/1#discussion_r1444819475

Clearly this is a kludge - we're wrong-sizing GType to pad out another type.

If we agree that GType should have the correct size/type, I propose that we create a type alias for GType in the templates:

type GType uintptr

And map gir GType to the Go gobject.GType. I suggest this for a couple of reasons:

  1. We can mess with this alias if we find that there is a size mismatch on some particular platform without breaking the API, but for the platforms that purego supports, I believe sizeof unitptr should match the C size for GType.
  2. It makes it clear in the API when a GType is expected.

Unfortunately it doesn't look like the #define'd fundamental types from here for example make it into the gir, so things will still be a bit unergonomic, but at least it will be possible to actually call these APIs.


Now to the second part of the problem of incorrect sizing for e.g. gobject.Value. There are a number of things going wrong here, and I've not had time to trace them all down to root causes.

First, union types in CGo are typically exposed as a byte array of a size large enough to accomodate the largest type in the union. Here, we appear to just map unions to uintptr. In the specific case of gobject.ValueDataUnion this happens to work out, since unintptr is 8 bytes on amd64/arm64, and all types in that union fit exactly into 8 bytes. I'm not certain that this holds true for all unions in the codebase, or for other architectures ([8]byte would be safer for the latter concern).

Second, I think there's a parsing issue with gir array types - the data for the field here doesn't appear to be parsed/templated correctly - we are generating the gobject.ValueDataUnion type from the union elsewhere in the gir, but that name reference is not used when rendering this field.

And finally, we're not handling fixed-size arrays, which is largely where your struct size mismatch comes from - that array type field is defined with fixed-size 2.

If the struct definition for gobject.Value was correctly rendered (based on all of the above) as:

type Value struct {
        GType GType // where: type GType uintptr

        Data [2]ValueDataUnion // where: type ValueDataUnion [8]byte; or type ValueDataUnion uinptr as currently
}

Then I believe the struct layout and size would exactly match that of the C type, and we'd be in much better shape generally.

jwijenbergh commented 7 months ago

Thanks a lot for the detailed issue report. FWIW I had a WIP pr that tries to deal with all sorts of alignment issues. Will try to get back to that and solve the issues mentioned in this PR too. Probably will be somewhere next week

jwijenbergh commented 2 months ago

hi just wanted to apologize for not picking this up @pdf. Hopefully I can get back to this soon

pdf commented 2 months ago

I absolutely understand how life goes, you should feel zero pressure or obligation from me - if or when you feel motivated to pick this up again just reach out and I'll be happy to help if I'm in the same place.

gen2brain commented 4 days ago

Since 1.23 there is a https://pkg.go.dev/structs#HostLayout that can help with struct sizes and padding, i.e. it will follow the C ABI. I just stumbled upon this issue, I know some people who missed this change, hope it can help.

jwijenbergh commented 4 days ago

Since 1.23 there is a https://pkg.go.dev/structs#HostLayout that can help with struct sizes and padding, i.e. it will follow the C ABI. I just stumbled upon this issue, I know some people who missed this change, hope it can help.

Hi Milan, I saw that purego also has an issue for it. It makes a lot of sense that I add this to puregotk. Thanks for your raylib bindings btw!