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

Add support for qThreadExtraInfo #106

Merged

Conversation

thefaxman
Copy link
Contributor

@thefaxman thefaxman commented Jun 8, 2022

Description

Add a new sub-IDET for MultiThreadBase to respond with extra information about a thread.

Closes #104

API Stability

  • This PR does not require a breaking API change

Checklist

  • Implementation
    • cargo build compiles without errors or warnings
    • cargo clippy runs without errors or warnings
    • cargo fmt was run
    • All tests pass
  • Documentation
    • rustdoc + approprate inline code comments
    • Updated CHANGELOG.md
    • (if appropriate) Added feature to "Debugging Features" in README.md
  • If implementing a new protocol extension IDET
    • Included a basic sample implementation in examples/armv4t
    • Included output of running examples/armv4t with RUST_LOG=trace + any relevant GDB output under the "Validation" section below
    • Confirmed that IDET can be optimized away (using ./scripts/test_dead_code_elim.sh and/or ./example_no_std/check_size.sh)

Validation

GDB output
(gdb) info thread
  Id   Target Id            Frame 
* 1    Thread 1.1 (CPU Cpu) 0x55550000 in ?? ()
  2    Thread 1.2 (CPU Cop) 0x55550000 in ?? ()
(gdb) 
armv4t output
 TRACE gdbstub::protocol::recv_packet     > <-- $qThreadExtraInfo,p1.1#85
 TRACE gdbstub::protocol::response_writer > --> $43505520437075#d2
 TRACE gdbstub::protocol::recv_packet     > <-- $qThreadExtraInfo,p1.2#86
 TRACE gdbstub::protocol::response_writer > --> $43505520436f70#02

@daniel5151 daniel5151 self-requested a review June 8, 2022 03:00
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.

All in all, this is looking good!

CHANGELOG.md Outdated Show resolved Hide resolved
examples/armv4t_multicore/gdb.rs Show resolved Hide resolved
examples/armv4t_multicore/gdb.rs Outdated Show resolved Hide resolved
src/protocol/commands/_qThreadExtraInfo.rs Outdated Show resolved Hide resolved
src/stub/core_impl/thread_extra_info.rs Outdated Show resolved Hide resolved
src/target/ext/thread_extra_info.rs Outdated Show resolved Hide resolved
src/stub/core_impl/thread_extra_info.rs Outdated Show resolved Hide resolved
src/target/ext/thread_extra_info.rs Outdated Show resolved Hide resolved
@thefaxman thefaxman force-pushed the thefaxman/thread_extra_info branch 2 times, most recently from e31f252 to ebaa9de Compare June 8, 2022 04:41
@thefaxman
Copy link
Contributor Author

I think I've addressed all the feedback - let me know if there's any additional items you'd like to see addressed

Copy link
Contributor

@bet4it bet4it left a comment

Choose a reason for hiding this comment

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

Comment on lines 25 to 26
let mut body = body.splitn_mut_no_panic(3, |b| *b == b',');
let id = SpecificThreadId::try_from(ThreadId::try_from(body.next()?).ok()?).ok()?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let mut body = body.splitn_mut_no_panic(3, |b| *b == b',');
let id = SpecificThreadId::try_from(ThreadId::try_from(body.next()?).ok()?).ok()?;
let id = SpecificThreadId::try_from(ThreadId::try_from(body).ok()?).ok()?;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

src/protocol/commands/_qThreadExtraInfo.rs Outdated Show resolved Hide resolved
@thefaxman thefaxman force-pushed the thefaxman/thread_extra_info branch 2 times, most recently from 0dd8086 to c6d81b1 Compare June 8, 2022 05:11
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.

Looks good.

It's a bit late for me here, so I'll give it one last once-over in the morning just in case, but it should be good to go after resolving this last thing.

src/stub/core_impl/thread_extra_info.rs Outdated Show resolved Hide resolved
Add a new sub-IDET for `MultiThreadBase` to respond with extra information about a thread

Signed-off-by: Ryan Fairfax <ryan@thefaxman.net>
@thefaxman thefaxman force-pushed the thefaxman/thread_extra_info branch from c6d81b1 to 2e148ea Compare June 8, 2022 05:23
@daniel5151 daniel5151 merged commit 6fce31a into daniel5151:master Jun 8, 2022
@daniel5151
Copy link
Owner

Thanks again for the contribution!

Once #103 lands, I'll push out 0.6.2 that includes these and other changes.

@daniel5151 daniel5151 mentioned this pull request Jun 8, 2022
11 tasks
@thefaxman thefaxman deleted the thefaxman/thread_extra_info branch June 11, 2022 15:20
@daniel5151
Copy link
Owner

@thefaxman 0.6.2 has been released :)

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.

qThreadExtraInfo support
3 participants