fuchsia: clean up module - #5127
Conversation
399a0a5 to
cc9e351
Compare
cc9e351 to
ddbf270
Compare
This comment has been minimized.
This comment has been minimized.
ddbf270 to
3783462
Compare
This comment has been minimized.
This comment has been minimized.
|
CI actually passes. There seems to be an issue with a glob import that is not used, but this has not |
3783462 to
53b65df
Compare
This comment has been minimized.
This comment has been minimized.
|
@rustbot blocked Like other deprecations, holding off until we can actually give the users an alternative. |
|
(https://snoozeth.is/db3wDy9dFKA) I will wait until Wed, 19 Aug 2026 08:31:22 UTC and then add label S-waiting-on-review and remove label S-blocked. @rustbot claim. |
53b65df to
b8d548c
Compare
This comment has been minimized.
This comment has been minimized.
|
There's some problems in Fuchsia. Each vendored libc implementation has a different implementation. Sometimes they can be unified. Other times it's not possible. An example of the latter is Should I also expose |
5d7d0ae to
48d5e6a
Compare
This comment has been minimized.
This comment has been minimized.
66e4bed to
dd07b99
Compare
off64_t type in Fuchsia70bef1c to
48d1e3d
Compare
4dd1622 to
15e946a
Compare
|
@rustbot ready |
|
I haven't done a completely final look-through here but at this point, holding off for a while to give the maintainers a chance to respond. |
|
Hi folks! Thanks for this from the fuchsia team! I’ll be a little delayed reviewing this but I asked other folks on the team that hopefully will be able to get to this sooner. |
| pub type intptr_t = isize; | ||
| pub type uintptr_t = usize; | ||
| pub type ssize_t = isize; | ||
| pub type ssize_t = crate::intptr_t; |
There was a problem hiding this comment.
I think it'd be more correct to use c_long here https://cs.opensource.google/fuchsia/fuchsia/+/main:zircon/third_party/ulib/musl/include/bits/alltypes.h;l=186.
There was a problem hiding this comment.
That's right. The reason why I left it as an intptr_t was that the type that
one aliases is isize, which we use later on in a C macro implemented as a
Rust free function, which itself uses <*const>::offset in Rust's libstd
(which expects an isize as its one parameter.)
I assumed that considering the Fuchsia project supports only 64-bit
architectures and of those supported, LP64 is prevalent, intptr_t (isize)
would be effectively equivalent to c_long.
Changed now. The one place where it's expected as an isize for
<*const>::offset has been made to use a primitive as cast. This shouldn't
silently overflow so long as the supported targets all follow LP64.
There was a problem hiding this comment.
I am a bit hesitant here: this is correct to the letter, but possibly not to the spirit:
- There has been discussion about whether
rustccould say thatusizeis compatible withsize_tandisizeis compatible withssize_t(considering it seems like POSIX is willing to get rid of the weird "only represent -1" requirement). - All but one platform currently has
ssize_t->isizeorssize_t->intptr_t->isize, even though they're probably technically defined to along
I don't think it's worth making this change with those couple things in consideration.
There was a problem hiding this comment.
So I'm guessing we should wait and hear back from the target maintainer?
Considering the change is minor, I will hold off from changing the patch
containing the change for now.
There was a problem hiding this comment.
I'd say just drop it, if there's a strong motivation to change things then we can do multiple platforms at once. I'm not really expecting that to be the case, though, given the first point above.
| pub f_files: crate::fsfilcnt_t, | ||
| pub f_ffree: crate::fsfilcnt_t, | ||
| pub f_favail: crate::fsfilcnt_t, | ||
| #[cfg(target_endian = "little")] |
There was a problem hiding this comment.
I think this is incorrect based off of https://cs.opensource.google/fuchsia/fuchsia/+/main:zircon/third_party/ulib/musl/include/sys/statvfs.h;l=16. Maybe we'd want this?
#[cfg(target_endian = "little")]
pub f_fsid: c_ulong,
pub __f_unused: Padding<[c_uchar; 2 * std::mem::size_of::<c_int>() - std::mem::size_of::<c_long>()]>,
#[cfg(not(target_endian = "little"))]
pub f_fsid: c_ulong,There was a problem hiding this comment.
There's no supported Rust target for Fuchsia whose endianness is not
little-endian, so that's why I skipped the whole conditional compilation aspect
in this instance.
I'm assuming the Fuchsia team doesn't discard adding support for some
big-endian target, then?
| pub struct pthread_attr_t { | ||
| __size: [u64; 7], | ||
| pub __name: *const c_char, | ||
| pub __c11: c_int, | ||
| pub _a_stacksize: crate::size_t, | ||
| pub _a_guardsize: crate::size_t, | ||
| pub _a_stackaddr: *mut c_void, | ||
| pub _a_detach: c_int, | ||
| pub _a_sched: c_int, | ||
| pub _a_policy: c_int, | ||
| pub _a_prio: c_int, | ||
| } |
There was a problem hiding this comment.
I'm guessing this is just an artifact from when we forked from musl a few years ago?
| pub si_code: c_int, | ||
| pub _pad: [c_int; 29], | ||
| _align: [usize; 0], | ||
| __si_fields: [c_int; 28], |
There was a problem hiding this comment.
From https://cs.opensource.google/fuchsia/fuchsia/+/main:zircon/third_party/ulib/musl/include/signal.h;l=137, I think this would be more correct:
__pad<[c_char; 128 - 2 * size_of::<c_int>() - size_of::<c_long>()]>,There was a problem hiding this comment.
I just added the whole anonymous union. It's not public but that should ensure
the size for that one field is correct.
| pub sigev_signo: c_int, | ||
| pub sigev_notify: c_int, | ||
| pub sigev_notify_function: fn(crate::sigval), | ||
| pub sigev_notify_function: Option<extern "C" fn(crate::sigval)>, |
There was a problem hiding this comment.
Since we don't have this type I don't think these changes are necessary.
There was a problem hiding this comment.
| pub const MAP_HUGE_SHIFT: u32 = 26; | ||
|
|
||
| // intentionally not public, only used for fd_set | ||
| cfg_if! { |
There was a problem hiding this comment.
Should these be deprecated rather than removed?
There was a problem hiding this comment.
I don't think so. They are not public and they were only used in the old
(before this commit) definition of fd_set. Removing them won't break anybody
and they're not used anywhere else.
This comment has been minimized.
This comment has been minimized.
Thanks for getting back. I've reviewed the changes and left some comments as Also, I always refer to the header files shipped under I'm wondering which place should we assume as being the definitive reference |
|
@rustbot label -S-waiting-on-maintainer |
| pub type intptr_t = isize; | ||
| pub type uintptr_t = usize; | ||
| pub type ssize_t = isize; | ||
| pub type ssize_t = crate::intptr_t; |
There was a problem hiding this comment.
I am a bit hesitant here: this is correct to the letter, but possibly not to the spirit:
- There has been discussion about whether
rustccould say thatusizeis compatible withsize_tandisizeis compatible withssize_t(considering it seems like POSIX is willing to get rid of the weird "only represent -1" requirement). - All but one platform currently has
ssize_t->isizeorssize_t->intptr_t->isize, even though they're probably technically defined to along
I don't think it's worth making this change with those couple things in consideration.
@erickt would you be able to clear this up a bit? We're trying to figure out what portions of CodeSearch get pulled together to make the SDK that Rust should be supporting. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
CI is failing because there's a type, That one type is declared under @rustbot ready |
This comment has been minimized.
This comment has been minimized.
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.
LGTM aside from the failure, @erickt if you get the change to rereview soon that would be appreciated, otherwise I'll merge in a few days.
CI is failing because there's a type,
epoll_data, that is triggering a warning about being used even though it's marked#[deprecated]. This is not really something I should fix in this PR because the reason why that happens is unrelated to the submitted patchset.
It's certainly not unrelated if applying the patchset causes CI to fail. It's fine to include a patch adapting for new behavior before a patch that would otherwise break them (or to do that in a separate PR), or, if it's tiny, just change it in the same patch.
@rustbot author since of course merging is blocked on that
That one type is declared under
s_no_extra_traits, which in the case of unions, will add a manualDebugimpl after declaring the type. If you mark the type as deprecated, then the impl shouldn't exist either, as otherwise it would count as using a deprecated item.
Deprecation is a hint to users of libc, what we do internally doesn't really matter. We have to use our own deprecated API all the time.
Fixes `fd_set`'s array field. The size was wrong [^1]. Now unused constants have been removed. The PR has been separated from the commit fixing miscellaneous records. [^1]: https://cs.opensource.google/fuchsia/fuchsia/+/main:zircon/third_party/ulib/musl/include/sys/select.h;l=23-25;drc=7c5e521391fddb98fd8f6970da7c410899ddf5cf
Mirror type aliases. They were previously only equivalent. This changes them to using C types. Remove architecture-specific definitions. They were the same.
Mirror constants. They were previously equivalent.
Fills in the pending definition. It's opaque in other platforms. It's public in the Fuchsia headers [^1]. Solves a `FIXME`. [^1]: https://cs.opensource.google/fuchsia/fuchsia/+/main:zircon/third_party/ulib/musl/include/stdio.h;l=47-50;drc=7c5e521391fddb98fd8f6970da7c410899ddf5cf
Remove nonexistent types. They are not part of the Fuchsia IDK. Replace `off64_t` in two routines. It's now `off_t`. `off64_t` doesn't exist in Fuchsia.
Add an `allow(deprecated)` to `s_no_extra_traits`'s `impl Debug` expansion on `union`s to avoid warnings on deprecated items. This fixes the prior patch's deprecation warnings on `epoll_data`.
Remove uses of 32-bit target conditional compilation. Fuchsia does not
support 32-bit targets [^1]. This is unnecessary.
Fix missing fields in records. No particular header file is targetted.
All wrong records have been fixed. c.f. all headers under
`arch/{arch}/sysroot/include` in the IDK.
Fix `cpu_set_t` macros [^2]. They were wrongly defined. They neither
returned the right values.
[^1]:
https://fuchsia.dev/fuchsia-src/contribute/governance/rfcs/0111_fuchsia_hardware_specifications#required_64-bit_cpu_and_platform
[^2]:
https://cs.opensource.google/fuchsia/fuchsia/+/main:zircon/third_party/ulib/musl/include/sched.h;drc=e99708804b3800218154f1cd153f4300c9ef66e6
Fix definition of types in x86_64. `mcontext_t` provided opaque field. `ucontext_t` had a slightly wrong definition. New records have been introduced. They are particular to `mcontext_t`. Add definitions in AArch64 and RISC-V64. They were missing. They are different for all three target architectures. These are declared in [^1]. [^1]: https://cs.opensource.google/fuchsia/fuchsia/+/main:zircon/third_party/ulib/musl/include/bits/signal.h;drc=bbff36aa507d5be390c5f09c8939bc3b7a2ebaf9
Remove opaque types. Replace with proper fields.
Remove conditional defintion for 32-bit targets. Fuchsia has no 32-bit
targets.
Static initializers needed modification. They are specified as `{ 0 }`
in POSIX for C. The new definition replicates that. It zeroes/nullifies
the new fields. Constants were used to initialize the previous opaque
array. These have been deprecated.
These are declared in [^1].
[^1]:
https://cs.opensource.google/fuchsia/fuchsia/+/main:zircon/third_party/ulib/musl/include/pthread.h;drc=e577fd0e9d34337a36d410aa89866c77ee95493c
|
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. |
|
I think it's done now. I went for the easy fix; Adding an @rustbot ready |
Description
This is a clean-up patch. Changes are concerned with the
fuchsiamodule.There were lacking definitions. There were records with the wrong alignment. There were types that don't exist upstream. This has been fixed. But there's more stuff to fix.
Testing is pending. This time testing is required. Two supported Fuchsia targets are tier 2. There's no CI job for them.
Sources
Paths are relative to the downloadable SDK. Look under the
objdirectory. Changes have been made relative to allNEXTreleases.wchar_tis not different across architectures. No specific definition was found. Verifying the current definition was not possible.nlink_tis not different across architectures.blksize_tis not different across architectures.ssizet_thad a wrong definition. Other_t-terminated types had similar issues. Those will not be further repeated in this list.cs.opensource.google. This includes suffixed offset types. These are omitted from further mention.stathad a mismatched definition.glob_thad a mismatched definition.signal.htypes often had a mismatched definition. This header file appears later on in this source list. They refer to different paths. They contain different definitions.siginfo_t.sigevent.sigaction.SIG_DFLand others are function pointers. They are cast to function pointers from integers. They are sometimesNULL. This is not allowed in Rust. I have wrapped them inOption. They are allNonenow. They should not all beNone. Transmutation from a pointer without provenance to a function pointer is required. This causes compile-time errors.stat64doesn't exist.ipc_permdoesn't exist.epoll_datadoesn't exist. Deprecation lints here keep popping up. I've attempted to silence them. My attempts have been futile.epoll_eventdoesn't exist.ff_effectdoesn't exist.termios2doesn't exist.sysinfodoesn't exist.mq_attrdoesn't exist.sockaddr_nldoesn't exist.RLIM_SAVED_MAXdoesn't exist.RLIM_SAVED_CURdoesn't exist.RLIM_INFINITYdoesn't exist.RLIMIT_RTTIMEdoesn't exist.RLIMIT_NLIMITSdoesn't exist.There were conditional checks against 32-bit targets. Rust does not support 32-bit Fuchsia targets. The Fuchsia project does not support 32-bit machine word targets.
There was an oddity with
pthread.htypes. They were defined with a single field. This field sometimes had size equivalent to the sum of upstream fields. This has been replaced with the real fields. This has also fixed some size and alignment mismatches.This has required changes in macros. An attempt has been made to fit the POSIX definitions. This means expanding
staticinitializers to{ 0 }. The new type definitions get all their fields zeroed. Some are made into null pointers. Should they be made intoOption::Nones? They are not function pointer fields.Macros needed fixing. Their definition did not return a value. But a value was returned upstream. Some types needed small fixes.
m_contexthad a mismatched definition. Some modules didn't have a definition.u_contexthad a mismatched definition. Some modules didn't have a definition.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