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!.
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.
simplify crate-relative item paths to use self-module-relative paths. replace uses of deprecated fixed-width c integer types with rust integer types.
add comment annotations to certain lfs bindings that need deprecation and eventual removal only under certain targets. this is due to the fact they are only considered equivalent when the target is either 32-bits or 64-bits. this applies to statfs, statvfs and routines that depend on them. c.f. sys/statfs.h, sys/statvfs.h under /usr/include in any aix 7.3 machine.
add comments for deprecation once stage 3 of the 64-bit plan is reached [^1]. [^1]: rust-lang#4805
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 create in-place on the c side of things. tweak anonymous union identifier to match skipping patter in test suite. if aix ever gets past tier 3 support, this should ensure we skip the anonymous union in the 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.
add a non-opaque type definition to lock_data_instrumented. this is supposed to be a kernel-only type that we shouldn't even expose. the current definition is that of an opaque type, when it's really not. this patch adds the right definition (with private fields) and a few auxiliary types required for that definition.
tweak padding fields in records where they were public or not using the dedicated Padding type. simplify item paths to use module-level paths instead of crate-level paths. rename pollfd_ext that was using the struct tag identifier instead of the typedeffed identifier. rename the anonymous union for ld_info's _file field to fit the skipping pattern in the test suite. aix isn't part of ci, but testing locally is still possible so this is necessary to skip those anonymous types.
add deprecation attribute to _kernel_simple_lock and change uses of it for the _simple_lock type. this type doesn't exist in aix 7.3 header files.
add deprecation notice to fileops_t type and tweak some of its fields. the function pointer fields had slightly wrong parameter types. the type itself is only exposed when programming against the kernel.
add deprecation notice to the file type and provide a correct definition under a slightly different identifier. the file type is only available when programming against the kernel and even then it's only available as an opaque structure. the new type will eventually be renamed to file once the current file type is removed. fix alignment requirement of file type to fit aix 7.3 header files.
add system header comments to architecture-specific constants.
I'm not sure whether there is a controversial change here, though. All conflicts
I was actually referring to |
|
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. |
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 |
4 participants
Description
this pr is concerned with the aix module. each patch addresses either one of (1) adding new bindings, (2) moving the powerpc64 module bindings into the top-level aix module, or (3) more gracefully handling lfs-compliant types and routines. other minor clean ups are also included.
i will not comment on new bindings. these were only added as a consequence of my checking through the header files with which the below items are concerned with.
the merging of the aix/powerpc64 module into the aix module has been made due to this being the only supported rust target using aix. there were already aix module definitions for sum types whose fields were conditionally compiled and assumed to be of one type or another depending on machine word size. in those cases, existing definitions preferred 64-bit-relative code paths. this transition has also required reworking some type definitions. they either (1) used currently non-standard naming for fields declared through anonymous records/unions, (2) were exposing kernel types when these should be provided as opaque types, or (3) were exposing incomplete types that actually had full definitions in upstream header files. lastly, there were some types that simply did not exist at all. i assume that ows to aix version differences between the time when the bindings were initially provided/modified and the current declarations.
lfs handling required some careful consideration. in 64-bit aix targets, most "64"-suffixed types have equivalent definitions with the unsuffixed types. the only exception to this was found in two record types; statvfs and statfs. each of these contains one field of type fsid_t. this type is unconditionally declared as having an effective size of 8 bytes and a 4-byte alignment requirement. the fsid64_t type has an effective size of 16 bytes and an 8-byte alignment requirement. the latter is used in the statvfs64 and statfs64 types. the routines making use of these can neither be assumed to have equivalent signatures, so the suffixed variants have been gated under the aix_large_file_api cfg. the gnu_file_offset_bits64 cfg could have probably been reused, but i have decided against that to favor tidiness.
Sources
files sys/types.h, sys/inttypes.h, sys/lock_def.h, sys/ptrace.h, sys/stat.h, sys/statfs.h, sys/statvfs.h under /usr/include on any aix 7.3 machine.
links cannot be provided. sources are not public.
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