Re: [PATCH 1/1] gdb, gdbserver: replace PBUFSIZ with a target op

[Pedro Alves <pedro@palves.net>]

Hi!

I am not sure who you are replying to.  It would be great if you please avoided top posting.

On 2023-09-21 07:02, Gerlicher, Klaus wrote:
> Hi,
> 
> I'm unsure why I'm confusing you about what this does. This is a patch for remote target packet buffer, so for gdbserver.

I had said:

  > I don't understand why the patch added a target method on the gdb side.

Is this what you are replying to?  You are describing this as a gdbserver patch, but the
patch touches the gdb side as well.  That's what I was asking about.  Why are these changes
needed:

 gdb/remote.h             |  8 +++---
 gdb/target-delegates.c   | 31 +++++++++++++++++++++--
 gdb/target.h             |  7 ++++++

I then had another separate question.

Pedro Alves

> 
> From original gdbserver/server.h:
> 
> /* Buffer sizes for transferring memory, registers, etc.  The target decides
>      how big this needs to be but this value must be at least as large as the
>      largest register set supported by gdbserver.  */
> 
> PBUFSIZ is defined as a constant( #define) and is used for allocating the buffer that gdbserver writes into for communication with GDB. 
> 
> This will remain the same size for targets not aware of the process_stratum_pbufsiz::query_pbuf_size override. I called this a target op but maybe that's the wrong term and that is the source of confusion?
> 
> Targets can override this to any size they see fit for what register size or memory transfer sizes they require. It doesn't make sense to have big allocation for targets that don't need that much but some newer accelerator/SIMD/GPU device (for a lack of a better term) targets need a much bigger buffer.
> 
> Thanks
> Klaus
> 
> -----Original Message-----
> From: Pedro Alves <pedro@palves.net> 
> Sent: Wednesday, September 20, 2023 6:32 PM
> To: Andrew Burgess <aburgess@redhat.com>; Gerlicher, Klaus <klaus.gerlicher@intel.com>; Simon Marchi <simon.marchi@polymtl.ca>; gdb-patches@sourceware.org
> Subject: Re: [PATCH 1/1] gdb, gdbserver: replace PBUFSIZ with a target op
> 
> Hi!
> 
> On 2023-09-20 13:59, Andrew Burgess via Gdb-patches wrote:
>> "Gerlicher, Klaus via Gdb-patches" <gdb-patches@sourceware.org> writes:
>>
>>> Hi Simon,
>>>
>>> Thanks for the quick response.
>>>
>>> At least the initial buffer size needs to be fixed since now most 
>>> clients aren't aware of any dynamic behavior here and therefore we 
>>> need at least something pre-allocated for these clients.
>>
>> I don't understand your concerns here.  For this patch we're only 
>> talking about the gdbserver client, right?  And your patch (rightly) 
>> doesn't change things on the GDB side.
> 
> The client is the GDB side, the server side is, well, gdbserver.  :-)
> 
>>
>> GDB already uses a dynamic packet buffer size.  
> 
> It is dynamic, but not in the sense that we just append to the buffer with push_back and let the buffer grow unbounded.  Instead, GDB tries to guess a sufficient packet size, but if the server tells it explicitly what packet size it supports, then GDB will grow its buffer to that size, no questions asked.
> 
>   /* If we increased the packet size, make sure to increase the global
>      buffer size also.  We delay this until after parsing the entire
>      qSupported packet, because this is the same buffer we were
>      parsing.  */
>   if (rs->buf.size () < rs->explicit_packet_size)
>     rs->buf.resize (rs->explicit_packet_size);
> 
> 
>> So the only initial
>> buffer I think you can be talking about here is the gdbserver buffer, 
>> which I think could be made dynamic, just as GDB's is.
> 
> I think he was really talking about the GDB side.  Or even other clients, like LLDB, etc.
> 
>>
>> We could hard-code gdbserver to return some stupidly large number for 
>> the PacketSize in the qSupported reply, say MAX_INT?  Or (MAX_INT / 
>> 4), you pick, this could be anything really, just something huge.
> 
> I don't think it can, due to the immediate resize mentioned above.
> 
> I don't understand why the patch added a target method on the gdb side.
> 
> Also, do we really need the new target method on the gdbserver side?
> We assert that the buffer size is bigger than the tdesc's register size plus a slack, but how about flipping that around and make the buffer size be dependent on the register size?  Maybe the packet size decision is done earlier than we know which tdesc we are using, though, that's something to check.
> 
> Pedro Alves
> 
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928