linux(uclibc): add cfg option for file offset bindings on 32-bit targets#5165
linux(uclibc): add cfg option for file offset bindings on 32-bit targets#5165dybucc wants to merge 5 commits into
cfg option for file offset bindings on 32-bit targets#5165Conversation
8755269 to
c883cf6
Compare
|
It would be ideal if this could be gated behind a
That very well could have been a copy+paste error, I very much doubt anybody is using these structs on uclibc today to the point that these kind of bugs would show up. |
dybucc
changed the title
19237c8 to
0ca05d1
Compare
bfeb785 to
c120d9a
Compare
|
Just read through that PR and the comments. I'm not quite sure I understand why that is the case. Hopefully the maintainer can clear things up (not pinging again.) I've already updated the PR with support for the A bunch of the suffixed type definitions were removed from the |
|
The CI label is wrong. At one point, I changed files concerning CI but I then realized all targets using uClibc are tier 3, and the only included target in |
|
Error: Label A-CI can only be set by Rust team members Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip. |
dybucc
changed the title
cfg option for file offset bindings on 32-bit targets
rustbot
added
the
stable-nominated
|
Do these changes necessitate a minimum version of uclibc in order to pass the libc tests? I see that the checkbox indicating the libc-test has not been marked complete. Please make sure you do so, because there's often some subtle bugs which the tests catch! As for the 32- vs 64-bit file lengths, it's my experience that the length of this field varies between uclibc configurations. Is that understanding not up to date? |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
There seem to be some struct fixes and cleanups that aren't related to uclibc_file_offset_bits64 changes. Can anything that is either a correction or a non-functional cleanup be split to a PR? Those don't need to be involved in LFS64 alias discussion, which seems more complicated on uclibc than other targets.
Not positive about what to do with the rest here if it's configurable on uclibc.
|
Reminder, once the PR becomes ready for a review, use |
About configurability:
About versioning:
|
32-bit targets are configurable. 64-bit targets also are. The suffixed and unsuffixed types are equivalent in 64-bit targets. There's no reason to support them. 32-bit targets are the exception. |
c120d9a to
b174093
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
b174093 to
39b2b06
Compare
This comment has been minimized.
This comment has been minimized.
39b2b06 to
35062e4
Compare
This comment has been minimized.
This comment has been minimized.
724a620 to
4515d23
Compare
They can't be separated. They may seem tangential. They're actually necessary. An example is Though changes have now been separated into separate commits. @rustbot ready |
4515d23 to
f433382
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| pub const SYS_set_mempolicy_home_node: c_long = 4000 + 450; | ||
|
|
||
| #[link(name = "util")] | ||
| extern "C" { |
There was a problem hiding this comment.
"linux(uclibc): remove explicit linking to libutil" seems reasonable to me, especially with #5265
| pub struct glob_t { | ||
| // FIXME(ulibc) | ||
| pub gl_pathc: size_t, | ||
| pub gl_pathv: *mut *mut c_char, | ||
| pub gl_offs: size_t, | ||
| pub gl_flags: c_int, | ||
| __unused1: Padding<*mut c_void>, | ||
| __unused2: Padding<*mut c_void>, | ||
| __unused3: Padding<*mut c_void>, | ||
| __unused4: Padding<*mut c_void>, | ||
| __unused5: Padding<*mut c_void>, | ||
| } |
There was a problem hiding this comment.
"linux(uclibc): remove conflicting records": The message currently says:
Remove records that are already declared in top-level modules. This
affects x86_64 only. There is no such supported Rust target. This change
should virtually affect nobody.
Who does this affect at all? The definitions look identical
There was a problem hiding this comment.
i didn't mean the changes to have an effect on end users. i meant that the changes were only to the x86_64 module. that's why i top it off by saying that it should virtually affect nobody.
| if target_env == "uclibc" | ||
| && target_ptr_width == "32" | ||
| && env_flag("CARGO_CFG_LIBC_UNSTABLE_UCLIBC_FILE_OFFSET_BITS") | ||
| { | ||
| set_cfg("uclibc_file_offset_bits64"); | ||
| } |
There was a problem hiding this comment.
"linux(uclibc): add cfg for lfs64 bindings": this should check for a value (32 or 64)
I don't believe this is accurate. The commit messages are "linux(uclibc): fix The confusing bit is probably the dependency: those fix commits depend on "linux(uclibc): add cfg for lfs64 bindings" but they shouldn't. Instead, any I realize that's annoying rebase work, so don't worry about it if the target maintainer is happy with the net changes as-is. Just noting that atomicity makes for history that's easier to understand. |
f433382 to
2310c84
Compare
|
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. |
Adds a `cfg` to enable LFS64 bindings under 32-bit targets. This does not apply to 64-bit targets. Their suffixed types are equivalent to their unsuffixed types. They unconditionally get both suffixed and unsuffixed types. This `cfg` replicates a build option upstream.
Removes linking directory to libutil in MIPS32. This did not seem necessary. Testing is pending.
Remove records that are already declared in top-level modules. This affects x86_64 only. There is no such supported Rust target. This change should virtually affect nobody.
Fixes the definitions of: - `stat`. - `statfs`. - `statvfs`. The analogous 64-bit types wer also tweaked. They are equivalent on some target architectures. They only needed to be aliased.
Replaces existing definitions. They were not correct under MIPS. This target needed some special casing.
2310c84 to
b69830c
Compare
the thing is that breakage is only made obvious when the corresponding feature test macro (i.e. FILE_OFFSET_BITS=64) is set. if some user has set it in the c standard library implementation they are linking with, but they are getting the fields in rust that would correspond with the macro not being set, things could get awkward when crossing ffi boundaries. considering this is a fairly popular macro that we already provide a cfg for in glibc environments, i thought this was necessary. the fix itself is in adding the cfg and conditionally defining some fields at compile-time. the only exception to this are some integer fields whose signedness was not quite right, and some padding fields that were not using our Padding util type. i just decided to package those up in the same commit. |
4 participants
Description
This PR adds a new
cfgfor file offset types in uClibc to ensure target triples with a 32-bit machine word size can also have 64-bit unsuffixed file offset types. None of these changes affect 64-bit platforms.The LFS64 types continue being exposed by default in all platforms, but when the
cfgoption is issued, they will have equivalent definitions to the unsuffixed types.There were also some records that were altogether removed because they collided with structures from top-level modules such as
unix/linux_like/linux_l4re_shared.Sources
Upstream uclibc-ng sources showing
ino_tdefined in terms of its 64-bit variant, and the definition for the 64-bit variant. There's a "chain" of types that leads to the final type with whichino64_tis defined. This chain is made most obvious by the final type alias, which appends a_TYPEand prepends two underscores to the type identifier in question. Notice how the__uquad_ttype is always defined as a 64-bit type, even on systems where that requires using a record type with a two-element array (32-bit per element).Upstream uclibc-ng sources showing
off_tdefined in terms of its 64-bit variant when LFS64 support and__USE_FILE_OFFSET64are enabled. Much like the above, there's a "chain" of types leading to the type whose bitwidth is made clear. The chain is shortened to the last few types.Upstream uclibc-ng sources showing
rlim_tdefined in terms of its 64-bit variant when LFS64 support and__USE_FILE_OFFSET64are enabled. This follows the same reasoning as the above.Upstream uclibc-ng sources showing
rlim_tdefined in terms of its 64-bit variant. This follows the same reasoning as the above.Upstream uclibc-ng sources showing
fsblckcnt_tandfsfilcnt_tdefined in terms of their 64-bit variants. This first of the below sources also includes references to the above types.Upstream uclibc-ng sources showing how
flockandflock64are independent, but when defined with__USE_FILE_OFFSET64, will end up having the same record layout. The second source shows how this definition can actually be unified across all child modules to theuclibcmodule because the fields that are conditionally compiled under MIPS will never be any different if__USE_FILE_OFFSET64is defined.Upstream uclibc-ng sources showing how
statvfsandstatvfs64are independent, but like the above, have equivalent definitions when__USE_FILE_OFFSET64is defined. This also shows how there does not seem to be a need for the fields to be reordered depending on the system's endianness.Upstream uclibc-ng sources showing how
statfsandstatfs64are independent, but like the above, have equivalent definitions when__USE_FILE_OFFSET64is defined. A bunch of field types were changed to both accurately reflect the upstream types used, and to add missing fields. The former has not been documented in full in the below sources because the types involved are already part of the sources cited in other list items. Some required special-casing in MIPS platforms has been necessary, as there were fields that had altogether different types. These were only ever so slightly tweaked for the same reasons as above.statfsin MIPS got unified as the definitions for both the o32 and N64/N32 ABIs are equivalent under__USE_FILE_OFFSET64.Upstream uclibc-ng sources showing the same as the above but for
statandstat64. There was also some special casing required in MIPS platforms, though the definitions for both x86_64 and arm converged. These have been kept in their corresponding modules because there's need for MIPS to have their own and it was decided against a top-level, field-specificcfg.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