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

ucontext usage update on macos 64 bits. #2817

Merged
merged 1 commit into from Jun 7, 2022

Conversation

devnexen
Copy link
Contributor

@devnexen devnexen commented Jun 4, 2022

closes #2812

@rust-highfive
Copy link

r? @Amanieu

(rust-highfive has picked a reviewer for you, use r? to override)

@devnexen devnexen force-pushed the macos_arm64_context_upd branch 2 times, most recently from ed28db1 to 15a88f3 Compare June 4, 2022 20:29
@@ -109,6 +109,9 @@ extern "C" {
path2: *const ::c_char,
options: ::c_uint,
) -> ::c_int;

#[allow(deprecated)]
Copy link
Member

Choose a reason for hiding this comment

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

Why is this attribute needed?

Comment on lines 327 to 328
// deprecated api
"getcontext" => true,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if adding a deprecated API is a good idea. If there are no good alternatives, users should report it upstream, adding it here just delays the problem.

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 hesitated to be honest.

Copy link

Choose a reason for hiding this comment

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

There isn't any good alternative afaik, and I'm not sure if Apple cares about low-level APIs enough to provide one

Copy link
Member

Choose a reason for hiding this comment

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

My concern is that deprecated items are likely to be removed, i.e. they would introduce a breaking change. Such a code should belong to userland IMHO.

Copy link
Member

Choose a reason for hiding this comment

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

@devnexen By the way, we don't have to ignore it on the test anymore, right?

@JohnTitor
Copy link
Member

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Jun 7, 2022

📌 Commit 2d11246 has been approved by JohnTitor

bors added a commit that referenced this pull request Jun 7, 2022
ucontext usage update on macos 64 bits.

closes #2812
@bors
Copy link
Contributor

bors commented Jun 7, 2022

⌛ Testing commit 2d11246 with merge 3e92470...

@bors
Copy link
Contributor

bors commented Jun 7, 2022

💔 Test failed - checks-actions

@JohnTitor
Copy link
Member

crates.io failure @bors retry

@bors
Copy link
Contributor

bors commented Jun 7, 2022

⌛ Testing commit 2d11246 with merge e534fd8...

@bors
Copy link
Contributor

bors commented Jun 7, 2022

☀️ Test successful - checks-actions, checks-cirrus-freebsd-12, checks-cirrus-freebsd-13
Approved by: JohnTitor
Pushing e534fd8 to master...

@bors bors merged commit e534fd8 into rust-lang:master Jun 7, 2022
bors pushed a commit that referenced this pull request Aug 15, 2023
This commit effectively reverts #2817. Currently `ucontext_t` has both
the wrong size and the wrong alignment for aarch64-apple-darwin which
causes problems for users referencing the structure [1]. The issue
linked from #2817 claimed that it fixed #2812 but that's still an issue
where FFI warnings are still emitted for usage of `ucontext_t` due to
its transitive usage of `u128`. I'm not sure how to fix #2812 myself,
but given that #2817 doesn't appear to solve its original intent and
additionally the size/align are currently wrong this commit reverts in
the meantime.

[1]: bytecodealliance/wasmtime#6785 (comment)
bors added a commit that referenced this pull request Aug 15, 2023
…ohnTitor

Fix size/align of `ucontext_t` on aarch64-apple-darwin

This commit effectively reverts #2817. Currently `ucontext_t` has both the wrong size and the wrong alignment for aarch64-apple-darwin which causes problems for users referencing the structure [1]. The issue linked from #2817 claimed that it fixed #2812 but that's still an issue where FFI warnings are still emitted for usage of `ucontext_t` due to its transitive usage of `u128`. I'm not sure how to fix #2812 myself, but given that #2817 doesn't appear to solve its original intent and additionally the size/align are currently wrong this commit reverts in the meantime.

[1]: bytecodealliance/wasmtime#6785 (comment)
bors added a commit that referenced this pull request Aug 17, 2023
…ohnTitor

Fix size/align of `ucontext_t` on aarch64-apple-darwin

This commit effectively reverts #2817. Currently `ucontext_t` has both the wrong size and the wrong alignment for aarch64-apple-darwin which causes problems for users referencing the structure [1]. The issue linked from #2817 claimed that it fixed #2812 but that's still an issue where FFI warnings are still emitted for usage of `ucontext_t` due to its transitive usage of `u128`. I'm not sure how to fix #2812 myself, but given that #2817 doesn't appear to solve its original intent and additionally the size/align are currently wrong this commit reverts in the meantime.

[1]: bytecodealliance/wasmtime#6785 (comment)
xevor11 pushed a commit to xevor11/libc that referenced this pull request Aug 25, 2023
This commit effectively reverts rust-lang#2817. Currently `ucontext_t` has both
the wrong size and the wrong alignment for aarch64-apple-darwin which
causes problems for users referencing the structure [1]. The issue
linked from rust-lang#2817 claimed that it fixed rust-lang#2812 but that's still an issue
where FFI warnings are still emitted for usage of `ucontext_t` due to
its transitive usage of `u128`. I'm not sure how to fix rust-lang#2812 myself,
but given that rust-lang#2817 doesn't appear to solve its original intent and
additionally the size/align are currently wrong this commit reverts in
the meantime.

[1]: bytecodealliance/wasmtime#6785 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Aarch64 macOS: ucontext_t is not FFI-Safe
6 participants