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

Mark void pointers as gpointer #478

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thiblahute
Copy link

Otherwise the ABI is not properly respected as for some reason,
mem::size_of::<c_void>() returns 1 on a 64bits system. In the case
of GstVideoDecoder(Class), the ABI mistmatch because of the last
void * padding[14] field which is translated to
pub padding: [c_void; 14] which size was 14(bytes) instead of
112 (on 64bits systems).

Otherwise the ABI is not properly respected as for some reason,
mem::size_of::<c_void>() returns 1 on a 64bits system. In the case
of GstVideoDecoder(Class), the ABI mistmatches because of the
`void * padding[14]` field which is translated to
`pub padding: [c_void; 14]` which size was 14(bytes) instead of
112 (on 64bits systems).
@sdroege
Copy link
Member

sdroege commented Oct 17, 2017

Yes they have to be *mut c_void aka gpointer

@sdroege
Copy link
Member

sdroege commented Oct 17, 2017

I'm not sure the change is correct though. It is detected as a pointer already in the code you change, but that knowledge disappears somewhere along the way.

@thiblahute
Copy link
Author

I'm not sure the change is correct though. It is detected as a pointer already in the code you change, but that knowledge disappears somewhere along the way.

I am not sure what you mean here :-)

@@ -92,7 +92,7 @@ fn ffi_inner(env: &Env, tid: library::TypeId, mut inner: String) -> Result {
Type => "GType",
Pointer => {
match &inner[..] {
"void" => "c_void",
"void" => "gpointer",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look 3 lines above, this is already the Pointer case and for resolving the "inner" part of the pointer. The inner part is c_void, but the outer part (*mut or `const) is for some reason not made use of. Or I need to read this code in more detail later :)

@EPashkin
Copy link
Member

Seems this really not right:
gio-sys:

pub struct GInputStreamClass {
     pub parent_class: gobject::GObjectClass,
-    pub read_fn: Option<unsafe extern "C" fn(*mut GInputStream, *mut c_void, size_t, *mut GCancellable, *mut *mut glib::GError) -> ssize_t>,
+    pub read_fn: Option<unsafe extern "C" fn(*mut GInputStream, *mut gpointer, size_t, *mut GCancellable, *mut *mut glib::GError) -> ssize_t>,
     pub skip: Option<unsafe extern "C" fn(*mut GInputStream, size_t, *mut GCancellable, *mut *mut glib::GError) -> ssize_t>,

glib-sys

-    pub fn g_once_init_enter(location: *mut c_void) -> gboolean;
-    pub fn g_once_init_leave(location: *mut c_void, result: size_t);
+    pub fn g_once_init_enter(location: *mut gpointer) -> gboolean;
+    pub fn g_once_init_leave(location: *mut gpointer, result: size_t);

gtk-sys

pub struct GtkFileChooserButtonClass {
     pub parent_class: GtkBoxClass,
     pub file_set: Option<unsafe extern "C" fn(*mut GtkFileChooserButton)>,
-    pub __gtk_reserved1: *mut c_void,
-    pub __gtk_reserved2: *mut c_void,
-    pub __gtk_reserved3: *mut c_void,
-    pub __gtk_reserved4: *mut c_void,
+    pub __gtk_reserved1: *mut gpointer,
+    pub __gtk_reserved2: *mut gpointer,
+    pub __gtk_reserved3: *mut gpointer,
+    pub __gtk_reserved4: *mut gpointer,
 }

etc.

And this code don't changed

pub struct GtkAppChooserButtonClass {
    pub parent_class: GtkComboBoxClass,
    pub custom_item_activated: Option<unsafe extern "C" fn(*mut GtkAppChooserButton, *const c_char)>,
    pub padding: [gpointer; 16],
}

as it have right type in gir, and other paddings in gtk too.

      <field name="padding" readable="0" private="1">
        <array zero-terminated="0" c:type="gpointer" fixed-size="16">
          <type name="gpointer" c:type="gpointer"/>
        </array>
      </field>

@thiblahute
Copy link
Author

Interesting, in Gst it is doing the right thing, but I clearly missed something, I will fix.

@sdroege
Copy link
Member

sdroege commented Oct 28, 2017

@thiblahute any news on this?

thiblahute pushed a commit to thiblahute/gstreamer-sys that referenced this pull request Nov 6, 2017
@ghost
Copy link

ghost commented Jan 31, 2018

gir for GstVideoDecoderClass looks as follows:

    <record name="VideoDecoderClass"
            c:type="GstVideoDecoderClass"
            glib:is-gtype-struct-for="VideoDecoder">
      ...
      <field name="padding" readable="0" private="1">
        <array zero-terminated="0" c:type="void" fixed-size="14">
          <type name="gpointer" c:type="void*"/>
        </array>
      </field>
    </record>

The value of array@c:type doesn't seem to make sense, and this is the one that
is used to determine how many pointers stick there. You can't also simply fix
this in codegen by swapping those two around as the second c:type is not
retained from parsing stage (neither would it be correct in general).

Furthermore, after investigating this further and cross checking with C, I am
even more confused. The values of array@c:type and element type@c:type values
are so inconsistent. A few related bugzilla items:

@sdroege
Copy link
Member

sdroege commented Feb 7, 2018

See #534 (comment)

Not a problem for this specific case anymore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants