Fix watchOS and visionOS for pread64 and pwrite64 calls #124089

simlay · 2024-04-17T18:41:54Z

In #122880, links to preadv64 and pwritev64 were added for watchOS however the underlying weak! macro did not include target_os = "watchos".

This resulted in an xcodebuild error when targeting watchOS:

Undefined symbols for architecture arm64:
  "_preadv64", referenced from:
      __rust_extern_with_linkage_preadv64 in libliveview_native_core.a[274](std-324fdd8d31e8eaa2.std.e18cf7e8d0336778-cgu.08.rcgu.o)
  "_pwritev64", referenced from:
      __rust_extern_with_linkage_pwritev64 in libliveview_native_core.a[274](std-324fdd8d31e8eaa2.std.e18cf7e8d0336778-cgu.08.rcgu.o)
ld: symbol(s) not found for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

So I added them. I also went ahead and added the same for visionOS because it's bound to create the same issue.

rustbot · 2024-04-17T18:42:01Z

r? @workingjubilee

rustbot has assigned @workingjubilee.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

madsmtm · 2024-04-17T19:17:34Z

Hmm, I don't think pread64/preadv64/pwrite64/pwritev64 exists anywhere on macOS/iOS/tvOS/watchOS/visionOS?

E.g. the following fails even on the newest version of macOS:

extern "C" {
    fn preadv64(w: libc::c_int, z: *const libc::iovec, y: libc::c_int, x: libc::off_t) -> isize;
}

fn main() {
    unsafe { preadv64(0, std::ptr::null(), 0, 0) };
}

Only the pread/preadv/pwrite/pwritev versions exist, so the code should probably rather be updated to use that?

library/std/src/sys/pal/unix/weak.rs

library/std/src/sys/pal/unix/fd.rs

simlay · 2024-04-18T18:54:59Z

Hmm, I don't think pread64/preadv64/pwrite64/pwritev64 exists anywhere on macOS/iOS/tvOS/watchOS/visionOS?

E.g. the following fails even on the newest version of macOS:
extern "C" {
    fn preadv64(w: libc::c_int, z: *const libc::iovec, y: libc::c_int, x: libc::off_t) -> isize;
}

fn main() {
    unsafe { preadv64(0, std::ptr::null(), 0, 0) };
}
Only the pread/preadv/pwrite/pwritev versions exist, so the code should probably rather be updated to use that?

Yeah, I'm confused how it's not broken for other apple targets in it's current setup?

More so, it looks like the preadv64 and other such calls are used for android targets with 32 bit pointers and I don't know android well enough to know (or how to look this up) if that should also change to preadv/pwritev/etc.

madsmtm · 2024-04-18T19:49:07Z

I don't know android well enough to know (or how to look this up) if that should also change to preadv/pwritev/etc.

Me neither, tagging original implementor @a1phyr

simlay · 2024-04-19T20:30:11Z

library/std/src/sys/pal/unix/fd.rs

+    // We support old MacOS, iOS, watchOS, tvOS and visionOS versions that do not have `preadv`.
+    // There is no `syscall` possible in these platform for following apple versions:


I'm not certain this comment (or the one one about pwritev) is accurate anymore. Went about looking into the apple SDKs and they all have:

ssize_t preadv(int, const struct iovec *, int, off_t) __DARWIN_NOCANCEL(preadv) __API_AVAILABLE(macos(11.0), ios(14.0), watchos(7.0), tvos(14.0)); ssize_t pwritev(int, const struct iovec *, int, off_t) __DARWIN_NOCANCEL(pwritev) __API_AVAILABLE(macos(11.0), ios(14.0), watchos(7.0), tvos(14.0));

Here's the iOS SDK header but they all look the same on my local SDK's. I found preadv/pwritev in the VisionOS tbd ("text-based stub" libraries)

Agree that it's the same in the headers for each SDK, can be checked using:

for sdk in macosx appletvos iphoneos watchos xros do cat $(xcrun --show-sdk-path --sdk $sdk)/usr/include/sys/uio.h | grep API_AVAILABLE done

That said, the comment is indeed correct that we still support e.g. macOS 10.12, while the API has only added in macOS 11. So we still need to do weak linking here.

My request was addressed!

workingjubilee

It's so much easier to think clearly about this now, thank you so much!

I notice we've had to re-answer the question of "what appleOS versions do we actually support" a few times here. Could we write down the answers, so that we can explicitly encode the version we're assuming we support, and thus that requires this weak linking? I can't find them actually documented (placed in an easily-findable location and an explicit note that this is the intentional version we support, like the platform support docs) for anything except macOS. I think it's reasonable to not want to sift through CI files, somewhat randomly located *_DEPLOYMENT_TARGET variables, or other random PRs for an answer.

workingjubilee · 2024-04-20T20:04:06Z

library/std/src/sys/pal/unix/fd.rs

+    // ios 14.0
+    // tvos 14.0
+    // macos 11.0
+    // watchos 7.0
+    // visionos 1.0


Thinking about this... do we support visionOS before 1.0? I would think not? We could strong-link on visionOS if we don't. Arguably it makes the implementation messier, I suppose.

I had to double check myself. Looks like visionOS 1.0 is the first version. In #121419, the default was set to 1.0. This should be something easier to check though.

After thinking about this more, I think that this comment is not to say "this is the minimum supported version", I think the original comment I paraphrased and amended meant "The following Operating system versions have this syscall: iOS 14.0, tvOS 14.0, macOS 11.0, watchOS 7.1".

I've updated code comments to reflect my understanding of things and better describe it.

workingjubilee · 2024-04-20T20:04:55Z

library/std/src/sys/pal/unix/fd.rs

+    #[cfg(target_vendor = "apple")]
+    pub fn read_vectored_at(&self, bufs: &mut [IoSliceMut<'_>], offset: u64) -> io::Result<usize> {
+        super::weak::weak!(fn preadv(libc::c_int, *const libc::iovec, libc::c_int, off64_t) -> isize);


super weak!

workingjubilee · 2024-04-20T20:34:25Z

I also am willing to accept this if you instead decide to open an issue to clarify the version situation here, fwiw! This is good enough for me.

madsmtm · 2024-04-20T22:53:42Z

I notice we've had to re-answer the question of "what appleOS versions do we actually support" a few times here. Could we write down the answers, so that we can explicitly encode the version we're assuming we support

I've been wanting to add that to the platform support docs too, will try to do so (in a way that doesn't conflict too much with #120745).

simlay · 2024-04-21T01:23:14Z

I also am willing to accept this if you instead decide to open an issue to clarify the version situation here, fwiw! This is good enough for me.

I authored #124215 to keep this PR small.

workingjubilee · 2024-04-21T03:22:35Z

Excellent!

r=me with the format commits squished!

@bors delegate+

bors · 2024-04-21T03:22:38Z

✌️ @simlay, you can now approve this pull request!

If @workingjubilee told you to "r=me" after making some further change, please make that change, then do @bors r=@workingjubilee

* Refactor apple OSs to use pwritev and preadv rather pwritev64 and preadv64 * Updated the comments for preadv and pwritev

simlay · 2024-04-21T04:45:35Z

@bors r=@workingjubilee

bors · 2024-04-21T04:45:37Z

📌 Commit fa53b9f has been approved by workingjubilee

It is now in the queue for this repository.

…llaumeGomez Rollup of 4 pull requests Successful merges: - rust-lang#124069 (enable clippy for bootstrap on CI PRs (in `mingw-check` image)) - rust-lang#124089 (Fix watchOS and visionOS for pread64 and pwrite64 calls) - rust-lang#124184 (Suggest using `unsigned_abs` in `abs` documentation) - rust-lang#124198 (Flip spans for precise capturing syntax not capturing a ty/const param, and for implicit captures of lifetime params) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#124089 - simlay:fix-preadv64-and-pwritev64-link-for-watchos-and-visionos, r=workingjubilee Fix watchOS and visionOS for pread64 and pwrite64 calls In rust-lang#122880, links to `preadv64` and `pwritev64` were added for `watchOS` however the underlying [`weak!` macro did not include `target_os = "watchos"`](https://github.com/rust-lang/rust/blob/c45dee5efd0c042e9d1e24559ebd0d6424d8aa70/library/std/src/sys/pal/unix/weak.rs#L30-L74). This resulted in an `xcodebuild` error when targeting `watchOS`: ``` Undefined symbols for architecture arm64: "_preadv64", referenced from: __rust_extern_with_linkage_preadv64 in libliveview_native_core.a[274](std-324fdd8d31e8eaa2.std.e18cf7e8d0336778-cgu.08.rcgu.o) "_pwritev64", referenced from: __rust_extern_with_linkage_pwritev64 in libliveview_native_core.a[274](std-324fdd8d31e8eaa2.std.e18cf7e8d0336778-cgu.08.rcgu.o) ld: symbol(s) not found for architecture arm64 clang: error: linker command failed with exit code 1 (use -v to see invocation) ``` So I added them. I also went ahead and added the same for visionOS because it's bound to create the same issue.

rustbot assigned workingjubilee Apr 17, 2024

rustbot added O-unix Operating system: Unix-like S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 17, 2024

This comment has been minimized.

Sign in to view

workingjubilee previously requested changes Apr 17, 2024

View reviewed changes

simlay force-pushed the fix-preadv64-and-pwritev64-link-for-watchos-and-visionos branch from 824d00c to 4477be0 Compare April 18, 2024 18:22

simlay marked this pull request as ready for review April 19, 2024 16:06

simlay commented Apr 19, 2024

View reviewed changes

simlay requested a review from workingjubilee April 19, 2024 20:48

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 19, 2024

workingjubilee reviewed Apr 20, 2024

View reviewed changes

simlay mentioned this pull request Apr 21, 2024

Document minimum supported versions for apple operating systems to platform documentation #124215

Closed

Fix watchOS and visionOS for pread64 and pwrite64 calls

fa53b9f

* Refactor apple OSs to use pwritev and preadv rather pwritev64 and preadv64 * Updated the comments for preadv and pwritev

simlay force-pushed the fix-preadv64-and-pwritev64-link-for-watchos-and-visionos branch from b411fb7 to fa53b9f Compare April 21, 2024 04:38

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 21, 2024

GuillaumeGomez mentioned this pull request Apr 21, 2024

Rollup of 4 pull requests #124222

Merged

madsmtm approved these changes Apr 21, 2024

View reviewed changes

bors merged commit 9efd147 into rust-lang:master Apr 21, 2024
12 checks passed

rustbot added this to the 1.79.0 milestone Apr 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix watchOS and visionOS for pread64 and pwrite64 calls #124089

Fix watchOS and visionOS for pread64 and pwrite64 calls #124089

simlay commented Apr 17, 2024

rustbot commented Apr 17, 2024

This comment has been minimized.

madsmtm commented Apr 17, 2024

simlay commented Apr 18, 2024

madsmtm commented Apr 18, 2024

simlay Apr 19, 2024

madsmtm Apr 19, 2024

workingjubilee left a comment

workingjubilee Apr 20, 2024

simlay Apr 20, 2024

simlay Apr 21, 2024

simlay Apr 21, 2024

workingjubilee Apr 20, 2024

workingjubilee commented Apr 20, 2024

madsmtm commented Apr 20, 2024

simlay commented Apr 21, 2024

workingjubilee commented Apr 21, 2024 •

edited

Loading

bors commented Apr 21, 2024

simlay commented Apr 21, 2024

bors commented Apr 21, 2024

		// We support old MacOS, iOS, watchOS, tvOS and visionOS versions that do not have `preadv`.
		// There is no `syscall` possible in these platform for following apple versions:

Fix watchOS and visionOS for pread64 and pwrite64 calls #124089

Fix watchOS and visionOS for pread64 and pwrite64 calls #124089

Conversation

simlay commented Apr 17, 2024

rustbot commented Apr 17, 2024

This comment has been minimized.

madsmtm commented Apr 17, 2024

simlay commented Apr 18, 2024

madsmtm commented Apr 18, 2024

simlay Apr 19, 2024

Choose a reason for hiding this comment

madsmtm Apr 19, 2024

Choose a reason for hiding this comment

workingjubilee left a comment

Choose a reason for hiding this comment

workingjubilee Apr 20, 2024

Choose a reason for hiding this comment

simlay Apr 20, 2024

Choose a reason for hiding this comment

simlay Apr 21, 2024

Choose a reason for hiding this comment

simlay Apr 21, 2024

Choose a reason for hiding this comment

workingjubilee Apr 20, 2024

Choose a reason for hiding this comment

workingjubilee commented Apr 20, 2024

madsmtm commented Apr 20, 2024

simlay commented Apr 21, 2024

workingjubilee commented Apr 21, 2024 • edited Loading

bors commented Apr 21, 2024

simlay commented Apr 21, 2024

bors commented Apr 21, 2024

workingjubilee commented Apr 21, 2024 •

edited

Loading