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

Remove volatile wrapper struct. #579

Merged
merged 5 commits into from Apr 19, 2018

Conversation

Projects
None yet
3 participants
@tmiasko
Copy link
Contributor

tmiasko commented Apr 17, 2018

Using additional struct could potentially change the ABI, in similar way
wrapper for bitflags did. Generate volatile types without wrapper.

@@ -45,10 +45,6 @@ pub fn only_for_glib(w: &mut Write) -> Result<()> {
"pub type gconstpointer = *const c_void;",
"pub type gpointer = *mut c_void;",
"",
"#[repr(C)]",
"#[derive(Copy, Clone)]",
"pub struct Volatile<T>(T);",

This comment has been minimized.

@sdroege

sdroege Apr 17, 2018

Member

This is not equivalent to T at the FFI level?

This comment has been minimized.

@tmiasko

tmiasko Apr 17, 2018

Author Contributor

Usually, but not always. For some examples see discussion about bitflags in #575.
It would be equivalent with #[repr(transparent)], which is another nice but unstable thing.

This comment has been minimized.

@sdroege

sdroege Apr 18, 2018

Member

Ok, so this is only about wrappers around "fundamental" types and not structs

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Apr 18, 2018

@tmiasko Sorry, currently I have bad internet access. I look at this PR later.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Apr 18, 2018

Good to go IMHO

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Apr 18, 2018

@tmiasko You sure that Volatile can cause problem?
I see 2 usage cases:

  • field of struct:
#[repr(C)]
#[derive(Copy, Clone)]
pub struct GDBusAnnotationInfo {
    pub ref_count: Volatile<c_int>,
    pub key: *mut c_char,
    pub value: *mut c_char,
    pub annotations: *mut *mut GDBusAnnotationInfo,
}
  • pointer function parameter:
    pub fn g_dbus_error_register_error_domain(error_domain_quark_name: *const c_char, quark_volatile: *mut Volatile<size_t>, entries: *const GDBusErrorEntry, num_entries: c_uint);
@tmiasko

This comment has been minimized.

Copy link
Contributor Author

tmiasko commented Apr 18, 2018

@EPashkin, I think you are right that they don't cause ABI problems in how they
are used in Gio right now. They should be fine when placed in struct, and fine
to pass indirectly through a pointer.

I also don't like them because Gio uses volatile for no reason in D-Bus API,
and auto-generated bindings don't work out of the box. For example they are used in:

  • g_dbus_connection_send_message
  • g_dbus_connection_send_message_with_reply
  • g_dbus_connection_send_message_with_reply_sync

But neither of those actually present a valid use case for volatile.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Apr 18, 2018

Then maybe better fix these cases (in .gir file)?
Reason about gir not supporting these types in normal mode is strong,
but I still want think about this.

@tmiasko

This comment has been minimized.

Copy link
Contributor Author

tmiasko commented Apr 18, 2018

The use of volatile in GLib is somewhat anachronistic. In modern C the
volatile has no useful meaning with respect to memory synchronization, i.e.,
conflicting unsynchronized access constitutes a data race and has undefined
behaviour.

The GLib uses volatile fields as a mark that access to the filed should be
performed using atomic operations, not that access should actually use volatile
semantics. This made some sense before C had memory model and compiler used to
treat volatile like that, but that was never part of standard, and is no longer
true.

I don't think there is actually a single place in bindings where volatile
wrapper is of any use.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Apr 19, 2018

Ok, lets remove Volatile wrapper.
But IMHO better leave marks of it,
so @tmiasko please restore if in src/codegen/sys/ffi_type.rs with changing format to "/*Volatile*/{}"

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Apr 19, 2018

Seems good to me to keep it in a comment. But the volatile wrapper itself should really disappear, it makes not much sense in the FFI bindings. It's only an inconvenience and has no effect

Tomasz Miąsko added some commits Apr 19, 2018

Tomasz Miąsko
Tomasz Miąsko
Remove volatile wrapper struct.
Using additional struct could potentially change the ABI, in similar way
wrapper for bitflags did. Volatile types in GLib aren't generally used
for their volatile semantic as per modern C standard, but rather as a
markers that access might need atomic operations.

Generate volatile types without wrapper.
@tmiasko

This comment has been minimized.

Copy link
Contributor Author

tmiasko commented Apr 19, 2018

I restored comment about volatile. A few examples from generated code:

pub fn g_atomic_int_exchange_and_add(atomic: *mut /*volatile*/c_int, val: c_int) -> c_int;
pub fn g_bit_lock(address: *mut /*volatile*/c_int, lock_bit: c_int);
pub fn g_dbus_connection_send_message(connection: *mut GDBusConnection, message: *mut GDBusMessage, flags: GDBusSendMessageFlags, out_serial: *mut /*volatile*/u32, error: *mut *mut glib::GError) -> gboolean;
pub fn g_dbus_connection_send_message_with_reply(connection: *mut GDBusConnection, message: *mut GDBusMessage, flags: GDBusSendMessageFlags, timeout_msec: c_int, out_serial: *mut /*volatile*/u32, cancellable: *mut GCancellable, callback: GAsyncReadyCallback, user_data: gpointer);
pub fn g_dbus_connection_send_message_with_reply_sync(connection: *mut GDBusConnection, message: *mut GDBusMessage, flags: GDBusSendMessageFlags, timeout_msec: c_int, out_serial: *mut /*volatile*/u32, cancellable: *mut GCancellable, error: *mut *mut glib::GError) -> *mut GDBusMessage;

This includes clippy cleanup from #587 now.

@EPashkin EPashkin merged commit 780030a into gtk-rs:master Apr 19, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

vhdirk pushed a commit to vhdirk/gir that referenced this pull request Jul 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.