-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Limit the number of frames in backtrace collection #12542
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
base: main
Are you sure you want to change the base?
Limit the number of frames in backtrace collection #12542
Conversation
Add `Config::wasm_backtrace_max_frames` option to limit the number of frames collected in backtraces and set the default at 20. This helps prevent expensive work from very deep call stacks. Setting the value to 0 is the same as disabling backtraces and so this change deprecates `Config::wasm_backtrace`. Addresses bytecodealliance#5052
fitzgen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! A couple nitpicks inline below before we merge this.
crates/wasmtime/src/config.rs
Outdated
| pub(crate) disabled_features: WasmFeatures, | ||
| pub(crate) wasm_backtrace: bool, | ||
| pub(crate) wasm_backtrace_details_env_used: bool, | ||
| pub(crate) wasm_backtrace_max_frames: usize, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we represent this as an Option<NonZeroUsize>? That will make use sites clearer, rather than checking for zero.
crates/wasmtime/src/config.rs
Outdated
| if enable { | ||
| if self.wasm_backtrace_max_frames == 0 { | ||
| self.wasm_backtrace_max_frames = DEFAULT_WASM_BACKTRACE_MAX_FRAMES; | ||
| } | ||
| } else { | ||
| self.wasm_backtrace_max_frames = 0; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nitpick, but I think the logic is a little more clear when collapsing the nested conditionals into a single match:
| if enable { | |
| if self.wasm_backtrace_max_frames == 0 { | |
| self.wasm_backtrace_max_frames = DEFAULT_WASM_BACKTRACE_MAX_FRAMES; | |
| } | |
| } else { | |
| self.wasm_backtrace_max_frames = 0; | |
| } | |
| match (enable, self.wasm_backtrace_max_frames) { | |
| (false, _) => self.wasm_backtrace_max_frames = None, | |
| // Wasm backtraces were disabled; enable them with the | |
| // default maximum number of frames to capture. | |
| (true, None) => self.wasm_backtrace_max_frames = { | |
| Some(NonZeroUsize::new(DEFAULT_WASM_BACKTRACE_MAX_FRAMES).unwrap()); | |
| } | |
| // Wasm backtraces are already enabled; keep the existing | |
| // max-frames configuration. | |
| (true, Some(_)) => {} | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Also, we could avoid the NonZeroUsize::new(...).unwrap() noise here by making the DEFAULT_WASM_BACKTRACE_MAX_FRAMES constant be a NonZeroUsize already.)
Label Messager: wasmtime:configIt looks like you are changing Wasmtime's configuration options. Make sure to
DetailsTo modify this label's message, edit the To add new label messages or remove existing label messages, edit the |
crates/wasmtime/src/runtime/trap.rs
Outdated
| trap_pc: Option<usize>, | ||
| max_frames: usize, | ||
| ) -> Self { | ||
| let mut wasm_trace = Vec::<FrameInfo>::with_capacity(runtime_trace.frames().len()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| let mut wasm_trace = Vec::<FrameInfo>::with_capacity(runtime_trace.frames().len()); | |
| let mut wasm_trace = Vec::<FrameInfo>::with_capacity(max_frames); |
|
Regarding the note on fuzzing - I guess we don't need to do anything because fuzzing always used the default of enabling backtraces and that isn't changing. |
fitzgen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Add
Config::wasm_backtrace_max_framesoption to limit the number of frames collected in backtraces and set the default at 20. This helps prevent expensive work from very deep call stacks.Setting the value to 0 is the same as disabling backtraces and so this change deprecates
Config::wasm_backtrace.Addresses #5052