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

Introduce sys_common::rt::rtprintpanic! to replace sys_common::util functionality #84697

Merged
merged 5 commits into from
May 20, 2021
Merged

Introduce sys_common::rt::rtprintpanic! to replace sys_common::util functionality #84697

merged 5 commits into from
May 20, 2021

Conversation

CDirkx
Copy link
Contributor

@CDirkx CDirkx commented Apr 29, 2021 •
edited
Loading

This PR introduces a new macro rtprintpanic!, similar to sys_common::util::dumb_print and uses that macro to replace all sys_common::util functionality.

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(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 Apr 29, 2021
CDirkx
CDirkx commented Apr 29, 2021
View reviewed changes
library/std/src/sys_common/rt.rs Outdated
@@ -38,8 +39,21 @@ pub fn cleanup() {
});
}

macro_rules! rterr {
Copy link
Contributor Author

@CDirkx CDirkx Apr 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The name could be something different, rtwrite, rtprint, rtoutput etc.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe something with print in the name would be better. err might suggest it returns from the function or panics or aborts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rtprintpanic!? I think I'll also add a comment about that this writes to the designated "panic output", which is just stderr on most platforms.

CDirkx
CDirkx commented Apr 29, 2021
View reviewed changes
library/std/src/panicking.rs Outdated
@@ -571,7 +571,7 @@ fn rust_panic_with_hook(
// process real quickly as we don't want to try calling it again as it'll
// probably just panic again.
if panics > 2 {
util::dumb_print(format_args!("thread panicked while processing panic. aborting.\n"));
rterr!("thread panicked while processing panic. aborting.\n");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a particular reason why here in panicking intrinsics::abort is used instead of sys::abort_internal? Otherwise this could also be written with rtabort!.

Copy link
Member

Choose a reason for hiding this comment

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

sys::abort_internal calls libc::abort() (at least on unix). intrinsics::abort does not rely on libc or the platform, but simply results in executing an invalid instruction. I don't know if there's a reason to avoid libc::abort() here. It'd require some digging through the git history to see if this was changed on purpose, or added before the alternative existed, or something like that. :(

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'll look into it

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 looked through the history, these intrinsics::abort were indeed added before sys::abort_internal existed. As far as I can tell these could be replaced, along with a safety comment added to abort_internal that it's implementation is not allowed to panic (else panicking is stuck in a loop). However there are more occurrences of intrinsics::abort in std that in my opinion could also be replaced, so I think I'm leaving it like this for now and will address those together in a future PR.

@CDirkx CDirkx changed the title Introduce sys_common::rt::rterr to replace sys_common::util functionality Introduce sys_common::rt::rterr! to replace sys_common::util functionality Apr 29, 2021
@m-ou-se m-ou-se added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 5, 2021
@CDirkx
Copy link
Contributor Author

CDirkx commented May 12, 2021

Ready for another look, only changed the name from rterr to rtprintpanic.

@CDirkx CDirkx changed the title Introduce sys_common::rt::rterr! to replace sys_common::util functionality Introduce sys_common::rt::rtprintpanic! to replace sys_common::util functionality May 14, 2021
@bors
Copy link
Contributor

bors commented May 16, 2021

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

@ijackson
Copy link
Contributor

I have some Opinions about intrinsics::abort vs abort_internal. I have a branch which I am going to turn in an MR shortly, now #81858 has landed. That has some information about the properties of abort_internal. Should be later today.

@ijackson
Copy link
Contributor

See #85377.

@m-ou-se
Copy link
Member

m-ou-se commented May 19, 2021

@CDirkx Thanks again! This looks good to me. Can you rebase it to include the new abort message in panicking.rs as well?

@m-ou-se m-ou-se 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-review Status: Awaiting review from the assignee but also interested parties. labels May 19, 2021
@CDirkx
Copy link
Contributor Author

CDirkx commented May 19, 2021

Rebased 👍🏻

@m-ou-se
Copy link
Member

m-ou-se commented May 19, 2021

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented May 19, 2021

📌 Commit 4ff5ab5 has been approved by m-ou-se

@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 19, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 19, 2021
@Dylan-DPC
Rollup merge of rust-lang#84697 - CDirkx:util, r=m-ou-se
ec9b360 
Introduce `sys_common::rt::rtprintpanic!` to replace `sys_common::util` functionality

This PR introduces a new macro `rtprintpanic!`, similar to `sys_common::util::dumb_print` and uses that macro to replace all `sys_common::util` functionality.
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request May 19, 2021
Closed
@bors
Copy link
Contributor

bors commented May 20, 2021

⌛ Testing commit 4ff5ab5 with merge 5ab0f37...

@bors
Copy link
Contributor

bors commented May 20, 2021

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 20, 2021
@bors bors merged commit 5ab0f37 into rust-lang:master May 20, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 20, 2021
This was referenced May 20, 2021
Merged
Merged
@CDirkx CDirkx deleted the util branch May 20, 2021 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m-ou-se m-ou-se m-ou-se left review comments

Assignees

@m-ou-se m-ou-se

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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.54.0
Development

Successfully merging this pull request may close these issues.
6 participants
@CDirkx @rust-highfive @bors @ijackson @m-ou-se @rustbot