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.

Sign up for GitHub

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

Jump to bottom

Adding ignore fuchsia tests for signal interpretation cases #102032

Merged
merged 1 commit into from
Sep 22, 2022
Merged

Adding ignore fuchsia tests for signal interpretation cases #102032

merged 1 commit into from
Sep 22, 2022

Conversation

andrewpollack
Copy link
Member

Tests where Signal interpreting is required. Since Fuchsia currently does not return signals of type libc::SIGSEGV etc., instead, use generalized !status.success() case.

cc. @djkoloski

r? @tmandry

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 19, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 19, 2022
@andrewpollack andrewpollack force-pushed the bad-signal-compiler-tests branch 2 times, most recently from 6a8bdb5 to 45e258e Compare September 20, 2022 19:20
djkoloski
djkoloski reviewed Sep 20, 2022
View reviewed changes
src/test/ui/runtime/out-of-stack.rs Outdated Show resolved Hide resolved
src/test/ui/abi/segfault-no-out-of-stack.rs Outdated Show resolved Hide resolved
@andrewpollack andrewpollack force-pushed the bad-signal-compiler-tests branch 2 times, most recently from e6bb6d3 to 812db90 Compare September 20, 2022 21:04
djkoloski
djkoloski reviewed Sep 20, 2022
View reviewed changes
src/test/ui/abi/stack-probes-lto.rs Outdated Show resolved Hide resolved
src/test/ui/process/signal-exit-status.rs Outdated Show resolved Hide resolved
@andrewpollack andrewpollack force-pushed the bad-signal-compiler-tests branch 3 times, most recently from 1fbad5d to b4383f5 Compare September 20, 2022 22:40
tmandry
tmandry reviewed Sep 20, 2022
View reviewed changes
src/test/ui/process/signal-exit-status.rs Outdated
@@ -2,6 +2,7 @@
// ignore-emscripten no processes
// ignore-sgx no processes
// ignore-windows
// ignore-fuchsia code returned as ZX_TASK_RETCODE_EXCEPTION_KILL, FIXME (#109976)
Copy link
Member

Choose a reason for hiding this comment

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

There's no Fuchsia issue here; the issue is on the standard library exposing these concepts correctly, so it belongs on this Github repo. Probably you can point directly to #58590. (Short of that, which is a major change, it's possible we could map this std::os::unix::process::ExitStatusExt API partly onto Fuchsia contexts.)

Btw, to link to the issue you created you want http://fxbug.dev/109976. A number starting with # refers to an issue in this Github repo.

src/test/ui/runtime/out-of-stack.rs Outdated
@@ -5,6 +5,7 @@
// ignore-android: FIXME (#20004)
// ignore-emscripten no processes
// ignore-sgx no processes
// ignore-fuchsia must translate zircon signal to SIGABRT, FIXME (#109976)
Copy link
Member

Choose a reason for hiding this comment

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

same as above

src/test/ui/abi/segfault-no-out-of-stack.rs Outdated
@@ -3,6 +3,7 @@
#![allow(unused_imports)]
// ignore-emscripten can't run commands
// ignore-sgx no processes
// ignore-fuchsia must translate zircon signal to SIGSEGV/SIGBUS, FIXME (#109976)
Copy link
Member

Choose a reason for hiding this comment

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

This test has a handler for not(unix); we can use that one for Fuchsia without disabling the test entirely.

Copy link
Member Author

Choose a reason for hiding this comment

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

A previous version changed those lines into:
#[cfg(all(unix, not(target_os = "fuchsia")))]
#[cfg(any(not(unix), target_os = "fuchsia"))]
Is this how I should approach the change?

Copy link
Member

@tmandry tmandry Sep 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah I think that's right and you could also leave a FIXME with a link to the same issue you're using for the other tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that manually adding Fuchsia to the #[cfg(not(unix))] case may cause us to forget that this test does not actually pass for Fuchsia. We have to either support this functionality or remove Fuchsia from #[cfg(unix)], upon which this test will pass with no modifications.

Copy link
Member Author

Choose a reason for hiding this comment

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

To clarify, the test "passes" in the sense that it does not have a successful status !status.success(), but "fails" in the sense that the signal is not what is expected (SIGSEGV/SIGBUS). I agree that this comes down to whether this is expected behavior or not for Posix-lite

@andrewpollack andrewpollack force-pushed the bad-signal-compiler-tests branch from b4383f5 to 9a2b5ba Compare September 21, 2022 00:19
@andrewpollack andrewpollack force-pushed the bad-signal-compiler-tests branch from 9a2b5ba to 6c29716 Compare September 21, 2022 15:57
@tmandry
Copy link
Member

tmandry commented Sep 21, 2022

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 21, 2022

📌 Commit 6c29716 has been approved by tmandry

It is now in the queue for this repository.

@bors 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 Sep 21, 2022
@JohnTitor JohnTitor mentioned this pull request Sep 22, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 22, 2022
@bors
Auto merge of rust-lang#102121 - JohnTitor:rollup-3fb1wrt, r=JohnTitor
626b02a 
Rollup of 12 pull requests

Successful merges:

 - rust-lang#101952 (Avoid panicking on missing fallback)
 - rust-lang#102030 (Don't crate-locally reexport walk functions in tidy)
 - rust-lang#102032 (Adding ignore fuchsia tests for signal interpretation cases)
 - rust-lang#102033 (Adding needs-unwind to nicer-assert-messages compiler ui tests)
 - rust-lang#102054 (Unify "all items" page's sidebar with other pages)
 - rust-lang#102071 (Adding needs-unwind for tests testing memory size of Futures/Closures)
 - rust-lang#102073 (Adding ignore fuchsia tests for execvp)
 - rust-lang#102075 (rustdoc: remove no-op CSS `.content > .methods > .method`)
 - rust-lang#102079 (Update books)
 - rust-lang#102084 (Adding needs-unwind for test using panic::catch_unwind)
 - rust-lang#102100 (Prevent usage of .stab elements to create scrollable areas in doc blocks)
 - rust-lang#102102 (Add doc aliases on Sized trait)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5542e50 into rust-lang:master Sep 22, 2022
@rustbot rustbot added this to the 1.66.0 milestone Sep 22, 2022
@tmandry tmandry added the O-fuchsia Operating system: Fuchsia label May 24, 2023
@c6c7 c6c7 mentioned this pull request Jul 7, 2024
Merged
jhpratt added a commit to jhpratt/rust that referenced this pull request Jul 10, 2024
@jhpratt
Rollup merge of rust-lang#127461 - c6c7:fixup-failing-fuchsia-tests, …
7490729 
…r=tmandry

Fixup failing fuchsia tests

The Fuchsia platform passes all tests with these changes. Two tests are ignored because they rely on Fuchsia not returning a status code upon a process aborting. See rust-lang#102032 and rust-lang#58590 for more details on that topic.

Many formatting changes are also included in this PR.

r? tmandry
r? erickt
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 10, 2024
@rust-timer
Unrolled build for rust-lang#127461
d682c84 
Rollup merge of rust-lang#127461 - c6c7:fixup-failing-fuchsia-tests, r=tmandry

Fixup failing fuchsia tests

The Fuchsia platform passes all tests with these changes. Two tests are ignored because they rely on Fuchsia not returning a status code upon a process aborting. See rust-lang#102032 and rust-lang#58590 for more details on that topic.

Many formatting changes are also included in this PR.

r? tmandry
r? erickt
@c6c7 c6c7 mentioned this pull request Jul 11, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djkoloski djkoloski djkoloski approved these changes

@tmandry tmandry Awaiting requested review from tmandry

Assignees

@tmandry tmandry

Labels
O-fuchsia Operating system: Fuchsia S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.66.0
Development

Successfully merging this pull request may close these issues.
6 participants
@andrewpollack @tmandry @bors @djkoloski @rust-highfive @rustbot