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

RFC: Target extension #2048

Open
wants to merge 57 commits into
base: master
from

Conversation

Projects
None yet
@semarie

semarie commented Jun 27, 2017

Rendered

semarie and others added some commits May 21, 2017

@semarie

This comment has been minimized.

Show comment
Hide comment
@semarie

semarie Jul 11, 2017

@alexcrichton I think we both agree that there is a problem of changes in version that need to be adressed. The RFC proposes to deal with it at target level (as LLVM itself), and the alternative you propose to deal with at function level.

For me, runtime detection doesn't address the problem in depth and could be error prone. But I could be wrong as I was about the overhead (lazy detection and caching could solve it, thanks to mention them). So fell free to explain it more in depth.

Personally I would prefer you do it early when you asked me to write this RFC.

But let's me explain why I think the runtime detection isn't a safe way, still using the struct stat changes in FreeBSD. But keep in mind that the breaking change in FreeBSD isn't about struct stat, but ino_t: so we globally speak about any C struct or function that use it (as struct stat did), and we should do runtime detection for each of them without missing only one (weird behaviour expected at runtime else).

Dealing with FFI functions

Adding stat_freebsd12 in libc is a possibility. But what about functions using the struct stat ?

Currently, in libc crate, there are:

pub fn fstat(fildes: ::c_int, buf: *mut stat) -> ::c_int;
pub fn stat(path: *const c_char, buf: *mut stat) -> ::c_int;
pub fn wstat(path: *const wchar_t, buf: *mut stat) -> ::c_int;

Does they should be versioned too ? The second argument wouldn't be the right type for FreeBSD 12.

It also open the problem for every use of struct stat in any crate doing FFI: does all Rust developers will take care of FreeBSD if there use ino_t or a struct like stat that embedded it ? I doubt.

Possible security issues

Secondly, keeping the struct stat with the same name is also really dangerous.

Any FFI function using it directly could lead to security issue if used on FreeBSD 12: the size of the struct isn't the same on FreeBSD 11 (have a 32 bits ino_t) and on FreeBSD 12 (have a 64 bits ino_t), and the compiler will not catch it (because struct stat is defined globally for FreeBSD, and not only for FreeBSD 12).

Without renaming the struct stat, people will think they write portable code using POSIX names, and the compiler will not complain.

The underlined problem is if the Rust part doesn't care about FreeBSD case (and many developers will not if they doesn't target it specifically), its uses under FreeBSD 12 could make the C part read/write a big chunk of bytes in a smaller reserved one: rustc will allocate a stat (size smaller than stat_freebsd_12), and the C part will read/write the bigger native stat: it will introduce buffer overflows.

And due to Rust ecosystem, this problem could come in a project by any dependency which will not do the right work manipulating a FFI struct.

So it would be better to change it to stat_freebsd_10 (but it is a breaking change in libc).

compat module

For now, the sole way that I could see (but as said I would strongly prefer you to explain how you see that) to properly deal with breaking changes using runtime detection is to write some compat module, in order to replace any struct or function implied in breaking change by a "compatible" one.

I dunno if it should be separated crate or be integrated to libc.

For me, the main drawback is it makes Rust responsible to compatibility between OS versions, whereas the OS itself wasn't able/didn't want to do it itself. And
I found it very presumptious.

Basically it means removing any conflictious definition from libc (again, a breaking change in libc) and forbid readding them.

It would be a big work, as it means checking every struct and functions that could imply leading into a breaking change. It requires someone really familiar with internals of FreeBSD as it isn't a trivial task and failing to doing it properly could lead to security issues.

Creating a new compat_freebsd module which defines, per OS version, types and functions (the ones removed from libc):

  • module compat_freebsd::v11
    • structure compat_freebsd::v11::stat (suitable for FFI)
    • function compat_freebsd::v11::fstat(fildes: ::c_int, buf: *mut compat_freebsd::v11::stat) -> ::c_int
  • module compat_freebsd::v12
    • structure compat_freebsd::v12::stat (suitable for FFI)
    • function compat_freebsd::v12::fstat(fildes: ::c_int, buf: *mut compat_freebsd::v12::stat) -> ::c_int

and make it to define a "compat" module layer that will workaround breaking changes. For the current FreeBSD case, it could be a reuse of the v12 definition (as v11 could fit into).

  • module compat_freebsd::compat
    • structure compat_freebsd::compat::stat
    • function compat_freebsd::compat::fstat(fildes: ::c_int, buf: *mut compat_freebsd::compat::stat) -> ::c_int

libc would reexports these parts from compat_freebsd::compat in order to make them available at same level than other.

It means the default stat struct for FreeBSD (compat_freebsd::compat::stat reexported as libc::stat) isn't suitable for FFI anymore. But users could use compat_freebsd::v11::stat or compat_freebsd::v12::stat depending the OS (and do the OS detection themself for proper use).

The counter part is any Rust code at higher level would be free to use the struct stat with fstat() function: there are just wrapper that ensure using the right OS version.

And this work (move to compat_freebsd::vXX) should be done for each structure or function using ino_t internally.

In conclusion

Here is how I understand runtime detection. Again, please explain how you see it: you have a better understanding than me for the Rust part.

Runtime detetion could add lot of work for people that would take care of BSDs, and put on them the responsability of the compatibility level. It is a really high responsability as failing to do it properly could open security issues or bad behaviour at runtime.

Personally I found it could become quickly too much work for me for OpenBSD: I am maintaining rustc on it only on my free time.

I understand that managing OS versions at target level is also problematic for Rust developers: it would add more work due to the fact that Mozilla infrastructure (with rustup) is the prefered way to get rustc/cargo binaries. Adding targets has impact on infrastructure.

I also agree that having only one target would be easier for end-users, but it could be a nightmare for people having to provide the compat level and any error could become a security issue.

Last, sorry for this long comment.

semarie commented Jul 11, 2017

@alexcrichton I think we both agree that there is a problem of changes in version that need to be adressed. The RFC proposes to deal with it at target level (as LLVM itself), and the alternative you propose to deal with at function level.

For me, runtime detection doesn't address the problem in depth and could be error prone. But I could be wrong as I was about the overhead (lazy detection and caching could solve it, thanks to mention them). So fell free to explain it more in depth.

Personally I would prefer you do it early when you asked me to write this RFC.

But let's me explain why I think the runtime detection isn't a safe way, still using the struct stat changes in FreeBSD. But keep in mind that the breaking change in FreeBSD isn't about struct stat, but ino_t: so we globally speak about any C struct or function that use it (as struct stat did), and we should do runtime detection for each of them without missing only one (weird behaviour expected at runtime else).

Dealing with FFI functions

Adding stat_freebsd12 in libc is a possibility. But what about functions using the struct stat ?

Currently, in libc crate, there are:

pub fn fstat(fildes: ::c_int, buf: *mut stat) -> ::c_int;
pub fn stat(path: *const c_char, buf: *mut stat) -> ::c_int;
pub fn wstat(path: *const wchar_t, buf: *mut stat) -> ::c_int;

Does they should be versioned too ? The second argument wouldn't be the right type for FreeBSD 12.

It also open the problem for every use of struct stat in any crate doing FFI: does all Rust developers will take care of FreeBSD if there use ino_t or a struct like stat that embedded it ? I doubt.

Possible security issues

Secondly, keeping the struct stat with the same name is also really dangerous.

Any FFI function using it directly could lead to security issue if used on FreeBSD 12: the size of the struct isn't the same on FreeBSD 11 (have a 32 bits ino_t) and on FreeBSD 12 (have a 64 bits ino_t), and the compiler will not catch it (because struct stat is defined globally for FreeBSD, and not only for FreeBSD 12).

Without renaming the struct stat, people will think they write portable code using POSIX names, and the compiler will not complain.

The underlined problem is if the Rust part doesn't care about FreeBSD case (and many developers will not if they doesn't target it specifically), its uses under FreeBSD 12 could make the C part read/write a big chunk of bytes in a smaller reserved one: rustc will allocate a stat (size smaller than stat_freebsd_12), and the C part will read/write the bigger native stat: it will introduce buffer overflows.

And due to Rust ecosystem, this problem could come in a project by any dependency which will not do the right work manipulating a FFI struct.

So it would be better to change it to stat_freebsd_10 (but it is a breaking change in libc).

compat module

For now, the sole way that I could see (but as said I would strongly prefer you to explain how you see that) to properly deal with breaking changes using runtime detection is to write some compat module, in order to replace any struct or function implied in breaking change by a "compatible" one.

I dunno if it should be separated crate or be integrated to libc.

For me, the main drawback is it makes Rust responsible to compatibility between OS versions, whereas the OS itself wasn't able/didn't want to do it itself. And
I found it very presumptious.

Basically it means removing any conflictious definition from libc (again, a breaking change in libc) and forbid readding them.

It would be a big work, as it means checking every struct and functions that could imply leading into a breaking change. It requires someone really familiar with internals of FreeBSD as it isn't a trivial task and failing to doing it properly could lead to security issues.

Creating a new compat_freebsd module which defines, per OS version, types and functions (the ones removed from libc):

  • module compat_freebsd::v11
    • structure compat_freebsd::v11::stat (suitable for FFI)
    • function compat_freebsd::v11::fstat(fildes: ::c_int, buf: *mut compat_freebsd::v11::stat) -> ::c_int
  • module compat_freebsd::v12
    • structure compat_freebsd::v12::stat (suitable for FFI)
    • function compat_freebsd::v12::fstat(fildes: ::c_int, buf: *mut compat_freebsd::v12::stat) -> ::c_int

and make it to define a "compat" module layer that will workaround breaking changes. For the current FreeBSD case, it could be a reuse of the v12 definition (as v11 could fit into).

  • module compat_freebsd::compat
    • structure compat_freebsd::compat::stat
    • function compat_freebsd::compat::fstat(fildes: ::c_int, buf: *mut compat_freebsd::compat::stat) -> ::c_int

libc would reexports these parts from compat_freebsd::compat in order to make them available at same level than other.

It means the default stat struct for FreeBSD (compat_freebsd::compat::stat reexported as libc::stat) isn't suitable for FFI anymore. But users could use compat_freebsd::v11::stat or compat_freebsd::v12::stat depending the OS (and do the OS detection themself for proper use).

The counter part is any Rust code at higher level would be free to use the struct stat with fstat() function: there are just wrapper that ensure using the right OS version.

And this work (move to compat_freebsd::vXX) should be done for each structure or function using ino_t internally.

In conclusion

Here is how I understand runtime detection. Again, please explain how you see it: you have a better understanding than me for the Rust part.

Runtime detetion could add lot of work for people that would take care of BSDs, and put on them the responsability of the compatibility level. It is a really high responsability as failing to do it properly could open security issues or bad behaviour at runtime.

Personally I found it could become quickly too much work for me for OpenBSD: I am maintaining rustc on it only on my free time.

I understand that managing OS versions at target level is also problematic for Rust developers: it would add more work due to the fact that Mozilla infrastructure (with rustup) is the prefered way to get rustc/cargo binaries. Adding targets has impact on infrastructure.

I also agree that having only one target would be easier for end-users, but it could be a nightmare for people having to provide the compat level and any error could become a security issue.

Last, sorry for this long comment.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 19, 2017

Member

I mostly just want to explore the possibility of resolving this at runtime instead of compile time. My motivation for wanting to pursue this route is it's a viable method to actually solve this in the near future. If we instead add new targets then "fixing" this is blocked on:

  • Landing this RFC
  • Implementing this RFC
  • Adding new targets
  • Developing a story for version support of the BSDs in rust-lang/rust
  • Adding new builders for new targets
  • Deploying new binaries
  • Deploying more binary releases for rustup

All of that is a pretty long way off and debatable whether it will happen at all. For example this RFC doesn't begin to cover what rust-lang/rust CI will actually be doing, and that's a whole separate discussion in and of itself.

On the other hand, if we were to add runtime detection and more structs to libc, we could solve this problem immediately and have new binaries deployed by the next release of Rust.

You're 100% right in that the runtime approach can and probably will have bugs. I've been trying to brainstorm ideas of how to mitigate these, but they're not bulletproof of course. The advantage of the runtime approach (which I personally perceive as a major advantage) is that it's not blocked on anything.

It seems fine by me to pursue a multiple target strategy instead, but it should be pursued with the understanding that it will take a very long time to implement completely, if at all.

Member

alexcrichton commented Jul 19, 2017

I mostly just want to explore the possibility of resolving this at runtime instead of compile time. My motivation for wanting to pursue this route is it's a viable method to actually solve this in the near future. If we instead add new targets then "fixing" this is blocked on:

  • Landing this RFC
  • Implementing this RFC
  • Adding new targets
  • Developing a story for version support of the BSDs in rust-lang/rust
  • Adding new builders for new targets
  • Deploying new binaries
  • Deploying more binary releases for rustup

All of that is a pretty long way off and debatable whether it will happen at all. For example this RFC doesn't begin to cover what rust-lang/rust CI will actually be doing, and that's a whole separate discussion in and of itself.

On the other hand, if we were to add runtime detection and more structs to libc, we could solve this problem immediately and have new binaries deployed by the next release of Rust.

You're 100% right in that the runtime approach can and probably will have bugs. I've been trying to brainstorm ideas of how to mitigate these, but they're not bulletproof of course. The advantage of the runtime approach (which I personally perceive as a major advantage) is that it's not blocked on anything.

It seems fine by me to pursue a multiple target strategy instead, but it should be pursued with the understanding that it will take a very long time to implement completely, if at all.

@main--

This comment has been minimized.

Show comment
Hide comment
@main--

main-- Jul 22, 2017

I ran into a need for this today. I'm playing around with cross-platform inter-process locking code and the semantics I'd like to implement are:

  • On linux >= 2.6.7: use futex (admittedly, this is covered by rust's requirements (2.6.18+) already)
  • On any other linux, freebsd >= 11, macos >= 10.7: use pthread process-shared locks
  • On any other unix, fall back to some sysv semaphore hackery

Selecting this at runtime is not really feasible:

  • A futex takes 4 bytes, a pthread rwlock is 56 bytes (worst-case 1300% memory overhead for no reason)
  • Would probably just version-check at startup, but then
    • either every call to the locking code is indirect (performance overhead! and can't inline at all)
    • or the entire application is generic over the type of the locking implementation and thus monomorphized 3 times

Having a binary that runs both on new and old versions sure is neat but is it worth it? Ideally I would just figure out what code to use in one big cfg_if! {} (in fact, I'm doing that right now but with a // FIXME: version check).

main-- commented Jul 22, 2017

I ran into a need for this today. I'm playing around with cross-platform inter-process locking code and the semantics I'd like to implement are:

  • On linux >= 2.6.7: use futex (admittedly, this is covered by rust's requirements (2.6.18+) already)
  • On any other linux, freebsd >= 11, macos >= 10.7: use pthread process-shared locks
  • On any other unix, fall back to some sysv semaphore hackery

Selecting this at runtime is not really feasible:

  • A futex takes 4 bytes, a pthread rwlock is 56 bytes (worst-case 1300% memory overhead for no reason)
  • Would probably just version-check at startup, but then
    • either every call to the locking code is indirect (performance overhead! and can't inline at all)
    • or the entire application is generic over the type of the locking implementation and thus monomorphized 3 times

Having a binary that runs both on new and old versions sure is neat but is it worth it? Ideally I would just figure out what code to use in one big cfg_if! {} (in fact, I'm doing that right now but with a // FIXME: version check).

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb
Member

eddyb commented Jul 22, 2017

@semarie

This comment has been minimized.

Show comment
Hide comment
@semarie

semarie Jul 23, 2017

@main-- it seems to me that Target Extension isn't the right solution for your problem. And generally speaking, basing feature detection on explicit OS version check isn't the right approch.

You should have a build.rs that detect features (does this target has futex symbol ? pthread_create symbol ?) and provide rustc-cfg according to the detection. This way, you don't have to check the OS or the version: if the futex symbol is here, uses futexes (for example OpenBSD will have it in next version). The limitation of this technique is for crosscompilation (Build Scripts are run on the host, and not on the target).

The purpose of Target Extension is to represent breaking changes in the OS (or the environment), not to track every version of each OS.

semarie commented Jul 23, 2017

@main-- it seems to me that Target Extension isn't the right solution for your problem. And generally speaking, basing feature detection on explicit OS version check isn't the right approch.

You should have a build.rs that detect features (does this target has futex symbol ? pthread_create symbol ?) and provide rustc-cfg according to the detection. This way, you don't have to check the OS or the version: if the futex symbol is here, uses futexes (for example OpenBSD will have it in next version). The limitation of this technique is for crosscompilation (Build Scripts are run on the host, and not on the target).

The purpose of Target Extension is to represent breaking changes in the OS (or the environment), not to track every version of each OS.

@semarie

This comment has been minimized.

Show comment
Hide comment
@semarie

semarie Jul 23, 2017

@alexcrichton with Target Extension, the purpose is to solve a problem on the long term.

For me, runtime detection could be used temporary for solving the FreeBSD stat() problem and let rustc to run on FreeBSD 12 or makes fs::metadata() to behave correctly. But it will not solve the underline problem: the libc definition for ino_t (and so for struct stat) is wrong under FreeBSD 12. Any FFI program that would rely on them will have unexpected behavior or crash at runtime, and it is a really bad thing.

It doesn't mean we should not use it, but for me it shouldn't be the norm for solving breaking changes at OS level.

semarie commented Jul 23, 2017

@alexcrichton with Target Extension, the purpose is to solve a problem on the long term.

For me, runtime detection could be used temporary for solving the FreeBSD stat() problem and let rustc to run on FreeBSD 12 or makes fs::metadata() to behave correctly. But it will not solve the underline problem: the libc definition for ino_t (and so for struct stat) is wrong under FreeBSD 12. Any FFI program that would rely on them will have unexpected behavior or crash at runtime, and it is a really bad thing.

It doesn't mean we should not use it, but for me it shouldn't be the norm for solving breaking changes at OS level.

@main--

This comment has been minimized.

Show comment
Hide comment
@main--

main-- Jul 23, 2017

@semarie Unfortunately, things are not that simple. Quoting futex(2):

Glibc does not provide a wrapper for this system call; call it using syscall(2).

There is no futex symbol. I might be missing something but AFAICS the only way to detect futex support is a version check.

Similarly, quoting FreeBSD's pthread_rwlockattr_setpshared(3):

The pthread_rwlockattr_setpshared() function first appeared in
FreeBSD 3.0. Support for process-shared read/write locks appeared in
FreeBSD 11.0.

What this means is that in FreeBSD 3.0 - 10.4, the function always returns an error code. The same goes for OSX before 10.7.

Now detecting this at build time would indeed be easy - except there are still some other platforms that just don't have the symbol at all (instead of implementing a stub that always errors). Still manageable but super awkward, don't you think? Why deal with this mess when a simple version check would do?


All I'm trying to say here is that while I understand the original motivation of this RFC (breaking changes), new features is another usecase.

main-- commented Jul 23, 2017

@semarie Unfortunately, things are not that simple. Quoting futex(2):

Glibc does not provide a wrapper for this system call; call it using syscall(2).

There is no futex symbol. I might be missing something but AFAICS the only way to detect futex support is a version check.

Similarly, quoting FreeBSD's pthread_rwlockattr_setpshared(3):

The pthread_rwlockattr_setpshared() function first appeared in
FreeBSD 3.0. Support for process-shared read/write locks appeared in
FreeBSD 11.0.

What this means is that in FreeBSD 3.0 - 10.4, the function always returns an error code. The same goes for OSX before 10.7.

Now detecting this at build time would indeed be easy - except there are still some other platforms that just don't have the symbol at all (instead of implementing a stub that always errors). Still manageable but super awkward, don't you think? Why deal with this mess when a simple version check would do?


All I'm trying to say here is that while I understand the original motivation of this RFC (breaking changes), new features is another usecase.

@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Aug 21, 2017

Contributor

@semarie could you consider signing off to the MIT/Apache 2.0 licensing terms inside this issue: #2096 ? Thanks!

Contributor

est31 commented Aug 21, 2017

@semarie could you consider signing off to the MIT/Apache 2.0 licensing terms inside this issue: #2096 ? Thanks!

@semarie

This comment has been minimized.

Show comment
Hide comment
@semarie

semarie Sep 21, 2017

@rust-lang/libs I would like to have some feedback on the RFC.

I consider outside the scope of this RFC to demonstrate why a particular alternative (like runtime detection) wouldn't be a good long term solution. I would like to prefer discussing that on a particular document related to the alternative (like another RFC proposition): it would be more simple for me to discuss on facts and statements, instead of just feeling.

I consider RFC process a way to target long term solution for Rust. The fact that implementing the RFC and makes it to be effectively used in infrastucture would take long time shouldn't being a problem for the RFC itself.

semarie commented Sep 21, 2017

@rust-lang/libs I would like to have some feedback on the RFC.

I consider outside the scope of this RFC to demonstrate why a particular alternative (like runtime detection) wouldn't be a good long term solution. I would like to prefer discussing that on a particular document related to the alternative (like another RFC proposition): it would be more simple for me to discuss on facts and statements, instead of just feeling.

I consider RFC process a way to target long term solution for Rust. The fact that implementing the RFC and makes it to be effectively used in infrastucture would take long time shouldn't being a problem for the RFC itself.

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Sep 21, 2017

Member

cc @rust-lang/libs (only a member of a team can refer to theirs or any other team)

Member

eddyb commented Sep 21, 2017

cc @rust-lang/libs (only a member of a team can refer to theirs or any other team)

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Sep 26, 2017

Member

The libs team discussed this briefly during triage today, but our conclusion is that we unfortunately just don't have the bandwidth to work through the design here at this time. We've always long wanted a "platform team" whose responsibility is porting Rust to all manner of platforms and ensuring it works well across them all, and this RFC hits squarely on what such a team work work with. We'd also be thrilled to have members of the *BSD community as part of this team!

With the impl period having recently started though it will likely be some time before we can begin to organize such a team and make progress on these sorts of questions. Our hope though is that near the beginning of 2018 we can start to organize such a team to tackle the problems such as those solved in this RFC.

Member

alexcrichton commented Sep 26, 2017

The libs team discussed this briefly during triage today, but our conclusion is that we unfortunately just don't have the bandwidth to work through the design here at this time. We've always long wanted a "platform team" whose responsibility is porting Rust to all manner of platforms and ensuring it works well across them all, and this RFC hits squarely on what such a team work work with. We'd also be thrilled to have members of the *BSD community as part of this team!

With the impl period having recently started though it will likely be some time before we can begin to organize such a team and make progress on these sorts of questions. Our hope though is that near the beginning of 2018 we can start to organize such a team to tackle the problems such as those solved in this RFC.

@dumbbell

This comment has been minimized.

Show comment
Hide comment
@dumbbell

dumbbell Dec 6, 2017

Hi!

I would like to add another example, still based on FreeBSD 12 (next major release) vs. previous branches. struct kevent, used by the kqueue(2) API, received several changes:

@@ -62,8 +66,9 @@ struct kevent {
        short           filter;         /* filter for event */
        unsigned short  flags;
        unsigned int    fflags;
-       __intptr_t      data;
+       __int64_t       data;
        void            *udata;         /* opaque user data identifier */
+       __uint64_t      ext[4];
 };

So:

  1. The data member changed type to become 64-bit on all platforms.
  2. There is a new ext member.

And thus, the API and the ABI were changed.

Now back to Rust: this structure is defined in libc but is initialized used in the mio crate. So patching the FreeBSD package of Rust isn't enough because mio is pulled in by Cargo. When built on FreeBSD 12, the crate obviously doesn't work: an executable using kqueue(2) (indirectly and implicitely through mio) hangs.

dumbbell commented Dec 6, 2017

Hi!

I would like to add another example, still based on FreeBSD 12 (next major release) vs. previous branches. struct kevent, used by the kqueue(2) API, received several changes:

@@ -62,8 +66,9 @@ struct kevent {
        short           filter;         /* filter for event */
        unsigned short  flags;
        unsigned int    fflags;
-       __intptr_t      data;
+       __int64_t       data;
        void            *udata;         /* opaque user data identifier */
+       __uint64_t      ext[4];
 };

So:

  1. The data member changed type to become 64-bit on all platforms.
  2. There is a new ext member.

And thus, the API and the ABI were changed.

Now back to Rust: this structure is defined in libc but is initialized used in the mio crate. So patching the FreeBSD package of Rust isn't enough because mio is pulled in by Cargo. When built on FreeBSD 12, the crate obviously doesn't work: an executable using kqueue(2) (indirectly and implicitely through mio) hangs.

bdrewery added a commit to bdrewery/libc that referenced this pull request Mar 1, 2018

Use pre-ino64 FreeBSD symbols to resolve binary compatibility.
This follows the same method as other platforms like OSX and NetBSD.

This fixes rustup and building from git (once libc is updated for bootstrap)
on FreeBSD 12 post-ino64 in freebsd/freebsd@f713b08.

The only real pitfall is that this will prevent interaction with inodes that
have an ino_t above the 32-bit limit due to truncation.  On the other hand Rust
won't work at all on 12 without doing this currently.

A better, or complementary, approach would be something like proposed in
rust-lang/rfcs#2048 to allow targetting a specific
version of FreeBSD. This would allow Rust to default to this compatibility mode
by targetting FreeBSD10 and still allow targetting FreeBSD12 for 64-bit ino_t.

Fixes rust-lang/rust#42681

bdrewery added a commit to bdrewery/libc that referenced this pull request Mar 1, 2018

Use pre-ino64 FreeBSD symbols to resolve binary compatibility.
This follows the same method as other platforms like OSX and NetBSD.

This fixes rustup and building from git (once libc is updated for bootstrap)
on FreeBSD12 post-ino64 in freebsd/freebsd@f713b08.
It also avoids having to hotpatch the stage0 compiler on FreeBSD12 to
build rust.

The only real pitfall is that this will prevent interaction with inodes that
have an ino_t above the 32-bit limit due to truncation.  On the other hand
Rust won't work at all on 12 without doing this currently.  In general
it should not be a problem for users and if they need 64-bit ino_t they
can use a patched libc, rather than the current state of affairs in
requiring a patched libc to use Rust on 12.

A better, or complementary, approach would be something like proposed in
rust-lang/rfcs#2048 to allow targetting a specific
version of FreeBSD. This would allow Rust to default to this compatibility
mode by targetting FreeBSD10 and still allow targetting FreeBSD12 for 64-bit
ino_t.

Fixes rust-lang/rust#42681

bdrewery added a commit to bdrewery/libc that referenced this pull request Mar 1, 2018

Use pre-ino64 FreeBSD symbols to resolve binary compatibility.
This follows the same method as other platforms like OSX and NetBSD.

This fixes rustup and building from git (once libc is updated for bootstrap)
on FreeBSD12 post-ino64 in freebsd/freebsd@f713b08.
It also avoids having to hotpatch the stage0 compiler on FreeBSD12 to
build rust.

The only real pitfall is that this will prevent interaction with inodes that
have an ino_t above the 32-bit limit due to truncation.  On the other hand
Rust won't work at all on 12 without doing this currently.  In general
it should not be a problem for users and if they need 64-bit ino_t they
can use a patched libc, rather than the current state of affairs in
requiring a patched libc to use Rust on 12.

A better, or complementary, approach would be something like proposed in
rust-lang/rfcs#2048 to allow targetting a specific
version of FreeBSD. This would allow Rust to default to this compatibility
mode by targetting FreeBSD10 and still allow targetting FreeBSD12 for 64-bit
ino_t.

The symbol versions used were taken from the old version in
freebsd/freebsd@f713b08#diff-61a32fcfb7ecd4517665fed591813c57
and
freebsd/freebsd@f713b08#diff-7f67ccf8b5f44ff2f54eaab0207abb8d.

Fixes rust-lang/rust#42681

bdrewery added a commit to bdrewery/libc that referenced this pull request Mar 1, 2018

Use pre-ino64 FreeBSD symbols to resolve binary compatibility.
This follows the same method as other platforms like OSX and NetBSD.

This fixes rustup and building from git (once libc is updated for bootstrap)
on FreeBSD12 post-ino64 in freebsd/freebsd@f713b08.
It also avoids having to hotpatch the stage0 compiler, and HOME/.cargo
libc files on FreeBSD12 to build rust.

The only real pitfall is that this will prevent interaction with inodes that
have an ino_t above the 32-bit limit due to truncation.  On the other hand
Rust won't work at all on 12 without doing this currently.  In general
it should not be a problem for users and if they need 64-bit ino_t they
can use a patched libc, rather than the current state of affairs in
requiring a patched libc to use Rust on 12.

A better, or complementary, approach would be something like proposed in
rust-lang/rfcs#2048 to allow targetting a specific
version of FreeBSD. This would allow Rust to default to this compatibility
mode by targetting FreeBSD10 and still allow targetting FreeBSD12 for 64-bit
ino_t.

The symbol versions used were taken from the old version in
freebsd/freebsd@f713b08#diff-61a32fcfb7ecd4517665fed591813c57
and
freebsd/freebsd@f713b08#diff-7f67ccf8b5f44ff2f54eaab0207abb8d.

Fixes rust-lang/rust#42681

bdrewery added a commit to bdrewery/libc that referenced this pull request Mar 1, 2018

Use pre-ino64 FreeBSD symbols to resolve binary compatibility.
This follows the same method as other platforms like OSX and NetBSD.

This fixes rustup and building from git (once libc is updated for bootstrap)
on FreeBSD12 post-ino64 in freebsd/freebsd@f713b08.
It also avoids having to hotpatch the stage0 compiler, and HOME/.cargo
libc files on FreeBSD12 to build rust.

The only real pitfall is that this will prevent interaction with inodes that
have an ino_t above the 32-bit limit due to truncation.  On the other hand
Rust won't work at all on 12 without doing this currently.  In general
it should not be a problem for users and if they need 64-bit ino_t they
can use a patched libc, rather than the current state of affairs in
requiring a patched libc to use Rust on 12.

A better, or complementary, approach would be something like proposed in
rust-lang/rfcs#2048 to allow targetting a specific
version of FreeBSD. This would allow Rust to default to this compatibility
mode by targetting FreeBSD10 and still allow targetting FreeBSD12 for 64-bit
ino_t.

The symbol versions used were taken from the old version in
freebsd/freebsd@f713b08#diff-61a32fcfb7ecd4517665fed591813c57
and
freebsd/freebsd@f713b08#diff-7f67ccf8b5f44ff2f54eaab0207abb8d.

The scope of functions versioned here differs from other platforms as
not all structs were modified that were on others, such as DIR for
`opendir`, `telldir`, etc..

Fixes rust-lang/rust#42681

bdrewery added a commit to bdrewery/libc that referenced this pull request Mar 1, 2018

Use pre-ino64 FreeBSD symbols to resolve binary compatibility.
This follows the same method as other platforms like OSX and NetBSD.

This fixes rustup and building from git (once libc is updated for bootstrap)
on FreeBSD12 post-ino64 in freebsd/freebsd@f713b08.
It also avoids having to hotpatch the stage0 compiler, and HOME/.cargo
libc files on FreeBSD12 to build rust.

The only real pitfall is that this will prevent interaction with inodes that
have an ino_t above the 32-bit limit due to truncation.  On the other hand
Rust won't work at all on 12 without doing this currently.  In general
it should not be a problem for users and if they need 64-bit ino_t they
can use a patched libc, rather than the current state of affairs in
requiring a patched libc to use Rust on 12.

A better, or complementary, approach would be something like proposed in
rust-lang/rfcs#2048 to allow targetting a specific
version of FreeBSD. This would allow Rust to default to this compatibility
mode by targetting FreeBSD10 and still allow targetting FreeBSD12 for 64-bit
ino_t.

The symbol versions used were taken from the old version in
freebsd/freebsd@f713b08#diff-61a32fcfb7ecd4517665fed591813c57
and
freebsd/freebsd@f713b08#diff-7f67ccf8b5f44ff2f54eaab0207abb8d.

The scope of functions versioned here differs from other platforms as
not all structs were modified that were on others, such as DIR for
`opendir`, `telldir`, etc.  Only functions using dirent, stat, glob_t,
and dev_t need the changes.

Fixes rust-lang/rust#42681

bdrewery added a commit to bdrewery/libc that referenced this pull request Mar 1, 2018

Use pre-ino64 FreeBSD symbols to resolve binary compatibility.
This follows the same method as other platforms like OSX and NetBSD.

This will fix rustup and building from git (once libc is updated for bootstrap)
on FreeBSD12 post-ino64 in freebsd/freebsd@f713b08.
It also avoids having to hotpatch the stage0 compiler, and HOME/.cargo
libc files on FreeBSD12 to build rust.

The only real pitfall is that this will prevent interaction with inodes that
have an ino_t above the 32-bit limit due to truncation.  On the other hand
Rust won't work at all on 12 without doing this currently.  In general
it should not be a problem for users and if they need 64-bit ino_t they
can use a patched libc, rather than the current state of affairs in
requiring a patched libc to use Rust on 12.

A better, or complementary, approach would be something like proposed in
rust-lang/rfcs#2048 to allow targetting a specific
version of FreeBSD. This would allow Rust to default to this compatibility
mode by targetting FreeBSD10 and still allow targetting FreeBSD12 for 64-bit
ino_t.

The symbol versions used were taken from the old version in
freebsd/freebsd@f713b08#diff-61a32fcfb7ecd4517665fed591813c57
and
freebsd/freebsd@f713b08#diff-7f67ccf8b5f44ff2f54eaab0207abb8d.

The scope of functions versioned here differs from other platforms as
not all structs were modified that were on others, such as DIR for
`opendir`, `telldir`, etc.  Only functions using dirent, stat, glob_t,
and dev_t need the changes.

Fixes rust-lang/rust#42681

bors added a commit to rust-lang/libc that referenced this pull request Mar 2, 2018

Auto merge of #937 - bdrewery:freebsd-ino64-compat, r=alexcrichton
Use pre-ino64 FreeBSD symbols to resolve binary compatibility.

This follows the same method as other platforms like OSX and NetBSD.

This will fix rustup and building from git (once libc is updated for bootstrap)
on FreeBSD12 post-ino64 in freebsd/freebsd@f713b08.
It also avoids having to hotpatch the stage0 compiler, and HOME/.cargo
libc files on FreeBSD12 to build rust.

The only real pitfall is that this will prevent interaction with inodes that
have an ino_t above the 32-bit limit due to truncation.  On the other hand
Rust won't work at all on 12 without doing this currently.  In general
it should not be a problem for users and if they need 64-bit ino_t they
can use a patched libc, rather than the current state of affairs in
requiring a patched libc to use Rust on 12.

A better, or complementary, approach would be something like proposed in
rust-lang/rfcs#2048 to allow targetting a specific
version of FreeBSD. This would allow Rust to default to this compatibility
mode by targetting FreeBSD10 and still allow targetting FreeBSD12 for 64-bit
ino_t.

The symbol versions used were taken from the old version in
freebsd/freebsd@f713b08#diff-61a32fcfb7ecd4517665fed591813c57
and
freebsd/freebsd@f713b08#diff-7f67ccf8b5f44ff2f54eaab0207abb8d.

The scope of functions versioned here differs from other platforms as
not all structs were modified that were on others, such as DIR for
`opendir`, `telldir`, etc.  Only functions using dirent, stat, glob_t,
and dev_t need the changes.

Fixes rust-lang/rust#42681
@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Mar 12, 2018

Member

Returning to this RFC after some time now, I think that we may be able to make some progess on this perhaps? Not a whole lot has changed in the meantime other than rust-lang/libc#937, but I think that we would be able to accept this and improve the story (at least on nightly) for the BSD-like systems with a few changes. @semarie would you be ok with changes like this?

  • We won't be distributing multiple versions of libraries any time soon, so I think we'll want to scope this RFC back to only mention the BSD targets rather than also platforms like OSX.
  • Could the RFC explicitly list what version of FreeBSD/NetBSD targets will be shipped along with a small transition plan of how to get from where we are today to the targets-of-tomorrow?

With that I think we'd definitely be open to implementing an unstable version of this RFC (aka with the relevant #[cfg] matchers and whatnot).

Member

alexcrichton commented Mar 12, 2018

Returning to this RFC after some time now, I think that we may be able to make some progess on this perhaps? Not a whole lot has changed in the meantime other than rust-lang/libc#937, but I think that we would be able to accept this and improve the story (at least on nightly) for the BSD-like systems with a few changes. @semarie would you be ok with changes like this?

  • We won't be distributing multiple versions of libraries any time soon, so I think we'll want to scope this RFC back to only mention the BSD targets rather than also platforms like OSX.
  • Could the RFC explicitly list what version of FreeBSD/NetBSD targets will be shipped along with a small transition plan of how to get from where we are today to the targets-of-tomorrow?

With that I think we'd definitely be open to implementing an unstable version of this RFC (aka with the relevant #[cfg] matchers and whatnot).

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Mar 12, 2018

Member

Oh also one aspect that came up during triage is that we may wish to leave out the comparators like version_le and such for now. Those can always be reimplemented in build scripts for the time being and otherwise would help trim this down a bit!

Member

alexcrichton commented Mar 12, 2018

Oh also one aspect that came up during triage is that we may wish to leave out the comparators like version_le and such for now. Those can always be reimplemented in build scripts for the time being and otherwise would help trim this down a bit!

@semarie

This comment has been minimized.

Show comment
Hide comment
@semarie

semarie Mar 14, 2018

@alexcrichton no problem to adapt the RFC.

Just a point, could you explain to me (or point to me an external document) about what you mean by stopping distributing multiple versions of libraries ? I am unsure to understand.

For the BSD version that should be supported, I think it is better than NetBSD and FreeBSD developers says what they expect and see if it is ok with Rust developers. I think difference should be done between "the code supports the version" and "Rust infrastructure provides a binary".

If I take OpenBSD as example, we only supports the two latest releases (and the current developpement HEAD). So it would be no sens for Rust to provides compatibility code for more than these. And it would be acceptable for me to have only latest release + HEAD supported in libc (for example). Regarding distribution (even if currently OpenBSD is tier-3 and no binary is distributed by Rust infrastructure) having only binary for latest release would be ok for me.

Please correct me if I didn't understand correctly something. Thanks.

semarie commented Mar 14, 2018

@alexcrichton no problem to adapt the RFC.

Just a point, could you explain to me (or point to me an external document) about what you mean by stopping distributing multiple versions of libraries ? I am unsure to understand.

For the BSD version that should be supported, I think it is better than NetBSD and FreeBSD developers says what they expect and see if it is ok with Rust developers. I think difference should be done between "the code supports the version" and "Rust infrastructure provides a binary".

If I take OpenBSD as example, we only supports the two latest releases (and the current developpement HEAD). So it would be no sens for Rust to provides compatibility code for more than these. And it would be acceptable for me to have only latest release + HEAD supported in libc (for example). Regarding distribution (even if currently OpenBSD is tier-3 and no binary is distributed by Rust infrastructure) having only binary for latest release would be ok for me.

Please correct me if I didn't understand correctly something. Thanks.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Mar 14, 2018

Member

@semarie oh sure I mostly mean that currently we are building and shipping x86_64-unknown-freebsd on CI, and after this RFC we would continue to only build/ship one target per platforms (like we do today). We probably won't be doing multiple versions of FreeBSD in the near future, for example. The code, however, can of course have compatibility for everything!

Member

alexcrichton commented Mar 14, 2018

@semarie oh sure I mostly mean that currently we are building and shipping x86_64-unknown-freebsd on CI, and after this RFC we would continue to only build/ship one target per platforms (like we do today). We probably won't be doing multiple versions of FreeBSD in the near future, for example. The code, however, can of course have compatibility for everything!

semarie added some commits Apr 13, 2018

Restrict the RFC to BSD OS
while in motivation section, update some historical points (some OS
version are released now)
Remove predicates from the RFC.
Only mention them as solution of possible drawbacks (long enumeration of
versions).
FreeBSD 12 is partially supported
under FreeBSD 12, libc uses FreeBSD 11 compat ABI. It limits `ino_t` to
32 bits (and uses truncation for not representable ino_t on 32 bits).
@semarie

This comment has been minimized.

Show comment
Hide comment
@semarie

semarie Apr 14, 2018

@alexcrichton I think the most important change concerns the addition of the migration path section. The intent is to take globally all changes and impacts involved in the implementation of this RFC regarding the whole Rust ecosystem, step by step.

Comments would be welcome, in under to ensure I meet your expectations.

semarie commented Apr 14, 2018

@alexcrichton I think the most important change concerns the addition of the migration path section. The intent is to take globally all changes and impacts involved in the implementation of this RFC regarding the whole Rust ecosystem, step by step.

Comments would be welcome, in under to ensure I meet your expectations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment