search

fitzgen / mach

A rust interface to the Mach 3.0 kernel that underlies OSX.
https://docs.rs/mach
Other
81 stars 28 forks source link

Use repr(packed) once it becomes stable #29

Closed gnzlbg closed 5 years ago

gnzlbg commented 6 years ago

All the remaining ABI issues cannot be easily solved without repr(packed). We could use an opaque type and then add methods to get/set its fields but, that's too much of a hack given that repr(packed) will be merged shortly.

The best we can do is probably add a build.rs that enables repr(packed) when mach is compiled using a nightly compiler. And once repr(packed) is stabilized we just enable it unconditionally.

dcuddeback commented 6 years ago

I would prefer not to enable features as soon as they land on a stable compiler, because that forces everyone to upgrade to the latest compiler, and sometimes teams can't upgrade right away or have valid reasons for delaying upgrades. That's not to say that this isn't a valid solution, just that we should be considerate of teams that might not be able to upgrade right away.

What ABI issues are you referring to and how does repr(packed) solve them?

gnzlbg commented 6 years ago

That's not to say that this isn't a valid solution, just that we should be considerate of teams that might not be able to upgrade right away.

This is a breaking change, so it is going to require a major semver version bump (from 0.x to 0.x+1 until mach achieves 1.0). If a team cannot upgrade right away, they should just not update their Cargo.toml to target the next version. Or in other words, this does not force anybody to upgrade.

What ABI issues are you referring to and how does repr(packed) solve them?

Some of the mach structs have an alignment of 4 in x86_64 targets. Without packed, they have an alignment of 8 in Rust, and a slightly larger size.

First, this is undefined behavior, leads to reading/writing garbage, and it is why I would like to support this as soon as its available on nightly. Without this, only x86 32-bit targets can use mach without undefined behavior (that is, you can only really target i686-apple-darwin safely right now).

In practice, Rust just typically inserts some padding after the first struct field, while C does not because in C the structs are pack(4). The consequence is that all fields after the first one in both C and Rust do not point to the same bytes. This is bad. If you pass via the C API a pointer to a struct, C won't write to the struct fields but somewhere in between fields, or potentially out-of-bounds. When you read in Rust from these structs, you are typically reading a mixture of the actual field, padding (garbage), and other fields.

dcuddeback commented 6 years ago

This is a breaking change, so it is going to require a major semver version bump

That's part of the solution, but it's easier than you're letting on to accidentally force a team to upgrade by merging new features after a breaking change. I would add that we should try to get new features and bug fixes into the current version before making the breaking change. It sounds like you're already thinking in this direction, given #22, so I think we're on the same page here.

Some of the mach structs have an alignment of 4 in x86_64 targets. Without packed, they have an alignment of 8 in Rust, and a slightly larger size. ... because in C the structs are pack(4)

Got it. Thanks for explaining. I just didn't see any details beyond "ABI Issues" anywhere, so I didn't know what was being discussed. This makes sense.

gnzlbg commented 6 years ago

It sounds like you're already thinking in this direction, given #22, so I think we're on the same page here.

Yes we should definitely do a minor release of the last commit of the 0.1 series. That was: https://github.com/fitzgen/mach/commit/592d9e995f46195f2b87bdeae3c316e125f7db2a (the commits afterwards start fixing ABI issues like missing repr(C), missing struct fields, etc).

dcuddeback commented 6 years ago

Okay. I was looking at #30 as something that adds new APIs that would be nice to merge first, but if master is already tracking 0.2, then no worries. It's not worth it to backport. I was also going to suggest tagging issues with milestones to track what's slated for each release series, but sounds like everything going forward will be 0.2.

@gnzlbg Thanks for catching and fixing these issues and then taking the time to explain them. As it stands, I have no objections to the direction this is headed.

gnzlbg commented 6 years ago

Status update: all the methods and types that required repr(packed(N)) to avoid undefined behavior were deprecated from the library in version 0.2, so people using that should have been getting a warning for a while. They have been able to silence this warning either by allowing deprecated items of mach, or by enabling the unstable feature which requires a nightly Rust toolchain.

Version 0.3 only exposes these types and methods behind the unstable feature. When repr(packed(N)) lands on stable we can remove the unstable feature and release a new version.

gnzlbg commented 5 years ago

So I needed to use mach from libstd, so went on and released version 0.3 which always uses repr(packed(N)) and is also always #[no_std]. Migrating from 0.2 to 0.3 should be painless, since all "breaking" changes have had a deprecation warning since november last year. Please let me know if there are any issues.