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

[Stan Cox <scox at redhat dot com>]

On 10/11/19 16:10, Tom Tromey wrote:

> I didn't point out formatting nits.  I recommend using "git send-email"
Will do

> 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?

Yes one fd per client.  Good point about the internal fds.  I'll check 
on that.


> 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.

Thanks for the heads up; I'll take a look at that patch series.

> 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.

Ah, good point.

> 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.

If there is a gdb client not catching syscalls and an strace client
catching syscalls then the multiple client packet interpretation got
difficult if the gdb client was doing a 'next', so this turned syscalls
off and after the next turned it back on.   I don't have the
packet details at my fingertips but will do so; perhaps a less
invasive solution is possible.

> 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...

Good idea!  I will do that next.

> 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.

Yes!  I am making the strace gdbserver backend smarter about thread
state and this can all go away.

> 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.

Okay.

> 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.

Aha, will do.

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

Right, this patch assumes the other client is strace.

> 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.

I have a follow-on patch that adds supporting an addition gdb client as 
mentioned above.  Good point about confusion.  I'll look into making 
that smarter.

> 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 have not done anything explicit about this; there is some
interrupt handling on the strace gdbserver backend side.  I'll note to
check this more thoroughly.

> 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, thanks for commenting on the patch.  I'll post another RFC when I
address the above.