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

core_impl: fix support for no threads #127

Merged
merged 3 commits into from
Mar 15, 2023

Conversation

xobs
Copy link
Contributor

@xobs xobs commented Mar 13, 2023

Sometimes, a target has no threads. This may be the case when a process is not yet selected, or when all threads have terminated in a specific process.

Support None as a return from get_sane_any_tid() and return an error to GDB. This addresses a NoActiveThreads error, and closes #122.

@daniel5151 daniel5151 self-requested a review March 13, 2023 14:46
Copy link
Owner

@daniel5151 daniel5151 left a comment

Choose a reason for hiding this comment

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

Aside from that one comment that needs addressing, this LGTM

Comment on lines 311 to 314
None => {
res.write_str("E01")?;
return Ok(HandlerStatus::Handled);
}
Copy link
Owner

Choose a reason for hiding this comment

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

I actually have a bit of plumbing in place to streamline returning Exx-style errors:

Suggested change
None => {
res.write_str("E01")?;
return Ok(HandlerStatus::Handled);
}
None => return Err(Error::NonFatalError(1))

It's both more concise than invoking write_str + returning early, and also results in some slightly better codegen (wrt. binary size).

Can you switch both blocks over?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I've made this change and pushed the result.

Copy link
Owner

Choose a reason for hiding this comment

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

Seems this change got lost somewhere?

@daniel5151
Copy link
Owner

Oh, also: can you mark the NoActiveThreads error enum variant as #[deprecated], mentioning that it isn't being used anymore?

I'll spin up a tracking issue to remove it entirely in 0.7

@xobs
Copy link
Contributor Author

xobs commented Mar 14, 2023

Alright, I also marked NoActiveThreads as #[deprecated]

@xobs xobs force-pushed the fix-noactivethreads-error branch from 741b84a to be7439e Compare March 14, 2023 02:18
Copy link
Owner

@daniel5151 daniel5151 left a comment

Choose a reason for hiding this comment

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

I think you had a bit of a branch-juggling mishap 😅


/// Allow the target to change the active PID from the default fake one.
#[inline(always)]
fn active_pid(&mut self) -> Result<Pid, Self::Error> {
Copy link
Owner

Choose a reason for hiding this comment

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

It seems you lumped together your two PRs and lost some stuff along the way...

Lets keep this PR focused on solving the bug at hand, and leave this stuff to #129

Comment on lines 311 to 314
None => {
res.write_str("E01")?;
return Ok(HandlerStatus::Handled);
}
Copy link
Owner

Choose a reason for hiding this comment

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

Seems this change got lost somewhere?

This variant is no longer generated, so note that it will be removed.

Signed-off-by: Sean Cross <sean@xobs.io>
@xobs xobs force-pushed the fix-noactivethreads-error branch from be7439e to 55e6ed2 Compare March 15, 2023 05:42
Sometimes, a target has no threads. This may be the case when a process
is not yet selected, or when all threads have terminated in a specific
process.

Support `None` as a return from `get_sane_any_tid()` and return an error
to GDB. This addresses a `NoActiveThreads` error, and closes daniel5151#122.

Signed-off-by: Sean Cross <sean@xobs.io>
Since this plumbing exists, use this to return an error.

Signed-off-by: Sean Cross <sean@xobs.io>
@xobs
Copy link
Contributor Author

xobs commented Mar 15, 2023

It's been confusing because I need both PRs on my branch in order to do development, even though they are necessarily different features.

I think I've got it sorted now and it should be all set for merging.

Copy link
Owner

@daniel5151 daniel5151 left a comment

Choose a reason for hiding this comment

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

Nice, LGTM!

@daniel5151 daniel5151 merged commit 4627ad7 into daniel5151:master Mar 15, 2023
@xobs xobs deleted the fix-noactivethreads-error branch March 16, 2023 14:32
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 this pull request may close these issues.

NoActiveThreads error when there are no active thread
2 participants