Re: [PATCH rust] Add new target for GNU/Hurd

[Sergey Bugaev]

Hi Damien, Vedant --

this is exciting! Some nitpicks (to add to what Amos says) inline.

On Sun, Jul 9, 2023 at 1:05 PM Damien Zammit <damien@zamaudio.com> wrote:
>  supported_targets! {
> +    ("i686-unknown-hurd-gnu", i686_unknown_hurd_gnu),

Arguably the target should be i686-unknown-gnu, and target_os = "gnu".
LLVM docs say "sys = none, linux, win32, darwin, cuda, etc.", so this
is certainly not supposed to be "nt" or "xnu".

But I'm sure this was not an oversight on your part, but a conscious
decision to go with "hurd" as the target_os, so it would make sense to
write down the reasoning for this.

Also I see that clang -v says i686-unknown-hurd-gnu0.9; should that be
i686-unknown-gnu too, like in gcc / binutils?
> diff --git a/library/std/src/os/hurd/fs.rs b/library/std/src/os/hurd/fs.rs
> new file mode 100644
> index 00000000000..464f7174982
> --- /dev/null
> +++ b/library/std/src/os/hurd/fs.rs
> @@ -0,0 +1,137 @@
> +#![stable(feature = "metadata_ext", since = "1.1.0")]
> +
> +use crate::fs::Metadata;
> +use crate::sys_common::AsInner;
> +
> +#[allow(deprecated)]
> +use crate::os::hurd::raw;
> +
> +/// OS-specific extensions to [`fs::Metadata`].
> +///
> +/// [`fs::Metadata`]: crate::fs::Metadata
> +#[stable(feature = "metadata_ext", since = "1.1.0")]
> +pub trait MetadataExt {
> +    /// Gain a reference to the underlying `stat` structure which contains
> +    /// the raw information returned by the OS.
> +    ///
> +    /// The contents of the returned `stat` are **not** consistent across
> +    /// Unix platforms. The `os::unix::fs::MetadataExt` trait contains the
> +    /// cross-Unix abstractions contained within the raw stat.
> +    #[stable(feature = "metadata_ext", since = "1.1.0")]
> +    #[rustc_deprecated(
> +        since = "1.8.0",
> +        reason = "deprecated in favor of the accessor \
> +                  methods of this trait"
> +    )]
> +    #[allow(deprecated)]
> +    fn as_raw_stat(&self) -> &raw::stat;

Since it's deprecated and not portable, should we even define it for a new port?

> +    #[stable(feature = "metadata_ext2", since = "1.8.0")]
> +    fn st_dev(&self) -> u64;
> +    #[stable(feature = "metadata_ext2", since = "1.8.0")]
> +    fn st_ino(&self) -> u64;
> +    #[stable(feature = "metadata_ext2", since = "1.8.0")]
> +    fn st_mode(&self) -> u32;
> +    #[stable(feature = "metadata_ext2", since = "1.8.0")]
> +    fn st_nlink(&self) -> u64;
> +    #[stable(feature = "metadata_ext2", since = "1.8.0")]
> +    fn st_uid(&self) -> u32;
> +    #[stable(feature = "metadata_ext2", since = "1.8.0")]
> +    fn st_gid(&self) -> u32;
> +    #[stable(feature = "metadata_ext2", since = "1.8.0")]
> +    fn st_rdev(&self) -> u64;
> +    #[stable(feature = "metadata_ext2", since = "1.8.0")]
> +    fn st_size(&self) -> u64;
> +    #[stable(feature = "metadata_ext2", since = "1.8.0")]
> +    fn st_atime(&self) -> i64;
> +    #[stable(feature = "metadata_ext2", since = "1.8.0")]
> +    fn st_atime_nsec(&self) -> i64;
> +    #[stable(feature = "metadata_ext2", since = "1.8.0")]
> +    fn st_mtime(&self) -> i64;
> +    #[stable(feature = "metadata_ext2", since = "1.8.0")]
> +    fn st_mtime_nsec(&self) -> i64;
> +    #[stable(feature = "metadata_ext2", since = "1.8.0")]
> +    fn st_ctime(&self) -> i64;
> +    #[stable(feature = "metadata_ext2", since = "1.8.0")]
> +    fn st_ctime_nsec(&self) -> i64;
> +    #[stable(feature = "metadata_ext2", since = "1.8.0")]
> +    fn st_birthtime(&self) -> i64;
> +    #[stable(feature = "metadata_ext2", since = "1.8.0")]
> +    fn st_birthtime_nsec(&self) -> i64;
> +    #[stable(feature = "metadata_ext2", since = "1.8.0")]
> +    fn st_blksize(&self) -> u64;
> +    #[stable(feature = "metadata_ext2", since = "1.8.0")]
> +    fn st_blocks(&self) -> u64;
> +    #[stable(feature = "metadata_ext2", since = "1.8.0")]
> +    fn st_flags(&self) -> u32;
> +    #[stable(feature = "metadata_ext2", since = "1.8.0")]
> +    fn st_gen(&self) -> u32;

Missing Hurd specifics: st_fstype, st_fsid (which is the same as
st_dev), st_author.

> diff --git a/library/std/src/os/hurd/raw.rs b/library/std/src/os/hurd/raw.rs
> new file mode 100644
> index 00000000000..475fcdcc4aa
> --- /dev/null
> +++ b/library/std/src/os/hurd/raw.rs
> @@ -0,0 +1,83 @@
> +//! NetBSD-specific raw type definitions
> +
> +#![stable(feature = "raw_ext", since = "1.1.0")]
> +#![rustc_deprecated(
> +    since = "1.8.0",
> +    reason = "these type aliases are no longer supported by \
> +              the standard library, the `libc` crate on \
> +              crates.io should be used instead for the correct \
> +              definitions"
> +)]
> +#![allow(deprecated)]
> +
> +use crate::os::raw::c_long;
> +use crate::os::unix::raw::{gid_t, uid_t};
> +
> +#[stable(feature = "raw_ext", since = "1.1.0")]
> +pub type blkcnt_t = u64;
> +#[stable(feature = "raw_ext", since = "1.1.0")]
> +pub type blksize_t = u64;
> +#[stable(feature = "raw_ext", since = "1.1.0")]
> +pub type dev_t = u64;
> +#[stable(feature = "raw_ext", since = "1.1.0")]
> +pub type fflags_t = u32;
> +#[stable(feature = "raw_ext", since = "1.1.0")]
> +pub type ino_t = u64;
> +#[stable(feature = "raw_ext", since = "1.1.0")]
> +pub type mode_t = u32;
> +#[stable(feature = "raw_ext", since = "1.1.0")]
> +pub type nlink_t = u64;
> +#[stable(feature = "raw_ext", since = "1.1.0")]
> +pub type off_t = u64;
> +#[stable(feature = "raw_ext", since = "1.1.0")]
> +pub type time_t = i64;

I don't think that's correct. For one thing, dev_t is __UWORD_TYPE
(like Rust usize) since glibc commit
0ec48e3337506fcd33abdd86b5ab9e331564b65c, and it was u32 before that.
This quite likely has to be split among arch-specific files.

> +#[stable(feature = "pthread_t", since = "1.8.0")]
> +pub type pthread_t = usize;
> +
> +#[repr(C)]
> +#[derive(Clone)]
> +#[stable(feature = "raw_ext", since = "1.1.0")]
> +pub struct stat {
> +    #[stable(feature = "raw_ext", since = "1.1.0")]
> +    pub st_dev: u64,
> +    #[stable(feature = "raw_ext", since = "1.1.0")]
> +    pub st_mode: u32,
> +    #[stable(feature = "raw_ext", since = "1.1.0")]
> +    pub st_ino: u64,
> +    #[stable(feature = "raw_ext", since = "1.1.0")]
> +    pub st_nlink: u32,
> +    #[stable(feature = "raw_ext", since = "1.1.0")]
> +    pub st_uid: uid_t,
> +    #[stable(feature = "raw_ext", since = "1.1.0")]
> +    pub st_gid: gid_t,
> +    #[stable(feature = "raw_ext", since = "1.1.0")]
> +    pub st_rdev: u64,
> +    #[stable(feature = "raw_ext", since = "1.1.0")]
> +    pub st_atime: i64,
> +    #[stable(feature = "raw_ext", since = "1.1.0")]
> +    pub st_atime_nsec: c_long,
> +    #[stable(feature = "raw_ext", since = "1.1.0")]
> +    pub st_mtime: i64,
> +    #[stable(feature = "raw_ext", since = "1.1.0")]
> +    pub st_mtime_nsec: c_long,
> +    #[stable(feature = "raw_ext", since = "1.1.0")]
> +    pub st_ctime: i64,
> +    #[stable(feature = "raw_ext", since = "1.1.0")]
> +    pub st_ctime_nsec: c_long,
> +    #[stable(feature = "raw_ext", since = "1.1.0")]
> +    pub st_birthtime: i64,
> +    #[stable(feature = "raw_ext", since = "1.1.0")]
> +    pub st_birthtime_nsec: c_long,
> +    #[stable(feature = "raw_ext", since = "1.1.0")]
> +    pub st_size: i64,
> +    #[stable(feature = "raw_ext", since = "1.1.0")]
> +    pub st_blocks: i64,
> +    #[stable(feature = "raw_ext", since = "1.1.0")]
> +    pub st_blksize: i32,
> +    #[stable(feature = "raw_ext", since = "1.1.0")]
> +    pub st_flags: u32,
> +    #[stable(feature = "raw_ext", since = "1.1.0")]
> +    pub st_gen: u32,
> +    st_spare: [u32; 2],
> +}

This is not the layout of struct stat on the Hurd at all. Please see
glibc:sysdeps/mach/hurd/bits/stat.h

> diff --git a/library/std/src/sys/unix/env.rs b/library/std/src/sys/unix/env.rs
> index 7f5e9b04dba..298b746e28c 100644
> --- a/library/std/src/sys/unix/env.rs
> +++ b/library/std/src/sys/unix/env.rs
> @@ -173,3 +173,14 @@ pub mod os {
>      pub const EXE_SUFFIX: &str = "";
>      pub const EXE_EXTENSION: &str = "";
>  }
> +
> +#[cfg(target_os = "hurd")]
> +pub mod os {
> +    pub const FAMILY: &str = "unix";
> +    pub const OS: &str = "hurd";

Same here, I think "OS" should be "gnu".

> +    pub const DLL_PREFIX: &str = "lib";
> +    pub const DLL_SUFFIX: &str = ".so";
> +    pub const DLL_EXTENSION: &str = "so";
> +    pub const EXE_SUFFIX: &str = "";
> +    pub const EXE_EXTENSION: &str = "";

That's quite some Windows-centric naming on their part, isn't it? :D

>      # Consider the direct transformation first and then the special cases
> @@ -287,6 +288,7 @@ def default_build_triple(verbose):
>          'i386': 'i686',
>          'i486': 'i686',
>          'i686': 'i686',
> +        'i686-AT386': 'i686',

Maybe we should just make uname not add AT386, that always bugs me.

Sergey