gtk-rs / sys

DEPRECATED, each crate has its own sys folder now.
http://gtk-rs.org/
MIT License
30 stars 25 forks source link

GPid and GPollFD ABI mismatch for Windows #82

Closed EPashkin closed 6 years ago

EPashkin commented 6 years ago

https://github.com/gtk-rs/gir/pull/558 shows this problem:

ABI mismatch for GPid Rust: ABI { size: 4, alignment: 4 } C: ABI { size: 8, alignment: 8 } ABI mismatch for GPollFD Rust: ABI { size: 8, alignment: 4 } C: ABI { size: 16, alignment: 8 }

GPid currently defined as pub type GPid = c_int; In C it defined on linux as typedef int GPid;, on windows: typedef void * GPid.

GPollFD currently

pub struct GPollFD {
    pub fd: c_int,
    pub events: c_ushort,
    pub revents: c_ushort,
}

In C it different only on 64-bit Windows

struct _GPollFD
{
#if defined (G_OS_WIN32) && GLIB_SIZEOF_VOID_P == 8
#ifndef __GTK_DOC_IGNORE__
  gint64    fd;
#endif
#else
  gint      fd;
#endif
  gushort   events;
  gushort   revents;
};
EPashkin commented 6 years ago

Seems simpler way just add manual part to glib-sys that change definition. IMHO we can use i64 for both types for #[cfg(all(windows,target_arch="x86_64"))]

@GuillaumeGomez, @sdroege, @tmiasko any comments?

GuillaumeGomez commented 6 years ago

We can totally use i64 for both even though it's not optimal.

EPashkin commented 6 years ago

Currently sys generation ignores object status for type generation and use it only for function, I will try to fix it tomorrow.

sdroege commented 6 years ago

Manual implementation seems like the best approach for this, yes

EPashkin commented 6 years ago

@sdroege GLib contains child_watch_add and child_watch_source_new that affected by this issue, they used u32 as pid type (not sure about your reason to do that). I tried done same as for sys in https://github.com/EPashkin/glib/commit/4e3100ed3f7da2c1648e2bae88623e30955ea62c, but it not looks good. Maybe better do this: define GPid as unsigned in sys in https://github.com/gtk-rs/sys/pull/81, export it from glib as Pid, and use in these functions directly?

EPashkin commented 6 years ago

Or define Pid as u64 or c_unt in glib and export it.

sdroege commented 6 years ago

IMHO we should better define a correct type for pid instead of having configuration-dependant closure signatures. It's ugly otherwise.

That type could then be converted to the pid type of libstd with one of the conversion traits.

sdroege commented 6 years ago

And I used u32 as pid type because that's what devhelp showed for the definition of GPid :) Should've expected Windows wanting to be special again.

EPashkin commented 6 years ago

GPid signed even on linux: https://developer.gnome.org/glib/stable/glib-The-Main-Event-Loop.html#GPid

If we go from https://doc.rust-lang.org/std/process/struct.Child.html#method.id then Windows have different in rust/src/libstd/sys/unix/process/process_unix.rs

    pub fn id(&self) -> u32 {
        self.pid as u32
    }

in rust/src/libstd/sys/unix/process/process_fuchsia.rs

    pub fn id(&self) -> u32 {
        self.handle.raw() as u32
    }

in rust/src/libstd/sys/windows/process.rs

    pub fn id(&self) -> u32 {
        unsafe {
            c::GetProcessId(self.handle.raw()) as u32
        }
    }

where Windows handle.raw() is your GPid and GetProcessId is WinApi function https://msdn.microsoft.com/ru-ru/library/windows/desktop/ms683215(v=vs.85).aspx and it not exported from std, also it seems don't have good inverse function: https://stackoverflow.com/questions/2221103/how-to-get-process-handle-from-process-id So using u32 as rust variant for GPid seems not good for Windows and maybe Fuchsia too.

sdroege commented 6 years ago

Nobody ported GLib to Fuchsia yet :)

So a process ID on Windows is a u32 but the handle for the process is a HANDLE like everything, so basically a pointer. And GPid is basically a *mut c_void then on Windows.

sdroege commented 6 years ago

So let's define GPid in -sys like that, and in glib as pub struct Pid(pub ffi::GPid) and ignore the problem for now.

EPashkin commented 6 years ago

This fixed by #81