aix: clean up module - #5259
Conversation
| let aix_large_file_api = env_flag("CARGO_CFG_LIBC_UNSTABLE_LARGE_FILE_API"); | ||
| if aix_large_file_api { | ||
| set_cfg("aix_large_file_api"); | ||
| } |
There was a problem hiding this comment.
"aix: add cfg for certain lfs bindings": This is effectively deleting API that people may be using?
There was a problem hiding this comment.
Please always call out breakage in commit messages, I almost overlooked this aspect.
What was the thought process here? Outright removing these is likely to break a lot of people.
There was a problem hiding this comment.
done. noted.
what do you mean by the thought process? much like other cfgs, i added them to fit an upstream feature test macro that is checked before making the type definitions available. i didn't mention it in the commit because i thought not marking the pr as stable-nominated would do.
There was a problem hiding this comment.
We won't have any use for this config in 1.0, we can just delete the *64 versions on platforms where they're identical.
Most PRs should be intended for backport - I hadn't realized you weren't on the same page. I'll post more about this at #5170 very soon.
There was a problem hiding this comment.
i think that much i already understand (meaning the 64-bit types, not the backporting stuff.) the thing with the types i specifically gated behind the cfg is that they are not equivalent to the unsuffixed types unless the upstream macro is defined. in fact, there's a bunch more 64-suffixed types that i did not annotate but rather deprecated because they are, indeed, equivalent if we assume the macro is defined. only those that i annotated in that commit are not; namely, statfs, statvfs, and routines that use them. the ones that i deprecated have now been annotated with the comment you mention in #4805.
i can't provide links because the sources are not public. there's neither any page in the ibm docs that comments on it.
There was a problem hiding this comment.
Are our current definitions (before this PR) correct if we assume the macro is defined?
There was a problem hiding this comment.
i just looked through the header files again. i'm not sure anymore why is it that i gated those records behind a cfg. there's no uses of the macro in their header files and they are always exposed.
with that, there's only two things that remain true:
- statfs is equivalent to statfs64 if targetting a 64-bit target.
- this is because statfs has a field that is only present when the target has 64-bit machine word size. otherwise they differ.
- statvfs is equivalent to statvfs64 if targetting a 32-bit target.
- statvfs has a field of type fsid_t. statvfs64 has in its stead a field of type fsid64_t. fsid_t is only equivalent to fsid64_t under 32-bit targets.
other than that, the definitions are correct as is.
considering 1, we can't really remove these nor deprecate them altogether. in the multistage plan proposed in #4805, i believe this would translate to:
- annotate with a fixme comment but also add some other "parameter" to the fixme that indicates it's not meant to be found and replaced with a deprecation notice for all targets. something like
// FIXME(1.0,deprecate,64): lfs binding to be removed - once we reach the deprecation stage, ensure these alternatively parameterized fixme comments are replaced with conditional deprecations.
- once we reach the removal stage, don't remove these but rather gate them behind a cfg that makes statfs64 available only under 32-bit targets (where they differ) and statvfs64 only available under 64-bit targets (where they differ.)
i've already went ahead and proceeded with step 1 above. though the parameterization may have to be tweaked a bit if we ever need to depend on some other cfg option (possibly by not just appending a 64 but by using the full predicate, like // FIXME(1.0,deprecate,target_pointer_width=64).)
Footnotes
There was a problem hiding this comment.
"aix: move and fix powerpc64 definitions into aix": I have mixed feelings on this change. On one hand there are likely no other platforms we will ever need to support with AIX, according to https://en.wikipedia.org/wiki/IBM_AIX. On the other hand, having types like mcontext_t in a non-arch-specific module feels very wrong.
I think I'd have a mild preference of moving this to src/new and matching system headers, if moved at all, that's one fewer history jump in the blame once everything makes it to new eventually anyway. But I'll leave it up to the target maintainers.
|
Reminder, once the PR becomes ready for a review, use |
This comment has been minimized.
This comment has been minimized.
|
i'll change the pr status once the target maintainer answers. |
b49621f to
faccc1c
Compare
This comment has been minimized.
This comment has been minimized.
|
Thanks for the effort, @dybucc! Your contribution is greatly appreciated. As discussed with the Rust product team, we believe it is not entirely out of the question that we may add AIX 32-bit support in the future. As such, we kindly suggest holding off on the cleanup for now. |
| #[cfg(debug_assertions)] | ||
| pub lock_lr: c_int, | ||
| #[cfg(debug_assertions)] | ||
| pub unlock_lr: c_int, | ||
| #[cfg(debug_assertions)] | ||
| pub lock_caller: tid_t, | ||
| #[cfg(debug_assertions)] | ||
| pub unlock_caller: tid_t, | ||
| #[cfg(debug_assertions)] | ||
| pub lock_cpuid: c_int, | ||
| #[cfg(debug_assertions)] | ||
| pub dbg_zero: c_int, | ||
| #[cfg(debug_assertions)] | ||
| pub unlock_cpuid: c_int, | ||
| #[cfg(debug_assertions)] | ||
| pub dbg_flags: c_int, |
There was a problem hiding this comment.
This appears to give the struct a different size based on how you build libc
There was a problem hiding this comment.
that's right. the header files conditionally include those fields if some DEBUG macro is defined. i thought the nearest equivalent would just be rust's debug_assertions.
There was a problem hiding this comment.
Hm, that's weird. #[cfg(debug_assertions)] wouldn't be accurate here because whatever API we're calling has no clue about Rust's debug assertions so toggling those means you will swap the struct between a correct definition and an incorrect one. Without knowing what it's actually doing with the internals, it's safer to keep this an extern_ty!.
There was a problem hiding this comment.
Noted. I'm remaking the PR now so the changes won't be public for some time.
This comment has been minimized.
This comment has been minimized.
i'm guessing that means i should hold off on moving the powerpc64 module bindings into the top-level aix module. can i keep then the modifications i made to the types i brought over from the powerpc64 module (of course, moving them back to the powerpc64 submodule)? |
Yes, please hold off on moving the PowerPC64 module for now. Otherwise, feel free to make any changes that improve the implementation. Could you also hold off on backporting any changes to the stable branch for the time being? Again, we greatly appreciate your contributions and thank you for your help! By the way, how are you testing these changes, and on which AIX OS level(s) have you observed or validated them? |
|
done. i've been checking the header files on an aix 7.3 machine that the cfarm project provides access to. thus far, testing is pending. i've had no luck cross-compiling a rust toolchain with support for the powerpc64-ibm-aix target, and building straight on the aix machine (to which i'm sshing into) is not quite feasible. |
rustbot
added
S-waiting-on-review
and removed
S-waiting-on-author
S-waiting-on-maintainer
labels
The CI for your branch stopped at ctest with 37 errors. ctest errors
|
|
Thanks! I'll start looking into this. |
This comment has been minimized.
This comment has been minimized.
|
Concerning the
Concerning the lock types: I provided bindings for Concerning the pointer field type errors, those I forgot to change back when The rest of the errors are with two Except for the first and last errors commented above, I just pushed a patch |
Thanks, reduced to 10 errors. Errors
|
|
Those I don't think can't do anything about. The |
|
I haven't yet taken a thorough look at this but will note that this is turning into a pretty big PR. It may be good to split uncontroversial changes into a separate PR so we can actually merge some of it and reduce what's left to focus on.
|
This comment has been minimized.
This comment has been minimized.
I'm not sure whether there is a controversial change here, though. All conflicts
I was actually referring to |
This comment has been minimized.
This comment has been minimized.
It currently passes without errors, right? That needs to be resolved and, of course, this needs the maintainer approval before merge. By uncontroversial, I mean changes like:
Which have more or less been reviewed without issue. "aix: tweak poll_ctl_ext ident", "aix(powerpc64): define full lock_data_instrumented" and "aix(powerpc64): deprecate file type and add _file" OTOH are more opinionated and seem to introduce test failures, and there's a "chore: wip" commit. It's just less for the reviewers to keep in our mental cache if we can cross some things off the list and look at a few dozen line diff for what's remaining, rather than ~1k total.
Unfortunately we need to skip fields containing a |
|
@rustbot author |
|
I agree with @tgross35 100%. It’s a good idea to have smaller PRs, each with a clear description of the issue being addressed and the rationale behind the change. This makes reviews more manageable and allows reviewers to focus on a well-defined scope. Smaller, self-contained PRs also make it easier to understand code history in the future, investigate regressions, and revert changes when necessary. |
|
I'm remaking the PR now into three separate patchsets that should target the
Edit: the PR is now separated into three PRs. This one is now concerned with the @rustbot ready |
|
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. |
Simplify crate-relative item paths to use `self`-relative paths. Replace uses of deprecated fixed-width C integer types with Rust integer types.
Change `poll_ctl_ext` type identifier to `poll_ctrl_ext_t`. This now aligns with the AIX header files. The previous identifier corresponded with the `struct` tag and not the `typedef` that is created in-place on the C side of things. Tweak anonymous `union` identifier to match skipping pattern in test suite.
Add `compile_error` macro invocation when declaring architecture-specific definitions. This should ensure any new target either gets proper review of the required types or otherwise provides its target-specific definitions and bindings.
rustbot
added
the
stable-nominated
4 participants
Description
This PR is concerned with a general clean up of the AIX module. See the patch
messages for details.
Checklist
libc-test/semverhave been updated*LASTor*MAXhave the standarddoc comment
cargo test -p libc-test --target mytarget);especially relevant for platforms that may not be checked in CI
@rustbot label +stable-nominated