Make `makedev`, `major`, `minor` const #4208

igankevich · 2024-12-18T07:19:56Z

Description

This PR marks all makedev, major, minor re-implementations (except Solarish) as const fn and tests major and minor against their C counterparts (macros). This PR closely follows the work that was done in this commit.

The motivation behind this PR is to make Rust code that uses these three functions consistent across different platforms. To give a concrete example: currently one has to wrap major and minor in unsafe blocks on MacOS, whereas on Linux these blocks are not needed. Unnecessary unsafe blocks produce a warning that one silences with #[allow(unused_unsafe)]. All of that confuses code reviewers and creates unnecessary noise. makedev, major and minor are macros on most Unixes, there is no need for them to be unsafe.

As far as I understand these changes do not break existing code: changing unsafe functions to const only produce warnings but not compilation errors.

@rustbot label stable-nominated

Sources

None.

Checklist

Relevant tests in libc-test/semver have been updated
No placeholder or unstable values like *LAST or *MAX are
included (see #3131)
Tested locally (cd libc-test && cargo test --target mytarget);
especially relevant for platforms that may not be checked in CI

rustbot · 2024-12-18T07:20:04Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @tgross35 (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

rustbot · 2024-12-18T07:20:08Z

Some changes occurred in OpenBSD module

cc @semarie

igankevich · 2024-12-18T19:44:28Z

After testing with the CI I found and fixed a bug in emscripten's major and minor: bit shifts and masking were done in the reverse order. The original macros are here, the same link is in the crate's source code.

tgross35 · 2024-12-22T04:17:57Z

This also changes from unsafe to safe, which should be fine and fixes #3759.

@hoodmane could you double check the emscripten changes?

tgross35 · 2024-12-22T04:19:09Z

libc-test/test/makedev.rs

+        assert_eq!(libc::major(dev), major as _);
+        let minor = unsafe { minor_ffi(dev) };
+        assert_eq!(libc::minor(dev), minor as _);


Could you name the types here rather than using _?

I wish I could, but the return type can be c_int, c_uint, major_t, minor_t depending on the platform.

Sorry for the late review, but could you change both the LHS and RHS to be as i64? At least that gives a concrete size so we don't have any chance of wrapping (in case some obscure platform has a major_t smaller than int).

I tried casting them to i64 and the test failed on FreeBSD. Ended up casting to proper target-specific types.

hoodmane

Maybe it would be nice to keep the bitwise or terms of the rust code in the same order as they appear in musl for easier validation?

hoodmane · 2024-12-22T09:12:15Z

src/unix/linux_like/emscripten/mod.rs

+}
+
+safe_f! {
+    pub {const} fn makedev(major: c_uint, minor: c_uint) -> crate::dev_t {


Emscripten has actual functions for these in addition to macros so it would be possible to call out to them. But it would be a lot slower than this.

hoodmane · 2024-12-22T09:18:56Z

Otherwise it looks good to me.

igankevich · 2024-12-25T06:16:54Z

Maybe it would be nice to keep the bitwise or terms of the rust code in the same order as they appear in musl for easier validation?

Thanks for pointing this out! I've changed the order. Kindly re-requesting the review 🙏

tgross35

I have one request but otherwise lgtm

tgross35 · 2025-02-22T21:18:45Z

libc-test/test/makedev.rs

+        assert_eq!(libc::major(dev), major as _);
+        let minor = unsafe { minor_ffi(dev) };
+        assert_eq!(libc::minor(dev), minor as _);


Sorry for the late review, but could you change both the LHS and RHS to be as i64? At least that gives a concrete size so we don't have any chance of wrapping (in case some obscure platform has a major_t smaller than int).

rustbot · 2025-02-24T06:12:47Z

Some changes occurred in the Android module

cc @maurer

tgross35 · 2025-02-24T08:01:58Z

Thanks for the updates, that's even better. Please squash and this should be ready to merge.

igankevich · 2025-02-24T20:05:59Z

Thanks for the updates, that's even better. Please squash and this should be ready to merge.

Done.

(backport <rust-lang#4208>) (cherry picked from commit 37c3333)

libc 0.2.171 makes all `major`, `minor`, `makedev` into `const fn` (rust-lang/libc#4208). So drop `unsafe`.

rustbot assigned tgross35 Dec 18, 2024

rustbot added O-android O-dragonfly O-linux O-macos O-unix S-waiting-on-review labels Dec 18, 2024

rustbot added the stable-nominated This PR should be considered for cherry-pick to libc's stable release branch label Dec 18, 2024

tgross35 reviewed Dec 22, 2024

View reviewed changes

hoodmane approved these changes Dec 22, 2024

View reviewed changes

igankevich requested a review from hoodmane December 25, 2024 06:17

tgross35 approved these changes Feb 22, 2025

View reviewed changes

Make all major, minor, makedev into const fn.

37c3333

igankevich force-pushed the const-fn-major-minor branch from 3064c91 to 37c3333 Compare February 24, 2025 19:35

tgross35 added this pull request to the merge queue Feb 24, 2025

Merged via the queue into rust-lang:main with commit 989b64a Feb 24, 2025
44 checks passed

tgross35 pushed a commit to tgross35/rust-libc that referenced this pull request Mar 10, 2025

Make all major, minor, makedev into const fn.

d965b1d

(backport <rust-lang#4208>) (cherry picked from commit 37c3333)

tgross35 mentioned this pull request Mar 10, 2025

[0.2] Backports #4314

Merged

tgross35 pushed a commit to tgross35/rust-libc that referenced this pull request Mar 10, 2025

Make all major, minor, makedev into const fn.

5d8deba

(backport <rust-lang#4208>) (cherry picked from commit 37c3333)

tgross35 pushed a commit to tgross35/rust-libc that referenced this pull request Mar 10, 2025

Make all major, minor, makedev into const fn.

d8cb8db

(backport <rust-lang#4208>) (cherry picked from commit 37c3333)

tgross35 pushed a commit to tgross35/rust-libc that referenced this pull request Mar 10, 2025

Make all major, minor, makedev into const fn.

24430c7

(backport <rust-lang#4208>) (cherry picked from commit 37c3333)

tgross35 added stable-applied This PR has been cherry-picked to libc's stable release branch and removed stable-nominated This PR should be considered for cherry-pick to libc's stable release branch labels Mar 10, 2025

bdrung added a commit to bdrung/3cpio that referenced this pull request Mar 30, 2025

libc: remove unsafe from libc::major/minor

c8a2dbd

libc 0.2.171 makes all `major`, `minor`, `makedev` into `const fn` (rust-lang/libc#4208). So drop `unsafe`.

bdrung added a commit to bdrung/3cpio that referenced this pull request Mar 30, 2025

libc: remove unsafe from libc::major/minor

52827a6

libc 0.2.171 makes all `major`, `minor`, `makedev` into `const fn` (rust-lang/libc#4208). So drop `unsafe`.

Make makedev, major, minor const #4208

Make makedev, major, minor const #4208

Uh oh!

Conversation

igankevich commented Dec 18, 2024

Description

Sources

Checklist

Uh oh!

rustbot commented Dec 18, 2024

Uh oh!

rustbot commented Dec 18, 2024

Uh oh!

igankevich commented Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgross35 commented Dec 22, 2024

Uh oh!

tgross35 Dec 22, 2024

Choose a reason for hiding this comment

Uh oh!

igankevich Dec 25, 2024

Choose a reason for hiding this comment

Uh oh!

tgross35 Feb 22, 2025

Choose a reason for hiding this comment

Uh oh!

igankevich Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hoodmane left a comment

Choose a reason for hiding this comment

Uh oh!

hoodmane Dec 22, 2024

Choose a reason for hiding this comment

Uh oh!

hoodmane commented Dec 22, 2024

Uh oh!

igankevich commented Dec 25, 2024

Uh oh!

tgross35 left a comment

Choose a reason for hiding this comment

Uh oh!

tgross35 Feb 22, 2025

Choose a reason for hiding this comment

Uh oh!

rustbot commented Feb 24, 2025

Uh oh!

tgross35 commented Feb 24, 2025

Uh oh!

igankevich commented Feb 24, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Make `makedev`, `major`, `minor` const #4208

Make `makedev`, `major`, `minor` const #4208

igankevich commented Dec 18, 2024 •

edited

Loading

igankevich Feb 24, 2025 •

edited

Loading