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

They weren't meant to be bit fields #675

Merged
merged 1 commit into from Dec 17, 2018

Conversation

Projects
None yet
4 participants
@tmiasko
Copy link
Contributor

tmiasko commented Dec 16, 2018

When bit field is not preceded by another bit field, generate the field
as if didn't specify bit width.

From Sys V ABI perspective two most important things about bit fields
are that they occupy storage unit appropriate for their declared type,
and that they might share it with other struct / union members.

When there are no other bit fields they could share storage with
they are laid out like fields without bit width specification.

Use this observation to generate definitions for additional types.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 16, 2018

Interesting idea, but there 2 problems:

  1. Seems need additional check for unions.
    GdkEvent now generated as struct with many fields.
  2. GtkEventKey has singlebit field is_modifier, that added as pub is_modifier: c_uint now,
    but from C side it IMHO be 1byte integer,
    strange but your layout test don't detect errors so I may be wrong about this.
@tmiasko

This comment has been minimized.

Copy link
Contributor Author

tmiasko commented Dec 16, 2018

  1. Please double check, are you sure that GdkEvent isn't a union? It is for me.
  2. Bit fields are placed in storage units appropraite for its declared type, so
    in this case of is_modifier it would be c_uint.
@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 16, 2018

@tmiasko Was wrong about GdkEvent, it really union in rust too, sorry.
About second I was wrong too.

Thanks.
So I waiting Travis just in case.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Dec 16, 2018

Can you link to a -sys branch with the additional types this allows to generate? I'd like to double check that first :)

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Dec 16, 2018

From Sys V ABI perspective

Also not everyhing is SysV ABI unfortunately (-> Windows), so we need to be very careful here.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 16, 2018

@sdroege EPashkin/rust-gnome-sys@7489e8d

on Windowsx64 layout test passed for gdk, gtk

@tmiasko

This comment has been minimized.

Copy link
Contributor Author

tmiasko commented Dec 16, 2018

@EPashkin thanks for testing. I ran ABI checks on Linx x86_64 as well.

@sdroege having looked at description of MVSC ABI, I wouldn't expect issues there.
There is a reason I opted to do this in the most trivial case, rather than
going form more general solution like the one proposed earlier.

Is something wrong with travis?

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Dec 16, 2018

@sdroege having looked at description of MVSC ABI, I wouldn't expect issues there.
There is a reason I opted to do this in the most trivial case, rather than
going form more general solution like the one proposed earlier.

Looks generally good, my only worry is a case like

struct {
  unsigned x: 1;
  uint8_t y;
}

You would now make the x a c_uint, but is it really clear from the ABI spec that it's not just going to occupy 8 bits? I.e. a case where following the bitfield is a field that has smaller alignment requirements than an unsigned int.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Dec 16, 2018

Is something wrong with travis?

Yes, the GString stuff for gir was merged before the corresponding PR in glib was merged :) Ignore that for now

@tmiasko

This comment has been minimized.

Copy link
Contributor Author

tmiasko commented Dec 16, 2018

struct {
  unsigned x: 1;
  uint8_t y;
}

You would now make the x a c_uint, but is it really clear from the ABI spec that it's not just going to occupy 8 bits? I.e. a case where following the bitfield is a field that has smaller alignment requirements than an unsigned int.

This could be an issue, if bit field shares storage with non bitfield that follows it, like in above example (it still 4 bytes in total).

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Dec 16, 2018

This could be an issue, if bit field shares storage with non bitfield that follows it, like in above example (it still 4 bytes in total).

What do you mean with "shares storage"? There's no sharing of storage in this example :) And the size of the overall struct would be 2 bytes in C and 5 bytes in Rust now, how do you get to 4 bytes? As it's a different size, we would at least detect this specific case with the ABI checks.

Your code would generate

struct {
  x: c_uint,
  y: u8,
}

and that would be wrong in this case. In the generated code for sys it seems all fine, unless I missed something.

@tmiasko

This comment has been minimized.

Copy link
Contributor Author

tmiasko commented Dec 16, 2018

overall struct would be 2 bytes in C

Nope. It would be 4, since x is placed in storage unit appropriate for unsigned int,
i.e., 4 bytes, and it shares this storage with y.

So far I have no simple ideas for avoiding this problem in general.
Do you think this is worth going forward with this anyway?

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Dec 16, 2018

overall struct would be 2 bytes in C

Nope. It would be 4, since x is placed in storage unit appropriate for unsigned int,
i.e., 4 bytes, and it shares this storage with y.

Are you sure about that? So to make it two bytes we would have to do char x: 1; in C? Can you link to the relevant parts of the ABI or C specs?

If it's stored in "a storage unit of 4 bytes" then there is no problem. y would not use the same space.

What happens if we do char x: 9; in C, does that use two chars then but still have 1 byte alignment? While short x: 9; would also use 16 bits but have 2 byte alignment?

@tmiasko

This comment has been minimized.

Copy link
Contributor Author

tmiasko commented Dec 16, 2018

Can you link to the relevant parts of the ABI or C specs?

From System V Application Binary Interface AMD64:

  • bit-fields must be contained in a storage unit appropriate for its declared type
  • bit-fields may share a storage unit with other struct / union members

For others take a look at spec linked here: https://uclibc.org/specs.html

What happens if we do char x: 9; in C, does that use two chars then but still have 1 byte alignment? While short x: 9; would also use 16 bits but have 2 byte alignment?

First one is invalid, since you can't fit 9 bits into char. Second one would
use storage unit appropriate for short type, so yes it would have its size and
alignmnent.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Dec 16, 2018

bit-fields may share a storage unit with other struct / union members

Ok that part is unfortunate and a problem :) That means a struct { unsigned x: 1; uint8_t y;} could indeed have size 4. But fortunately we would notice that problem by the ABI checks, our Rust struct would have size 5.

In other cases it wouldn't be that obvious: struct { short x: 1; uint8_t y; void *z;} would always have the size of two pointers due to alignment, but the y could be stored anywhere in the first two bytes.

What do you suggest? IMHO this is exactly the same problem as with multiple bitfield fields one after another.

@tmiasko

This comment has been minimized.

Copy link
Contributor Author

tmiasko commented Dec 16, 2018

  1. Accept the risk (the whole run-time introspection implementation already works
    as if all fields didn't have width specification, and I didn't find any layout
    breakage in GLib, GObject, Gdk, Gtk, Pango that would involve only one bit field).
  2. Extend ABI checks to field offsets, for all non-bit fields. (I though it was
    already there in Rust, but I only implemented it in ABI checker for typelibs ...).
  3. Wait for bit fields in Rust.
@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Dec 16, 2018

2. sounds best to me, just needs someone to implement it of course :) 3. won't be anytime soon, especially as some platforms have multiple bitfield ABIs (mingw has a commandline parameter for switching between MSVC and its own...).

Out of curiosity, what does the & operator in C do on bitfields? Implementation defined behaviour?

@tmiasko

This comment has been minimized.

Copy link
Contributor Author

tmiasko commented Dec 16, 2018

ctest crate includes offsets checks, so we could use that,
with caveat that it suffers from all-or-nothing approach
with a single compilation unit.

@tmiasko

This comment has been minimized.

Copy link
Contributor Author

tmiasko commented Dec 16, 2018

Based on my experience from gtk-rs/sys#119, I think we
are in position to create tests that would detect all potential issues, and
doing so doesn't require too much effort. The problem described earlier
shouldn't be consider a blocker any longer.

@EPashkin I would appreciate if you could try those new ctests on Windows as
well. The headers and pkg-config might need some additional cfg conditionals.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 17, 2018

@tmiasko Currently it don't builds for me:
Still finding solution, as package from msys2/MSYS2-packages#1086
don't works on msys64 for me

cargo:rerun-if-changed=../glib-sys/src/lib.rs
cargo:rerun-if-changed=../glib-sys/src\manual.rs
OPT_LEVEL = Some("0")
HOST = Some("x86_64-pc-windows-gnu")
CC_x86_64-pc-windows-gnu = None
CC_x86_64_pc_windows_gnu = None
HOST_CC = None
CC = None
CFLAGS_x86_64-pc-windows-gnu = None
CFLAGS_x86_64_pc_windows_gnu = None
HOST_CFLAGS = None
CFLAGS = None
DEBUG = Some("true")
running: "gcc.exe" "-O0" "-ffunction-sections" "-fdata-sections" "-g" "-fno-omit-frame-pointer" "-m64" "-Wall" "-Wextra" "-Wall" "-Wextra" "-Werror" "-Wno-unused-parameter" "-Wno-type-limits" "-Wno-address-of-packed-member" "-Wno-deprecated-declarations" "-mms-bitfields" "-ID:/P/msys64/mingw64/include/glib-2.0" "-ID:/P/msys64/mingw64/lib/glib-2.0/include" "-ID:/P/msys64/mingw64/include" "-o" "D:\\eap\\rust\\0\\./target\\debug\\build\\glib-sys-test-c9ebadc72b49efae\\out\\all.o" "-c" "D:\\eap\\rust\\0\\./target\\debug\\build\\glib-sys-test-c9ebadc72b49efae\\out\\all.c"
cargo:warning=In file included from D:\eap\rust\0\./target\debug\build\glib-sys-test-c9ebadc72b49efae\out\all.c:4:
cargo:warning=D:/P/msys64/mingw64/include/glib-2.0/glib-unix.h:29:10: fatal error: sys/wait.h: No such file or directory
cargo:warning= #include <sys/wait.h>
cargo:warning=          ^~~~~~~~~~~~
cargo:warning=compilation terminated.
exit code: 1
@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 17, 2018

If I comment cfg.header("glib-unix.h"); then I get need skip type "GUnixFDSourceFunc" too.
On gio-sys same problem, need comment these lines and some types and structs.
After this all test passed for 3 sec.

+++ b/gio-sys-test/build.rs
@@ -5,7 +5,7 @@ fn pkg_config_cflags() -> Result<Vec<String>, Box<std::error::Error>> {
     let mut cmd = std::process::Command::new("pkg-config");
     cmd.arg("--cflags");
     cmd.arg("gio-2.0");
-    cmd.arg("gio-unix-2.0");
+//    cmd.arg("gio-unix-2.0");
     let out = cmd.output()?;
     if !out.status.success() {
         return Err(format!("command {:?} returned {}",
@@ -25,6 +25,7 @@ fn main() {
     cfg.header("gio/gio.h");
     cfg.define("G_SETTINGS_ENABLE_BACKEND", None);
     cfg.header("gio/gsettingsbackend.h");
+/*
     cfg.header("gio/gdesktopappinfo.h");
     cfg.header("gio/gfiledescriptorbased.h");
     cfg.header("gio/gunixconnection.h");
@@ -33,6 +34,7 @@ fn main() {
     cfg.header("gio/gunixinputstream.h");
     cfg.header("gio/gunixoutputstream.h");
     cfg.header("gio/gunixsocketaddress.h");
+*/
 
     cfg.skip_const(|_| true);
     cfg.skip_fn(|_| true);
@@ -84,6 +86,11 @@ fn main() {
         _ => field,
     }.to_string());
 
+    cfg.skip_type(|typ| match typ {
+        "GDesktopAppLaunchCallback" => true,
+        _ => false,
+    });
+
     cfg.skip_struct(|typ| match typ {
         // Incomplete types, need manual exclusion since ctype
         // doesn't understand "pub struct Type(c_void)" pattern:
@@ -246,6 +253,24 @@ fn main() {
         "GZlibCompressor" => true,
         "GZlibDecompressor" => true,
 
+"GDesktopAppInfoClass" => true,
+"GDesktopAppInfoLookupIface" => true,
+"GFileDescriptorBasedIface" => true,
+"GUnixConnectionClass" => true,
+"GUnixCredentialsMessageClass" => true,
+"GUnixFDListClass" => true,
+"GUnixFDMessageClass" => true,
+"GUnixInputStreamClass" => true,
+"GUnixOutputStreamClass" => true,
+"GUnixSocketAddressClass" => true,
+"GUnixConnection" => true,
+"GUnixCredentialsMessage" => true,
+"GUnixFDList" => true,
+"GUnixFDMessage" => true,
+"GUnixInputStream" => true,
+"GUnixOutputStream" => true,
+"GUnixSocketAddress" => true,
+
         _ => false,
     });
@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Dec 17, 2018

It seems for constants to work we somehow need ctest to extern crate libc; use libc::*;. Why doesn't it do that anyway?

@tmiasko

This comment has been minimized.

Copy link
Contributor Author

tmiasko commented Dec 17, 2018

Thanks for testing @EPashkin! I made pkg-config and headers conditional on target.

It seems for constants to work we somehow need ctest to extern crate libc; use libc::*;. Why doesn't it do that anyway?

Unused warnings if you add them to src/main.rs when not testing constants.

@tmiasko

This comment has been minimized.

Copy link
Contributor Author

tmiasko commented Dec 17, 2018

BTW, would you prefer to merge this or tests first? Before you answer that
tests, consider that the list of incomplete types in tests will have to be
updated to match new codgen (I am still thinking about making this automatic,
but it will most likely require changing gir generator or ctest itself).

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Dec 17, 2018

First this, then tests. But for tests we should IMHO first cover constants too so that the old tests can be removed. It's not great to have two testsuites doing basically the same.

Tomasz Miąsko
They weren't meant to be bit fields
When bit field is not preceded by another bit field, generate the field
as if didn't specify bit width.

From Sys V ABI perspective two most important things about bit fields
are that they occupy storage unit appropriate for their declared type,
and that they might share it with other struct / union members.  When
there are no other fields they could share storage with, bit fields are
laid out like fields without bit width specification.

Under assumption that bit fields are unlikely to share storage with
other fields which are not bit fields themselves, this transformation
should be safe. This assumption is verified experimentally by
comprehensive ABI tests.
@tmiasko

This comment has been minimized.

Copy link
Contributor Author

tmiasko commented Dec 17, 2018

I updated the commit message to describe assumptions made by this
transformation.

First this, then tests.

Ok, great. I will update incomplete types in tests and add ctest for gtk-sys once this is merged.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Dec 17, 2018

Maybe also stop gir from generating tests for -sys :)

@tmiasko

This comment has been minimized.

Copy link
Contributor Author

tmiasko commented Dec 17, 2018

Out of scope for this PR :).

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 17, 2018

@sdroege Old test still needed for constants.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 17, 2018

@GuillaumeGomez Please take look at this.
Regen result for this PR: EPashkin/rust-gnome-sys@7489e8d

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Dec 17, 2018

Nice fix! All good for me.

@EPashkin You can merge. :)

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 17, 2018

@tmiasko Thanks

@EPashkin EPashkin merged commit 6e02e0e into gtk-rs:master Dec 17, 2018

2 checks passed

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

@tmiasko tmiasko deleted the tmiasko:bitfields branch Dec 17, 2018

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Dec 17, 2018

@sdroege Old test still needed for constants.

That's my point, we shouldn't merge the new tests without them fully replacing the old ones

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.