Fix invalid freebsd version selection

[GuillaumeGomez]

Just realized when working on sysinfo that my stafs struct filled by getmntinfo was missing a lot of information. After trying to find out from my size what I missed, I simply check the size_of the rust struct vs the C struct and found 484 != 2384. After looking a bit more, I discovered that only the CI was using the right freebsd version, which is quite problematic.

r? @Amanieu

[Amanieu]

I'm not actually sure about this, isn't this a breaking change? I suppose that matching the version of freebsd that you are compiling on makes sense, so I'm inclined to accept this. But cc @JohnTitor just in case.

[GuillaumeGomez]

I'm not sure if it is but currently it's unusable and broken. Let's wait to hear from @JohnTitor then. :)

[JohnTitor]

I don't have enough time to take a closer look at this, @Amanieu if you think it's reasonable and doesn't break anything, feel free to go ahead :)

[Amanieu]

@bors r+

[bors]

📌 Commit 53e79b8 has been approved by Amanieu

[bors]

⌛ Testing commit 53e79b8 with merge 6fe1ff9...

[bors]

☀️ Test successful - checks-actions, checks-cirrus-freebsd-11, checks-cirrus-freebsd-12, checks-cirrus-freebsd-13
Approved by: Amanieu
Pushing 6fe1ff9 to master...

[messense]

I think this change might have broke mio on FreeBSD 12, see tokio-rs/mio#1539

[GuillaumeGomez]

It means their code was broken from the start unfortunately... They were using a struct with the wrong size on freebsd 12. I'm not teaching them anything new but they should use mem::zeroed. Unfortunately, freebsd changes the layout of its types from one version to another (adding field, changing field type, changing their order). It's a real nightmare...

[asomers]

This is incorrect. FreeBSD uses ELF symbol versioning to change the interface for libc in a backwards-compatible way. But Rust, because it is designed for cross-compilability, doesn't work with ELF symbol versioning. That's why Rust's libc has always exposed a FreeBSD 11 ABI. Your change broke that guarantee, and causes libc to now expose the native ABI for whatever platform the crate is compiled on. mio broke because it was programming for the FreeBSD 11 ABI, and now suddenly libc is using a FreeBSD 12 ABI. Please revert this change. We ought to raise the ABI at some point, but it must be done very carefully, and please include me in the PR.

[GuillaumeGomez]

I'm a bit lost by your explanations: this is part of the freebsd header files and libraries.

mio broke because it was programming for the FreeBSD 11 ABI, and now suddenly libc is using a FreeBSD 12 ABI.

For cross-compilation, I guess we can add a cargo feature to handle it if you target a specific freebsd version. However, building using a freebsd 11 definitions version on a freebsd non-11 version is completely wrong. In my case it simply created UB because I passed types with the wrong ABI/size/layout.

[Thomasdezeeuw]

I'm a bit lost by your explanations: this is part of the freebsd header files and libraries.

mio broke because it was programming for the FreeBSD 11 ABI, and now suddenly libc is using a FreeBSD 12 ABI.

For cross-compilation, I guess we can add a cargo feature to handle it if you target a specific freebsd version. However, building using a freebsd 11 definitions version on a freebsd non-11 version is completely wrong. In my case it simply created UB because I passed types with the wrong ABI/size/layout.

@GuillaumeGomez for some context Mio has been used libc stuff pretty heavily and works without issues on FreeBSD 12.

(also for some further context @asomers knows FreeBSD pretty well ;) )

[asomers]

There's a been a lot of discussion about how to enable FreeBSD 12+ features in libc. Unfortunately, there's no agreed plan yet.

And not, it's not completely wrong to use the FreeBSD 11 ABI. That's the magic of ELF symbol versioning! When you run mio under FreeBSD 12, the libc::kevent structure corresponds to what in C is known as freebsd11_kevent. And the libc::kevent FFI function actually calls kevent@FBSD_1.0 rather than plain kevent. And this function calls the freebsd11_kevent syscall, rather than the plain kevent syscall. That's why everything is backwards compatible.

[nox]

As an aside about structures such as libc::kevent etc, aren't those structures expected to get new fields when OS get new features? Shouldn't they be marked #[non_exhaustive]?

[asomers]

As an aside about structures such as libc::kevent etc, aren't those structures expected to get new fields when OS get new features? Shouldn't they be marked #[non_exhaustive]?

That's not a bad idea. But which structures should get the non-exhaustive treatment? Of the hundreds of structures exported by libc, there are very few that are ever guaranteed not to grow.

[asomers]

In my case it simply created UB because I passed types with the wrong ABI/size/layout.

How did this happen? Were you mixing libc's FFI functions with your own struct definitions or something?

[GuillaumeGomez]

What about this: we add features to cargo which allows you to pick the freebsd version you want at compile-time instead of targetting the FreeBSD 11 in all cases? For people like me, we get what we want by default and you can choose which version to use. Does that sound reasonable to you?

[GuillaumeGomez]

To provide more context: I added freebsd in sysinfo recently and was testing it on freebsd 14. However, some types didn't have the expected ABI, preventing me to query disk information. I realized that valgrind was also complaining about some other structs I was using because they had the freebsd 11 ABI and I was on freebsd 14. The current situation is pretty bad in that regard because a highly specialized case is the default and anyone not aware of it (I still have no clue how you're doing what you're doing) will end up in my situation: it cannot work because the ABI is incompatible.

EDIT: Some extra details: the function in question is getmntinfo used with the statfs struct. You can see how I use them in this function.

[GuillaumeGomez]

In my case it simply created UB because I passed types with the wrong ABI/size/layout.

How did this happen? Were you mixing libc's FFI functions with your own struct definitions or something?

No, using 100% libc.

[Amanieu]

I think the issue is from #2058 which added getmntinfo without specifying an explicit version with link_name. This causes it to always use the latest version available on the host even though libc uses the FreeBSD 11 version of the statfs struct.

[GuillaumeGomez]

Here is the nm output:

nm -D /lib/libc.so.7 | grep getmntinfo
000000000000a1360 T getmntinfo
000000000000a1250 T getmntinfo

[Amanieu]

Sorry, wrong PR, the FreeBSD getmntinfo was added in #2572.

[GuillaumeGomez]

This is a ticking bomb and we need to do something about it. libc is one of the most used crate in the rust ecosystem and this is a huge bug. The CI didn't catch it and I don't know if it can. The fact that I'm the first to encounter this bug is quite surprising though.

Also, I was very happy to be able to get full disk information and I'll be forced to go back to freebsd 11's limitations. I'm very frustrated to be limited to a version like that.

@asomers What do you think about what I suggested above? I don't want to be stuck forever on one version without a way to be able to use the current version. If what I suggested doesn't work, how should we do it?

[asomers]

@asomers What do you think about what I suggested above? I don't want to be stuck forever on one version without a way to be able to use the current version. If what I suggested doesn't work, how should we do it?

Adding an on-by-default Cargo feature like "freebsd11-compat" was actually my first suggestion when this originally came up a long time ago. However, the rest of the community didn't like it. It is too FreeBSD-specific. The same technique doesn't work with OpenBSD or Linux, for example. OpenBSD users favor an approach of adding the major version number to the target-triple, making it a target-quadruple instead.

For the specific question of FreeBSD 11 compatibility, I favor dropping it entirely now that FreeBSD 11.4 is EoL. See rust-lang/rust#89083 . That will resolve most issues like yours. There are far fewer symbol changes between 12->13 and 13->14 than there were from 11->12.

[GuillaumeGomez]

That still leaves a lot of potential issues. Also, maybe it's worth opening the debate once again. Being stuck on one version is just too problematic. And there is also no way to "unlock" it too...

I'm surprised your suggestion was rejected though. What's the problem to have features specific for freebsd? Do you still have the conversation around by any chance?

[asomers]

I'm surprised your suggestion was rejected though. What's the problem to have features specific for freebsd? Do you still have the conversation around by any chance?

#570

[GuillaumeGomez]

Wow, quite the thread. It's still far from done by at least there is hope. I am really surprised that such a big issue that a lot of people were aware of was not addressed much earlier. I'm really just the last one in a long list of people who encountered this issue...