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 certain panicky stdlib functions behave better under panic_immediate_abort #91737

Merged
merged 3 commits into from
Dec 12, 2021
Merged

Make certain panicky stdlib functions behave better under panic_immediate_abort #91737

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3cf9ae6
inline slice panics on panic_immediate_abort
Manishearth Dec 10, 2021
0adee2c
inline Option panics on panic_immediate_abort
Manishearth Dec 10, 2021
917dafc
Add separate impl of unwrap_failed to avoid constructing trait objects
Manishearth Dec 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion library/core/src/option.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1656,7 +1656,8 @@ impl<T, E> Option<Result<T, E>> {
}

// This is a separate function to reduce the code size of .expect() itself.
#[inline(never)]
#[cfg_attr(not(feature = "panic_immediate_abort"), inline(never))]
#[cfg_attr(feature = "panic_immediate_abort", inline)]
#[cold]
Comment on lines +1659 to 1661
Copy link
Member

Choose a reason for hiding this comment

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

Does #[inline] have any effect when it's also #[cold]?

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea! We could gate that attribute too, I guess

#[track_caller]
const fn expect_failed(msg: &str) -> ! {
Expand Down
13 changes: 13 additions & 0 deletions library/core/src/result.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1653,13 +1653,26 @@ impl<T> Result<T, T> {
}

// This is a separate function to reduce the code size of the methods
#[cfg(not(feature = "panic_immediate_abort"))]
#[inline(never)]
#[cold]
#[track_caller]
fn unwrap_failed(msg: &str, error: &dyn fmt::Debug) -> ! {
panic!("{}: {:?}", msg, error)
}

// This is a separate function to avoid constructing a `dyn Debug`
// that gets immediately thrown away, since vtables don't get cleaned up
// by dead code elimination if a trait object is constructed even if it goes
Copy link
Member

Choose a reason for hiding this comment

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

Is this a limitation of LLVM, or is it something that could be addressed on our end?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this can be addressed on our end because it's something that has to be run after the rest of the optimizations

Copy link
Member

Choose a reason for hiding this comment

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

Isn't #[inline] enough? Would be useful to look at the optimized LLVM IR that still contains the vtable (for something that doesn't really have any code other than what's rooted by the vtable), and take stock of what's exported from the LLVM module.

I am pretty sure LLVM can handle removing unused fn params, but if a global (static/fn) isn't marked private/internal/etc. to avoid export, then the ABI demands its signature be unchanged (and potentially even non-inlining interprocedural optimizations might be less effective, but I'm not sure if LLVM considers an exported definition to be opaque in any way, I kinda doubt it).

Copy link
Member Author

Choose a reason for hiding this comment

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

@eddyb In my experience LLVM is eager to keep around vtable specs that are never used. I think it treats the construction of a dyn and its use differently and if one goes away it doesn't necessarily figure out the other should too.

// unused
#[cfg(feature = "panic_immediate_abort")]
#[inline]
#[cold]
#[track_caller]
fn unwrap_failed<T>(_msg: &str, _error: &T) -> ! {
panic!()
}

/////////////////////////////////////////////////////////////////////////////
// Trait implementations
/////////////////////////////////////////////////////////////////////////////
Expand Down
15 changes: 10 additions & 5 deletions library/core/src/slice/index.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,35 +27,40 @@ where
}
}

#[inline(never)]
#[cfg_attr(not(feature = "panic_immediate_abort"), inline(never))]
#[cfg_attr(feature = "panic_immediate_abort", inline)]
#[cold]
#[track_caller]
fn slice_start_index_len_fail(index: usize, len: usize) -> ! {
panic!("range start index {} out of range for slice of length {}", index, len);
}

#[inline(never)]
#[cfg_attr(not(feature = "panic_immediate_abort"), inline(never))]
#[cfg_attr(feature = "panic_immediate_abort", inline)]
#[cold]
#[track_caller]
fn slice_end_index_len_fail(index: usize, len: usize) -> ! {
panic!("range end index {} out of range for slice of length {}", index, len);
}

#[inline(never)]
#[cfg_attr(not(feature = "panic_immediate_abort"), inline(never))]
#[cfg_attr(feature = "panic_immediate_abort", inline)]
#[cold]
#[track_caller]
fn slice_index_order_fail(index: usize, end: usize) -> ! {
panic!("slice index starts at {} but ends at {}", index, end);
}

#[inline(never)]
#[cfg_attr(not(feature = "panic_immediate_abort"), inline(never))]
#[cfg_attr(feature = "panic_immediate_abort", inline)]
#[cold]
#[track_caller]
fn slice_start_index_overflow_fail() -> ! {
panic!("attempted to index slice from after maximum usize");
}

#[inline(never)]
#[cfg_attr(not(feature = "panic_immediate_abort"), inline(never))]
#[cfg_attr(feature = "panic_immediate_abort", inline)]
#[cold]
#[track_caller]
fn slice_end_index_overflow_fail() -> ! {
Expand Down