On musl targets assume certain symbols exist (like pipe2 and accept4).

[adrian-budau]

This fixes #56675.

I don't know if this is the best solution, or if I should also add some tests so I'm waiting for some feedback.

Thanks!

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[alexcrichton]

Thanks for the PR! Looks pretty good!

I wonder though if we could perhaps avoid the duplication at callsites? Could weak! generate code slightly differently on musl to perhaps unconditionally link-to and return the symbol? That way we could use weak! and it'd do dynamic lookups on other platforms but not actually do any work on musl. (I think it'd optimize to what you have here as well)

[adrian-budau]

This is what I tried initially, but the only solution I found for that was to hardcode every possible call of weak. The reason for this is that some symbols (like __pthread_get_minstack) are gnu specific and not present on musl.

I don’t think there is a way to check if a symbol exists in scope in rust (with a macro or other mechanism) to automate this.

[alexcrichton]

Ah right good point, and bummer! I wonder though if we could perhaps use the syscall function to get around this? That way we'd use pipe2 on musl I think as it wouldn't require a dlsym?

[adrian-budau]

Are you refering to runtime detection using syscall? That still wouldn’t work for the symbol I listed because it’s not a syscall, it’s from gnu libc.

Or maybe I misunderstood you.

[alexcrichton]

Oh yeah that's what I mean for pipe2/accept4, I misunderstood the pthread one (thought that was only on glibc). For that yeah I think we're forced to use #[cfg]

[adrian-budau]

Ok, so I’ll change the weak macro to assume the symbol exists in the libc crate (since that’s the case for accept4/pipe2) and let the pthread function be under #[cfg]. Sounds okay like this?

[alexcrichton]

Seems plausible to me!

[alexcrichton]

Oops sorry I didn't see that there were updates here. I've been thinking a bit more about this and I think it's actually probably best if we leave the weak! macro as-is and use the syscall function in libc itself. @adrian-budau would you be up for switching to that for pipe2/accept4? I don't mind sending a PR as well!

[alexcrichton]

Oh I'll assume that this comment was intended for here, so gonna reply here!

I forgot about other non-Linux platforms, sorry about that. The duplication is definitely one I think we'll want to avoid here! One thing I think we could do is have a syscall! macro perhaps which expands to weak! on non-Linux platforms and on Linux it expands to returning a Rust-defined function from get which uses libc::syscall under the hood.

I feel like it's still probably best to use syscall directly if we can to avoid dynamic lookups and changing the binary requirements for musl in case anyone's playing around with an older implementation. How's that sound though?

[adrian-budau]

Sorry about that commnet, wasn't paying attention where I was posting.

Sounds good so far, but I also have a small change to your proposal. We can just make this syscall! macro return ENOSYS if weak! returns None on non-Linux unixes. This way we should have the least amount of code duplicated.

Also because we are using syscalls directly it means we are effectively giving up the emulation of system calls from musl, which is fine I guess because this is already being done in rust code.

[alexcrichton]

Sounds reasonable to me!

Yeah it's a bummer that we don't leverage the musl code too much, but the reality is that we end up doing the same thing anyway for other platforms like glibc, so we end up needing to vendor the logic :(

[adrian-budau]

I don't know if force-pushing was the right call, I'm trying not to pollute the git history with the previous attempts. Hopefully this is also what you imagined the changes to be :-).

[alexcrichton]

@bors: r+

Thanks!

[bors]

📌 Commit bf172c5 has been approved by alexcrichton

[bors]

⌛ Testing commit bf172c5 with merge e40548b...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing e40548b to master...