hurd: clean up module#5242
Conversation
rustbot
added
S-waiting-on-review
stable-nominated
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
I just saw the one thing then noticed the links, are you sure that's the most recent source? Per https://cgit.git.savannah.gnu.org/cgit/hurd/glibc.git/log/?h=t/sysvshm the branch you're linking hasn't been updated since 2021, and none of the branches at https://cgit.git.savannah.gnu.org/cgit/hurd/glibc.git/refs/heads are any newer.
There was a problem hiding this comment.
"hurd: remove duplicate routines": LGTM @ 62ccc91
| #[cfg(target_pointer_width = "32")] | ||
| pub l_type: c_int, | ||
| #[cfg(target_pointer_width = "32")] | ||
| pub l_whence: c_int, | ||
| #[cfg(target_pointer_width = "64")] | ||
| pub l_type: c_short, | ||
| #[cfg(target_pointer_width = "64")] | ||
| pub l_whence: c_short, | ||
| pub l_start: __off_t, | ||
| pub l_len: __off_t, | ||
| pub l_start: off_t, | ||
| pub l_len: off_t, | ||
| pub l_pid: __pid_t, | ||
| } | ||
|
|
||
| pub struct flock64 { | ||
| #[cfg(target_pointer_width = "32")] | ||
| pub l_type: c_int, | ||
| #[cfg(target_pointer_width = "32")] | ||
| pub l_whence: c_int, | ||
| #[cfg(target_pointer_width = "64")] | ||
| pub l_type: c_short, | ||
| #[cfg(target_pointer_width = "64")] | ||
| pub l_whence: c_short, | ||
| pub l_start: __off_t, | ||
| pub l_start: __off64_t, | ||
| pub l_len: __off64_t, | ||
| pub l_pid: __pid_t, |
There was a problem hiding this comment.
"hurd: use gnu_file_offset_bits64 to expose lfs": Is this accurate? Looking at https://github.com/bminor/glibc/blob/04e750e75b73957cf1c791535a3f4319534a52fc/sysdeps/mach/hurd/bits/types/struct_flock.h and https://github.com/bminor/glibc/blob/04e750e75b73957cf1c791535a3f4319534a52fc/sysdeps/mach/hurd/i386/bits/types/struct_flock.h, it looks like c_short is the right type on non-x86-32.
There was a problem hiding this comment.
just reviewed it with the updated glibc sources.
There was a problem hiding this comment.
yes, only i386 is to keep the non-posix-compatible int type for l_type/whence
|
Yeah - the author of #4127 is the lead Hurd developer as I understand it, and that PR links to the regular glibc sourceware repos. I assume that at some point the deviations were resolved and they just switched to developing upstream. @rustbot author since there's probably some things to revisit with that in mind. |
|
Reminder, once the PR becomes ready for a review, use |
There was a problem hiding this comment.
"hurd: deprecate lfs types and routines": Note that like the others, I am not comfortable deprecating the time bindings while users don't have a clean-cut solution.
There was a problem hiding this comment.
so, to a first approximation, how would a clean-cut solution look like?
There was a problem hiding this comment.
I think that what's at #4805 is the best bet, then we can find+replace these back once everything else is ready.
There was a problem hiding this comment.
I'm a bit surprised to see these deprecation warnings indeed, since the Linux port does not have them on its *64 variants.
There was a problem hiding this comment.
Claiming The definitions are equivalent when compiling on a 64bit target is wrong: yes on the 64b target they are equivalent, but then the programmer will migrate to the non-64 version and thusly break the 32bit builds unless gnu_file_offset_bits64 is used. So at most the warnings should be enabled only when the programmer did request for gnu_file_offset_bits64, in which case, yes, using the *64 variants is spurious.
There was a problem hiding this comment.
but again, better do this on all targets (Linux too notably) altogether rather than just Hurd
There was a problem hiding this comment.
those deprecations have now been replaced with comments. they were made before the plan in #4805 had been formulated. now they're just comments tagging some item for future deprecation.
also,
Claiming The definitions are equivalent when compiling on a 64bit target is wrong: yes on the 64b target they are equivalent, but then the programmer will migrate to the non-64 version and thusly break the 32bit builds unless gnu_file_offset_bits64 is used. So at most the warnings should be enabled only when the programmer did request for gnu_file_offset_bits64, in which case, yes, using the *64 variants is spurious.
aren't libc crate users just meant to take care of having the unsuffixed types be used on 64-bit targets, and of having gnu_file_offset_bits64 set on 32-bit targets?
if they're building for multiple targets, surely they must take into account platform differences.
There was a problem hiding this comment.
if they're building for multiple targets, surely they must take into account platform differences.
That's theory...
In practice, people don't think about everything, only really test on 64b platforms, etc. And notably:
of having gnu_file_offset_bits64 set on 32-bit targets?
AFAIK that's a new thing, not everybody will be doing this for a long time still, so we have to care for them not getting shot in the foot too much.
There was a problem hiding this comment.
Noted. I believe the plan at #4805 should better align with having library
users get more comfortable over time with these options.
This comment has been minimized.
This comment has been minimized.
6be7cc8 to
7159a66
Compare
|
@rustbot ready |
| pub type __squad_type = crate::__int64_t; | ||
| pub type __uquad_type = crate::__uint64_t; | ||
| pub type __squad_type = __int64_t; | ||
| pub type __uquad_type = __uint64_t; |
There was a problem hiding this comment.
"hurd: add proper handling of lfs": looks pretty reasonable to me but I'd like the maintainer to ack
| bits: [u32; 32], | ||
| #[cfg(not(all(target_pointer_width = "32", not(target_arch = "x86_64"))))] | ||
| bits: [u64; 16], | ||
| __bits: [__cpu_mask; 1024 / (8 * size_of::<__cpu_mask>())], |
There was a problem hiding this comment.
"hurd: adjust cpu_set_t definition": use __CPU_SETSIZE and __NCPUBITS intermediates like they do rather than hardcoding, just as nonpublic constants.
There was a problem hiding this comment.
decided i would make them public. __CPU_SETSIZE was already public before this pr so i made __NCPUBITS also public.
There was a problem hiding this comment.
"hurd: remove duplicate definitions": lgtm @ 8b36f4b
|
@rustbot author |
This comment has been minimized.
This comment has been minimized.
No, it's not.... The last commit there was 6 years ago... |
| pub st_fstype: c_int, | ||
| pub st_dev: __fsid_t, /* Actually st_fsid */ | ||
| pub st_ino: __ino_t, | ||
| pub st_fsid: __fsid_t, |
There was a problem hiding this comment.
No, we really want st_dev here, to match what the rest of the world expects. st_fsid is the real name in the structure, but st_dev is an alias for it, and that's what people expect. See #3785
|
|
||
| pub fn dirfd(dirp: *mut crate::DIR) -> c_int; | ||
|
|
||
| #[link_name = "__xpg_strerror_r"] |
There was a problem hiding this comment.
Why dropping this?
See https://sourceware.org/git/?p=glibc.git;a=blob;f=string/string.h;h=743395b3e3b2dd52194b1a6b8c34df4f8b56d8a1;hb=HEAD#l464 and linux does the same (except on musl, which is not ported to GNU/Hurd)
There was a problem hiding this comment.
did that because that one declaration exists when !__USE_GNU 1, but throughout the libc crate there's definitions affecting the hurd which assume __USE_GNU.
Footnotes
There was a problem hiding this comment.
assuming gnu functions are available is not the same as assuming that it's the gnu behavior that is desired.
Really, the linux crate does use the xpg redirection: https://github.com/rust-lang/libc/blob/main/src/unix/linux_like/linux_l4re_shared.rs#L1699 , and thus programs expect the xpg behavior, not the gnu behavior.
There was a problem hiding this comment.
i still don't get it. the linux definitions don't affect the hurd but i'm assuming you mean that as an example. why should we provide bindings to the non-gnu version if we have assumed otherwise in other parts of the codebase? granted, the current signature would have to change to that of the gnu version, but that can be readily done as we should be capable of breaking the api on tier 3 targets.
There was a problem hiding this comment.
the linux definitions don't affect the hurd
Sure. But the linux definition affects what people believe the correct behavior is supposed to be.
And in glibc, the definition of strerror_r is exactly the same on Linux and on the Hurd, so it makes full sense to expose the same interface in the linux crate and in the hurd crate.
why should we provide bindings to the non-gnu version if we have assumed otherwise in other parts of the codebase?
Because as I said above, “assuming gnu functions are available is not the same as assuming that it's the gnu behavior that is desired”. And I indeed took the Linux case as an example of this, but it's not only that: it's the very same function, with the very same file inside glibc. So really no reason to diverge from Linux on this.
There was a problem hiding this comment.
So IIUC, we're skipping the actual definition to match the Linux definition we
already expose so that users aren't surprised by the GNU/Hurd definitions
differing? If so, done.
There was a problem hiding this comment.
by the GNU/Hurd definitions differing?
The GNU/Hurd definition of strerror_r does not differ from the GNU/Linux definition. Both Linux and Hurd implementations of strerror_r come from the same glibc source code.
done
Thanks.
There was a problem hiding this comment.
So we're basically assumming that both GNU/Linux and the GNU/Hurd do not have
_GNU_SOURCE defined and would always want the POSIX version of this routine?
I understand that this seems to be the default from 1, but I'm not
sure.
Footnotes
There was a problem hiding this comment.
So we're basically assumming that both GNU/Linux and the GNU/Hurd do not have
_GNU_SOURCE defined and would always want the POSIX version of this routine?
I'm basically assuming that what people will want is what people are currently getting: all ports are currently returning an int, i.e. the posix behavior. So better just align on that, otherwise we will get unexpected issues.
Really, in general any divergence from what the rest of the world is currently doing only brings troubles.
|
|
||
| #[cfg_attr( | ||
| all(gnu_time_bits64, not(gnu_file_offset_bits64)), | ||
| link_name = "__glob64_time64" |
There was a problem hiding this comment.
? this doesn't match the conditions of https://sourceware.org/git/?p=glibc.git;a=blob;f=posix/glob.h;h=4c824eb0d30ec557f9c5d5e1d85d823e1a1900c6;hb=HEAD#l158
And note that GNU/Hurd doesn't define __USE_TIME64_REDIRECTS
There was a problem hiding this comment.
fixed. there's one thing to note, though: we recently made it so that our cfgs to handle redirects of time64 and fileoffset64 could not be set separately (though at present we only issue a warning at compile time.) this may conflict with the behavior in upstream glibc where you seem to be capable of opting into one or the other.
| )] | ||
| #[cfg_attr( | ||
| all(gnu_file_offset_bits64, not(gnu_time_bits64)), | ||
| link_name = "glob64" |
There was a problem hiding this comment.
the link_names must be glob64free, not glob64
There was a problem hiding this comment.
I believe it's actually none of those; globfree64 seems to be the right
choice 1. Fixed now.
Footnotes
| flags: c_int, | ||
| errfunc: Option<extern "C" fn(epath: *const c_char, errno: c_int) -> c_int>, | ||
| pglob: *mut glob64_t, | ||
| pglob: *mut glob_t, |
There was a problem hiding this comment.
the type must remain glob64_t, otherwise this will be bogus in the non-gnu_file_offset_bits64 case
There was a problem hiding this comment.
Will it, now? For all we care on the Rust side of things, the types for
glob_t and glob64_t are the same across FFI boundaries. The function
pointer fields are the only ones that differ upstream, but those we currently
choose to expose as both *mut c_void and private.
There was a problem hiding this comment.
But don't we want to enforce through typing that a glob_t returned by glob is freed by globfree, and a glob64_t returned by glob64 is freed by globfree64?
There was a problem hiding this comment.
I mean, this is a set of C bindings. Actually providing good typing would
likely involve setting up a higher-level abstraction that enforces relatively
well-defined semantics. But this is not really a point of contention for me, so
I can change it if it seems more coherent to you.
There was a problem hiding this comment.
Yes, I believe coherency is important, to avoid people getting surprised by what they read
| since = "0.2.187", | ||
| note = "Use `globfree` instead. Their definitions are equivalent." | ||
| )] | ||
| pub fn globfree64(pglob: *mut glob_t); |
There was a problem hiding this comment.
the type must remain glob64_t, otherwise this will be bogus in the non-gnu_file_offset_bits64 case
07191f1 to
8bc9118
Compare
This comment has been minimized.
This comment has been minimized.
|
@rustbot ready |
|
@rustbot label -S-waiting-on-review |
|
@rustbot label -S-waiting-on-maintainer |
|
@tgross35 I didn't find in the documentation what more I should be doing to tell that this looks ready to me to go in, so just pinging to make sure it doesn't stay stuck :) |
I'm not sure if the |
The
|
|
I don't think the tests in Though of course, you may have to use some of the |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Just a few followup bits. Thank you @sthibaul for a very thorough review!
@tgross35 I didn't find in the documentation what more I should be doing to tell that this looks ready to me to go in, so just pinging to make sure it doesn't stay stuck :)
Removing the label and pinging is all :) I tend to go through libc PRs in batches so sometimes it takes me a bit.
cargo testat the root doesn't seem to be testing much. Inlibc-testthere seems to be more content, but not that much, and it seems it wasn't ported to the Hurd target.
By default cargo test only runs the root crate which is pretty much nothing. libc-test is really what we rely on since it looks at all the public API and compares the Rust and C versions (using ctest). I wouldn't be surprised if this was never run on Hurd though, unfortunately (indeed there is no test_hurd function).
If it did work then running once as-is then, once time64 support lands, once with RUSTFLAGS="--cfg=libc_unstable_gnu_time_bits=\"64\"" would be the thing to do (that's the relevant part of https://github.com/rust-lang/libc/blob/47a05e94e2448665b27eb715e7f23f13d7c152bd/ci/run.sh).
| s_no_extra_traits! { | ||
| pub union __c_anonymous___mbstate_t___value { | ||
| __wch: c_int, | ||
| __wchb: [c_char; 4], | ||
| } | ||
| } |
There was a problem hiding this comment.
"hurd: fill in definition for fpos_t": This need not be public
| pub struct fpos_t { | ||
| __pos: off_t, | ||
| __state: __mbstate_t, | ||
| } | ||
|
|
||
| pub struct __mbstate_t { | ||
| __count: c_int, | ||
| __value: __c_anonymous___mbstate_t___value, | ||
| } |
There was a problem hiding this comment.
"hurd: fill in definition for fpos_t": __mbstate_t doesn't need to be public
This is also adding traits that we don't need, put them in s_no_extra_traits which will allow dropping the custom impls for __c_anonymous___mbstate_t___value.
| #[cfg_attr( | ||
| all(gnu_file_offset_bits64, gnu_time_bits64), | ||
| link_name = "__glob64_time64" | ||
| )] | ||
| #[cfg_attr( | ||
| all(gnu_file_offset_bits64, not(gnu_time_bits64)), | ||
| link_name = "glob64" | ||
| )] | ||
| pub fn glob( | ||
| pattern: *const c_char, | ||
| flags: c_int, | ||
| errfunc: Option<extern "C" fn(epath: *const c_char, errno: c_int) -> c_int>, | ||
| pglob: *mut crate::glob_t, | ||
| pglob: *mut glob_t, | ||
| ) -> c_int; | ||
| pub fn globfree(pglob: *mut crate::glob_t); | ||
|
|
||
| #[cfg_attr( | ||
| all(gnu_file_offset_bits64, gnu_time_bits64), | ||
| link_name = "__globfree64_time64" | ||
| )] | ||
| #[cfg_attr( | ||
| all(gnu_file_offset_bits64, not(gnu_time_bits64)), | ||
| link_name = "globfree64" | ||
| )] | ||
| pub fn globfree(pglob: *mut glob_t); | ||
|
|
||
| #[cfg_attr(gnu_time_bits64, link_name = "__glob64_time64")] | ||
| pub fn glob64( | ||
| pattern: *const c_char, | ||
| flags: c_int, | ||
| errfunc: Option<extern "C" fn(epath: *const c_char, errno: c_int) -> c_int>, | ||
| pglob: *mut glob64_t, | ||
| ) -> c_int; | ||
| #[cfg_attr(gnu_time_bits64, link_name = "__globfree64_time64")] | ||
| pub fn globfree64(pglob: *mut glob64_t); |
There was a problem hiding this comment.
"hurd: add proper handling of lfs": could you clarify the interaction with gnu_time_bits64 here? time64 bits aren't otherwise touched in this PR, time_t remains a long, so is there going to be a mismatch somewhere?
At first glance I'd expect dropping the gnu_time_bits64 portions from here and including them in a followup time64 PR would be more accurate, but I'm not sure what the exact effects are.
There was a problem hiding this comment.
It would make a difference if we chose to expose the function pointer fields of
the glob_t type. Currently, we have them as non-public Padding fields with
the size and alignment requirement of a void *. That's also why we can
deprecate glob64_t and advise in favor of using glob_t. These types differ
upstream only in their function pointer fields.
The actual function pointers upstream take stats, which itself contains one of
timespec or time_t (depending on some feature test macros.) timespec
contains time_t fields, so yes, whether gnu_time_bits64 is set or not would
make a difference if we ever chose to expose those fields of either (if not
both) glob_t and glob64_t.
That's currently not the case, and I assumed it would continue being so.
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Add GNU/Hurd definition and skip the opaque type definition in the top-level `unix` module. The definition fits that of [^1]. [^1]: https://sourceware.org/git/?p=glibc.git;a=blob;f=libio/bits/types/__fpos_t.h;h=bb04576651b9097b3027e4299cc30c88f334535f;hb=b5eecf35d91a3b29cff80b731bbc6bcce6863827
Add uses of the `gnu_file_offset_bits64` `cfg` to the `hurd` module. GNU/Hurd systems use glibc and that includes both the file offset and `time_t` feature test macros. This patch ensures the right definition is exposed on 64-unsuffixed types. Affected records don't directly use the `cfg` to define their fields conditionally. They instead use the types not prefixed with two underscores to define their fields. These types are themselves conditionally defined. This avoids cluttering both the type alias definition and the record definition.
Modify definition of `cpu_set_t` to use the same constant expression as used upstream [^1]. This resolves to the same values as before but relies on the conditional definition of the type `__cpu_mask` is defined in terms of. [^1]: https://sourceware.org/git/?p=glibc.git;a=blob;f=posix/bits/cpu-set.h;h=ddb79cefc2e5d93003f8da84eca6302f99153a22;hb=b5eecf35d91a3b29cff80b731bbc6bcce6863827
Remove routine bindings that were already present in the `unix` module. Those in the top-level `unix` module were already outfit with the corresponding link name redirection under one of `gnu_file_offset_bits64` or `gnu_time_bits64`. They have been preferred over those in the `hurd` submodule.
Add deprecation notices to `glob64_t` and 64-suffixed routines. The bindings we expose have no difference whatsoever between `glob_t` and `glob64_t`. Differing fields are exposed here as `void *`s.
Add deprecation notice to `fallocate64` because this routine is only exposed under Linux in glibc [^1]. [^1]: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/bits/fcntl-linux.h;h=587b815124318864320973b864899b19466e46b8;hb=HEAD#l450
Add comments annotating file offset types and routines whose identifier is 64-suffixed as slated for deprecation and eventual removal. this holds only for 64-bit targets, so the comment adds a "parameter" to the FIXME that will be found+replaced with a deprecation attribute later on. These types otherwise provide redundant definitions.
4 participants
Description
This PR consists of an overall clean up of the GNU Hurd module. It adds proper
handling of types and routines when
_USE_FILE_OFFSET64is defined by usingthe existing
gnu_file_offset_bits64cfg. It also fixes a type definitionand more closely mirrors those of upstream.
It also adds a number of future deprecation annotations. The annotations on
type aliases apply only to 64-bit targets. Other annotations apply to both
64-bit targets and any target with the afore mentioned
cfgset.Finally, there were some routines that were duplicated in both the
unixmodule and the
unix/hurdmodule. These have been removed from the latter.This decision was made because the
unixmodule's definitions already includedthe right link name redirection for bindings where the actual identifier is the
64-bit suffixed one.
Sources
Checklist
libc-test/semverhave been updated*LASTor*MAXare included(see #3131)
cd libc-test && cargo test --target mytarget);especially relevant for platforms that may not be checked in CI
@rustbot label +stable-nominated