-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix baud rate constants for glibc 2.42 and up. #4697
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
Conversation
f1b1858
to
4c92494
Compare
In glibc 2.42, baud rate constants changed. Their value is now simply the baud rate as integer. Additionally, the `tcset{i,o,}speed` function accepts any arbitrary baud rate, and the `tcget{i,o,}speed` can return any arbitrary baud rate. On MIPS and SPARC, the termios struct from glibc now also has the `c_ispeed` and `c_ospeed` fields.
4c92494
to
2b49289
Compare
By the way, 2.42 isn't 2.4.2 :) |
How come this is present in the "new" definition?
|
cfg_if! { | ||
if #[cfg(not(any(target_arch = "sparc", target_arch = "sparc64")))] { | ||
pub const B2500000: crate::speed_t = 2500000; | ||
pub const B3000000: crate::speed_t = 3000000; | ||
pub const B3500000: crate::speed_t = 3500000; | ||
pub const B4000000: crate::speed_t = 4000000; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hpax: You mean this? This is just a pragmatic choice to avoid having constants in the libc
crate whose presence depends on the glibc version at build time. They're not available pre-2.42. Starting from 2.42, the baud rate constants are basically pointless (which is nice), so I don't think anyone is hurt by not having them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR runs contrary to existing libc practice. The existing practice is to provide bindings that work with the oldest supported version of each OS. That's why libc doesn't run bindgen at build time. Instead, it relies on pregenerated bindings. For glibc, the bindings must be compatible with glibc 2.17, because that's Rust's minimum requirement, and Rust depends on libc.
Furthermore, attempting to detect the installed glibc version at build time makes cross-compiling much harder.
So I think that instead of attempting to detect the glibc version at runtime, we should only use the old baud rate symbols. And we should set the ELF symbol version (i.e. Rust's #[link_name]
attribute) for any glibc function that takes those symbols. That way Rust programs using libc will continue to work with any glibc version.
This isn't true now though. If you compile a program with new glibc it will fail to link at runtime with an older glibc. That's exactly what will happen with this PR too. That said, I know this isn't ideal. But using the old link names is extremely annoying, as they are architecture dependent. Using the old link names was also my first idea. |
Which are the offending symbols? They probably need to be fixed, too. |
Any symbol really. The linker will automatically use the latest version of any symbol you use. And if that one isn't present in the old glibc being used at runtime, the dynamic linker will fail to load the binary. For example: when I compile a program on Arch Linux, I can not run it on Ubuntu, because the glibc of ubuntu is too old. The other way around works fine. |
Note that I did try this, and the tests will fail because the symbol used by the generated C code is not affected. |
Are you talking about libc's CI? That can be made to work. FreeBSD's C library has many versioned symbols, and libc still used the FreeBSD 11-compat versions by default until very recently (PR #2406) . Look for example at the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @asomers mentioned, there is no guarantee that the host glibc matches the version that is getting linked, so this isn't going to be robust. I don't think we have a good way to detect that version, so our best bet (for now) is going to have to be based on link_name
.
This isn't totally true. If your program calls cfgetispeed (for example), it will end up linking If you try to run the program with an older glibc it will fail to resolve the symbol at runtime. This already happens now with all symbols from glibc. So I think technically the solution in the PR is fine, and mimics how glibc is supposed to be used. /edit: Ah, fair, the host glibc used to compile build.rs doesn't have to match the target glibc. Hmmmm... okay, that's a very important problem with this approach. |
I see you got it with your edit - but yeah, doing something like e.g. building on Arch and deploying on Debian means you'll hit the same problems, just a bit more sneaky. I'm going to ask around and see if anyone can think of a good way for us to do more proper symbol versioning (more to talk about at #4700). In the meantime, is there any downside of specifying a specific older symbol version? Aside from, of course, not having the latest API. |
This is indeed a perpetual problem. For 1.0 we'll be making everything |
Yeah, my point was that this already happens now, so I didn't think it is a new problem. But when cross compiling for a totally different architecture it is certainly a new problem. (Or if otherwise using a different glibc for build.rs and the output binaries. Note sure if this is possible when host == target, but I suppose it might be.)
It should be fine to link the older versions. That's what binaries linked against older glibc versions do anyway. As long as we get all the right ones :] |
Closing this as not the desired solution. |
In glibc 2.42, baud rate constants changed. Their value is now simply the baud rate as integer. Additionally, the
tcset{i,o,}speed
function accepts any arbitrary baud rate, and thetcget{i,o,}speed
can return any arbitrary baud rate.On MIPS and SPARC, the termios struct from glibc now also has the
c_ispeed
andc_ospeed
fields. This is not a backwards compatible change, and I'm not sure how this should be addressed. Adding public or private fields will break code that create these structs manually. But we need to make sure the size is correct for glibc 2.42.Making the existence of these fields depend on the glibc version is a nightmare for docs.rs and portable code. But adding these fields on mips and sparc without glibc 2.42 means the fields will not be initialized by libc calls, so anyone using
MaybeUninit::assume_init()
could be triggering undefined behaviour.Also note: there is no explicit linking against glibc 2.42 symbols. I was thinking we could force having
GLIBC_2.42
as undefined symbol, but if you call any of the libc functions that interpret or return the baud rates, you will already be linking against versioned symbols implicitly and the binary will fail to load at runtime on systems with an older glibc. I think this is probably good enough.Should fix #4692.
Sources
Baud rate constants: https://github.com/bminor/glibc/blob/13d67746cbe1273afaf6b9de9d6065ab76ee7697/bits/termios-baud.h#L24-L70
termios struct: https://github.com/bminor/glibc/blob/13d67746cbe1273afaf6b9de9d6065ab76ee7697/sysdeps/unix/sysv/linux/bits/termios-struct.h#L32-L42