rust-lang / rfcs

RFCs for changes to Rust
https://rust-lang.github.io/rfcs/
Apache License 2.0
5.89k stars 1.56k forks source link

deactivate all panics (obviously optional) #3280

Closed WormPatch closed 2 years ago

WormPatch commented 2 years ago

unreachable_unchecked is a way to tell the compiler: "this will never happend", example:

fn index(array:&[u8], x:usize) {
    if x < array.len() {
        return array[x]; //return is more readable
    } else {
        panic!("out of bounds");
    }
}

if we replace panic!() with unreachable_unchecked(), this function converts into:

fn index(array:&[u8], x:usize) { array[x] }

what happened here?, we "despanicked" a function.

panic = 'unreachable_unchecked' in Cargo.toml. will despanic all functions in project. disabling all checks

WormPatch commented 2 years ago

so yes, the way is by modifing panic source code, that is painful af, i want it to be in my dev profiles

SOF3 commented 2 years ago

i dont understand what is wrong with this is totally optional and its sane in correct code

How do you prove that the code is correct? Testing is not sufficient, as mentioned above, "works on my machine" does not imply safety.

Plus, what are you worried about a panic if it just aborts the program? A panic that just aborts should have almost no impact on performance.

WormPatch commented 2 years ago

i dont understand what is wrong with this is totally optional and its sane in correct code

How do you prove that the code is correct? Testing is not sufficient, as mentioned above, "works on my machine" does not imply safety.

Plus, what are you worried about a panic if it just aborts the program? A panic that just aborts should have almost no impact on performance.

well, in process as minimax where some easy task is repeated a lot of times it an affect a little bit, not too much, but a little bit. and i think adding this is good idea, maybe making it a little harder to implement. but this would/will make rust more general propouse programming language. i mean its already but there are levels of how general it can be

SOF3 commented 2 years ago

i dont understand what is wrong with this is totally optional and its sane in correct code

How do you prove that the code is correct? Testing is not sufficient, as mentioned above, "works on my machine" does not imply safety. Plus, what are you worried about a panic if it just aborts the program? A panic that just aborts should have almost no impact on performance.

well, in process as minimax where some easy task is repeated a lot of times it an affect a little bit, not too much, but a little bit. and i think adding this is good idea, maybe making it a little harder to implement. but this would/will make rust more general propouse programming language. i mean its already but there are levels of how general it can be

Most of the time, "easy" panic branches that can be manually verified can also be identified by the compiler and elided during optimization, so it has almost no performance impact.

WormPatch commented 2 years ago

i dont understand what is wrong with this is totally optional and its sane in correct code

How do you prove that the code is correct? Testing is not sufficient, as mentioned above, "works on my machine" does not imply safety. Plus, what are you worried about a panic if it just aborts the program? A panic that just aborts should have almost no impact on performance.

well, in process as minimax where some easy task is repeated a lot of times it an affect a little bit, not too much, but a little bit. and i think adding this is good idea, maybe making it a little harder to implement. but this would/will make rust more general propouse programming language. i mean its already but there are levels of how general it can be

Most of the time, "easy" panic branches that can be manually verified can also be identified by the compiler and elided during optimization, so it has almost no performance impact.

optimizations are good but I can't trust them 100% of the time

Lokathor commented 2 years ago

Do you have specific benchmarks that demonstrate the speed problem you think exists?

WormPatch commented 2 years ago

Do you have specific benchmarks that demonstrate the speed problem you think exists?

no because i cant remove panic edit: but i know bad minimax (recursive), is performance sensitive

Lokathor commented 2 years ago

You could construct an example where all panic paths have been manually replaced with unsafe panicless options, such as unwrap_unchecked for Option/Result, and get_unchecked for indexing

WormPatch commented 2 years ago

You could construct an example where all panic paths have been manually replaced with unsafe panicless options, such as unwrap_unchecked for Option/Result, and get_unchecked for indexing

manually? i dont know asm enought to do that

CryZe commented 2 years ago

One thing that used to work when not using std, is simply calling unreachable_unchecked() in the panic handler. That used to remove all panic paths. Not sure if it still does.

Also from some testing I did (though also a long time ago) is that bounds checks are basically free when working with floating point numbers, but when working with integer data, it's not entirely free.

nagisa commented 2 years ago

I’m so sorry about that assign, CryZe! I’ll blame this one on a playful otter ^^


More on topic, the current panic = abort implementation already utilizes the intrinsics::abort intrinsic that unconditionally generates an ud2 on platforms other than windows, unix and a few other exceptions. This will produce largely the maximally small code that still honors the prescribed properties of panics.

It is important to remember that the definition of panics do not require that they are unreachable, only that they do not return normally. unreachable_unchecked does not honor that property today, despite its signature. As such, unreachable_unchecked is not a suitable implementation for panic without changing at least the way panic is defined. Doing so would be a breaking change and require a Rust two-point-oh.

Lokathor commented 2 years ago

@WormPatch You don't need to know any assembly to make the benchmarks.

1) write some code that is normal code with panicking. 2) the also write a version that computes the same thing but uses various forms of "unreachable" 3) benchmark the two sets of code.

I strongly suspect that there will not be a huge difference. If there is a significant difference you can demonstrate, people will probably be willing to try and reduce the difference (while maintaining safe code).

However, without a specific benchmark to discuss, or other form of specific evidence, this proposal amounts to you just guessing that there will be a difference even though you admit that you've never written a program to try it.

WormPatch commented 2 years ago

@WormPatch You don't need to know any assembly to make the benchmarks.

  1. write some code that is normal code with panicking.
  2. the also write a version that computes the same thing but uses various forms of "unreachable"
  3. benchmark the two sets of code.

I strongly suspect that there will not be a huge difference. If there is a significant difference you can demonstrate, people will probably be willing to try and reduce the difference (while maintaining safe code).

However, without a specific benchmark to discuss, or other form of specific evidence, this proposal amounts to you just guessing that there will be a difference even though you admit that you've never written a program to try it.

i have tried minimax of course, the first i do to test a lang is chess minimax, python does 2 depth, C# 5 depth, and Rust 8 depth. i have lost Rust one (i used rm command wrong), i will make it and i will benchmark

WormPatch commented 2 years ago

@WormPatch You don't need to know any assembly to make the benchmarks.

  1. write some code that is normal code with panicking.
  2. the also write a version that computes the same thing but uses various forms of "unreachable"
  3. benchmark the two sets of code.

I strongly suspect that there will not be a huge difference. If there is a significant difference you can demonstrate, people will probably be willing to try and reduce the difference (while maintaining safe code).

However, without a specific benchmark to discuss, or other form of specific evidence, this proposal amounts to you just guessing that there will be a difference even though you admit that you've never written a program to try it.

what about index bounds? raw pointers are painful

Lokathor commented 2 years ago

Slices have a get_unchecked method for unchecked indexing. This is a fairly basic fact about them, and if you're not familiar with at least that much of the standard library then you should probably skim the methods of all the other library types to find out what sorts of unchecked operations are already supported, without any changes to the compiler.

However, tech support for a longer project like this isn't really what the issues of the RFCs repo should be used for. I'd encourage you to ask in one of the many places for Rust programming support:

WormPatch commented 2 years ago

@WormPatch You don't need to know any assembly to make the benchmarks.

  1. write some code that is normal code with panicking.
  2. the also write a version that computes the same thing but uses various forms of "unreachable"
  3. benchmark the two sets of code.

I strongly suspect that there will not be a huge difference. If there is a significant difference you can demonstrate, people will probably be willing to try and reduce the difference (while maintaining safe code).

However, without a specific benchmark to discuss, or other form of specific evidence, this proposal amounts to you just guessing that there will be a difference even though you admit that you've never written a program to try it.

i dont know how to make unpanic index, using raw pointers is slower, im doing something wrong

Lokathor commented 2 years ago

I already told you: if you have a slice called data, you call data.get_unchecked(index)

https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.get_unchecked

Kixiron commented 2 years ago

Like Lokathor said, slice::get_unchecked()

WormPatch commented 2 years ago

I already told you: if you have a slice called data, you call data.get_unchecked(index)

https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.get_unchecked

that is literally 25% slower than raw pointers one

Lokathor commented 2 years ago

Well, you should show some specific code and people can try to figure out what's wrong.

But again, you should use one of the other mediums I mentioned before. RFCs repo issues are not the place for tech support.

WormPatch commented 2 years ago

Well, you should show some specific code and people can try to figure out what's wrong.

But again, you should use one of the other mediums I mentioned before. RFCs repo issues are not the place for tech support.

yes but im trying to get to my point, sometimes a task uses too much index and stuff, and i know it doesnt care in most apps, because that apps are maded to be user efficent or market efficent, so cleaning errors is the best option. but people like me that wants to make heavy calcs or embedding. its important to make the final product efficent, because bound check is fast but index is fast too so bound check can be important. its just having the good of both sides

BurntSushi commented 2 years ago

I promise you that you will get exactly nowhere with this feature request if you can't or won't produce real examples motivating your request.

Please take the advice of others: this isn't really a place for tech support.

ChayimFriedman2 commented 2 years ago

get_unchecked() 25% slower than direct access? Sounds suspicious. Are you compiling with optimizations?

WormPatch commented 2 years ago

get_unchecked() 25% slower than direct access? Sounds suspicious. Are you compiling with optimizations?

only --release edit: sorry, my bad, it goes 25% slower WITHOUT --release edit 2: no, --release was optimizing my mega dumb test code, so they both have the almost same assembly

WormPatch commented 2 years ago

I promise you that you will get exactly nowhere with this feature request if you can't or won't produce real examples motivating your request.

Please take the advice of others: this isn't really a place for tech support.

i cant do it because i havent unpanic, if you want i can try it with C++

ChayimFriedman2 commented 2 years ago

You're compiling without --release and complaining about performance??

WormPatch commented 2 years ago

You're compiling without --release and complaining about performance??

its a test edit: but the compilers is smart enough to skip it

WormPatch commented 2 years ago

this feature is easy to implement, useful in some cases, it doesn't get worse compile time or runtime. it is only a little unsafe, but the one that uses it assumes his own risk, like unsafe things in rust

shepmaster commented 2 years ago

Maybe an additional person telling you will help: produce real code that is slower than you want.

I recommend showing the code and generated assembly and explaining what is slow. Include how you generate the assembly (compiler version, build arguments, target, etc).

Then fix that code by manually replacing panics with undefined behavior. Show what changed in the Rust and assembly.

This is the bare minimum I’d expect to prove that your idea would even be beneficial. It’s not enough to cause it to be implemented, but it’s enough to show you are willing to work with others.

My own interpretation is that you are enjoying wasting the time of people that would generally like to help you/Rust get better and it’s rather tiresome.

shepmaster commented 2 years ago

this feature is easy to implement

you are welcome to implement it and submit a pull request. Then this same discussion will occur there before it gets merged.

Noratrieb commented 2 years ago

(Note that implementing this feature is a waste of time. It would never get merged.)

WormPatch commented 2 years ago

Maybe an additional person telling you will help: produce real code that is slower than you want.

I recommend showing the code and generated assembly and explaining what is slow. Include how you generate the assembly (compiler version, build arguments, target, etc).

Then fix that code by manually replacing panics with undefined behavior. Show what changed in the Rust and assembly.

This is the bare minimum I’d expect to prove that your idea would even be beneficial. It’s not enough to cause it to be implemented, but it’s enough to show you are willing to work with others.

My own interpretation is that you are enjoying wasting the time of people that would generally like to help you/Rust get better and it’s rather tiresome.

that is the problem i used rm command wrong and deleted all my projects i'm trying to make it from 0 to try it

WormPatch commented 2 years ago

this feature is easy to implement

you are welcome to implement it and submit a pull request. Then this same discussion will occur there before it gets merged.

the problem is, i don't understand rust compiler code, but i guess this is easy to implement because its probably made with 1 conditional and a function. if this is closed i will try to fork rust compiler

shepmaster commented 2 years ago

if this is closed i will try to fork rust compiler

feel free to do it before then, no reason to wait!

WormPatch commented 2 years ago

if this is closed i will try to fork rust compiler

feel free to do it before then, no reason to wait!

i'm trying to clone panic = 'abort' code, but rust compiler is complex af even when code is surprisingly clean

WormPatch commented 2 years ago

do someone knows where rust matches panic handling?

Lokathor commented 2 years ago

Like BurntSushi said, please use one of the other communication systems that I posted before, because this is not for tech support.

WormPatch commented 2 years ago

Like BurntSushi said, please use one of the other communication systems that I posted before, because this is not for tech support.

* https://www.reddit.com/r/rust/

* https://users.rust-lang.org/

* https://discord.com/invite/rust-lang-community

srry

WormPatch commented 2 years ago

sum, subtraction, division, multiplication, index.etc are common operations, i would like a clean way to uncheck them all, making a cleaner asm. there is nothing wrong there, sometimes you just want panics at testing and developing, and leaving it free at release.

TimNN commented 2 years ago

Even without explicit invitation, this thread was sufficient to get me curios (nerd snipe me) into coming up with an experiment / micro benchmark.

The benchmark iterates over pairs of integers embedded in the binary and divides them, summing the results. This should contain at least two potentially panicking operations: divide-by-zero and indexing. I didn't get around to looking at the asm and currently do not have access to linux machine where I can get performance counters.

The benchmark was run in three configurations:

stable-abort unstable-immediate-abort unsafe-immediate-unreachable
.text size 741b 661b 613b
mean runtime 1.009s 1.408s 0.906s

Make of this what you will. My takeaways are:

Edit/Disclaimer: I didn't spent too much time on this, especially not on analyzing the results, so the benchmark may be flawed in a myriad of ways. The primary purpose was to see how/if it was possible to mark every panic "unreachable", and what the impact of that would be.


Click to see benchmark code ````rust // Copyright 2022 Google LLC. // SPDX-License-Identifier: Apache-2.0 /* # Instructions Tested on Linux. Use a recent nightly and create an empty project. Append to `Cargo.toml`: ``` [profile.release] lto = "fat" codegen-units = 1 panic = "abort" ``` ``` # Update this to match your project name and platform. EXE_NAME="demo" HOST_TARGET="$(rustc -vV | grep host | cut -d' ' -f2)" rustup component add rust-src # Update `bs` for more or less test data. dd if=/dev/urandom | tr -d '\000' | dd bs=64K count=1 iflag=fullblock of=src/rand.dat mkdir out ``` Compile with stable features: ``` cargo clean cargo build --release --target=$HOST_TARGET cp target/$HOST_TARGET/release/$EXE_NAME out/stable-abort ``` Compile with immediate abort: ``` cargo clean cargo build --release --target=$HOST_TARGET -Zbuild-std=core -Zbuild-std-features=panic_immediate_abort cp target/$HOST_TARGET/release/$EXE_NAME out/unstable-immediate-abort ``` Compile with immediate unreachable: ``` cp -r "$(rustc --print=sysroot)/lib/rustlib/src/rust" std-src sed -i -e 's/super::intrinsics::abort()/unsafe{super::hint::unreachable_unchecked()}/' std-src/library/core/src/panicking.rs cargo clean __CARGO_TESTS_ONLY_SRC_ROOT=$PWD/std-src cargo build --release --target=$HOST_TARGET -Zbuild-std=core -Zbuild-std-features=panic_immediate_abort cp target/$HOST_TARGET/release/$EXE_NAME out/unsafe-immediate-unreachable ``` Analysis: ``` ls out/* | xargs -I% bash -c 'echo -n "%: "; %; echo $?' size -Ad out/* | grep -e out/ -e .text hyperfine -i out/* ``` */ */ */ */ #![feature(bench_black_box, core_ffi_c, core_intrinsics, rustc_private)] #![no_std] #![no_main] #[link(name = "c")] extern {} #[panic_handler] fn panic(_: &core::panic::PanicInfo) -> ! { core::intrinsics::abort() } static DATA: &[u8] = include_bytes!("rand.dat"); fn compute() -> u32 { let (_, data, _) = unsafe { DATA.align_to::() }; let mut res = 0u32; for i in 0 .. data.len() { for j in 0 .. data.len() { let i = core::hint::black_box(i); let j = core::hint::black_box(j); res = res.wrapping_add(data[i] / data[j]) } } res } #[no_mangle] extern "C" fn main(_argc: core::ffi::c_int, _argv: *const *const u8) -> core::ffi::c_int { compute() as core::ffi::c_int } ````
ChayimFriedman2 commented 2 years ago

I'd argue that benchmark is meaningless (cool, but meaningless). Maybe even proving our point. With profiling you can very easily spot the problem (if there is one) and fix it by introducing one usage of unsafe (for the division; if you remove the black_box() LLVM optimizes the indexing correctly). Maybe even swap the loop and assert j != 0, getting it safely. This is exactly the best usage of unsafe in Rust: specific, after benchmarking, easy to justify (of course you should make the function unsafe if you don't assert it). Global dispanicking makes no sense, neither here nor probably in any other place.

TimNN commented 2 years ago

I'd argue that benchmark is meaningless

I agree! This was firmly in the "was so interested in whether I could, that I didn't stop to think if I should" territory.

This is exactly the best usage of unsafe in Rust: specific, after benchmarking, easy to justify

Strong +1 to that.

WormPatch commented 2 years ago

Even without explicit invitation, this thread was sufficient to get me curios (nerd snipe me) into coming up with an experiment / micro benchmark.

The benchmark iterates over pairs of integers embedded in the binary and divides them, summing the results. This should contain at least two potentially panicking operations: divide-by-zero and indexing. I didn't get around to looking at the asm and currently do not have access to linux machine where I can get performance counters.

The benchmark was run in three configurations:

* `stable-abort`: Uses only stable features with `panic=abort`.

* `unstable-immediate-abort`: Uses a re-built `libcore` with the `panic_immediate_abort` feature enabled.

* `unsafe-immediate-unreachable`: Uses a patched `libcore` where the aborts from `panic_immediate_abort` were replaced by `unreachable_unchecked()`.

stable-abort unstable-immediate-abort unsafe-immediate-unreachable .text size 741b 661b 613b mean runtime 1.009s 1.408s 0.906s

Make of this what you will. My takeaways are:

* There is a measurable difference in considering panics unreachable, though I imagine that explicit and targeted use of "unchecked" function could get most of the same performance.

* I'm a bit surprised at the slowness of `unstable-immediate-abort`.

* There's a reasonably easy way to get what @WormPatch wants by (ab)using (very) unstable features. This could at least serve as a base for more comprehensive benchmarks.

Edit/Disclaimer: I didn't spent too much time on this, especially not on analyzing the results, so the benchmark may be flawed in a myriad of ways. The primary purpose was to see how/if it was possible to mark every panic "unreachable", and what the impact of that would be. Click to see benchmark code

Thanks for doing the technical analysis that I didn't do because I didn't know how. do you have the result in asm?

SOF3 commented 2 years ago

this feature is easy to implement, useful in some cases, it doesn't get worse compile time or runtime. it is only a little unsafe, but the one that uses it assumes his own risk, like unsafe things in rust

@WormPatch actually this isn't really true. Without enabling more aggressive optimizations, this feature has virtually no impact on performance because a branch that continues vs a branch that aborts have no difference at all if compiler does not try to infer that one of the branches never gets hit.

ChayimFriedman2 commented 2 years ago

@WormPatch actually this isn't really true. Without enabling more aggressive optimizations, this feature has virtually no impact on performance because a branch that continues vs a branch that aborts have no difference at all if compiler does not try to infer that one of the branches never gets hit.

Technically this is true; however, actually you're more likely to get better compile times even because the eliminated branch will save the cost of codegen'ing the panic.

WormPatch commented 2 years ago

this feature is easy to implement, useful in some cases, it doesn't get worse compile time or runtime. it is only a little unsafe, but the one that uses it assumes his own risk, like unsafe things in rust

@WormPatch actually this isn't really true. Without enabling more aggressive optimizations, this feature has virtually no impact on performance because a branch that continues vs a branch that aborts have no difference at all if compiler does not try to infer that one of the branches never gets hit.

most functions can't optimize it because inputs are unknown or compilers are not smart enough (yes, even with Rust+LLVM)

WormPatch commented 2 years ago

I'd argue that benchmark is meaningless (cool, but meaningless). Maybe even proving our point. With profiling you can very easily spot the problem (if there is one) and fix it by introducing one usage of unsafe (for the division; if you remove the black_box() LLVM optimizes the indexing correctly). Maybe even swap the loop and assert j != 0, getting it safely. This is exactly the best usage of unsafe in Rust: specific, after benchmarking, easy to justify (of course you should make the function unsafe if you don't assert it). Global dispanicking makes no sense, neither here nor probably in any other place.

tell me why its "no sense" to you

ChayimFriedman2 commented 2 years ago

tell me why its "no sense" to you

Because of what I said a sentence earlier.

WormPatch commented 2 years ago

tell me why its "no sense" to you

Because of what I said a sentence earlier.

you cant depend of compiler optimization all time, and again, remember that compiler cant optimize it with functions inputs because inputs are unknown

SOF3 commented 2 years ago

In fact, the whole point of unreachable_unchecked is to allow the compiler to optimize it by asserting that a certain condition is met. If we do not consider any compiler optimizations, this whole discussion is completely pointless because the performance gain from removing a branch alone is close to zero.