Add __errno_location on Redox, DragonFlyBSD, Haiku #1432

josephlr · 2019-07-09T07:54:24Z

Currently libstd and the getrandom crate have to use external declarations in order to implement errno_location across all UNIX platforms. This change adds bindings for the remaining UNIX platforms, allowing everyone to just use normal libc bindings.

This also removes the need for a seperate errno-dragonfly crate.

Links to the relevant header files:

Redox: relibc's bits/errno.h uses __errno_location
Haiku: posix/errno.h uses _errnop
DragonFlyBSD: sys/errno.h uses __error just like FreeBSD (which makes sense)
- Note that the __error function is inlined. I don't think this causes a problem.

rust-highfive · 2019-07-09T07:54:28Z

r? @gnzlbg

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

gnzlbg · 2019-07-09T08:31:59Z

@bors: r+

Thanks!

bors · 2019-07-09T08:32:00Z

📌 Commit 4fc38ca has been approved by gnzlbg

bors · 2019-07-09T08:32:08Z

⌛ Testing commit 4fc38ca with merge acd8db5...

Add __errno_location on Redox, DragonFlyBSD, Haiku Currently [`libstd`](https://github.com/rust-lang/rust/blob/909f5a049415a815b23502a5498de9a99bbf276b/src/libstd/sys/unix/os.rs#L29-L49) and the [`getrandom` crate](https://github.com/rust-random/getrandom/pull/54/files#diff-027a56068e2bf3d31dc4273ee6e07034R12) have to use external declarations in order to implement `errno_location` across all UNIX platforms. This change adds bindings for the remaining UNIX platforms, allowing everyone to just use normal `libc` bindings. This also removes the need for a seperate [`errno-dragonfly`](https://crates.io/crates/errno-dragonfly) crate. Links to the relevant header files: - Redox: [`relibc`'s `bits/errno.h`](https://github.com/redox-os/relibc/blob/master/include/bits/errno.h) uses `__errno_location` - Haiku: [`posix/errno.h`](https://github.com/luciang/haiku/blob/master/headers/posix/errno.h) uses `_errnop` - DragonFlyBSD: [`sys/errno.h`](https://github.com/DragonFlyBSD/DragonFlyBSD/blob/master/sys/sys/errno.h) uses `__error` just like FreeBSD (which makes sense) - Note that the `__error` function is inlined. I don't think this causes a problem.

gnzlbg · 2019-07-09T08:33:03Z

@bors: r-

The paths need fixing.

bors · 2019-07-09T08:54:24Z

💔 Test failed - checks-travis

josephlr · 2019-07-09T09:50:26Z

The paths need fixing.

Copy paste bites again, should be fixed now.

gnzlbg · 2019-07-09T09:53:41Z

Thanks! @bors: r+

bors · 2019-07-09T09:53:42Z

📌 Commit e9c1ade has been approved by gnzlbg

bors · 2019-07-09T09:53:49Z

⌛ Testing commit e9c1ade with merge d829520...

Add __errno_location on Redox, DragonFlyBSD, Haiku Currently [`libstd`](https://github.com/rust-lang/rust/blob/909f5a049415a815b23502a5498de9a99bbf276b/src/libstd/sys/unix/os.rs#L29-L49) and the [`getrandom` crate](https://github.com/rust-random/getrandom/pull/54/files#diff-027a56068e2bf3d31dc4273ee6e07034R12) have to use external declarations in order to implement `errno_location` across all UNIX platforms. This change adds bindings for the remaining UNIX platforms, allowing everyone to just use normal `libc` bindings. This also removes the need for a seperate [`errno-dragonfly`](https://crates.io/crates/errno-dragonfly) crate. Links to the relevant header files: - Redox: [`relibc`'s `bits/errno.h`](https://github.com/redox-os/relibc/blob/master/include/bits/errno.h) uses `__errno_location` - Haiku: [`posix/errno.h`](https://github.com/luciang/haiku/blob/master/headers/posix/errno.h) uses `_errnop` - DragonFlyBSD: [`sys/errno.h`](https://github.com/DragonFlyBSD/DragonFlyBSD/blob/master/sys/sys/errno.h) uses `__error` just like FreeBSD (which makes sense) - Note that the `__error` function is inlined. I don't think this causes a problem.

bors · 2019-07-09T10:55:50Z

💔 Test failed - status-appveyor

BurntSushi · 2019-07-09T11:14:31Z

Note that both std and errno-dragonfly seem to implement this differently than what's in this PR for Dragonfly. Was that intentional?

gnzlbg · 2019-07-09T11:32:24Z

cc @strangelittlemonkey - could you review the dragonfly impl ?

gnzlbg · 2019-07-09T11:38:59Z

cc @sikmir could you review this for haiku?
cc @jackpot51 could you review this for redox ?

josephlr · 2019-07-09T18:46:28Z

Note that both std and errno-dragonfly seem to implement this differently than what's in this PR for Dragonfly. Was that intentional?

This was intentional. Implementing errno with a #[thread_local] would be slightly more efficient (as you don't have a function call). However, that's not possible on stable Rust, so using the function instead seems like the best bet.

The only difference between DragonFlyBSD an all the other UNIXes is that __error declared as:

static __inline int *__error(void) { ... }

in the header file (see the above link). I think Rust can handle such a function OK, but that could be the reason for the difference.

BurntSushi · 2019-07-09T19:53:49Z

Interesting. The errno-dragonfly crate actually uses custom C code to return errno and then links to that. Is that just an oversight on their part?

N.B. I am speaking from ignorance here. I'm only asking these questions because I'm curious. :-)

gnzlbg · 2019-07-10T07:52:38Z

I think Rust can handle such a function OK

The __error function is static (TU-private) and inline, so it probably isn't part of the generated binary, and we can't link against that. That is, we would need to implement it in this crate.

DragonflyBSD defines errno here as :

extern __thread int	errno;
static __inline int* __error() { return &errno; }

This:

extern __thread int	errno;
int* foo() { return &errno; }

emits:

define dso_local i32* @foo() #0 {
  ret i32* @errno
}

Notice the dso_local in there.

I think that in Rust we would need: extern "C" { #[thread_local] static errno: i32; } (using feature(thread_local)), but I don't know whether this currently works when used on extern statics. Right now we don't emit dso_local, and I don't know how important that is. cc @eddyb @rkruppe

gnzlbg · 2019-07-10T07:54:36Z

Also cc @joshtriplett: there is no stable Rust way to link against extern #[thread_local] statics, that should probably be added to the list of missing features for C FFI.

gnzlbg · 2019-07-10T07:55:58Z

@BurntSushi

Is that just an oversight on their part?

I don't think so. They can't create a pointer to the __thread int errno; from Rust, so they have to do that from C. See above.

eddyb · 2019-07-10T08:50:05Z

dso_local is a function attribute and likely the static on foo.
You need to compare the declarations of the @errno global to check if the #[thread_local] works (it should, since we lower it pretty straightforwardly).

gnzlbg · 2019-07-10T09:05:50Z

@eddyb so we lower the extern "C" { #[thread_local] static errno: i32; } to

@errno = external thread_local global i32

clang lowers this to:

@errno = external thread_local global i32, align 4

So I think #[thread_local] should work.

gnzlbg · 2019-07-10T09:09:55Z

@josephlr we do have a cargo feature feature = "unstable" that you could use to only provide the #[thread_local]s for nightly users that enable that feature via a #[cfg()].

I don't think there is a pure Rust solution available for stable Rust users. Linking to a C snippet that returns a pointer to the thread local like the errno-dragonfly crate does feels like the simplest solution to this problem.

eddyb · 2019-07-10T15:13:26Z

Could that platform's errno be gated by #[cfg(feature = "unstable")] to land this, and have an issue for stable support?

gnzlbg · 2019-07-10T15:29:01Z

@eddyb sure, and libstd enables the "unstable" feature, so it can make use of this to simplify the implementation. The getrandom crate would need to wait a bit.

jackpot51

This is correct for Redox

sikmir

LGTM

josephlr · 2019-07-10T18:24:17Z

@josephlr we do have a cargo feature feature = "unstable" that you could use to only provide the #[thread_local]s for nightly users that enable that feature via a #[cfg()].

@gnzlbg that works, I’ll make the change to add __error and errno behind this guard. For getrandom, we might just be OK requiring nightly for DragonFlyBSD.

josephlr · 2019-07-11T04:06:50Z

It turns out the CI is failing due to a transitive serde_derive dependency, which is breaking because of rust-lang/rust#62562

All the errors (as far as I can tell) are related to this bug, so the breakage is not caused by this PR.

strangelittlemonkey · 2019-07-11T15:40:24Z

You’re fine to write DragonFly code for something needing nightly but it’s dead code because we have no nightly build and I’m not sure we’ll ever have nightly support or a nightly build even. If you’re requiring nightly you should take DragonFly off the support list for the crate or any other like that because it would just be misleading.

…

On Jul 10, 2019, at 9:06 PM, Joseph Richey ***@***.***> wrote: It turns out the CI is failing due to a transitive serde_derive dependency, which is breaking because of rust-lang/rust#62562 All the errors (as far as I can tell) are related to this bug, so the breakage is not caused by this PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

eddyb · 2019-07-11T15:56:35Z

@strangelittlemonkey "nightly" is misleading - what's important here is "unstable", which is allowed for any dependency of std/rustc.

Even if users of libc from crates.io might not use the unstable feature on DragonFlyBSD, to build any release of Rust, including stable/beta, std needs errno from libc, for which it can enable some "unstable" flag to provide errno.

gnzlbg · 2019-07-11T16:24:20Z

@strangelittlemonkey or in other words, when the compiler is compiling itself, which is something that has to be done to build a compiler for DragonflyBSD, it can use unstable features that are generally only available on nightly.

strangelittlemonkey · 2019-07-11T18:41:58Z

I’m just saying requiring nightly to build it at all would make DragonFly a moot point, but allowing extra features if building with nightly would be different and fine, but still dead code if it applied to DragonFly specifically.

…

On Jul 11, 2019, at 9:24 AM, gnzlbg ***@***.***> wrote: @strangelittlemonkey or in other words, when the compiler is compiling itself, which is something that has to be done to build a compiler for DragonflyBSD, it can use unstable features that are generally only available on nightly. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

eddyb · 2019-07-12T08:19:05Z

I’m just saying requiring nightly to build it at all would make DragonFly a moot point, but allowing extra features if building with nightly would be different and fine

It's the latter, we're talking about an opt-in way to enable errno on DragonFly that requires unstable. This is either for nightly users or building Rust itself.

Like I said before, that opt-in would be enabled by std, which depends on this crate for its FFI to implement std::io, std::fs and other such functionality.

So when you build any future release of Rust (including stable ones) for DragonFlyBSD, the errno added here would be used, even if a use of that build of Rust wouldn't have that errno available.

So, yes, it's dead code for normal users of this crate, but would still be needed and used for the standard library (which does its OS FFI through this very same crate).

josephlr · 2019-07-12T18:34:58Z

@gnzlbg rust-lang/rust#62562 has been fixed, so the CI is passing again

@strangelittlemonkey Nightly build issues aside, the code added in this PR is required to build libstd for DragonFlyBSD (not necessarily building on DragonFlyBSD). Will this code work for DragonFlyBSD? and if not, what should we do instead?

I updated the docs to point out why we need unstable at all

gnzlbg · 2019-07-13T15:30:50Z

Just to triple check, is there any crate that wants to use this from crates.io ?

If not, then we don't need the unstable feature, and can just feature gate this using the "dep-of-std" feature.

josephlr · 2019-07-13T17:22:49Z

Just to triple check, is there any crate that wants to use this from crates.io ?

Initially the thought was to also use this for getrandom, but due to rust-lang/cargo#4866 we can't reasonably turn on the unstable feature for only one target, so I don't know of any crates that currently want it.

If not, then we don't need the unstable feature, and can just feature gate this using the "dep-of-std" feature.

Sounds good, I'll make the change unless anyone else wants to use this.

gnzlbg · 2019-07-13T17:55:12Z

@josephlr you can chime in in the thread_local tracking issue in rust-lang/rust, and mention that getrandom wants to use it. I don't know if anybody has needed to use extern thread-locals before, but that's something required for being able to FFI with C..

BurntSushi · 2019-07-13T21:31:42Z

Just to triple check, is there any crate that wants to use this from crates.io ?

Yes. walkdir will eventually want this. I'm currently using the same hack as the errno-dragonfly crate in my branch: https://github.com/BurntSushi/walkdir/blob/ag/sys/src/os/unix/errno.rs (Which I'll have to continue using if errno on DragonFly is only available with nightly Rust.)

josephlr · 2019-07-13T21:45:06Z

Just to triple check, is there any crate that wants to use this from crates.io ?

Yes. walkdir will eventually want this. I'm currently using the same hack as the errno-dragonfly crate in my branch: https://github.com/BurntSushi/walkdir/blob/ag/sys/src/os/unix/errno.rs (Which I'll have to continue using if errno on DragonFly is only available with nightly Rust.)

OK, as walkdir and getrandom can’t use this until it’s on stable, we don’t need the unstable feature. I’ll mention this on the main Rust tracking issue.

gnzlbg · 2019-07-14T07:05:32Z

Looking at the tracking issue, and the thread_local! discussions, it appears that 1) thread_local! solves the problem for most people, and 2) the main uncertainty is that #[thread_local] isn't portable.

Maybe we could reduce the scope of an initial version of #[thread_local] to extern static declarations. If the target does not support #[thread_local], well then the extern static declaration is incorrect since there cannot be a definition anywhere, and using it would already be UB. We could add on top of this a "best effort" compilation error, e.g., if the compiler knows that the target doesn't support it.

josephlr · 2019-07-15T05:34:10Z

@gnzlbg I removed the unstable feature, and commented on the appropriate thread_local tracking issue.

gnzlbg · 2019-07-15T07:55:24Z

Thanks!

@bors: r+

bors · 2019-07-15T07:55:25Z

📌 Commit df34d17 has been approved by gnzlbg

bors · 2019-07-15T07:55:33Z

⌛ Testing commit df34d17 with merge 47535e6...

Add __errno_location on Redox, DragonFlyBSD, Haiku Currently [`libstd`](https://github.com/rust-lang/rust/blob/909f5a049415a815b23502a5498de9a99bbf276b/src/libstd/sys/unix/os.rs#L29-L49) and the [`getrandom` crate](https://github.com/rust-random/getrandom/pull/54/files#diff-027a56068e2bf3d31dc4273ee6e07034R12) have to use external declarations in order to implement `errno_location` across all UNIX platforms. This change adds bindings for the remaining UNIX platforms, allowing everyone to just use normal `libc` bindings. This also removes the need for a seperate [`errno-dragonfly`](https://crates.io/crates/errno-dragonfly) crate. Links to the relevant header files: - Redox: [`relibc`'s `bits/errno.h`](https://github.com/redox-os/relibc/blob/master/include/bits/errno.h) uses `__errno_location` - Haiku: [`posix/errno.h`](https://github.com/luciang/haiku/blob/master/headers/posix/errno.h) uses `_errnop` - DragonFlyBSD: [`sys/errno.h`](https://github.com/DragonFlyBSD/DragonFlyBSD/blob/master/sys/sys/errno.h) uses `__error` just like FreeBSD (which makes sense) - Note that the `__error` function is inlined. I don't think this causes a problem.

bors · 2019-07-15T08:49:50Z

☀️ Test successful - checks-cirrus-freebsd-11, checks-cirrus-freebsd-12, checks-travis, status-appveyor
Approved by: gnzlbg
Pushing 47535e6 to master...

gnzlbg · 2019-07-15T08:54:16Z

libc 0.2.60 has been published with this - I'll update libc on rust-lang/rust

rust-highfive assigned gnzlbg Jul 9, 2019

jackpot51 approved these changes Jul 10, 2019

View reviewed changes

sikmir approved these changes Jul 10, 2019

View reviewed changes

Update documentation to point to Rust issue

1b4747e

Remove use of unstable feature

df34d17

josephlr mentioned this pull request Jul 15, 2019

Tracking issue for thread_local stabilization rust-lang/rust#29594

Open

6 tasks

bors merged commit df34d17 into rust-lang:master Jul 15, 2019

bors mentioned this pull request Jul 15, 2019

Increase Redox & Relibc support #1438

Merged

josephlr deleted the errno branch July 15, 2019 17:45

josephlr mentioned this pull request Jul 15, 2019

Improve Error handling rust-random/getrandom#54

Merged

This was referenced Jan 6, 2020

Compilation failure on DragonFlyBSD rust-random/getrandom#124

Closed

dragonfly: Don't try to read errno value rust-random/getrandom#129

Merged

josephlr mentioned this pull request Feb 25, 2020

__error() not provided for x86_64-unknown-dragonfly #1627

Closed

Add __errno_location on Redox, DragonFlyBSD, Haiku #1432

Add __errno_location on Redox, DragonFlyBSD, Haiku #1432

Conversation

josephlr commented Jul 9, 2019 • edited Loading

rust-highfive commented Jul 9, 2019

gnzlbg commented Jul 9, 2019

bors commented Jul 9, 2019

bors commented Jul 9, 2019

gnzlbg commented Jul 9, 2019

bors commented Jul 9, 2019

josephlr commented Jul 9, 2019

gnzlbg commented Jul 9, 2019

bors commented Jul 9, 2019

bors commented Jul 9, 2019

bors commented Jul 9, 2019

BurntSushi commented Jul 9, 2019

gnzlbg commented Jul 9, 2019 • edited Loading

gnzlbg commented Jul 9, 2019

josephlr commented Jul 9, 2019 • edited Loading

BurntSushi commented Jul 9, 2019

gnzlbg commented Jul 10, 2019

gnzlbg commented Jul 10, 2019

gnzlbg commented Jul 10, 2019 • edited Loading

eddyb commented Jul 10, 2019

gnzlbg commented Jul 10, 2019

gnzlbg commented Jul 10, 2019

eddyb commented Jul 10, 2019

gnzlbg commented Jul 10, 2019

jackpot51 left a comment

Choose a reason for hiding this comment

sikmir left a comment

Choose a reason for hiding this comment

josephlr commented Jul 10, 2019

josephlr commented Jul 11, 2019

strangelittlemonkey commented Jul 11, 2019 via email

eddyb commented Jul 11, 2019 • edited Loading

gnzlbg commented Jul 11, 2019

strangelittlemonkey commented Jul 11, 2019 via email

eddyb commented Jul 12, 2019 • edited Loading

josephlr commented Jul 12, 2019

gnzlbg commented Jul 13, 2019

josephlr commented Jul 13, 2019 • edited Loading

gnzlbg commented Jul 13, 2019

BurntSushi commented Jul 13, 2019 • edited Loading

josephlr commented Jul 13, 2019

gnzlbg commented Jul 14, 2019

josephlr commented Jul 15, 2019

gnzlbg commented Jul 15, 2019

bors commented Jul 15, 2019

bors commented Jul 15, 2019

bors commented Jul 15, 2019

gnzlbg commented Jul 15, 2019

josephlr commented Jul 9, 2019 •

edited

Loading

gnzlbg commented Jul 9, 2019 •

edited

Loading

josephlr commented Jul 9, 2019 •

edited

Loading

gnzlbg commented Jul 10, 2019 •

edited

Loading

eddyb commented Jul 11, 2019 •

edited

Loading

eddyb commented Jul 12, 2019 •

edited

Loading

josephlr commented Jul 13, 2019 •

edited

Loading

BurntSushi commented Jul 13, 2019 •

edited

Loading