Skip to content
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

Mark -1 as an available niche for file descriptors #74699

Merged
merged 4 commits into from
Dec 20, 2020

Conversation

notriddle
Copy link
Contributor

Based on discussion from https://internals.rust-lang.org/t/can-the-standard-library-shrink-option-file/12768, the file descriptor -1 is chosen based on the POSIX API designs that use it as a sentinel to report errors. A bigger niche could've been chosen, particularly on Linux, but would not necessarily be portable.

This PR also adds a test case to ensure that the -1 niche (which is kind of hacky and has no obvious test case) works correctly. It requires the "upper" bound, which is actually -1, to be expressed in two's complement.

@rust-highfive
Copy link
Collaborator

r? @kennytm

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 23, 2020
@notriddle notriddle force-pushed the fd-non-negative branch 2 times, most recently from 33e3333 to 7590613 Compare July 23, 2020 22:22
@joshtriplett
Copy link
Member

I posted a few comments; otherwise this looks good to me.

@joshtriplett joshtriplett added the relnotes Marks issues that should be documented in the release notes of the next release. label Jul 23, 2020
@joshtriplett
Copy link
Member

joshtriplett commented Jul 23, 2020

Draft release notes text:

On Unix platforms, the `std::fs::File` type now has a "niche" of `-1`, which cannot be a valid file descriptor.
This means `Option<File>` now takes up no more space than `File`.

@notriddle
Copy link
Contributor Author

This should probably get a crater run, too. The FromRawFd doc comment ought to ban invalid file descriptors, but ... you know, any externally visible behaviour might as well be part of the API contract.

@jstarks
Copy link

jstarks commented Jul 24, 2020

Is it worth doing this for Windows (but with 0/NULL) in the same change?

@retep998
Copy link
Member

Windows generally uses INVALID_HANDLE_VALUE to represent invalid handles, which should be fine for file handles. For other types of handles it sometimes has special meaning beyond just being an invalid handle. Like for process handles GetCurrentProcess returns a pseudo handle of the same value as INVALID_HANDLE_VALUE and functions which take a process handle will interpret that as the current process.

@jstarks
Copy link

jstarks commented Jul 24, 2020

The Win32 APIs conventionally return INVALID_HANDLE_VALUE (-1) for file handles and only file handles. This is just a convention, and as you point out there are some collisions with pseudo handles used for other purposes. This was done for compatibility with 16-bit Windows code.

Non-file APIs such as CreateEvent return NULL on failure. INVALID_HANDLE_VALUE is probably used in a few other esoteric Win32 APIs by people who didn't understand the history.

But I'd say let's keep it simple. NULL/0 is never a valid handle in any context, file or otherwise. It's the conventional "no handle" value in Windows kernel code. I think it's a better niche for this purpose than -1.

src/libstd/sys/unix/fd.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

Is it worth doing this for Windows (but with 0/NULL) in the same change?

It is an entirely separate discussion (requiring knowledge about an independent set of APIs) so I propose this should be done in a separate PR.

@the8472
Copy link
Member

the8472 commented Jul 24, 2020

Note that some APIs might use -1 as a "no file" placeholder. At least io_uring does. Of course this could be hanlded with a separate type, but then RawFd would not be all that raw anymore.

Edit: nvm, I mistook FileDesc for RawFd

@oli-obk
Copy link
Contributor

oli-obk commented Jul 25, 2020

For the crater, this would need a full test run crater, right? Not just a check, since successful compilation is not the issue here.

@programmerjake
Copy link
Member

It seems worthwhile to explicitly add a test or a comment in whatever pre-existing test that asserts c_int is i32, that way, if some platform has a different c_int they'd be less likely to miss changing the bounds for File.

@birkenfeld
Copy link
Contributor

Alternatively, make it possible in rustc to specify the bound as -2 which should keep the correct semantics even for a 64 bit c_int.

@bors
Copy link
Contributor

bors commented Jul 28, 2020

☔ The latest upstream changes (presumably #73265) made this pull request unmergeable. Please resolve the merge conflicts.

@notriddle
Copy link
Contributor Author

Manually rebased.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 28, 2020

@bors try

@bors
Copy link
Contributor

bors commented Jul 28, 2020

⌛ Trying commit e918808c2c27141008ab89875137727603cdc305 with merge 68da184b2b3962d69cccd2f8a627dd35a0c2a1e7...

@m-ou-se m-ou-se added the S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. label Dec 7, 2020
@Dylan-DPC-zz
Copy link

@Amanieu @KodrAus @withoutboats this is waiting on your votes/concerns

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Dec 9, 2020
@rfcbot
Copy link

rfcbot commented Dec 9, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Dec 9, 2020
@@ -28,7 +33,9 @@ const READ_LIMIT: usize = libc::ssize_t::MAX as usize;

impl FileDesc {
pub fn new(fd: c_int) -> FileDesc {
FileDesc { fd }
assert_ne!(fd, -1i32);
Copy link
Contributor

@vi vi Dec 10, 2020

Choose a reason for hiding this comment

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

Will there be unsafe fn new_unchecked, to avoid including panicking code in a program that otherwise avoid it?

If this is a private type, not directly available in public API, will panicking and string formatting code suddenly appear in places where it was absent before, breaking things like no-panic?

notriddle and others added 3 commits December 10, 2020 13:31
Based on discussion from https://internals.rust-lang.org/t/can-the-standard-library-shrink-option-file/12768,
the file descriptor -1 is chosen based on the POSIX API designs that use it as a sentinel to report errors.
A bigger niche could've been chosen, particularly on Linux, but would not necessarily be portable.

This PR also adds a test case to ensure that the -1 niche
(which is kind of hacky and has no obvious test case) works correctly.
It requires the "upper" bound, which is actually -1, to be expressed in two's complement.
Co-authored-by: Elichai Turkel <elichai.turkel@gmail.com>
@pickfire
Copy link
Contributor

File::from_raw_fd(-1) will panic but it is possible to have the compiler emit an error during compilation if the compiler knew it was -1? Like compile time out of bounds check that we have now.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Dec 19, 2020
@rfcbot
Copy link

rfcbot commented Dec 19, 2020

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@m-ou-se
Copy link
Member

m-ou-se commented Dec 20, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Dec 20, 2020

📌 Commit 094b1da has been approved by m-ou-se

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 20, 2020
@bors
Copy link
Contributor

bors commented Dec 20, 2020

⌛ Testing commit 094b1da with merge b0e5c7d...

@bors
Copy link
Contributor

bors commented Dec 20, 2020

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing b0e5c7d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 20, 2020
@bors bors merged commit b0e5c7d into rust-lang:master Dec 20, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 20, 2020
@notriddle notriddle deleted the fd-non-negative branch December 20, 2020 21:45
@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Dec 31, 2020
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Apr 19, 2021
Pkgsrc changes:
 * Adjust patches, re-compute line offsets, fix capitalization.
 * Remove i686/FreeBSD support, no longer provided upstream.
 * Bump bootstraps to 1.49.0.
 * Change USE_TOOLS from bsdtar to gtar.
 * Reduce diffs to pkgsrc-wip package patches.
 * Allow rust.BUILD_TARGET to override automatic choice of target.
 * Add an i586/NetBSD (pentium) bootstrap variant (needs testing),
   not yet added as bootstrap since 1.49 doesn't have that variant.

Upstream changes:

Version 1.50.0 (2021-02-11)
============================

Language
-----------------------
- [You can now use `const` values for `x` in `[x; N]` array
  expressions.][79270] This has been technically possible since
  1.38.0, as it was unintentionally stabilized.
- [Assignments to `ManuallyDrop<T>` union fields are now considered
  safe.][78068]

Compiler
-----------------------
- [Added tier 3\* support for the `armv5te-unknown-linux-uclibceabi`
  target.][78142]
- [Added tier 3 support for the `aarch64-apple-ios-macabi` target.][77484]
- [The `x86_64-unknown-freebsd` is now built with the full toolset.][79484]

\* Refer to Rust's [platform support page][forge-platform-support] for more
information on Rust's tiered platform support.

Libraries
-----------------------

- [`proc_macro::Punct` now implements `PartialEq<char>`.][78636]
- [`ops::{Index, IndexMut}` are now implemented for fixed sized
  arrays of any length.][74989]
- [On Unix platforms, the `std::fs::File` type now has a "niche"
  of `-1`.][74699] This value cannot be a valid file descriptor,
  and now means `Option<File>` takes up the same amount of space
  as `File`.

Stabilized APIs
---------------

- [`bool::then`]
- [`btree_map::Entry::or_insert_with_key`]
- [`f32::clamp`]
- [`f64::clamp`]
- [`hash_map::Entry::or_insert_with_key`]
- [`Ord::clamp`]
- [`RefCell::take`]
- [`slice::fill`]
- [`UnsafeCell::get_mut`]

The following previously stable methods are now `const`.

- [`IpAddr::is_ipv4`]
- [`IpAddr::is_ipv6`]
- [`Layout::size`]
- [`Layout::align`]
- [`Layout::from_size_align`]
- `pow` for all integer types.
- `checked_pow` for all integer types.
- `saturating_pow` for all integer types.
- `wrapping_pow` for all integer types.
- `next_power_of_two` for all unsigned integer types.
- `checked_power_of_two` for all unsigned integer types.

Cargo
-----------------------

- [Added the `[build.rustc-workspace-wrapper]` option.][cargo/8976]
  This option sets a wrapper to execute instead of `rustc`, for
  workspace members only.
- [`cargo:rerun-if-changed` will now, if provided a directory, scan the entire
  contents of that directory for changes.][cargo/8973]
- [Added the `--workspace` flag to the `cargo update` command.][cargo/8725]

Misc
----

- [The search results tab and the help button are focusable with
  keyboard in rustdoc.][79896]
- [Running tests will now print the total time taken to execute.][75752]

Compatibility Notes
-------------------

- [The `compare_and_swap` method on atomics has been deprecated.][79261]
  It's recommended to use the `compare_exchange` and
  `compare_exchange_weak` methods instead.
- [Changes in how `TokenStream`s are checked have fixed some cases
  where you could write unhygenic `macro_rules!` macros.][79472]
- [`#![test]` as an inner attribute is now considered unstable like
  other inner macro attributes, and reports an error by default
  through the `soft_unstable` lint.][79003]
- [Overriding a `forbid` lint at the same level that it was set is
  now a hard error.][78864]
- [Dropped support for all cloudabi targets.][78439]
- [You can no longer intercept `panic!` calls by supplying your
  own macro.][78343] It's recommended to use the `#[panic_handler]`
  attribute to provide your own implementation.
- [Semi-colons after item statements (e.g. `struct Foo {};`) now
  produce a warning.][78296]

[74989]: rust-lang/rust#74989
[79261]: rust-lang/rust#79261
[79896]: rust-lang/rust#79896
[79484]: rust-lang/rust#79484
[79472]: rust-lang/rust#79472
[79270]: rust-lang/rust#79270
[79003]: rust-lang/rust#79003
[78864]: rust-lang/rust#78864
[78636]: rust-lang/rust#78636
[78439]: rust-lang/rust#78439
[78343]: rust-lang/rust#78343
[78296]: rust-lang/rust#78296
[78068]: rust-lang/rust#78068
[75752]: rust-lang/rust#75752
[74699]: rust-lang/rust#74699
[78142]: rust-lang/rust#78142
[77484]: rust-lang/rust#77484
[cargo/8976]: rust-lang/cargo#8976
[cargo/8973]: rust-lang/cargo#8973
[cargo/8725]: rust-lang/cargo#8725
[`IpAddr::is_ipv4`]: https://doc.rust-lang.org/stable/std/net/enum.IpAddr.html#method.is_ipv4
[`IpAddr::is_ipv6`]: https://doc.rust-lang.org/stable/std/net/enum.IpAddr.html#method.is_ipv6
[`Layout::align`]: https://doc.rust-lang.org/stable/std/alloc/struct.Layout.html#method.align
[`Layout::from_size_align`]: https://doc.rust-lang.org/stable/std/alloc/struct.Layout.html#method.from_size_align
[`Layout::size`]: https://doc.rust-lang.org/stable/std/alloc/struct.Layout.html#method.size
[`Ord::clamp`]: https://doc.rust-lang.org/stable/std/cmp/trait.Ord.html#method.clamp
[`RefCell::take`]: https://doc.rust-lang.org/stable/std/cell/struct.RefCell.html#method.take
[`UnsafeCell::get_mut`]: https://doc.rust-lang.org/stable/std/cell/struct.UnsafeCell.html#method.get_mut
[`bool::then`]: https://doc.rust-lang.org/stable/std/primitive.bool.html#method.then
[`btree_map::Entry::or_insert_with_key`]: https://doc.rust-lang.org/stable/std/collections/btree_map/enum.Entry.html#method.or_insert_with_key
[`f32::clamp`]: https://doc.rust-lang.org/stable/std/primitive.f32.html#method.clamp
[`f64::clamp`]: https://doc.rust-lang.org/stable/std/primitive.f64.html#method.clamp
[`hash_map::Entry::or_insert_with_key`]: https://doc.rust-lang.org/stable/std/collections/hash_map/enum.Entry.html#method.or_insert_with_key
[`slice::fill`]: https://doc.rust-lang.org/stable/std/primitive.slice.html#method.fill
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.