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

make abort intrinsic safe, and correct its documentation #72204

Merged
merged 1 commit into from
May 17, 2020
Merged

make abort intrinsic safe, and correct its documentation #72204

merged 1 commit into from
May 17, 2020

Conversation

RalfJung
Copy link
Member

Turns out std::process::abort is not the same as the intrinsic, the comment was just wrong. Quoting from the unix implementation:

// On Unix-like platforms, libc::abort will unregister signal handlers
// including the SIGABRT handler, preventing the abort from being blocked, and
// fclose streams, with the side effect of flushing them so libc buffered
// output will be printed.  Additionally the shell will generally print a more
// understandable error message like "Abort trap" rather than "Illegal
// instruction" that intrinsics::abort would cause, as intrinsics::abort is
// implemented as an illegal instruction.

@rust-highfive
Copy link
Collaborator

r? @shepmaster

(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 May 14, 2020
@Mark-Simulacrum
Copy link
Member

This looks correct to me. Since it's just changing an unstable intrinsic I'm going to go ahead and @bors r+

@bors
Copy link
Contributor

bors commented May 14, 2020

📌 Commit 43b479b14afa54f11a48a3752176f21f60604a59 has been approved by Mark-Simulacrum

@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 May 14, 2020
@RalfJung
Copy link
Member Author

./x.py check was not enough build-testing, dang.
@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 14, 2020
@rust-highfive

This comment has been minimized.

Sign in to view
@RalfJung
Copy link
Member Author

Oh that's a weird build failure:

error[E0308]: intrinsic has wrong type
   --> src/libcore/intrinsics.rs:923:5
    |
923 |     pub fn abort() -> !;
    |     ^^^^^^^^^^^^^^^^^^^^ expected normal fn, found unsafe fn
    |
    = note: expected fn pointer `extern "rust-intrinsic" fn() -> _`
               found fn pointer `unsafe extern "rust-intrinsic" fn() -> _`

@oli-obk any idea in which other place I need to mark the intrinsic as safe?

@Mark-Simulacrum
Copy link
Member

rust/src/librustc_typeck/check/intrinsic.rs

Lines 133 to 134 in 750db09

} else if &name[..] == "abort" || &name[..] == "unreachable" {
(0, Vec::new(), tcx.types.never, hir::Unsafety::Unsafe)

@RalfJung
Copy link
Member Author

Interesting, that seems redundant...

@Mark-Simulacrum
Copy link
Member

It seems to be yeah, but perhaps not entirely -- I'm not sure.

@RalfJung
Copy link
Member Author

RalfJung commented May 14, 2020 •
edited
Loading

sigh tidy (sometimes) doesn't like attributes with comments in the same line... :(

@rust-highfive

This comment has been minimized.

Sign in to view
@RalfJung
Copy link
Member Author

r? @Mark-Simulacrum
test suite seems to be passing now, please have another look.

@Mark-Simulacrum
Copy link
Member

r=me with commits squashed

@RalfJung
Copy link
Member Author

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented May 15, 2020

📌 Commit 4839a1c2e801794a945a5bf3cc9a8874ea9519e0 has been approved by Mark-Simulacrum

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 15, 2020
oli-obk
oli-obk reviewed May 15, 2020
View reviewed changes
src/librustc_typeck/check/intrinsic.rs
} else if &name[..] == "abort" || &name[..] == "unreachable" {
} else if &name[..] == "abort" {
(0, Vec::new(), tcx.types.never, hir::Unsafety::Normal)
} else if &name[..] == "unreachable" {
(0, Vec::new(), tcx.types.never, hir::Unsafety::Unsafe)
} else {
let unsafety = intrinsic_operation_unsafety(&name[..]);
Copy link
Contributor

Choose a reason for hiding this comment

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

yea, we should just pull this function call out of the if and remove the manual computation of it in for the above cases.

@bors
Copy link
Contributor

bors commented May 17, 2020

⌛ Testing commit 4839a1c2e801794a945a5bf3cc9a8874ea9519e0 with merge a4455c9a8e09b1ebdb7da88a6a1cb885dcb46c46...

@bors
Copy link
Contributor

bors commented May 17, 2020

💔 Test failed - checks-azure

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 17, 2020
@RalfJung
Copy link
Member Author

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented May 17, 2020

📌 Commit 09d30a883e308396d2ad8d8eb018a16a6679c018 has been approved by Mark-Simulacrum

@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 May 17, 2020
@RalfJung RalfJung mentioned this pull request May 17, 2020
Merged
@RalfJung
Copy link
Member Author

Fixed formatting.
@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented May 17, 2020

📌 Commit 5980d97 has been approved by Mark-Simulacrum

@bors
Copy link
Contributor

bors commented May 17, 2020

⌛ Testing commit 5980d97 with merge 34cce58...

@bors
Copy link
Contributor

bors commented May 17, 2020

☀️ Test successful - checks-azure
Approved by: Mark-Simulacrum
Pushing 34cce58 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 17, 2020
@bors bors merged commit 34cce58 into rust-lang:master May 17, 2020
@RalfJung RalfJung deleted the abort branch May 17, 2020 17:34
RalfJung added a commit to RalfJung/rust that referenced this pull request May 17, 2020
@RalfJung
Rollup merge of rust-lang#72289 - RalfJung:abort_internal, r=Mark-Sim…
0a17be9 
…ulacrum

abort_internal is safe

`sys::abort_internal` is stably exposed as a safe function. Forward that assumption "inwards" to the `sys` module by making the function itself safe, too.

This corresponds to what rust-lang#72204 did for the intrinsic. We should probably wait until that lands because some of the intrinsic calls in this PR might then need adjustments.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 18, 2020
@bors
Auto merge of rust-lang#72289 - RalfJung:abort_internal, r=Mark-Simul…
9e2a6a2 
…acrum

abort_internal is safe

`sys::abort_internal` is stably exposed as a safe function. Forward that assumption "inwards" to the `sys` module by making the function itself safe, too.

This corresponds to what rust-lang#72204 did for the intrinsic. We should probably wait until that lands because some of the intrinsic calls in this PR might then need adjustments.
@ZackPierce ZackPierce mentioned this pull request Jun 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@RalfJung @rust-highfive @Mark-Simulacrum @bors @oli-obk @shepmaster