SingleRegisterAccess: Support unavailable regs #107

ptosi · 2022-07-20T22:40:04Z

Description

Make gdbstub report to the GDB client the state of an unavailable (but valid) register by following the registers packet protocol:

[...] the stub may also return a string of literal xs in place of
the register data digits, to indicate that the corresponding register
has not been collected, thus its value is unavailable.

API Stability

This PR does not require a breaking API change but it would probably benefit from introducing an Option instead of the C-ish approach of giving special meaning to 0

Checklist

Documentation
- Ensured any public-facing rustdoc formatting looks good (via cargo doc)
- (if appropriate) Added feature to "Debugging Features" in README.md
Validation
- Included output of running examples/armv4t with RUST_LOG=trace + any relevant GDB output under the "Validation" section below
- Included output of running ./example_no_std/check_size.sh before/after changes under the "Validation" section below

Validation

GDB output

(gdb) info registers SPSR_EL3
SPSR_EL3       <unavailable>

examples/armv4t

$ RUST_LOG=trace cargo run --example armv4t
   Compiling gdbstub v0.6.2 (/usr/local/google/home/ptosi/src/gdbstub)
    Finished dev [unoptimized + debuginfo] target(s) in 1.76s
     Running `target/debug/examples/armv4t`
loading section ".text" into memory from [0x55550000..0x55550078]
Setting PC to 0x55550000
Waiting for a GDB connection on "127.0.0.1:9001"...

then

(gdb) target remote localhost:9001

makes the GDB client learn about the Unavailable register through the XML:

Debugger connected from 127.0.0.1:33718
 TRACE gdbstub::protocol::recv_packet > <-- +
 TRACE gdbstub::protocol::recv_packet > <-- $qSupported:multiprocess+;swbreak+;hwbreak+;qRelocInsn+;fork-events+;vfork-events+;exec-events+;vContSupported+;QThreadEvents+;no-resumed+;xmlRegisters=i386#6a
 TRACE gdbstub::protocol::response_writer > --> $PacketSize=1000;vContSupported+;multiprocess+;QStartNoAckMode+;ReverseContinue+;ReverseStep+;QDisableRandomization+;QEnvironmentHexEncoded+;QEnvironmentUnset+;QEnvironmentReset+;QStartupWithShell+;QSetWorkingDir+;swbreak+;hwbreak+;QCatchSyscalls+;qXfer:features:read+;qXfer:memory-map:read+;qXfer:exec-file:read+;qXfer:auxv:read+#fa

[...]

 TRACE gdbstub::protocol::recv_packet     > <-- $qXfer:features:read:extra.xml:0,ffb#16
 TRACE gdbstub::protocol::response_writer > --> $m<?xml version="1.0"?>
<!DOCTYPE target SYSTEM "gdb-target.dtd">
<feature name="custom-armv4t-extension">

[...]

    <!--
        pseudo-register that is always unavailable.

        it is supposed to be reported as 'x'-ed bytes in replies to 'p' packets
        and shown by the GDB client as "<unavailable>".
    -->
    <reg name="unavailable" bitsize="32" type="uint32"/>
</feature>#7d

Then requesting

(gdb) info registers unavailable

shows the appropriate request (0x1c = 28) and response (xxxxxxxx = 4 unknown bytes):

 TRACE gdbstub::protocol::recv_packet     > <-- $p1c#04
 TRACE gdbstub::protocol::response_writer > --> $xxxxxxxx#c6

which the GDB client reports:

(gdb) info registers unavailable
unavailable    <unavailable>

Before/After `./example_no_std/check_size.sh` output

Before

File  .text    Size             Crate Name
2.9%  72.7% 10.9KiB         [Unknown] main
0.2%   4.6%    706B           gdbstub gdbstub::stub::state_machine::GdbStubStateMachineInner<gdbstub::stub::state_machine::state::Running,T,C>::report_stop
0.1%   2.0%    310B           gdbstub gdbstub::protocol::commands::breakpoint::BasicBreakpoint::from_slice
0.1%   1.7%    268B           gdbstub gdbstub::protocol::common::hex::decode_hex_buf
0.1%   1.7%    259B           gdbstub <gdbstub::protocol::common::thread_id::ThreadId as core::convert::TryFrom<&[u8]>>::try_from
0.1%   1.6%    253B           gdbstub gdbstub::protocol::response_writer::ResponseWriter<C>::write_specific_thread_id
0.1%   1.6%    249B           gdbstub gdbstub::stub::core_impl::resume::<impl gdbstub::stub::core_impl::GdbStubImpl<T,C>>::write_stop_common
0.1%   1.3%    207B           gdbstub gdbstub::protocol::response_writer::ResponseWriter<C>::write
0.0%   0.8%    130B           gdbstub gdbstub::protocol::response_writer::ResponseWriter<C>::inner_write
0.0%   0.8%    127B           gdbstub gdbstub::protocol::common::hex::decode_hex
0.0%   0.7%    114B           gdbstub gdbstub::protocol::common::hex::decode_hex
0.0%   0.7%    111B           gdbstub gdbstub::protocol::response_writer::ResponseWriter<C>::flush
0.0%   0.7%    104B           gdbstub gdbstub::protocol::response_writer::ResponseWriter<C>::write_num
0.0%   0.7%    103B           gdbstub gdbstub::protocol::response_writer::ResponseWriter<C>::write_num
0.0%   0.7%    102B    gdbstub_nostd? <gdbstub_nostd::gdb::DummyTarget as gdbstub::target::ext::base::multithread::MultiThreadBase>::read_addrs
0.0%   0.6%     96B           gdbstub <gdbstub::protocol::common::thread_id::IdKind as core::convert::TryFrom<&[u8]>>::try_from
0.0%   0.6%     93B         [Unknown] __libc_csu_init
0.0%   0.6%     88B           gdbstub gdbstub::protocol::response_writer::ResponseWriter<C>::write_hex
0.0%   0.5%     79B           gdbstub <usize as gdbstub::internal::be_bytes::BeBytes>::from_be_bytes
0.0%   0.5%     77B           gdbstub <u32 as gdbstub::internal::be_bytes::BeBytes>::from_be_bytes
0.0%   0.4%     65B      gdbstub_arch <gdbstub_arch::arm::reg::arm_core::ArmCoreRegs as gdbstub::arch::Registers>::gdb_deserialize
0.0%   0.4%     65B    gdbstub_nostd? <gdbstub_nostd::gdb::DummyTarget as gdbstub::target::ext::base::multithread::MultiThreadBase>::write_addrs
0.0%   0.4%     65B    gdbstub_nostd? <gdbstub_nostd::gdb::DummyTarget as gdbstub::target::ext::base::multithread::MultiThreadBase>::write_registers
0.0%   0.4%     65B    gdbstub_nostd? <gdbstub_nostd::gdb::DummyTarget as gdbstub::target::ext::base::multithread::MultiThreadBase>::read_registers
0.0%   0.4%     57B compiler_builtins __rust_probestack
0.0%   0.3%     50B    gdbstub_nostd? <gdbstub_nostd::gdb::DummyTarget as gdbstub::target::ext::base::multithread::MultiThreadResume>::set_resume_action_continue
0.0%   0.3%     50B    gdbstub_nostd? <gdbstub_nostd::gdb::DummyTarget as gdbstub::target::ext::base::multithread::MultiThreadResume>::clear_resume_actions
0.0%   0.3%     50B    gdbstub_nostd? <gdbstub_nostd::gdb::DummyTarget as gdbstub::target::ext::base::multithread::MultiThreadResume>::resume
0.0%   0.3%     43B         [Unknown] _start
0.0%   0.0%      6B    gdbstub_nostd? <gdbstub_nostd::gdb::DummyTarget as gdbstub::target::ext::breakpoints::SwBreakpoint>::remove_sw_breakpoint
0.0%   0.0%      6B    gdbstub_nostd? <gdbstub_nostd::gdb::DummyTarget as gdbstub::target::ext::breakpoints::SwBreakpoint>::add_sw_breakpoint
0.0%   0.0%      1B         [Unknown] __libc_csu_fini
0.0%   0.0%      0B                   And 0 smaller methods. Use -n N to show more.
4.1% 100.0% 15.0KiB                   .text section size, the file size is 369.6KiB
target/release/gdbstub-nostd  :
section               size    addr
.interp                 28     792
.note.gnu.property      32     824
.note.gnu.build-id      36     856
.note.ABI-tag           32     892
.gnu.hash               36     928
.dynsym                360     968
.dynstr                193    1328
.gnu.version            30    1522
.gnu.version_r          48    1552
.rela.dyn              408    1600
.init                   23    4096
.plt                    16    4128
.plt.got                 8    4144
.text                15345    4160
.fini                    9   19508
.rodata                914   20480
.eh_frame_hdr          284   21396
.eh_frame             1472   21680
.init_array              8   28072
.fini_array              8   28080
.dynamic               448   28088
.got                   136   28536
.data                    8   28672
.bss                     8   28680
.comment                30       0
Total                19920

After

File  .text    Size             Crate Name
2.9%  72.7% 10.9KiB         [Unknown] main
0.2%   4.6%    706B           gdbstub gdbstub::stub::state_machine::GdbStubStateMachineInner<gdbstub::stub::state_machine::state::Running,T,C>::report_stop
0.1%   2.0%    310B           gdbstub gdbstub::protocol::commands::breakpoint::BasicBreakpoint::from_slice
0.1%   1.7%    268B           gdbstub gdbstub::protocol::common::hex::decode_hex_buf
0.1%   1.7%    259B           gdbstub <gdbstub::protocol::common::thread_id::ThreadId as core::convert::TryFrom<&[u8]>>::try_from
0.1%   1.6%    253B           gdbstub gdbstub::protocol::response_writer::ResponseWriter<C>::write_specific_thread_id
0.1%   1.6%    249B           gdbstub gdbstub::stub::core_impl::resume::<impl gdbstub::stub::core_impl::GdbStubImpl<T,C>>::write_stop_common
0.1%   1.3%    207B           gdbstub gdbstub::protocol::response_writer::ResponseWriter<C>::write
0.0%   0.8%    130B           gdbstub gdbstub::protocol::response_writer::ResponseWriter<C>::inner_write
0.0%   0.8%    127B           gdbstub gdbstub::protocol::common::hex::decode_hex
0.0%   0.7%    114B           gdbstub gdbstub::protocol::common::hex::decode_hex
0.0%   0.7%    111B           gdbstub gdbstub::protocol::response_writer::ResponseWriter<C>::flush
0.0%   0.7%    104B           gdbstub gdbstub::protocol::response_writer::ResponseWriter<C>::write_num
0.0%   0.7%    103B           gdbstub gdbstub::protocol::response_writer::ResponseWriter<C>::write_num
0.0%   0.7%    102B    gdbstub_nostd? <gdbstub_nostd::gdb::DummyTarget as gdbstub::target::ext::base::multithread::MultiThreadBase>::read_addrs
0.0%   0.6%     96B           gdbstub <gdbstub::protocol::common::thread_id::IdKind as core::convert::TryFrom<&[u8]>>::try_from
0.0%   0.6%     93B         [Unknown] __libc_csu_init
0.0%   0.6%     88B           gdbstub gdbstub::protocol::response_writer::ResponseWriter<C>::write_hex
0.0%   0.5%     79B           gdbstub <usize as gdbstub::internal::be_bytes::BeBytes>::from_be_bytes
0.0%   0.5%     77B           gdbstub <u32 as gdbstub::internal::be_bytes::BeBytes>::from_be_bytes
0.0%   0.4%     65B      gdbstub_arch <gdbstub_arch::arm::reg::arm_core::ArmCoreRegs as gdbstub::arch::Registers>::gdb_deserialize
0.0%   0.4%     65B    gdbstub_nostd? <gdbstub_nostd::gdb::DummyTarget as gdbstub::target::ext::base::multithread::MultiThreadBase>::write_addrs
0.0%   0.4%     65B    gdbstub_nostd? <gdbstub_nostd::gdb::DummyTarget as gdbstub::target::ext::base::multithread::MultiThreadBase>::write_registers
0.0%   0.4%     65B    gdbstub_nostd? <gdbstub_nostd::gdb::DummyTarget as gdbstub::target::ext::base::multithread::MultiThreadBase>::read_registers
0.0%   0.4%     57B compiler_builtins __rust_probestack
0.0%   0.3%     50B    gdbstub_nostd? <gdbstub_nostd::gdb::DummyTarget as gdbstub::target::ext::base::multithread::MultiThreadResume>::set_resume_action_continue
0.0%   0.3%     50B    gdbstub_nostd? <gdbstub_nostd::gdb::DummyTarget as gdbstub::target::ext::base::multithread::MultiThreadResume>::clear_resume_actions
0.0%   0.3%     50B    gdbstub_nostd? <gdbstub_nostd::gdb::DummyTarget as gdbstub::target::ext::base::multithread::MultiThreadResume>::resume
0.0%   0.3%     43B         [Unknown] _start
0.0%   0.0%      6B    gdbstub_nostd? <gdbstub_nostd::gdb::DummyTarget as gdbstub::target::ext::breakpoints::SwBreakpoint>::remove_sw_breakpoint
0.0%   0.0%      6B    gdbstub_nostd? <gdbstub_nostd::gdb::DummyTarget as gdbstub::target::ext::breakpoints::SwBreakpoint>::add_sw_breakpoint
0.0%   0.0%      1B         [Unknown] __libc_csu_fini
0.0%   0.0%      0B                   And 0 smaller methods. Use -n N to show more.
4.1% 100.0% 15.0KiB                   .text section size, the file size is 369.6KiB
target/release/gdbstub-nostd  :
section               size    addr
.interp                 28     792
.note.gnu.property      32     824
.note.gnu.build-id      36     856
.note.ABI-tag           32     892
.gnu.hash               36     928
.dynsym                360     968
.dynstr                193    1328
.gnu.version            30    1522
.gnu.version_r          48    1552
.rela.dyn              408    1600
.init                   23    4096
.plt                    16    4128
.plt.got                 8    4144
.text                15345    4160
.fini                    9   19508
.rodata                914   20480
.eh_frame_hdr          284   21396
.eh_frame             1472   21680
.init_array              8   28072
.fini_array              8   28080
.dynamic               448   28088
.got                   136   28536
.data                    8   28672
.bss                     8   28680
.comment                30       0
Total                19920

daniel5151

Don't mind those failing clippy tests - I'll need to go in and fix those up myself prior to pushing out the next minor release.

[...] but it would probably benefit from introducing an Option instead of the C-ish approach of giving special meaning to 0

I completely agree. This approach is fine for now, but once this PR goes in, we should open a tracking issue to change the API in 0.7 to use an Option instead.

Some general comments:

This new functionality needs to be documented. Please update the SingleRegisterAccess::read_register docs accordingly.
Please include the diff of ./example_no_std/check_size.sh as part of your validation
Please update the armv4t example in-tree to include a new register that does not get collected (so this code path has an example + gets tested), and then update the validation section with GDB output from the example + the packet trace log.

fwiw, these are all part of the PR checklist, and are there for a reason. Please don't ignore them, and stick to the PR checklist

ptosi · 2022-07-21T18:26:26Z

Sorry for forgetting about documentation, that's fixed. And thanks for explaining what's expected from the armv4t test, I've added support for an "Unavailable" register and updated this PR description with the GDB/test outputs.

Note that check_size.sh seems broken:

$ example_no_std/check_size.sh
   Compiling gdbstub v0.6.2 (/usr/local/google/home/ptosi/src/gdbstub)
    Finished release [optimized] target(s) in 3.21s
    Finished release [optimized] target(s) in 0.02s
Error: only 'bin', 'dylib' and 'cdylib' crate types are supported.

Should it be fixed?

daniel5151 · 2022-07-21T18:36:00Z

All good 👍

Ahh, yeah, check_size.sh assumes you're running it from inside the example_no_std directory.
I should probably make that more clear.

daniel5151

aside from this one comment, LGTM.

(oh, and it looks like rustfmt is complaining)

src/protocol/response_writer.rs

ptosi · 2022-07-22T07:59:20Z

rustfmt should be happy, now!

daniel5151

Reminder that you'll still need to include the output of example_no_std/check_size.sh in your PR description. It might seem trivial, but one of gdbstub's stated goals is to have as small of a footprint on embedded systems as possible, so keeping an eye on the minimal configuration code size is a necessity in all PRs.

src/stub/core_impl/base.rs

src/protocol/response_writer.rs

Enable an implementation of SingleRegisterAccess to signal to gdbstub that a read register is recognized (as per RegId::from_raw_id()) but is unavailable by reporting a 0-byte read. In that case, make gdbstub report to the GDB client the state of the unavailable register by following the registers packet protocol: > [...] the stub may also return a string of literal `x`s in place of > the register data digits, to indicate that the corresponding register > has not been collected, thus its value is unavailable. Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>

ptosi · 2022-07-28T09:55:13Z

Output of example_no_std/check_size.sh added.

daniel5151

Awesome!

Thanks for putting up with all my back-and-forth, this looks ready to rock 🪨

Let me know if you're in a rush to use this feature, and I can push out a 0.6.3 release at some point in the next week :)

daniel5151 self-requested a review July 21, 2022 17:14

daniel5151 requested changes Jul 21, 2022

View reviewed changes

ptosi force-pushed the unavailable-regs branch from 8346528 to 0910f08 Compare July 21, 2022 18:26

ptosi mentioned this pull request Jul 21, 2022

check_size.sh: Allow running script from other dirs #108

Merged

daniel5151 requested changes Jul 21, 2022

View reviewed changes

src/protocol/response_writer.rs Outdated Show resolved Hide resolved

ptosi force-pushed the unavailable-regs branch from 0910f08 to de46637 Compare July 22, 2022 07:58

daniel5151 requested changes Jul 22, 2022

View reviewed changes

src/stub/core_impl/base.rs Outdated Show resolved Hide resolved

src/protocol/response_writer.rs Outdated Show resolved Hide resolved

ptosi force-pushed the unavailable-regs branch from de46637 to 77fb1c5 Compare July 28, 2022 09:53

ptosi mentioned this pull request Jul 28, 2022

gdbstub_arch: Add support for AArch64 #109

Merged

5 tasks

daniel5151 approved these changes Jul 28, 2022

View reviewed changes

daniel5151 merged commit 1458f21 into daniel5151:master Jul 28, 2022

daniel5151 mentioned this pull request Jul 28, 2022

Use Option in return type for SingleRegisterAccess::read_register #110

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SingleRegisterAccess: Support unavailable regs #107

SingleRegisterAccess: Support unavailable regs #107

ptosi commented Jul 20, 2022 •

edited by daniel5151

daniel5151 left a comment •

edited

ptosi commented Jul 21, 2022

daniel5151 commented Jul 21, 2022

daniel5151 left a comment

ptosi commented Jul 22, 2022

daniel5151 left a comment

ptosi commented Jul 28, 2022

daniel5151 left a comment

Navigation Menu

SingleRegisterAccess: Support unavailable regs #107

SingleRegisterAccess: Support unavailable regs #107

Conversation

ptosi commented Jul 20, 2022 • edited by daniel5151

Description

API Stability

Checklist

Validation

Before

After

daniel5151 left a comment • edited

Choose a reason for hiding this comment

ptosi commented Jul 21, 2022

daniel5151 commented Jul 21, 2022

daniel5151 left a comment

Choose a reason for hiding this comment

ptosi commented Jul 22, 2022

daniel5151 left a comment

Choose a reason for hiding this comment

ptosi commented Jul 28, 2022

daniel5151 left a comment

Choose a reason for hiding this comment

ptosi commented Jul 20, 2022 •

edited by daniel5151

daniel5151 left a comment •

edited