Skip to content

FreeBSD: ucontext/mcontext, aarch64 fixes, librt linking, etc #1630

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

Merged
merged 8 commits into from
Jul 6, 2020

Conversation

valpackett
Copy link
Contributor

@rust-highfive
Copy link

r? @gnzlbg

(rust_highfive has picked a reviewer for you, use r? to override)

@valpackett valpackett force-pushed the freebsd-mcontext branch 3 times, most recently from d65b0da to 2481232 Compare December 30, 2019 14:20
@valpackett
Copy link
Contributor Author

ping. this is really needed for Lucet, which is in pretty good shape now.

we could leave FreeBSD/aarch64 untested because the tests currently don't work in master anyway. or I could split the aarch64 mcontext into a separate PR that would wait for proper 128 bit support in tests or whatever is needed.

@JohnTitor
Copy link
Member

Sorry for the delay, I'll check it later.
r? @JohnTitor

@rust-highfive rust-highfive assigned JohnTitor and unassigned gnzlbg May 9, 2020
@JohnTitor
Copy link
Member

Hm, CI fails: nightly x86_64-unknown-freebsd-10. Also needs to fix style issues.

@valpackett
Copy link
Contributor Author

CI fails: nightly x86_64-unknown-freebsd-10

Added ucontext.h to libc-test headers. (On newer versions it was picked up transitively).

Now docs for i686 fail to generate??

cannot find type ucontext_t in this scope

but.. I see there's plenty of target_arch usage and I think this particular CI didn't fail on the previous push o_0

@valpackett
Copy link
Contributor Author

  • Fixed style
  • Moved the cfg outside of the s_no_extra_traits!, now docs don't fail
  • Added new fix: aio_* and mq_* functions need linking to librt! How did no one notice this o_0

Now all CI failures seem unrelated

@valpackett
Copy link
Contributor Author

ping

@JohnTitor
Copy link
Member

Oh, sorry for the delay! Unfortunately, the build log has been gone, re-triggering CI.

@JohnTitor JohnTitor closed this Jun 30, 2020
@JohnTitor JohnTitor reopened this Jun 30, 2020
@valpackett
Copy link
Contributor Author

Do you want us to ship this ASAP?

Yes, that would be very good.

@JohnTitor
Copy link
Member

CI still fails with importing and Copy trait issues.

@valpackett
Copy link
Contributor Author

That looks like some really ancient rustcs not being able to apply cfg to a macro invocation >_<

I guess I can move the whole ucontext_t to x86_64 for now, and deal with making it multi platform later..

@@ -78,6 +80,16 @@ s_no_extra_traits! {
pub xmm_reg: [[u8; 16]; 8],
pub xmm_pad: [u8; 224],
}

#[cfg_attr(feature = "extra_traits", derive(Debug))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unnecessary since s! macro already derives it.

Copy link
Contributor Author

@valpackett valpackett Jul 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is s_no_extra_traits!

I guess since we implement everything on mcontext, ucontext could go into s..

upd: well after adding Hash we do

#[cfg_attr(feature = "extra_traits", derive(Debug))]
pub struct ucontext_t {
pub uc_sigmask: ::sigset_t,
pub uc_mcontext: ::mcontext_t,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This causes importing failure:

error[E0412]: cannot find type `mcontext_t` in the crate root
  --> src/unix/bsd/freebsdlike/freebsd/x86_64/mod.rs:87:28
   |
87 |         pub uc_mcontext: ::mcontext_t,
   |                            ^^^^^^^^^^ did you mean `ucontext_t`?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess these ancient rustcs were single pass and the pub use self::align::* at the bottom doesn't bring it into this scope?!

Screw it, moving ucontext_t into the align module too.

@valpackett valpackett force-pushed the freebsd-mcontext branch 4 times, most recently from c638821 to 443cf7b Compare July 5, 2020 22:41
@JohnTitor
Copy link
Member

Okay, now looks fine to me!
Remaining CI failures are spurious and #1812 fixed one of them, could you rebase?

sys/stat.h is a machine-independent header, but it has ifdefs for i386.
The version that was called x86_64.rs should be used on powerpc64 too,
but I don't have a machine to test that on.
Wide chars are unsigned when normal chars are
It was picked up transitively on >10, but tests failed on 10
@valpackett
Copy link
Contributor Author

Done!

@valpackett valpackett changed the title FreeBSD: ucontext/mcontext, aarch64 fixes FreeBSD: ucontext/mcontext, aarch64 fixes, librt linking, etc Jul 6, 2020
Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@JohnTitor JohnTitor merged commit 4fc045c into rust-lang:master Jul 6, 2020
@asomers
Copy link
Contributor

asomers commented Jan 3, 2021

For the case of aio_ functions, nobody noticed the lack of librt before because it isn't required. Linking to librt is only required if you wish to use SIGEV_THREAD completion notifications. Everything else requires only libc. That requirement is documented in the sigevent(3) man page.

Using SIGEV_THREAD is very rare in practice, because the performance is not so good. In fact, due to Rust's originally poor support for C unions, libc's definition for struct sigevent doesn't even allow you to set the fields needed by SIGEV_THREAD! So we shouldn't link those functions to librt.

Your change was of course correct as regards mq_ functions.

asomers added a commit to asomers/libc that referenced this pull request Jan 3, 2021
On FreeBSD, the aio_ functions require librt _only_ if they use
SIGEV_THREAD completion notification.  However, due to Rust's originally
poor support for C unions, libc doesn't even expose some of the fields of
struct sigevent that SIGEV_THREAD requires.  Accordingly, there is no
need to link librt to programs using aio via libc.

This change partially reverts 8c23f77
from PR rust-lang#1630 .
asomers added a commit to asomers/libc that referenced this pull request Jan 3, 2021
On FreeBSD, the aio_ functions require librt _only_ if they use
SIGEV_THREAD completion notification.  However, due to Rust's originally
poor support for C unions, libc doesn't even expose some of the fields of
struct sigevent that SIGEV_THREAD requires.  Accordingly, there is no
need to link librt to programs using aio via libc.

This change partially reverts 8c23f77
from PR rust-lang#1630 .

While I'm here, fix the linkage of lio_listio on DragonflyBSD.  It
_does_ require librt.
bors added a commit that referenced this pull request Jan 5, 2021
aio functions do not require librt on FreeBSD

On FreeBSD, the aio_ functions require librt _only_ if they use
SIGEV_THREAD completion notification.  However, due to Rust's originally
poor support for C unions, libc doesn't even expose some of the fields of
struct sigevent that SIGEV_THREAD requires.  Accordingly, there is no
need to link librt to programs using aio via libc.

This change partially reverts 8c23f77
from PR #1630 .

While I'm here, fix the linkage of lio_listio on DragonflyBSD.  It
_does_ require librt.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants