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.

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

Rust backend can panic because of 'attempt to multiply with overflow' #432

Open
bluenote10 opened this issue May 2, 2020 · 11 comments
Open

Comments

@bluenote10
Copy link
Contributor

bluenote10 commented May 2, 2020

An easy way to reproduce this problem is to use

faust2portaudiorust ./examples/gameaudio/rain.dsp

Running cargo run crashes with:

Faust Rust code running with Portaudio: sample-rate = 44100 buffer-size = 64
getNumInputs: 0
getNumOutputs: 2
thread '<unnamed>' panicked at 'attempt to multiply with overflow', src/main.rs:170:35
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
fatal runtime error: failed to initiate panic, error 5
[1]    20977 abort (core dumped)  cargo run
Full traceback
thread '<unnamed>' panicked at 'attempt to multiply with overflow', src/main.rs:170:35
stack backtrace:
   0: backtrace::backtrace::libunwind::trace
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.40/src/backtrace/libunwind.rs:88
   1: backtrace::backtrace::trace_unsynchronized
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.40/src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print_fmt
             at src/libstd/sys_common/backtrace.rs:77
   3: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
             at src/libstd/sys_common/backtrace.rs:61
   4: core::fmt::write
             at src/libcore/fmt/mod.rs:1028
   5: std::io::Write::write_fmt
             at src/libstd/io/mod.rs:1412
   6: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:65
   7: std::sys_common::backtrace::print
             at src/libstd/sys_common/backtrace.rs:50
   8: std::panicking::default_hook::{{closure}}
             at src/libstd/panicking.rs:188
   9: std::panicking::default_hook
             at src/libstd/panicking.rs:205
  10: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:464
  11: std::panicking::continue_panic_fmt
             at src/libstd/panicking.rs:373
  12: rust_begin_unwind
             at src/libstd/panicking.rs:302
  13: core::panicking::panic_fmt
             at src/libcore/panicking.rs:139
  14: core::panicking::panic
             at src/libcore/panicking.rs:70
  15: rain_portaudiorust::mydsp::compute
             at src/main.rs:170
  16: rain_portaudiorust::run::{{closure}}
             at src/main.rs:248
  17: portaudio::stream::Stream<portaudio::stream::NonBlocking,F>::open::{{closure}}
             at /home/fabian/.cargo/registry/src/github.com-1ecc6299db9ec823/portaudio-0.7.0/src/stream.rs:1309
  18: <alloc::boxed::Box<F> as core::ops::function::FnMut<A>>::call_mut
             at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/liballoc/boxed.rs:949
  19: portaudio::stream::stream_callback_proc
             at /home/fabian/.cargo/registry/src/github.com-1ecc6299db9ec823/portaudio-0.7.0/src/stream.rs:1365
  20: NonAdaptingProcess
             at src/common/pa_process.c:871
  21: PaUtil_EndBufferProcessing
             at src/common/pa_process.c:1562
  22: PaOSS_AudioThreadProc
             at src/hostapi/oss/pa_unix_oss.c:1726
  23: start_thread
  24: __clone
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
fatal runtime error: failed to initiate panic, error 5

The problematic line of code is:

let mut iTemp0: i32 = (1103515245 * (self.iRec0[1] + 12345));

The overflows here are probably on purpose for noise generation. We'll have to look into how this is possible in Rust.

Edit: It looks like std::num::Wrapping could be used to allow for intentionally-wrapped arithmetic.

CC @bkfox maybe you're more experienced on this.

@bluenote10 bluenote10 changed the title Rust backend can panick because of 'attempt to multiply with overflow' Rust backend can panic because of 'attempt to multiply with overflow' May 2, 2020
@sletz
Copy link
Member

sletz commented May 2, 2020

The problem is known and yes the overflow is needed. And I would be interested to know the correct solution.

@bluenote10
Copy link
Contributor Author

As a quick test I have edited the generated code to:

let mut iTemp0: Wrapping<i32> = (Wrapping(1103515245) * Wrapping(self.iRec0[1] + 12345));
self.iRec0[0] = (Wrapping(1103515245i32) * (iTemp0 + Wrapping(12345))).0;
let mut iRec1: i32 = iTemp0.0;

and then there is no panic. Since the example is noise anyway, it is hard to say if it has the right semantics, but most likely it does.

So basically the generator would have to use Wrapping<i32> (or u32?) to allow for overflows.

@sletz
Copy link
Member

sletz commented May 2, 2020

Wrapping<i32> is the semantic we want, but having to use this kind of ugly syntax is not really satisfactory. Is there any way in Rust to keep the standard syntax but be sure the underlying type is Wrapping<i32> ?

@bluenote10
Copy link
Contributor Author

I'm no expert on this, but given Rust focus on being explicit about safety, it is likely that this is the only option.

My hacky example above maybe made it look more ugly than it has to be. Basically it comes down to:

  • replacing the literals,
  • replacing the types,
  • unwrapping with .0 where necessary.

At least the first two could be made nicer by using type aliases and a macro for the literals. For instance:

use std::num::Wrapping;

type I32 = Wrapping<i32>;

macro_rules! w {
    ($lit:expr) => {
        Wrapping($lit)
    };
}

fn main() {
    // syntax becomes relatively lightweight
    let a: I32 = w!(1) * w!(2);
}

If the code uses these types consistently there shouldn't be many places where unwrapping is needed.

@sletz
Copy link
Member

sletz commented May 2, 2020

Yes, using macro is probably the way to go, I'll have a look ASAP. As curiosity, in what use-case are you testing the Rust backend ?

@bluenote10
Copy link
Contributor Author

bluenote10 commented May 2, 2020

No hurry, I should have mentioned that this is only a problem in debug builds. For now it is easy to work-around via cargo run --release where Rust disables overflow checks anyway.

As curiosity, in what use-case are you testing the Rust backend?

I've always been a fan of Faust, but I never found the time to learn it. I heard that it has Rust support now, and thought that is a nice combination, and a good opportunity to look into it again. For now I'm just experimenting to see how it works / what is possible.

(It was actually pretty straightforward to get something running. Now I'm at a point where I can run examples through the portaudiorust architecture, but I have like ~1 sec audio pauses every 0.2 sec or so -- will investigate that, most likely an issue with portaudio. Edit: Turned out to be an issue with the duplex mode of portaudio, see RustAudio/rust-portaudio#180. Switching to a pure output stream works fine, and is sufficient since I don't need audio input.)

@bkfox
Copy link

bkfox commented May 18, 2020

I actually see that there are standard methods implemented for overflowing integer operations. This would do the job:

let mut iTemp0: i32 = 1103515245.wrapping_mul(self.iRec0[1].wrapping_add(12345));

@plule
Copy link
Contributor

plule commented Sep 18, 2022

I've opened a PR implementing the wrapping_* operations.

As of now, a possible workaround for now is to disable entirely the overflow checks. It can be done in Cargo.toml:

[profile.dev]
overflow-checks = false

This is however global to the entire project. I did not manage to scope it to the module with #![allow(arithmetic_overflow)], so I think the way to go is to use the verbose wrapping_* methods.

@bluenote10
Copy link
Contributor Author

It can be done in Cargo.toml:

I also think your PR #802 is the way to go. I simply never had the time to figure out where it has to be integrated. Thanks for taking care of that!

I think the way to go is to use the verbose wrapping_* methods.

Yes, the generated code isn't supposed to be the most human-readable piece of code anyway. If we really care, we could later shorten it slightly by introducing a macro, but I'm not sure if it is worth the effort / added complexity.

@obsoleszenz
Copy link

@sletz Isn't this fixed by 47cc3a7 ?

@sletz
Copy link
Member

sletz commented May 9, 2023

AFAICS it should yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants