Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Implement Rust 2021 panic #80851

Merged
merged 5 commits into from
Feb 1, 2021
Merged

Implement Rust 2021 panic #80851

merged 5 commits into from
Feb 1, 2021

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Jan 9, 2021

This implements the Rust 2021 versions of panic!(). See #80162 and rust-lang/rfcs#3007.

It does so by replacing {std, core}::panic!() by a bulitin macro that expands to either $crate::panic::panic_2015!(..) or $crate::panic::panic_2021!(..) depending on the edition of the caller.

This does not yet make std's panic an alias for core's panic on Rust 2021 as the RFC proposes. That will be a separate change: c5273bd That change is blocked on figuring out what to do with #80846 first.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 10, 2021
@rust-log-analyzer

This comment has been minimized.

@Aaron1011
Copy link
Member

I don't think this will require any Miri changes - all of the Miri panic logic/hooks occur much deeper into the panic code.

@rust-log-analyzer

This comment has been minimized.

@m-ou-se
Copy link
Member Author

m-ou-se commented Jan 10, 2021

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@bors

This comment has been minimized.

@bors

This comment has been minimized.

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 10, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (18bbe6416d2f0b96850a82cf214a118ca792bc4c): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jan 11, 2021
@m-ou-se m-ou-se marked this pull request as ready for review January 12, 2021 17:24
@m-ou-se m-ou-se removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 12, 2021
@m-ou-se
Copy link
Member Author

m-ou-se commented Jan 12, 2021

Not sure who would be the right reviewer for this. Pinging some people whose names I saw in related git blame outputs: r? @Mark-Simulacrum @petrochenkov @SimonSapin @RalfJung

@bors

This comment has been minimized.

@m-ou-se m-ou-se force-pushed the panic-2021 branch 2 times, most recently from f5d9853 to 4b47a4a Compare January 14, 2021 18:38
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 1, 2021

📌 Commit 5b11a97 has been approved by petrochenkov

@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 Feb 1, 2021
@bors
Copy link
Collaborator

bors commented Feb 1, 2021

⌛ Testing commit 5b11a97 with merge e5451054558902869b7121a3c2f0c1cc180283ad...

@JohnTitor
Copy link
Member

@bors retry yielding

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Collaborator

bors commented Feb 1, 2021

⌛ Testing commit 5b11a97 with merge e0d9f79...

@bors
Copy link
Collaborator

bors commented Feb 1, 2021

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing e0d9f79 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 1, 2021
@bors bors merged commit e0d9f79 into rust-lang:master Feb 1, 2021
@rustbot rustbot added this to the 1.51.0 milestone Feb 1, 2021
@m-ou-se m-ou-se deleted the panic-2021 branch February 1, 2021 17:14
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 3, 2021
Implement Rust 2021 panic

This implements the Rust 2021 versions of `panic!()`. See rust-lang#80162 and rust-lang/rfcs#3007.

It does so by replacing `{std, core}::panic!()` by a bulitin macro that expands to either `$crate::panic::panic_2015!(..)` or `$crate::panic::panic_2021!(..)` depending on the edition of the caller.

This does not yet make std's panic an alias for core's panic on Rust 2021 as the RFC proposes. That will be a separate change: rust-lang@c5273bd That change is blocked on figuring out what to do with rust-lang#80846 first.
@Aurel300
Copy link

This is a minor point but we can no longer tell apart panic!() and assert!(false, "msg") in the macro_backtrace(), because both result in ["std::panic::panic_2015", "std::panic"].

@m-ou-se
Copy link
Member Author

m-ou-se commented Feb 15, 2021

@Aurel300 On Rust 2015 and 2018, nothing should've changed there other than an extra panic_2015 at the end, I think? Why doesn't the assert result in ["std::panic::panic_2015", "std::panic", "core::assert"]?

@Aurel300
Copy link

Aurel300 commented Feb 15, 2021

I would have expected it to, yes, but maybe it is inlined somehow? The macro_backtrace() of the two is identical (except for call location of course).

macro_backtrace: [
    ExpnData {
        kind: Macro(
            Bang,
            "$crate::panic::panic_2015",
        ),
        parent: ExpnId(
            2,
        ),
        call_site: prusti-tests/tests/verify/fail/no-annotations/panic-reason.rs:2:5: 2:14 (#6),
        def_site: /rustc/5fa22fe6f821ac3801d05f624b123dda25fde32c/library/std/src/panic.rs:26:1: 36:2 (#0),
        allow_internal_unstable: Some(
            [
                "libstd_sys_internals",
            ],
        ),
        allow_internal_unsafe: false,
        local_inner_macros: false,
        edition: Edition2018,
        macro_def_id: Some(
            DefId(1:4000 ~ std[a906]::panic::panic_2015),
        ),
        krate: crate0,
        orig_id: Some(
            21,
        ),
        disambiguator: 0,
    },
    ExpnData {
        kind: Macro(
            Bang,
            "panic",
        ),
        parent: ExpnId(
            0,
        ),
        call_site: prusti-tests/tests/verify/fail/no-annotations/panic-reason.rs:2:5: 2:14 (#0),
        def_site: /rustc/5fa22fe6f821ac3801d05f624b123dda25fde32c/library/std/src/macros.rs:28:1: 34:2 (#0),
        allow_internal_unstable: Some(
            [
                "edition_panic",
            ],
        ),
        allow_internal_unsafe: false,
        local_inner_macros: false,
        edition: Edition2018,
        macro_def_id: Some(
            DefId(1:9 ~ std[a906]::macros::panic),
        ),
        krate: crate0,
        orig_id: Some(
            2,
        ),
        disambiguator: 0,
    },
]

Edit: even with assert!(unimplemented_bool_call(), "msg") the backtrace is the same, so not inlining.

@m-ou-se
Copy link
Member Author

m-ou-se commented Feb 15, 2021

Can you open a new issue for this? Comments on an already merged PR tend to get lost easily.

# for free to join this conversation on GitHub. Already have an account? # to comment
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-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.