Re: [RFC] [PATCH] Add gdbserver multiclient support for strace

[Tom Tromey <tom at tromey dot com>]

>>>>> "Stan" == Stan Cox <scox@redhat.com> writes:

Stan> Looking for comments, thoughts, and suggestions on this patch
Stan> to add multiclient support for strace.

I finally looked at this patch.  I'm sorry for the delay... in this
particular case I think I'd expected someone else to look at it, and
then I forgot about it for a long time :(

I realize that the patch is still in a pretty early state, so I didn't
try to do a full review.  For example I didn't point out formatting
nits.

The patch seemed to be a little mangled by some MTA.  I recommend using
"git send-email"; I haven't had any problems with that.

Stan> diff --git a/gdb/gdbserver/event-loop.c b/gdb/gdbserver/event-loop.c

Stan> @@ -377,3 +378,7 @@ delete_file_handler (gdb_fildes_t fd)
Stan>      }
Stan> -  free (file_ptr);
Stan> +
Stan> +  /* Do not free in case another client tries to attach */
Stan> +  /* free (file_ptr); */
Stan> +  struct multi_client_states & client_states = get_client_states();
Stan> +  client_states.delete_client_state (fd);

It seems to me that there are two cases here.

One, if there's an fd that's the result of accepting a connection from
some client, then I would assume it can't really be shared.  That is,
gdbserver will have a single fd for each client.  Is that correct?

So then, maybe only internal fds might need this treatment?

If so then I think that is a detail that should be kept near where the
internal fd is used.

The reason I bring this up is that there is another patch series trying
to merge the gdb and gdbserver event loop code; but adding special cases
like this to the event loop would make that harder... and also seems
like a layering violation.

Another case is the listening fd, but the patch also does:

Stan> -  delete_file_handler (listen_desc);
Stan> +  /* Do not delete in case another client tries to attach */
Stan> +  /*  delete_file_handler (listen_desc); */

... which I guess bypasses this additional mechanism anyhow.

Stan> +	      /* Switch client states if we have multiple clients */
Stan> +	      if (have_multiple_clients (file_ptr->fd))
Stan> +		{
Stan> +		  struct multi_client_states & client_states = get_client_states();
Stan> +		  client_states.set_client_state (file_ptr->fd);
Stan> +		}

It seems like this is something that can be handled in the callback that
is called in response to a file descriptor readability event.

Stan> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c

Stan> +  if (proc->syscalls_to_catch[0] == NO_SYSCALL)
Stan> +    return 0;

I didn't really get why this was needed.

Stan> @@ -896,3 +923,3 @@ static unsigned char *readchar_bufp;
Stan>  static int
Stan> -readchar (void)
Stan> +readchar (gdb_fildes_t fd)

If you're looking to break up the patch a bit, a patch to just add the
file descriptor parameter to these lower-level functions would be super
easy to review in isolation...

Stan> @@ -1282,2 +1309,14 @@ prepare_resume_reply (char *buf, ptid_t ptid,
Stan>  		buf += strlen (buf);
Stan> +#if 1
Stan> +		if (non_stop
Stan> +		    && (status->kind == TARGET_WAITKIND_SYSCALL_ENTRY
Stan> +			|| status->kind == TARGET_WAITKIND_SYSCALL_RETURN))
Stan> +		  {
Stan> +		    sprintf (buf, "thread:");
Stan> +		    buf += strlen (buf);
Stan> +		    buf = write_ptid (buf, cs.general_thread);
Stan> +		    strcat (buf, ";");
Stan> +		    buf += strlen (buf);
Stan> +		  }
Stan> +#endif

I guess naively I'd expect this patch to mostly be concerned with
refactoring gdbserver internals, so I was surprised to see a change to a
response in here.

Stan> +	  client_states.current_cs = previous_cs;
Stan> +	  free (cs->own_buf);
Stan> +	  delete (cs);

It's better to make "client_state" responsible for its own cleanup;
i.e. move that "free" into the destructor, or change it to be a
self-managing type.

Stan> +      else if (strncmp (cs.own_buf, "vStopped", 4) == 0)

The length here is wrong.  Using "startswith" for all of these would
avoid that problem.


Looking at this as a whole... first, I feel like I probably don't fully
understand it, so of course you should push back if my critiques don't
make sense.

It seems like this is targeted at exactly supporting gdb in parallel
with strace, and not any combination of clients.  Is that correct?

It seems to me that supporting two gdbs would mean needing to understand
a bit more about breakpoints.  Like, if both gdb clients set a
breakpoint at the same location, the stop would have to be reported to
both -- but the subsequent "step off" requests from each gdb would have
to be merged.

I guess that's fine, but I wonder if we have or need some way to prevent
users from confusing themselves by attempting this.

Also I somewhat expected to see some kind of special handling of
interrupt requests... though maybe this is a mistaken expectation.  I'm
not sure.  How does it work if the user types C-c in gdb?

I was expecting to see "O" packets sent to all listeners but I think I
was just confused because I couldn't find any spot in gdbserver that
actually generates such a packet.

Tom