Discussion:
gdbstub initial code, v3
Oleg Nesterov
2010-08-11 23:58:10 UTC
Permalink
Please see the attached ugdb.c.

It supports multiple inferiors/threads, stop/cont, clone/exit.
It doesn't report W/X when the process exits yet, but this is
only because I'd like to discuss the current problems with the
exited leader first, then implement this as a separate change.

Any code review is very much appreciated.

Problems:

- It doesn't support all-stop mode.

Please tell me if this is needed. I hope not, this needs
a lot of nasty complications :/

- We should discuss utrace limitations: UTRACE_RESUME
races/problems, utrace_prepare_examine() issues.

- I just finished this code, and it doesn't work with gdb ;)

I will investigate tomorrow, but I am almost sure gdb is
wrong.

Oleg.
Oleg Nesterov
2010-08-12 01:11:13 UTC
Permalink
Post by Oleg Nesterov
- It doesn't support all-stop mode.
Please tell me if this is needed. I hope not, this needs
a lot of nasty complications :/
- We should discuss utrace limitations: UTRACE_RESUME
races/problems, utrace_prepare_examine() issues.
- I just finished this code, and it doesn't work with gdb ;)
Noticed the typo in ugdb.c, it needs this one-liner fix:

--- x/ugdb.c
+++ x/ugdb.c
@@ -603,7 +603,7 @@ static int ugdb_cont_thread(struct ugdb_
ugdb_ck_stopped(ugdb);

// XXX: gdb shouldn't explicitly cont an unreported thread
- WARN_ON(!all && !(thread->t_stop_state == T_STOP_STOPPED));
+ WARN_ON(!all && !(thread->t_stop_state & T_STOP_STOPPED));

if (thread->t_stop_state == T_STOP_RUN)
return 0;

I attached the patched ugdb.c.
Post by Oleg Nesterov
I will investigate tomorrow, but I am almost sure gdb is
wrong.
Yes, this seems to be true... Tomorrow.

Oleg.
Oleg Nesterov
2010-08-12 02:37:50 UTC
Permalink
Post by Oleg Nesterov
Post by Oleg Nesterov
I will investigate tomorrow, but I am almost sure gdb is
wrong.
Yes, this seems to be true... Tomorrow.
Can't sleep because of this problem ;)

So, the patch below fixes the problem, and gdb + /proc/ugdb seems
to work.

Indeed, gdb sees that this fd is not pipe/tcp and uses the "hardwire"
serial_ops, but hardwire_readchar() doesn't play well with select().

Please teach gdb to use poll/select ?

Oleg.

--- gdb-7.1/gdb/ser-unix.c
+++ gdb-7.1/gdb/ser-unix.c
@@ -452,7 +452,7 @@ hardwire_raw (struct serial *scb)
static int
wait_for (struct serial *scb, int timeout)
{
-#ifdef HAVE_SGTTY
+#if 1
while (1)
{
struct timeval tv;
@@ -474,13 +474,14 @@ wait_for (struct serial *scb, int timeou
else
numfds = gdb_select (scb->fd + 1, &readfds, 0, 0, 0);

- if (numfds <= 0)
+ if (numfds <= 0) {
if (numfds == 0)
return SERIAL_TIMEOUT;
else if (errno == EINTR)
continue;
else
return SERIAL_ERROR; /* Got an error from select or poll */
+ }

return 0;
}
Tom Tromey
2010-08-12 04:37:37 UTC
Permalink
Oleg> So, the patch below fixes the problem, and gdb + /proc/ugdb seems
Oleg> to work.

Oleg> Indeed, gdb sees that this fd is not pipe/tcp and uses the "hardwire"
Oleg> serial_ops, but hardwire_readchar() doesn't play well with select().

Oleg> Please teach gdb to use poll/select ?

I looked at this a little bit. It seems to me that the "hardwire" stuff
is for talking to ttys, and we instead want gdb to be using the pipe code.
Maybe a little hack to stat the open_name in the hardwire case (in
serial_open) would be appropriate.

Tom
Oleg Nesterov
2010-08-12 23:52:28 UTC
Permalink
Post by Tom Tromey
Oleg> So, the patch below fixes the problem, and gdb + /proc/ugdb seems
Oleg> to work.
Oleg> Indeed, gdb sees that this fd is not pipe/tcp and uses the "hardwire"
Oleg> serial_ops, but hardwire_readchar() doesn't play well with select().
Oleg> Please teach gdb to use poll/select ?
I looked at this a little bit. It seems to me that the "hardwire" stuff
is for talking to ttys, and we instead want gdb to be using the pipe code.
I didn't verify this, but I don't think so. Please look at pipe_open().
Perhaps it makes sense to serial_add_interface("ugdb"), I dunno.


OK. I was going to add some cleanups and send the new version today.
But after some testing (without gdb) I hit the kernel crashes. This
makes me think that probably something is wrong ;)

As usual, I can't blame my code. I am still investigating, the crash
is not easy to reproduce, but so far I _suspect_ the problems in utrace
code. At least utrace_barrier()->signal_pending() is definitely not
right.

Will continue tomorrow.

Oleg.
Tom Tromey
2010-08-13 01:44:59 UTC
Permalink
Tom> I looked at this a little bit. It seems to me that the "hardwire" stuff
Tom> is for talking to ttys, and we instead want gdb to be using the pipe code.

Oleg> I didn't verify this, but I don't think so. Please look at pipe_open().

Yeah, I wasn't clear enough. I meant a hybrid with most code coming
from ser-pipe.c.

Oleg> Perhaps it makes sense to serial_add_interface("ugdb"), I dunno.

I started a quick implementation of my idea, I'll finish it tomorrow.
I think we should start an archer branch to hold whatever gdb hacks we
need... I will do that tomorrow as well.

Tom
Roland McGrath
2010-08-13 01:58:44 UTC
Permalink
I don't really know the gdb code, but I'm surprised it really has multiple
different "serial" backends. I would have thought that after opening the
fd, you would treat them all about the same. If tcsetattr et al work on
it, then you set the baud rate and whatever, if they say ENOTTY then fine.
It might make sense to default to a short read timeout for an actual serial
port (or UDP port), which might be unplugged or the other end dead, or
whatever; and to an infinite timeout for a non-tty, which should more
presumably have its fd shut down and read EOF or EPIPE or whatever when the
stub goes away, and otherwise perhaps the stub is just taking that long.
But aside from that, I don't know why you wouldn't treat all kinds of
remote protocol fd's the same wrt select/poll and blocking/nonblocking
reads/writes, be they serial-port fds, "pipe" socketpair sockets, network
sockets, or fds to a new magic file that pretends to be a tty or a socket
or whatever (or even a disk file of canned response playback!).


Thanks,
Roland
Tom Tromey
2010-08-13 04:37:18 UTC
Permalink
Roland> I don't really know the gdb code, but I'm surprised it really
Roland> has multiple different "serial" backends.

I don't know this area well, but considering that ser-unix.c is just
chock full of tty-related goo, I think it is probably important for
something. My impression is that this API is not just used for target
communication but also for manipulating gdb's own terminal.

I looked closer and aside from open/close, ser-pipe is just falling back
to some generic code.

I've appended the patch I came up with. I have not tried it at all, but
it should all be pretty obvious.

Maybe I'd do it a little differently if this were going upstream.
That doesn't seem necessary though.

Tom

diff --git a/gdb/ser-unix.c b/gdb/ser-unix.c
index 266453c..672b2a8 100644
--- a/gdb/ser-unix.c
+++ b/gdb/ser-unix.c
@@ -32,6 +32,7 @@
#include "gdb_select.h"
#include "gdb_string.h"
#include "gdbcmd.h"
+#include "gdb_stat.h"

#ifdef HAVE_TERMIOS

@@ -103,15 +104,56 @@ static int hardwire_setstopbits (struct serial *, int);

void _initialize_ser_hardwire (void);

+static struct serial_ops *
+get_pipe_like_ops (void)
+{
+ static struct serial_ops *ops;
+
+ if (ops == NULL)
+ {
+ ops = XMALLOC (struct serial_ops);
+ memset (ops, 0, sizeof (struct serial_ops));
+ ops->name = NULL;
+ ops->open = NULL;
+ ops->close = hardwire_close;
+ ops->readchar = ser_base_readchar;
+ ops->write = ser_base_write;
+ ops->flush_output = ser_base_flush_output;
+ ops->flush_input = ser_base_flush_input;
+ ops->send_break = ser_base_send_break;
+ ops->go_raw = ser_base_raw;
+ ops->get_tty_state = ser_base_get_tty_state;
+ ops->set_tty_state = ser_base_set_tty_state;
+ ops->print_tty_state = ser_base_print_tty_state;
+ ops->noflush_set_tty_state = ser_base_noflush_set_tty_state;
+ ops->setbaudrate = ser_base_setbaudrate;
+ ops->setstopbits = ser_base_setstopbits;
+ ops->drain_output = ser_base_drain_output;
+ ops->async = ser_base_async;
+ ops->read_prim = ser_unix_read_prim;
+ ops->write_prim = ser_unix_write_prim;
+ }
+
+ return ops;
+}
+
/* Open up a real live device for serial I/O */

static int
hardwire_open (struct serial *scb, const char *name)
{
+ struct stat buf;
+
scb->fd = open (name, O_RDWR);
if (scb->fd < 0)
return -1;

+ if (fstat (scb->fd, &buf) == 0 && S_ISFIFO (buf.st_mode))
+ {
+ /* Super hack! */
+ scb->ops = get_pipe_like_ops ();
+ }
+
return 0;
}
Roland McGrath
2010-08-13 08:29:49 UTC
Permalink
Post by Tom Tromey
I don't know this area well, but considering that ser-unix.c is just
chock full of tty-related goo, I think it is probably important for
something. My impression is that this API is not just used for target
communication but also for manipulating gdb's own terminal.
Ah, I see.
Post by Tom Tromey
I've appended the patch I came up with. I have not tried it at all, but
it should all be pretty obvious.
Yeah, that seems like it should be reasonable, i.e. more or less like what
I'd figured it would do if it didn't have all these different backends.

However, I think the test you probably want is !S_ISCHR.
I believe that /proc/ugdb as is stats as S_ISREG, not S_ISFIFO.
Actually, better yet, just make it !isatty (fd).
Another pseudodevice that also behaves more like a socket than
like a tty might be S_ISCHR, but only a tty isatty.


Thanks,
Roland
Tom Tromey
2010-08-13 16:31:44 UTC
Permalink
Roland> However, I think the test you probably want is !S_ISCHR.

Thanks. I wasn't sure.

Roland> Actually, better yet, just make it !isatty (fd).

Ok.

I made a new branch, 'archer-ugdb'.
This patch is there. (Actually 2 patches due to me not reading
carefully enough the first time -- but whatever.)

Oleg, use this branch and give the feature a try.
We can push whatever fixes or hacks you need to this branch.
I can also merge from gdb's master as needed.

Tom
Oleg Nesterov
2010-08-13 21:12:24 UTC
Permalink
Post by Tom Tromey
Roland> However, I think the test you probably want is !S_ISCHR.
Thanks. I wasn't sure.
Roland> Actually, better yet, just make it !isatty (fd).
Ok.
Good. IIUC, this also allows to remove the ugly "return 0
to pretend TCGETS/TCSETS/TCFLSH works" from ->ioctl().
Post by Tom Tromey
I made a new branch, 'archer-ugdb'.
I assume, you mean git://sourceware.org/git/gdb.git ?
Post by Tom Tromey
This patch is there. (Actually 2 patches due to me not reading
carefully enough the first time -- but whatever.)
Oleg, use this branch and give the feature a try.
Yes, once I resolve the current problems I'll test it.

Thanks!

Oleg.
Tom Tromey
2010-08-13 21:18:17 UTC
Permalink
Tom> I made a new branch, 'archer-ugdb'.

Oleg> I assume, you mean git://sourceware.org/git/gdb.git ?

git://sourceware.org/git/archer.git

I can give you write access to this repository if you want.
Just let me know.

Tom
Daniel Jacobowitz
2010-08-13 14:08:39 UTC
Permalink
Post by Tom Tromey
Roland> I don't really know the gdb code, but I'm surprised it really
Roland> has multiple different "serial" backends.
I don't know this area well, but considering that ser-unix.c is just
chock full of tty-related goo, I think it is probably important for
something. My impression is that this API is not just used for target
communication but also for manipulating gdb's own terminal.
Correct. Also, the abstraction layer is important on non-Unix
systems; "everything is a file descriptor" works poorly on Windows :-)
--
Daniel Jacobowitz
CodeSourcery
Oleg Nesterov
2010-08-13 20:53:19 UTC
Permalink
Post by Oleg Nesterov
As usual, I can't blame my code. I am still investigating, the crash
is not easy to reproduce, but so far I _suspect_ the problems in utrace
code. At least utrace_barrier()->signal_pending() is definitely not
right.
I seem to understand the problem(s). I am a bit surprized. Basically
I have the questions about almost every utrace_ function. I'll try
to recheck and summarize my concerns tomorrow with a fresh head, I
hope I missed something.

And I think at least we should cleanup utrace_set_events(), I'll try
to send the patch a bit later.

Oleg.
Roland McGrath
2010-08-13 21:31:09 UTC
Permalink
Post by Oleg Nesterov
I seem to understand the problem(s). I am a bit surprized. Basically
I have the questions about almost every utrace_ function. I'll try
to recheck and summarize my concerns tomorrow with a fresh head, I
hope I missed something.
Ok. That stuff about pure kernel implementation issues doesn't need
to go to the archer list and Tom, only to utrace-devel.


Thanks,
Roland

Roland McGrath
2010-08-13 01:02:08 UTC
Permalink
Post by Oleg Nesterov
Indeed, gdb sees that this fd is not pipe/tcp and uses the "hardwire"
serial_ops, but hardwire_readchar() doesn't play well with select().
Please teach gdb to use poll/select ?
If it makes it easier, could use:

bash$ nc -l -U /tmp/socket <> /proc/ugdb &
(gdb) target remote |nc -U /tmp/socket

for the moment. Silly of course, but just not to be blocked on cleaning up
gdb's serial-device handling to be more nicely nonblocking.


Thanks,
Roland
Kevin Buettner
2010-08-12 14:52:11 UTC
Permalink
On Thu, 12 Aug 2010 03:11:13 +0200
Post by Oleg Nesterov
Post by Oleg Nesterov
- It doesn't support all-stop mode.
Please tell me if this is needed. I hope not, this needs
a lot of nasty complications :/
I'm pretty sure this will be needed since it's GDB's default mode
of operation.
Post by Oleg Nesterov
I attached the patched ugdb.c.
I've skimmed the code. If I'm not mistaken, support for the 's', 'G',
and 'M' commands isn't implemented yet? (That's single-step, write
registers, and write memory, all of which must be supported by a stub.)

Kevin
Roland McGrath
2010-08-13 00:53:38 UTC
Permalink
Post by Kevin Buettner
Post by Oleg Nesterov
- It doesn't support all-stop mode.
Please tell me if this is needed. I hope not, this needs
a lot of nasty complications :/
I'm pretty sure this will be needed since it's GDB's default mode
of operation.
That is indeed most likely the mode in which the gdb side of things has had
the most testing. So starting with non-stop may open more gdb pain while
avoiding some pain in the kernel module.
Post by Kevin Buettner
I've skimmed the code. If I'm not mistaken, support for the 's', 'G',
and 'M' commands isn't implemented yet? (That's single-step, write
registers, and write memory, all of which must be supported by a stub.)
Yes, that's fine. For a starting place, having only reading support is OK.
These bits are relatively trivial to fill in later as needed, so I think
made plenty of sense for Oleg not to include them yet.


Thanks,
Roland
Kevin Buettner
2010-08-13 03:30:53 UTC
Permalink
On Thu, 12 Aug 2010 17:53:38 -0700 (PDT)
Post by Roland McGrath
Post by Kevin Buettner
I've skimmed the code. If I'm not mistaken, support for the 's', 'G',
and 'M' commands isn't implemented yet? (That's single-step, write
registers, and write memory, all of which must be supported by a stub.)
Yes, that's fine. For a starting place, having only reading support is OK.
These bits are relatively trivial to fill in later as needed, so I think
made plenty of sense for Oleg not to include them yet.
It certainly makes sense for Oleg to get communication between the
stub and gdb sorted out first. As another early step, it also makes
sense to attempt to stop the process and determine its status.

Once that's sorted though, I should think that memory write and
single-step will become important to implement. The 'M' (memory
write) packet may be used by GDB for inserting and removing
breakpoints. (Or, alternately, Z0/z0 packets may be used. I'd
rather see 'M' implemented first though.) Single-step, whether
implemented as an 's' packet or 'vCont;s' packet, will be needed in
order to continue from a breakpoint as well as for single-stepping.

Implementation of the 'G' command (register write) is probably the
least important of the three that I mentioned earlier.

Kevin
fche-H+wXaHxf7aLQT0dZR+ (Frank Ch. Eigler)
2010-08-12 15:06:39 UTC
Permalink
Post by Oleg Nesterov
[...]
- It doesn't support all-stop mode.
Please tell me if this is needed. I hope not, this needs
a lot of nasty complications :/
[...]
As opposed to non-stop mode? I'm pretty sure all-stop will be needed
as that is the common & default gdb usage model.


- FChE
Loading...