Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Curious makedev function signature mismatch on macOS #4360

Open
nohajc opened this issue Apr 1, 2025 · 6 comments
Open

Curious makedev function signature mismatch on macOS #4360

nohajc opened this issue Apr 1, 2025 · 6 comments
Labels
C-bug Category: bug E-help-wanted Call for participation: Help is requested to fix this issue.

Comments

@nohajc
Copy link

nohajc commented Apr 1, 2025

I want to ask about the makedev function signature. In the macOS implementation of libc crate, major and minor arguments are signed. This is consistent with the return type of major and minor functions.

    pub {const} fn makedev(major: i32, minor: i32) -> dev_t {
        (major << 24) | minor
    }

    pub {const} fn major(dev: dev_t) -> i32 {
        (dev >> 24) & 0xff
    }

    pub {const} fn minor(dev: dev_t) -> i32 {
        dev & 0xffffff
    }

However, Apple declares major and minor arguments as unsigned in their documentation.
https://developer.apple.com/documentation/kernel/1547212-makedev

While the major/minor return types are still signed.
https://developer.apple.com/documentation/kernel/1547203-major
https://developer.apple.com/documentation/kernel/1547204-minor

It is technically Objective-C documentation and also the actual definitions in <sys/types.h> are just macros, so strictly speaking the argument types to makedev are not well defined.

#define major(x)        ((int32_t)(((u_int32_t)(x) >> 24) & 0xff))
#define minor(x)        ((int32_t)((x) & 0xffffff))
#define makedev(x, y)    ((dev_t)(((x) << 24) | (y)))

I suppose it would be logical to assume they're signed since major and minor macros use an explicit cast to a signed type, but here we have a contradicting documentation by Apple.

Interestingly, if I compare this to the Android implementation, it also has this exact input/output type mismatch but this time it is also reflected in libc.

    pub {const} fn makedev(ma: c_uint, mi: c_uint) -> crate::dev_t {
        let ma = ma as crate::dev_t;
        let mi = mi as crate::dev_t;
        ((ma & 0xfff) << 8) | (mi & 0xff) | ((mi & 0xfff00) << 12)
    }

    pub {const} fn major(dev: crate::dev_t) -> c_int {
        ((dev >> 8) & 0xfff) as c_int
    }

    pub {const} fn minor(dev: crate::dev_t) -> c_int {
        ((dev & 0xff) | ((dev >> 12) & 0xfff00)) as c_int
    }

I guess my question is whether this has been done on purpose or if there's any room for improvement (maybe just mention these ambiguities and inconsistencies in libc documentation).

@tgross35
Copy link
Contributor

tgross35 commented Apr 2, 2025

I don't think this was done on purpose, probably just a difference that got overlooked. A fix would be welcome if you are interested.

@tgross35 tgross35 added C-bug Category: bug E-help-wanted Call for participation: Help is requested to fix this issue. labels Apr 2, 2025
@nohajc
Copy link
Author

nohajc commented Apr 2, 2025

That fix would effectively be a breaking change at this point, right? Isn't that problematic?
Also, documentation aside, it's kind of more convenient as it is now...

I don't even understand why there is a mismatch in the makedev/major/minor signatures in the first place. It doesn't make much sense in my opinion.

@tgross35
Copy link
Contributor

tgross35 commented Apr 2, 2025

We have 1.0 coming out so breaking changes can land for that version, we just likely can't backport it (though that is still on the table depending on how bad breakage actually is). I don't know why there would be a mismatch either and that is kind of unfortunate, but it's probably better for us to match the platform API as-written.

@nohajc
Copy link
Author

nohajc commented Apr 2, 2025

It's even more unfortunate when you consider it should probably match the FreeBSD implementation but Apple made a mistake in their documentation (can't prove it but that would be my guess).
https://man.freebsd.org/cgi/man.cgi?query=makedev

@tgross35
Copy link
Contributor

tgross35 commented Apr 2, 2025

It's not just their docs, but the implementation is unsigned right? https://github.com/apple-oss-distributions/xnu/blob/8d741a5de7ff4191bf97d57b9f54c2f6d4a15585/bsd/sys/types.h#L154

@nohajc
Copy link
Author

nohajc commented Apr 2, 2025

Oh, you're right. I completely overlooked this even though I had the same header file opened on my system. Only saw the macros.

In that case, I could submit a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug E-help-wanted Call for participation: Help is requested to fix this issue.
Projects
None yet
Development

No branches or pull requests

2 participants