deduplicate abort implementations #139103
Conversation
rustbot
added
O-hermit
|
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
This comment has been minimized.
This comment has been minimized.
|
The Miri subtree was changed cc @rust-lang/miri |
This comment has been minimized.
This comment has been minimized.
| /// In Windows 8 and later, this will terminate the process immediately without | ||
| /// running any in-process exception handlers. In earlier versions of Windows, | ||
| /// this sequence of instructions will be treated as an access violation, | ||
| /// terminating the process but without necessarily bypassing all exception | ||
| /// handlers. |
There was a problem hiding this comment.
| /// In Windows 8 and later, this will terminate the process immediately without | |
| /// running any in-process exception handlers. In earlier versions of Windows, | |
| /// this sequence of instructions will be treated as an access violation, | |
| /// terminating the process but without necessarily bypassing all exception | |
| /// handlers. | |
| /// In Windows 8 and later, this will terminate the process immediately, | |
| /// bypassing all in-process exception handlers. In earlier versions of Windows, | |
| /// this sequence of instructions will be treated as an access violation, | |
| /// terminating the process but without necessarily bypassing all exception | |
| /// handlers. |
Let's use the same verb both times, otherwise this is unnecessarily confusing since it uses "without" twice to actually make statements that are opposites of each other.
There was a problem hiding this comment.
I've rephrased the comment a bit to make it even clearer.
| @@ -1,6 +1,6 @@ | |||
| //@error-in-other-file: the program aborted execution | |||
| //@normalize-stderr-test: "\| +\^+" -> "| ^" | |||
| //@normalize-stderr-test: "unsafe \{ libc::abort\(\); \}|core::intrinsics::abort\(\);" -> "ABORT();" | |||
| //@normalize-stderr-test: "unsafe \{ libc::abort\(\) \}|core::intrinsics::abort\(\)" -> "ABORT()" | |||
There was a problem hiding this comment.
| //@normalize-stderr-test: "unsafe \{ libc::abort\(\) \}|core::intrinsics::abort\(\)" -> "ABORT()" | |
| //@normalize-stderr-test: "unsafe \{ libc::abort\(\) \}|crate::intrinsics::abort\(\)" -> "ABORT()" |
There was a problem hiding this comment.
Are either of these needed? Since crate::intrinsics::abort is now normalized in src/tools/miri/tests/ui.rs
| // Note: contrary to the `__rust_abort` in `crate::rt`, this uses `no_mangle` | ||
| // because it is actually used from C code. Because symbols annotated with | ||
| // #[rustc_std_internal_symbol] get mangled, this will not lead to linker | ||
| // conflicts. |
There was a problem hiding this comment.
Could they get different symbol names? Having the same extern signature reused but distinguished by attributes is a bit tricky.
There was a problem hiding this comment.
__rust_abort is the obvious name for the symbol shared between panic_abort and std, so I'm a bit hesitant about changing that. Changing the SGX symbol seems better, but it's a lot of effort as it requires changing libunwind as well; I don't really think it's worth it.
rustbot
removed
the
S-waiting-on-review
|
Reminder, once the PR becomes ready for a review, use |
rustbot
added
the
S-waiting-on-author
|
Makes sense for now. In the future maybe we could make panic_abort and panic_unwind depend on std rather than the other way around. Both of them are only used with std, but currently std has a kind of weak dependency on panic_abort and panic_unwind where both get registered into the crate store (and thus for example participate in the duplicate lang item check), but only one of the two will actually "activated" and end up getting linked in the end. Flipping the dependency around would allow getting rid of this weird dependency state and allow directly using libstd symbols from both crates. |
|
☔ The latest upstream changes (presumably #139746) made this pull request unmergeable. Please resolve the merge conflicts. |
Currently, the code for process aborts is duplicated across `panic_abort` and `std`. This PR uses `#[rustc_std_internal_symbol]` to make the `std` implementation available to `panic_abort` via the linker, thereby deduplicating the code.
|
@rustbot ready I've rebased, blessed the tests and improved the comment. |
rustbot
added
S-waiting-on-review
|
☔ The latest upstream changes (presumably #140122) made this pull request unmergeable. Please resolve the merge conflicts. |
| @@ -1,6 +1,6 @@ | |||
| //@error-in-other-file: the program aborted execution | |||
| //@normalize-stderr-test: "\| +\^+" -> "| ^" | |||
| //@normalize-stderr-test: "unsafe \{ libc::abort\(\); \}|core::intrinsics::abort\(\);" -> "ABORT();" | |||
| //@normalize-stderr-test: "unsafe \{ libc::abort\(\) \}|core::intrinsics::abort\(\)" -> "ABORT()" | |||
There was a problem hiding this comment.
Are either of these needed? Since crate::intrinsics::abort is now normalized in src/tools/miri/tests/ui.rs
Currently, the code for process aborts is duplicated across
panic_abortandstd. This PR uses#[rustc_std_internal_symbol]to make thestdimplementation available topanic_abortvia the linker, thereby deduplicating the code.