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.

Sign up for GitHub

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

Jump to bottom

Rework vAttach + custom pid reporting #133

Merged
merged 1 commit into from Apr 6, 2023
Merged

Rework vAttach + custom pid reporting #133

merged 1 commit into from Apr 6, 2023

Conversation

daniel5151
Copy link
Owner

@daniel5151 daniel5151 commented Mar 25, 2023 •
edited

Description

Follow up to #129

Spent some time reworking the implementation in order to:

  1. crunch down the binary size of targets uninterested in this functionality.
  2. allow using vAttach on single-threaded targets

There are some "warts" in this implementation, owing to the fact that I'd like to land this as part of a non-breaking 0.6.x release. Namely: the runtime bound that only allows using attach if CurrentActivePid is implemented.

Assuming we're happy with this PR, I'll open a follow-up tracking issue to resolve these API warts in 0.7.x.

API Stability

  • This PR does not require a breaking API change

Checklist

  • Documentation
    • Ensured any public-facing rustdoc formatting looks good (via cargo doc)
  • 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
  • If implementing a new protocol extension IDET
    • Included a basic sample implementation in examples/armv4t
    • IDET can be optimized out (confirmed via ./example_no_std/check_size.sh)

Validation

GDB output 
(gdb) target extended-remote :9001
Remote debugging using :9001
Reading /test.elf from remote target...
warning: File transfers from remote targets can be slow. Use "set sysroot" to access files locally instead.
Reading /test.elf from remote target...
Reading symbols from target:/test.elf...
main () at test.c:1
1       int main() {
(gdb) s
2           int x = 4;
(gdb) attach 7
A program is being debugged already.  Kill it? (y or n) y
Attaching to program: target:/test.elf, process 7
`target:/test.elf' has disappeared; keeping its symbols.
main () at test.c:2
2           int x = 4;
(gdb)
armv4t output 
... snip ...
 TRACE gdbstub::protocol::recv_packet     > <-- $vKill;1#6e
GDB sent a kill request for pid Some(1)
 TRACE gdbstub::protocol::response_writer > --> $OK#9a
 TRACE gdbstub::protocol::recv_packet     > <-- $vAttach;7#3d
GDB attached to a process with PID 7
 TRACE gdbstub::protocol::response_writer > --> $T05thread:p07.01;#0c
 TRACE gdbstub::protocol::recv_packet     > <-- $qC#b4
 TRACE gdbstub::protocol::response_writer > --> $QCp07.01#fa
 TRACE gdbstub::protocol::recv_packet     > <-- $Hgp7.1#b5
 TRACE gdbstub::protocol::response_writer > --> $OK#9a
 TRACE gdbstub::protocol::recv_packet     > <-- $qXfer:features:read:target.xml:0,ffb#79
 TRACE gdbstub::protocol::response_writer > --> $m<?xml version="1.0"?>
<!DOCTYPE target SYSTEM "gdb-target.dtd">
<target version="1.0">
    <architecture>armv4t</architecture>
    <feature name="org.gnu.gdb.arm.core">
        <vector id="padding" type="uint32" count="25"/>

        <reg name="r0" bitsize="32" type="uint32"/>
        <reg name="r1" bitsize="32" type="uint32"/>
        <reg name="r2" bitsize="32" type="uint32"/>
        <reg name="r3" bitsize="32" type="uint32"/>
        <reg name="r4" bitsize="32" type="uint32"/>
        <reg name="r5" bitsize="32" type="uint32"/>
        <reg name="r6" bitsize="32" type="uint32"/>
        <reg name="r7" bitsize="32" type="uint32"/>
        <reg name="r8" bitsize="32" type="uint32"/>
        <reg name="r9" bitsize="32" type="uint32"/>
        <reg name="r10" bitsize="32" type="uint32"/>
        <reg name="r11" bitsize="32" type="uint32"/>
        <reg name="r12" bitsize="32" type="uint32"/>
        <reg name="sp" bitsize="32" type="data_ptr"/>
        <reg name="lr" bitsize="32"/>
        <reg name="pc" bitsize="32" type="code_ptr"/>

        <!--
            For some reason, my version of `gdb-multiarch` doesn't seem to
            respect "regnum", and will not parse this custom target.xml unless I
            manually include the padding bytes in the target description.

            On the bright side, AFAIK, there aren't all that many architectures
            that use padding bytes. Heck, the only reason armv4t uses padding is
            for historical reasons (see comment below).

            Odds are if you're defining your own custom arch, you won't run into
            this issue, since you can just lay out all the registers in the
            correct order.
        -->
        <reg name="padding" type="padding" bitsize="32"/>

        <!-- The CPSR is register 25, rather than register 16, because
        the FPA registers historically were placed between the PC
        and the CPSR in the "g" packet. -->
        <reg name="cpsr" bitsize="32" regnum="25"/>
    </feature>
    <xi:include href="extra.xml"/>
</target>#38
 TRACE gdbstub::protocol::recv_packet     > <-- $qXfer:features:read:target.xml:80d,ffb#15
 TRACE gdbstub::protocol::response_writer > --> $l#6c
 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">
    <!--
        maps to a simple scratch register within the emulator. the GDB
        client can read the register using `p }�custom` and set it using
        `set }�custom=1337`
    -->
    <reg name="custom" bitsize="32" type="uint32"/>

    <!--
        pseudo-register that return the current time when read.

        notably, i've set up the target to NOT send this register as part of
        the regular register list, which means that GDB will fetch/update
        this register via the 'p' and 'P' packets respectively
    -->
    <reg name="time" bitsize="32" type="uint32"/>

    <!--
        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
 TRACE gdbstub::protocol::recv_packet     > <-- $qXfer:features:read:extra.xml:3c5,ffb#b1
 TRACE gdbstub::protocol::response_writer > --> $l#6c
 TRACE gdbstub::protocol::recv_packet     > <-- $g#67
 TRACE gdbstub::protocol::response_writer > --> $0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000fcffff0f00000000e8ffff0f785634120c005555xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx1000000078563412#06
 TRACE gdbstub::protocol::recv_packet     > <-- $qOffsets#4b
 TRACE gdbstub::protocol::response_writer > --> $Text=00;Data=00;Bss=00#94
 TRACE gdbstub::protocol::recv_packet     > <-- $qXfer:auxv:read::0,ffb#d8
 TRACE gdbstub::protocol::response_writer > --> $m#bb
 TRACE gdbstub::protocol::recv_packet     > <-- $qXfer:auxv:read::8,ffb#e0
 TRACE gdbstub::protocol::response_writer > --> $l#6c
 TRACE gdbstub::protocol::recv_packet     > <-- $qfThreadInfo#bb
 TRACE gdbstub::protocol::response_writer > --> $mp07.01#d3
 TRACE gdbstub::protocol::recv_packet     > <-- $qsThreadInfo#c8
 TRACE gdbstub::protocol::response_writer > --> $l#6c
 TRACE gdbstub::protocol::recv_packet     > <-- $qXfer:memory-map:read::0,ffb#18
 TRACE gdbstub::protocol::response_writer > --> $m<?xml version="1.0"?>
<!DOCTYPE memory-map
    PUBLIC "+//IDN gnu.org//DTD GDB Memory Map V1.0//EN"
            "http://sourceware.org/gdb/gdb-memory-map.dtd">
<memory-map>
    <memory type="ram" start="0x0" length="0x100000000"/>
</memory-map>#76
 TRACE gdbstub::protocol::recv_packet     > <-- $qXfer:memory-map:read::f4,ffb#82
 TRACE gdbstub::protocol::response_writer > --> $l#6c
 TRACE gdbstub::protocol::recv_packet     > <-- $m5555000c,4#94
 TRACE gdbstub::protocol::response_writer > --> $0430a0e3#f0
 TRACE gdbstub::protocol::recv_packet     > <-- $m55550008,4#69
 TRACE gdbstub::protocol::response_writer > --> $14d04de2#28
 TRACE gdbstub::protocol::recv_packet     > <-- $m5555000c,4#94
 TRACE gdbstub::protocol::response_writer > --> $0430a0e3#f0
 TRACE gdbstub::protocol::recv_packet     > <-- $m55550008,4#69
 TRACE gdbstub::protocol::response_writer > --> $14d04de2#28
 TRACE gdbstub::protocol::recv_packet     > <-- $m5555000c,2#92
 TRACE gdbstub::protocol::response_writer > --> $0430#c7
 TRACE gdbstub::protocol::recv_packet     > <-- $m5555000a,2#90
 TRACE gdbstub::protocol::response_writer > --> $4de2#2f
 TRACE gdbstub::protocol::recv_packet     > <-- $m55550008,2#67
 TRACE gdbstub::protocol::response_writer > --> $14d0#f9
 TRACE gdbstub::protocol::recv_packet     > <-- $m5555000c,2#92
 TRACE gdbstub::protocol::response_writer > --> $0430#c7
 TRACE gdbstub::protocol::recv_packet     > <-- $m5555000a,2#90
 TRACE gdbstub::protocol::response_writer > --> $4de2#2f
 TRACE gdbstub::protocol::recv_packet     > <-- $m55550008,2#67
 TRACE gdbstub::protocol::response_writer > --> $14d0#f9
 TRACE gdbstub::protocol::recv_packet     > <-- $m5555000c,4#94
 TRACE gdbstub::protocol::response_writer > --> $0430a0e3#f0
 TRACE gdbstub::protocol::recv_packet     > <-- $m55550008,4#69
 TRACE gdbstub::protocol::response_writer > --> $14d04de2#28
 TRACE gdbstub::protocol::recv_packet     > <-- $m5555000c,4#94
 TRACE gdbstub::protocol::response_writer > --> $0430a0e3#f0
 TRACE gdbstub::protocol::recv_packet     > <-- $m55550008,4#69
 TRACE gdbstub::protocol::response_writer > --> $14d04de2#28
 TRACE gdbstub::protocol::recv_packet     > <-- $m5555000c,4#94
 TRACE gdbstub::protocol::response_writer > --> $0430a0e3#f0
 TRACE gdbstub::protocol::recv_packet     > <-- $m5555000c,4#94
 TRACE gdbstub::protocol::response_writer > --> $0430a0e3#f0
 TRACE gdbstub::protocol::recv_packet     > <-- $m5555000c,4#94
 TRACE gdbstub::protocol::response_writer > --> $0430a0e3#f0
Before/After `./example_no_std/check_size.sh` output

Before #129

target/release/gdbstub-nostd  :
section               size    addr
.interp                 28     680
.note.gnu.build-id      36     708
.note.ABI-tag           32     744
.gnu.hash               36     776
.dynsym                360     816
.dynstr                193    1176
.gnu.version            30    1370
.gnu.version_r          48    1400
.rela.dyn              408    1448
.init                   27    4096
.plt                    16    4128
.plt.got                 8    4144
.text                14853    4160
.fini                   13   19016
.rodata                946   20480
.eh_frame_hdr          276   21428
.eh_frame             1408   21704
.init_array              8   28072
.fini_array              8   28080
.dynamic               448   28088
.got                   136   28536
.data                    8   28672
.bss                     8   28680
.comment                43       0
Total                19377

After #129

target/release/gdbstub-nostd  :
section               size    addr
.interp                 28     680
.note.gnu.build-id      36     708
.note.ABI-tag           32     744
.gnu.hash               36     776
.dynsym                360     816
.dynstr                193    1176
.gnu.version            30    1370
.gnu.version_r          48    1400
.rela.dyn              408    1448
.init                   27    4096
.plt                    16    4128
.plt.got                 8    4144
.text                15429    4160
.fini                   13   19592
.rodata                954   20480
.eh_frame_hdr          276   21436
.eh_frame             1408   21712
.init_array              8   28072
.fini_array              8   28080
.dynamic               448   28088
.got                   136   28536
.data                    8   28672
.bss                     8   28680
.comment                43       0
Total                19961

This PR

target/release/gdbstub-nostd  :
section               size    addr
.interp                 28     680
.note.gnu.build-id      36     708
.note.ABI-tag           32     744
.gnu.hash               36     776
.dynsym                360     816
.dynstr                193    1176
.gnu.version            30    1370
.gnu.version_r          48    1400
.rela.dyn              408    1448
.init                   27    4096
.plt                    16    4128
.plt.got                 8    4144
.text                14853    4160
.fini                   13   19016
.rodata                946   20480
.eh_frame_hdr          276   21428
.eh_frame             1408   21704
.init_array              8   28072
.fini_array              8   28080
.dynamic               448   28088
.got                   136   28536
.data                    8   28672
.bss                     8   28680
.comment                43       0
Total                19377

@daniel5151
Copy link
Owner Author

@xobs after finding some time to play around with vAttach in earnest, I ended up making some changes to the implementation from #129.

Can you please try out this PR and let me know that it works as intended? And of course - feel free to leave any review comments on it.

@xobs
Copy link
Contributor

xobs commented Mar 26, 2023

I've implemented this, and it does still work! Though as a minor nit, there's a pointy thing for users to watch out for -- forgetting to implement support_current_active_pid() results in a GDB assertion error due to qC not being implemented:

(gdb) att 2
Attaching to program: E:\Code\Xous\Core\target\riscv32imac-unknown-xous-elf\release\xous-ticktimer, process 2
[remote] Sending packet: $vAttach;2#38
[remote] getpkt_or_notif_sane_1: Timed out.
[remote] getpkt_or_notif_sane_1: Timed out.
[remote] getpkt_or_notif_sane_1: Timed out.
Ignoring packet error, continuing...
[remote] packet_ok: Packet vAttach (attach) is supported
[remote] Sending packet: $qC#b4
[remote] Packet received:
/__w/riscv-none-elf-gcc-xpack/riscv-none-elf-gcc-xpack/build/win32-x64/sources/gdb-12.1/gdb/thread.c:85: internal-error: inferior_thread: Assertion `current_thread_ != nullptr' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) y

This is a bug, please report it.  For instructions, see:
<https://www.gnu.org/software/gdb/bugs/>.

/__w/riscv-none-elf-gcc-xpack/riscv-none-elf-gcc-xpack/build/win32-x64/sources/gdb-12.1/gdb/thread.c:85: internal-error: inferior_thread: Assertion `current_thread_ != nullptr' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Create a core file of GDB? (y or n) n

@xobs
Copy link
Contributor

xobs commented Mar 26, 2023

Happy to see the removal of the comment about it being an untested code path.

Once the Renode changes land, I'll finish up the kernel stuff and then share instructions on how to use gdbstub with the emulator. It should also work with real hardware at that point.

@daniel5151
Copy link
Owner Author

Are you sure that error is from qC not being implemented, or that the stub has crashed with a MissingCurrentActivePidImpl error?

Because the code should throw that error if you try and use vAttach without having implemented the CurrentActivePid IDET...

@xobs
Copy link
Contributor

xobs commented Mar 27, 2023

The stub didn't crash, the issue definitely came from within gdb. It was similar to what I experienced when I didn't have qC implemented before.

@daniel5151
Copy link
Owner Author

Are you certain of that? Because those

[remote] getpkt_or_notif_sane_1: Timed out.
[remote] getpkt_or_notif_sane_1: Timed out.
[remote] getpkt_or_notif_sane_1: Timed out.
Ignoring packet error, continuing...

messages make me think that the stub died...

Any chance you could try and repro using the in-tree armv4t example? i.e: comment out the support_current_active_pid() implementation, and see if it still happens.

@xobs
Copy link
Contributor

xobs commented Mar 27, 2023

I won't be able to test it for another week -- I'm attending Latchup in Santa Barbara this weekend.

@daniel5151
Copy link
Owner Author

Sure, no rush.

Also, when you get the chance: can you let me know what GDB client version you're using?

@xobs
Copy link
Contributor

xobs commented Apr 5, 2023

I had a chance to test it, and you're right in that it's caused by the error:

(base) user@Ondo:/opt/Xous/gdbstub/examples/armv4t$ cargo run --example armv4t
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `/opt/Xous/gdbstub/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"...
Debugger connected from 127.0.0.1:52558
GDB queried if it was attached to a process with PID 1
GDB sent a kill request for pid Some(1)
gdbstub encountered a fatal error: GDB client attempted to attach to a new process, but the target has not implemented support for `ExtendedMode::support_current_active_pid`
Program completed. Return value: 0

This wasn't caught in my system because unlike armv4t, my connection goes over a serial port which does not have any concept of "Remote connection closed". Thus, when it panics like that, it immediately restarts and is in a bad state when gdb sends $vAttach;2#38 and the stub isn't expecting it.

Ordinarily I'm testing riscv stuff with https://github.com/xpack-dev-tools/riscv-none-elf-gcc-xpack/ but for this I'm using gdb-multiarch from Ubuntu 22.04 (version 12.1).

@daniel5151
Copy link
Owner Author

daniel5151 commented Apr 6, 2023 •
edited

Yeah, that lines up with what I expected.

gdbstub explicitly works really hard to avoid introducing any panic paths to avoid these sorts of situations where things suddenly die without explanation.

I suggest that you make sure you're properly logging gdbstub errors before panicking + restarting the system.

In any case though, with that out of the way - does this code look good to you? Any other issues, or can I merge it in?

@xobs
Copy link
Contributor

xobs commented Apr 6, 2023

It looks good to me. Would you mind merging it and bumping the revision so I can pull it into the kernel?

@daniel5151
Copy link
Owner Author

Sounds good. I'll go ahead and do that now (and hopefully push out a revision at some point in the next ~1h or so)

@daniel5151 daniel5151 merged commit 30a4695 into master Apr 6, 2023
4 checks passed
@daniel5151 daniel5151 deleted the feature/improve-vattach branch April 6, 2023 01:11
@daniel5151
Copy link
Owner Author

gdbstub 0.6.5 has been published to crates.io

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@daniel5151 @xobs