Skip to content

Commit

Permalink
gdbserial: fixes for rr 5.7.0 (#3718)
Browse files Browse the repository at this point in the history
The following fixes have been applied to make delve work with rr 5.7.0

- added a new launch prefix to exclude from stderr output
- allow the thread selection packet to be sent for 'c' commands even
  when the stub supports thread suffixes, because the specification is
  unclear over what should be done with bc and bs packets with thread
  suffixes.
- changed the way qRRCmd are escaped and added a thread selector to
  them to match changes to rr codebase
  • Loading branch information
aarzilli authored May 16, 2024
1 parent 2331fa8 commit 7c7265f
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 7 deletions.
15 changes: 12 additions & 3 deletions pkg/proc/gdbserial/gdbserver_conn.go
Original file line number Diff line number Diff line change
Expand Up @@ -985,7 +985,10 @@ func (conn *gdbConn) queryThreads(first bool) (threads []string, err error) {
}

func (conn *gdbConn) selectThread(kind byte, threadID string, context string) error {
if conn.threadSuffixSupported {
if conn.threadSuffixSupported && kind != 'c' {
// kind == 'c' is still allowed because 'rr' is weird about it's support
// for thread suffixes and the specification for it doesn't really say how
// it should be used with packets 'bc' and 'bs'.

This comment has been minimized.

Copy link
@rocallahan

rocallahan May 17, 2024

I don't quite understand the issue here. What's weird about rr's support for thread suffixes?

This comment has been minimized.

Copy link
@aarzilli

aarzilli May 17, 2024

Author Member

I was expecting bc/bs to also take the thread suffix but they don't (although the specification doesn't say anything about that so it would probably be more correct here to say that it is the specification that it's weird, but we knew that).

This comment has been minimized.

Copy link
@rocallahan

rocallahan May 17, 2024

gdb doesn't use thread suffixes and lldb doesn't use bs/bc so there's no prior art here that I know of. If it's easier for you or makes more sense to you for rr to take thread suffixes for bs/bc, I'm happy to do that.

This comment has been minimized.

Copy link
@aarzilli

aarzilli May 17, 2024

Author Member

No, let's not go and invent more things.

panic("selectThread when thread suffix is supported")
}
conn.outbuf.Reset()
Expand Down Expand Up @@ -1189,9 +1192,15 @@ func (conn *gdbConn) qRRCmd(args ...string) (string, error) {
}
conn.outbuf.Reset()
fmt.Fprint(&conn.outbuf, "$qRRCmd")
for _, arg := range args {
for i, arg := range args {
fmt.Fprint(&conn.outbuf, ":")
writeAsciiBytes(&conn.outbuf, []byte(arg))
if i == 0 && conn.threadSuffixSupported {
// newer versions of RR require the command to be followed by a thread id
// and the command name to be unescaped.
fmt.Fprintf(&conn.outbuf, "%s:-1", arg)

This comment has been minimized.

Copy link
@rocallahan

rocallahan May 17, 2024

Sorry about breaking you. I had no idea anyone was sending their own qRRCmds... In the future, if we break something of yours, let me know :-)

This comment has been minimized.

Copy link
@aarzilli

aarzilli May 17, 2024

Author Member

Don't worry about it.

} else {
writeAsciiBytes(&conn.outbuf, []byte(arg))
}
}
resp, err := conn.exec(conn.outbuf.Bytes(), "qRRCmd")
if err != nil {
Expand Down
9 changes: 5 additions & 4 deletions pkg/proc/gdbserial/rr.go
Original file line number Diff line number Diff line change
Expand Up @@ -217,9 +217,10 @@ type rrInit struct {

const (
rrGdbCommandLegacyPrefix = " gdb "
rrGdbCommandPrefix = " 'gdb' "
rrGdbLaunchPrefix = "Launch gdb with"
targetCmd = "target extended-remote "
rrGdbCommandPrefix = " 'gdb' "
rrGdbLaunchLegacyPrefix = "Launch gdb with"
rrGdbLaunchPrefix = "Launch debugger with"
targetCmd = "target extended-remote "
)

func rrStderrParser(stderr io.ReadCloser, initch chan<- rrInit, quiet bool) {
Expand All @@ -245,7 +246,7 @@ func rrStderrParser(stderr io.ReadCloser, initch chan<- rrInit, quiet bool) {
break
}

if strings.HasPrefix(line, rrGdbLaunchPrefix) {
if strings.HasPrefix(line, rrGdbLaunchPrefix) || strings.HasPrefix(line, rrGdbLaunchLegacyPrefix) {
continue
}

Expand Down

0 comments on commit 7c7265f

Please sign in to comment.