-
Notifications
You must be signed in to change notification settings - Fork 199
Conversation
fn module_from_address(&mut self, address: u64) -> &mut Module { | ||
let module_base = self | ||
.module_base_from_address(address) | ||
.unwrap_or(unsafe { NonZeroU64::new_unchecked(module::UNKNOWN_MODULE_BASE_ADDRESS) }); |
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.
How about avoiding unsafe
and doing:
.unwrap_or(unsafe { NonZeroU64::new_unchecked(module::UNKNOWN_MODULE_BASE_ADDRESS) }); | |
.unwrap_or(NonZeroU64::new(module::UNKNOWN_MODULE_BASE_ADDRESS).unwrap()); |
Alternately, it looks like new_unchecked()
is a const fn
, so maybe we could just make the exported constant a NonZeroU64
.
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.
We have too many unwrap
calls already and I started replacing them with expect
or adding proper error handling.
Unfortunately const fn
doesn't alleviate the need to use unsafe
, and you can't use unwrap
or expect
on a const
.
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.
Ah, darn. Maybe someday soon: rust-lang/rust#51999 (comment)
There is a bunch of refactoring in the PR but also a couple of important fixes.
Breakpoints are now cleared after
ContinueDebugEvent
returns and restored just before callingContinueDebugEvent
again. This ensures that debugger clients can read process memory and not see the breakpoints.Additionally, the original code byte for a breakpoint is reread to correctly handle the rare case of self modifying code.
Note that with this change, debug event processing is now significantly slower than before this PR. For infrequent events it might not be a big deal, but single stepping performance is much worse.
I may look at adding a way to simulate process reads to account for our breakpoints to improve performance, but I think this refactoring is worth taking as is because it is more correct and also I think cleans up breakpoints in general.