Unify definitions of `siginfo_t`, `statvfs` and `statfs` in `musl` targets #3261

bossmc · 2023-06-01T18:03:16Z

During #2935 I noticed there were multiple identical definitions of some of the bits/***.h types in the musl target, as well as a few places where a type was defined twice in the module tree (leading to the "upper-most" definition being the one exported by the library, in contradiction to the expectation that the "most-specific" definition would be used.

This change moves the definitions of struct statvfs(64) and struct siginfo_t to be global for all musl targets (see https://git.musl-libc.org/cgit/musl/tree/include/sys/statvfs.h and https://git.musl-libc.org/cgit/musl/tree/include/signal.h which are architecture-agnostic headers) and struct statfs(64) to be global for all 64-bit musl targets (see https://git.musl-libc.org/cgit/musl/tree/arch/generic/bits/statfs.h). This also required moving fsblkcnt64_t and fsfilcnt64_t to be musl-wide too (for use in struct statvfs64).

It also removes a bunch of redundant (and unreachable) definitions in the riscv64 and riscv32 targets (there seems to be a riscv32 target in the crate, but not in musl itself or at least there's no arch/riscv32 folder in tree).

Upshot of the above is that this change has no externally visible effect, if the more specific types were intended to be used they weren't being so removing them is a no-op. To actually use more specific type definitions one would need to cfg out the general definition as well as providing the specific one.

To find most of these issues I used this process

$ for target in $(rustc --print target-list | grep musl)
do
  echo $target
  RUSTDOCFLAGS="--output-format json --document-private-items" cargo +nightly doc -Z build-std=core --target $target
done
$ for json in target/**/doc/libc.json
do
  echo $json
  jq '.index[] | select(.inner | keys[0] == "struct") | .name' $json | sort | uniq -d
done

The first command uses rustdoc to create a JSON representation of the API of the crate for each (musl) target and the second searches that output for two exported structs of the same name within a single target. Where there's a duplicate, only one of the two symbols is actually usable (and due to import rules, symbols defined locally take precedence over symbols imported from submodules so the less specific symbol is the one that wins).

You can do similar tests for enum, typedef, union, constant by changing the second command in the obvious way, you can also do the same for function though you need to additionally filter on extern "C" (since e.g. there's many many clone functions defined in the crate):

$ jq '.index[] | select(.inner | keys[0] == "function") | select(.inner.function.header.abi | (type == "object" and keys[0] == "C"))
| .name' $json | sort | uniq -d

It feels like adding the checks in that methodology to CI for each target would be a good way to catch issues where a more specific definition is masked by a less-specific one.

rustbot · 2023-06-01T18:03:21Z

r? @JohnTitor

(rustbot has picked a reviewer for you, use r? to override)

JohnTitor · 2023-06-04T00:17:04Z

Thanks! @bors r+

bors · 2023-06-04T00:17:06Z

📌 Commit ec384c7 has been approved by JohnTitor

It is now in the queue for this repository.

bors · 2023-06-04T00:17:14Z

⌛ Testing commit ec384c7 with merge 99927dc...

Unify definitions of `siginfo_t`, `statvfs` and `statfs` in `musl` targets During #2935 I noticed there were multiple identical definitions of some of the `bits/***.h` types in the musl target, as well as a few places where a type was defined twice in the module tree (leading to the "upper-most" definition being the one exported by the library, in contradiction to the expectation that the "most-specific" definition would be used. This change moves the definitions of `struct statvfs(64)` and `struct siginfo_t` to be global for all `musl` targets (see https://git.musl-libc.org/cgit/musl/tree/include/sys/statvfs.h and https://git.musl-libc.org/cgit/musl/tree/include/signal.h which are architecture-agnostic headers) and `struct statfs(64)` to be global for all 64-bit `musl` targets (see https://git.musl-libc.org/cgit/musl/tree/arch/generic/bits/statfs.h). This also required moving `fsblkcnt64_t` and `fsfilcnt64_t` to be musl-wide too (for use in `struct statvfs64`). It also removes a bunch of redundant (and unreachable) definitions in the `riscv64` and `riscv32` targets (there seems to be a `riscv32` target in the crate, but not in `musl` itself or at least there's no `arch/riscv32` folder in tree). Upshot of the above is that this change has no externally visible effect, if the more specific types were intended to be used they weren't being so removing them is a no-op. To actually use more specific type definitions one would need to `cfg` out the general definition as well as providing the specific one. <details> <summary>To find most of these issues I used this process</summary> ``` $ for target in $(rustc --print target-list | grep musl) do echo $target RUSTDOCFLAGS="--output-format json --document-private-items" cargo +nightly doc -Z build-std=core --target $target done $ for json in target/**/doc/libc.json do echo $json jq '.index[] | select(.inner | keys[0] == "struct") | .name' $json | sort | uniq -d done ``` The first command uses rustdoc to create a JSON representation of the API of the crate for each (`musl`) target and the second searches that output for two exported structs of the same name within a single target. Where there's a duplicate, only one of the two symbols is actually usable (and due to import rules, symbols defined locally take precedence over symbols imported from submodules so the less specific symbol is the one that wins). You can do similar tests for `enum`, `typedef`, `union`, constant` by changing the second command in the obvious way, you can also do the same for `function` though you need to additionally filter on `extern "C"` (since e.g. there's many many `clone` functions defined in the crate): ``` $ jq '.index[] | select(.inner | keys[0] == "function") | select(.inner.function.header.abi | (type == "object" and keys[0] == "C")) | .name' $json | sort | uniq -d ``` </details> It feels like adding the checks in that methodology to CI for each target would be a good way to catch issues where a more specific definition is masked by a less-specific one.

bors · 2023-06-04T00:30:43Z

💔 Test failed - checks-actions

bossmc · 2023-06-06T21:57:37Z

Turns out I was both too aggressive and not aggressive enough - every target uses the same statfs definition apart from the mips targets (that re-arranges the fields) so I've lifted statfs to by musl-wide. OTOH mips also re-orders the fields in siginfo_t so I've corrected the (global) definition to get that right, I considered adding a mips-specific definition but that seemed overkill.

bossmc · 2023-07-18T20:15:28Z

This MR is in danger of going stale so I'd like to resolve the style issue. However I can't find a way to write the code that meets all the conflicting requirements/intentions:

the style check only allows one s! per file (so I can't have a #[cfg(not(mips))] s! { struct statvfs ... } alongside the common s!)
The s! macro can't have a cfg_if nested inside it (it possibly could but it's quite tricky to make work)
If you use a #[cfg(not(mips))] inside the s! macro it removes the struct definition but keeps the impl Copy/impl Clone which leads to dual-definitions when the real version is added.
Moving the impl Copy and impl Clone blocks to a #[derive(Copy, Clone)] combined with (3) works, but the auto-derived Clone implementation doesn't use the fact that it's Copy to optimize itself to a memcpy in debug mode

I'm trying to avoid having 10 identical copies of this type defined simply because mips needs a special copy (I'm also baffled why mips has so many mips-only layout differences compared to other targets when it's a fairly recent addition to the linux architectures but that ship has long sailed)

bors · 2023-10-27T15:08:13Z

☔ The latest upstream changes (presumably #3302) made this pull request unmergeable. Please resolve the merge conflicts.

tgross35 · 2024-08-28T23:41:20Z

Hey @bossmc, sorry this has been stale for so long. If you are interested in rebasing so we can merge this than that would be excellent; if not, no worries.

I do need to note that I don't think this is backportable, meaning we will only get the changes once 1.0 is released (which may still be a few months off). I have to assume this isn't a problem since most of this is refactoring, but just an FYI.

This MR is in danger of going stale so I'd like to resolve the style issue. However I can't find a way to write the code that meets all the conflicting requirements/intentions:
1. the style check only allows one `s!` per file (so I can't have a `#[cfg(not(mips))] s! { struct statvfs ... }` alongside the common `s!`)

2. The `s!` macro can't have a `cfg_if` nested inside it (it possibly could but it's quite tricky to make work)

3. If you use a `#[cfg(not(mips))]` inside the `s!` macro it removes the struct definition but keeps the `impl Copy`/`impl Clone` which leads to dual-definitions when the real version is added.

4. Moving the `impl Copy` and `impl Clone` blocks to a `#[derive(Copy, Clone)]` combined with (3) works, but the auto-derived `Clone` implementation doesn't use the fact that it's `Copy` to optimize itself to a `memcpy` in debug mode
I'm trying to avoid having 10 identical copies of this type defined simply because mips needs a special copy (I'm also baffled why mips has so many mips-only layout differences compared to other targets when it's a fairly recent addition to the linux architectures but that ship has long sailed)

Your 3+4 is totally fine here - how well things optimize in debug mode is essentially a nonissue. For cases where it is, it more common for those crates to just compile dependencies in release mode, so nothing for us to worry about here.

tgross35 · 2024-08-28T23:41:47Z

Let me know if you need anything else. Just @rustbot review when you are ready.

@rustbot author

bossmc · 2024-10-10T01:22:25Z

Thanks! I've made that change and rebased to (what was) main.

@rustbot review

tgross35 · 2024-11-12T22:35:55Z

src/macros.rs

+            #[derive(Copy, Clone)]
            #[allow(deprecated)]
            $(#[$attr])*
            pub struct $i { $($field)* }
        }
-        #[allow(deprecated)]
-        impl ::Copy for $i {}
-        #[allow(deprecated)]
-        impl ::Clone for $i {
-            fn clone(&self) -> $i { *self }
-        }


These changes are a nice cleanup, but would you mind moving them to a separate PR? To keep different types of changes atomic.

tgross35 · 2024-11-12T22:37:21Z

src/unix/linux_like/linux/musl/mod.rs

+            note="Please leave a comment on \
+                  https://github.com/rust-lang/libc/pull/1316 if you're using \
+                  this field"


Suggested change

note="Please leave a comment on \

https://github.com/rust-lang/libc/pull/1316 if you're using \

this field"

note="Please leave a comment on https://github.com/rust-lang/libc/pull/1316 \

if you are using this field"

Just to make use of the line length

tgross35

A couple small nits and this lgtm, please rebase and squash.

Regarding your top post - I am entirely in favor of using rustdoc JSON output for testing. If you are up to it, please do consider putting something like this in the libc-test crate.

rustbot assigned JohnTitor Jun 1, 2023

rustbot added the S-waiting-on-review label Jun 1, 2023

bossmc mentioned this pull request Jun 2, 2023

s390x-musl: define O_LARGEFILE constant #3262

Merged

JohnTitor approved these changes Jun 4, 2023

View reviewed changes

bors added S-waiting-on-bors and removed S-waiting-on-review labels Jun 4, 2023

bors added S-waiting-on-review and removed S-waiting-on-bors labels Jun 4, 2023

rustbot added S-waiting-on-author and removed S-waiting-on-review labels Aug 28, 2024

bossmc and others added 7 commits October 10, 2024 01:57

Unify definitions of statvfs and statfs in musl target

6ab1986

Remove all redundant definitions in musl backend

86b0b82

Fix up siginfo_t for mips targets

50d9fd2

Only mips uses a special statfs(64)

04c09ae

Add fanotify_event_info_fid to FAM-exempt types

2199836

Allow cfg attributes in s! and use it for mips-special statfs structure

1cf4443

Remove new redundant definitions

d7887af

bossmc force-pushed the remove-duplicate-types branch from 7c42066 to d7887af Compare October 10, 2024 01:01

rustbot added S-waiting-on-review and removed S-waiting-on-author labels Oct 10, 2024

tgross35 reviewed Nov 12, 2024

View reviewed changes

tgross35 requested changes Nov 12, 2024

View reviewed changes

tgross35 added S-waiting-on-author and removed S-waiting-on-review labels Nov 12, 2024

tgross35 mentioned this pull request Nov 13, 2024

change PR to "waiting on author" after "changes requested" review #1373 rust-lang/triagebot#1857

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify definitions of `siginfo_t`, `statvfs` and `statfs` in `musl` targets #3261

Unify definitions of `siginfo_t`, `statvfs` and `statfs` in `musl` targets #3261

bossmc commented Jun 1, 2023 •

edited

Loading

rustbot commented Jun 1, 2023

JohnTitor commented Jun 4, 2023

bors commented Jun 4, 2023

bors commented Jun 4, 2023

bors commented Jun 4, 2023

bossmc commented Jun 6, 2023

bossmc commented Jul 18, 2023

bors commented Oct 27, 2023

tgross35 commented Aug 28, 2024

tgross35 commented Aug 28, 2024

bossmc commented Oct 10, 2024

tgross35 Nov 12, 2024

tgross35 Nov 12, 2024

tgross35 left a comment

Unify definitions of siginfo_t, statvfs and statfs in musl targets #3261

Are you sure you want to change the base?

Unify definitions of siginfo_t, statvfs and statfs in musl targets #3261

Conversation

bossmc commented Jun 1, 2023 • edited Loading

rustbot commented Jun 1, 2023

JohnTitor commented Jun 4, 2023

bors commented Jun 4, 2023

bors commented Jun 4, 2023

bors commented Jun 4, 2023

bossmc commented Jun 6, 2023

bossmc commented Jul 18, 2023

bors commented Oct 27, 2023

tgross35 commented Aug 28, 2024

tgross35 commented Aug 28, 2024

bossmc commented Oct 10, 2024

tgross35 Nov 12, 2024

Choose a reason for hiding this comment

tgross35 Nov 12, 2024

Choose a reason for hiding this comment

tgross35 left a comment

Choose a reason for hiding this comment

Unify definitions of `siginfo_t`, `statvfs` and `statfs` in `musl` targets #3261

Unify definitions of `siginfo_t`, `statvfs` and `statfs` in `musl` targets #3261

bossmc commented Jun 1, 2023 •

edited

Loading