RFC: Deprecate type aliases in std::os::*::raw

[alexcrichton]

Deprecate type aliases and structs in std::os::$platform::raw in favor of
trait-based accessors which return Rust types rather than the equivalent C type
aliases.

[cuviper]

Right, but as your motivation says, the current FFI compatibility is a bit of an illusion anyway, thanks to LFS -DFILE_OFFSET_BITS.

[alexcrichton]

Yeah that's a good point! There's sort of a case to be made with "well they're FFI compatible if you pass no flags to the compiler", but that's shaky at best at this point.

[cuviper]

I'd aim for lossless casts only. Widening should always be safe, but it would be bad to lose the sign on something like time_t. For another example, dev_t is u64 on Linux and i32 on OSX -- there's no universal upcast for these (i128?), so it will have to have platform-specific signedness. (Consider using integer From/Into instead of manual as casts to keep things safe.)

[alexcrichton]

Yeah I'd definitely want to only go below 64 bits if we're 100% absolutely positive that they're not necessary. For example anything related to timestamps, sizes, amounts, etc, will be 64 bits. I also think it's fine that even if dev_t is i32 on OSX if we return it as u64 that seems fine to me (it'll just require some fiddling to get the real value back out).

I guess I'm mostly wondering here that if all current platforms agree that a type should be signed, should we return it as i64? I think the answer should be yes, but it makes it somewhat more difficult to select what the best type to return is.

[cuviper]

It's a tricky question, and I think it has to be considered case-by-case. Take file size, for instance -- I can't think of any reasonable scenario where this would be negative, so Metadata::len() -> u64 makes sense. But every platform uses a signed type for stat.st_size, usually signed off_t, so seek offsets can be negative. Rust's Seek and SeekFrom already reflects this signed/unsigned conceptual split. So what should MetadataExt::size() return?

[alexcrichton]

Hm that is a good question! I had no idea everything was signed everywhere.

In some sense it doesn't matter too much so long as we ship the bits in some form out of the standard library in a lossless fashion, even if it's just painful to recover on the other end. I do think, though, that for MetadataExt::size it should definitely return u64 (like the standard library already does)

[briansmith]

For example anything related to timestamps, sizes, amounts, etc, will be 64 bits.

I think it is worth thinking about this in the context of very (small) embedded devices where it is desirable for variables to hold these types of values to be small in memory, and which might trade off support for large things in favor of smaller memory requirements.

I think timestamps are the only thing that should be assumed to be larger than 32-bits.

[tbu-]

What should happen if you pass a large integer to such a u64 parameter on a platform that internally has u32 as a type? Should the Rust code panic? Should it silently truncate the value?

[cuviper]

Most of the types in question are only return values, AFAICS, except for mode_t which I think is fine to leave as its smaller native type. Any specific parameters you're concerned about?

[tbu-]

Well, what should you do if you want to use those integers? Currently, you could pass them to the relevant C APIs directly, but after this change you have to do some kind of cast.

[cuviper]

It's stuck between a rock and a hard place. We can't both support large files in libstd and still support passing those values directly to non-LFS C APIs. As the RFC says, the new advice is to use types from the libc crate for FFI, and it's up to you to make sure all your LFS ducks are in a row.

[alexcrichton]

@tbu- yeah the burden would be on consumers of libstd to cast appropriately and panic if out-of-bounds. I would expect that 95% of the time the types would always fit, but for the cases like LFS that @cuviper mentions it could be that the two applications are compiled differently (e.g. linking to C which isn't using LFS and hence can't handle file sizes > 4GB on 32-bit Linux).

[remram44]

Rendered

[alexcrichton]

🔔 This RFC is now entering its week-long final comment period 🔔

[alexcrichton]

The libs team discussed this today and the conclusion was that there is not a clear enough path moving forward that doesn't involve theoretical breakage. We decided to leave this in FCP while I gather some statistics and flesh out a more concrete plan.

[alexcrichton]

Ok, I've done some analysis of what this change might have. The numbers here were gathered against these changes to the standard library, which I believe fully implement this RFC (enabling us to move to LFS on Linux at the same time).

A crater report reports four regressions, three of which were spurious. I have submitted a PR for the other regression. Specifically, this regression involved code that looked like:

libc::foo(..., value as std::os::raw::off_t)

In other words, the regression came from the reliance that the types in std agreed with those in libc, which this RFC would be breaking.

I then compiled Servo against a standard library with the patches above applied. With the fs2-rs changes Servo compiled for both x86_64-unknown-linux-gnu and arm-linux-androideabi with no other modifications necessary.

Finally, I did a manual survey of all code on crates.io for usage of MetadataExt or the std::os::*::raw types. The investigation revealed two cases which would break

• fs2-0.2.2 - visible breakage
  • passed a std off_t into a libc off_t
  • fixed in danburkert/fs2-rs#2 (now merged)
• gphoto2-sys-0.1.1 - silent breakage
  • uses time_t in FFI
  • fixed in dcuddeback/gphoto2-sys#1 (now merged)

The following usage was all observed but would not break from the change in this RFC (confirmed from the crater run)

• cargo-extras-0.3.0
  • casts .mtime() to u64 immediately
• cargo-script-0.1.4
  • casts .mtime() to u64 immediately
• devicemapper-0.3.2
  • mode() & constant == constant
  • assumes rdev is u64 (ok)
• filetime-0.1.9
  • immediately casts
  • only user of as_raw_stat
• fs2-0.2.2
  • immediately casts
• lalrpop-0.9.0
  • compares mtime() ot mtime()
• lalrpop-snap-0.9.0
  • compares mtime() ot mtime()
• logwatcher-0.1.0
  • assumes ino() returns u64 (ok)
• pnacl-build-helper-1.4.10
  • immediately casts mtime()
• psutil-1.0.0
  • reads uid/gid
• systemd-crontab-generator-1.0.0-rc6
  • reads uid
  • casts mtime
• tango-0.3.3
  • casts mtime immediately
  • prints mtime
• utime-0.1.3
  • casts atime/mtime immediately
  • equates atime/mtime with constant
• walkdir-0.1.5
  • compares dev/ino with another dev/ino
• keyutils-0.2.0
  • uses uid/gid
• procinfo-0.1.1
  • uses uid/gid/pid
• tar-0.4.0
  • creates permissions from a mode_t

---

My conclusion from this is that we can likely take the path set forth in this RFC (aka merge the two commits above at the same time). In summary, they:

• Deprecate all raw types that aren't in std::os::raw
• Expand the MetadataExt trait on all platforms for method accessors of all fields. The fields now returned widened types which are the same across platforms (consistency across platforms is not required, however, it's just convenient)
• [breaking] Change the definition of all std::os::*::raw type aliases to correspond to the newly widened types that are being returned on each platform.
• [breaking] Change the definition of std::os::*::raw::stat on Linux to match the LFS definitions rather than the standard ones.

The breaking changes are specifically breaking if you assume std/libc or std/gcc agree on the types, which from the above analysis looks like is limited to just a few crates (both of which have PRs to be updated).

[alexcrichton]

We discussed this during libs team triage today, and the conclusion was that given the limited fall out we should push forward with this. Thanks again for the discussion everyone!

[cuviper]

🎉