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

  From: "Gerlicher, Klaus" <klaus.gerlicher@intel.com>

PBUFSIZ is a hardcoded value that needs to be as least twice as big as
the register set.

This means that this could be tailored to the target register size as
not all targets need a big buffer. For SIMD targets the buffer will
obviously be much bigger than for regular scalar targets.

This patch changes this and adds a target op query_pbuf_size (). It also
moves the static global client_state into the get_client_state ()
function and lazily dynamically allocates storage for its own_buf.
---
 gdb/remote.h             |  8 +++---
 gdb/target-delegates.c   | 31 +++++++++++++++++++++--
 gdb/target.h             |  7 ++++++
 gdbserver/hostio.cc      | 54 ++++++++++++++++++++++++----------------
 gdbserver/notif.cc       |  8 +++---
 gdbserver/server.cc      | 26 +++++++++++--------
 gdbserver/server.h       | 31 ++++++++++++++++-------
 gdbserver/target.cc      |  9 +++++++
 gdbserver/target.h       |  9 +++++++
 gdbserver/tdesc.cc       | 11 +++++---
 gdbserver/tracepoint.cc  |  4 +--
 gdbsupport/common-defs.h |  5 ++++
 12 files changed, 146 insertions(+), 57 deletions(-)
  

On 9/19/23 01:45, Klaus Gerlicher via Gdb-patches wrote:
> From: "Gerlicher, Klaus" <klaus.gerlicher@intel.com>
> 
> PBUFSIZ is a hardcoded value that needs to be as least twice as big as
> the register set.
> 
> This means that this could be tailored to the target register size as
> not all targets need a big buffer. For SIMD targets the buffer will
> obviously be much bigger than for regular scalar targets.
> 
> This patch changes this and adds a target op query_pbuf_size (). It also
> moves the static global client_state into the get_client_state ()
> function and lazily dynamically allocates storage for its own_buf.

Just throwing an idea out there... instead of trying to determine a
buffer size that is large enough, can we instead pass something like an
std::vector (gdb::char_vector or gdb::byte_vector) around, and whatever
builds the packets and replies makes sure there is enough space?

Simon
  

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.

For the clients interested we could change the CS buffer to a re-allocatable type that is pre-allocated with the current PBUFSIZ. We could pass this buffer around with a target op. The target could resize at will instead of letting it return a fixed number.

Is this something in line that you would support?

Klaus

-----Original Message-----
From: Simon Marchi <simon.marchi@polymtl.ca> 
Sent: Tuesday, September 19, 2023 4:08 PM
To: Gerlicher, Klaus <klaus.gerlicher@intel.com>; gdb-patches@sourceware.org
Subject: Re: [PATCH 1/1] gdb, gdbserver: replace PBUFSIZ with a target op

On 9/19/23 01:45, Klaus Gerlicher via Gdb-patches wrote:
> From: "Gerlicher, Klaus" <klaus.gerlicher@intel.com>
> 
> PBUFSIZ is a hardcoded value that needs to be as least twice as big as 
> the register set.
> 
> This means that this could be tailored to the target register size as 
> not all targets need a big buffer. For SIMD targets the buffer will 
> obviously be much bigger than for regular scalar targets.
> 
> This patch changes this and adds a target op query_pbuf_size (). It 
> also moves the static global client_state into the get_client_state () 
> function and lazily dynamically allocates storage for its own_buf.

Just throwing an idea out there... instead of trying to determine a buffer size that is large enough, can we instead pass something like an std::vector (gdb::char_vector or gdb::byte_vector) around, and whatever builds the packets and replies makes sure there is enough space?

Simon
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
  

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

GDB already uses a dynamic packet buffer 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.

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.

Then on the gdbserver side we can use a dynamic packet size, and we just
take care to throw an error if GDB ever tries to grow it past our
(stupidly large) max packet size -- but this doesn't mean we actually
need to pre-allocate that large of a buffer.

Alternatively, we can always extend the remote protocol with a new
reply, say 'PacketSize=*' to indicate that the client supports variable
packet buffers -- but that would require GDB to first advertise that it
understands that syntax, then clients will need to deal with GDBs that
don't support it .... and it's a whole big thing.  Maybe easier just to
work within the current framework?  Though maybe not as clean.

Now for other clients (i.e. not gdbserver) they are free to keep using a
pre-allocated buffer, and they'll send back a much smaller PaketSize,
but that should be fine.  Nothing changes for them.

> For the clients interested we could change the CS buffer to a
> re-allocatable type that is pre-allocated with the current PBUFSIZ. We
> could pass this buffer around with a target op. The target could
> resize at will instead of letting it return a fixed number.

I didn't understand this paragraph.  I don't know what 'CS buffer'
means.  And I'm not sure why we're need a target op to pass the buffer
around -- but maybe that would become obvious if I looked at the code a
little more...

Thanks,
Andrew

>
> Is this something in line that you would support?
>
> Klaus
>
> -----Original Message-----
> From: Simon Marchi <simon.marchi@polymtl.ca> 
> Sent: Tuesday, September 19, 2023 4:08 PM
> To: Gerlicher, Klaus <klaus.gerlicher@intel.com>; gdb-patches@sourceware.org
> Subject: Re: [PATCH 1/1] gdb, gdbserver: replace PBUFSIZ with a target op
>
> On 9/19/23 01:45, Klaus Gerlicher via Gdb-patches wrote:
>> From: "Gerlicher, Klaus" <klaus.gerlicher@intel.com>
>> 
>> PBUFSIZ is a hardcoded value that needs to be as least twice as big as 
>> the register set.
>> 
>> This means that this could be tailored to the target register size as 
>> not all targets need a big buffer. For SIMD targets the buffer will 
>> obviously be much bigger than for regular scalar targets.
>> 
>> This patch changes this and adds a target op query_pbuf_size (). It 
>> also moves the static global client_state into the get_client_state () 
>> function and lazily dynamically allocates storage for its own_buf.
>
> Just throwing an idea out there... instead of trying to determine a buffer size that is large enough, can we instead pass something like an std::vector (gdb::char_vector or gdb::byte_vector) around, and whatever builds the packets and replies makes sure there is enough space?
>
> Simon
> 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
  

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
  

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.

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
  

"Gerlicher, Klaus" <klaus.gerlicher@intel.com> writes:

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

No target_ops is the correct name (so target ops is fine).  For me, I
think you should drop all of the changes in the gdb/ tree, leaving just
the gdbserver/ changes.

GDB should not need to know about PBUFSIZ, and indeed, there's just one
reference to it, in a comment, and this should be rewritten to indicate
that the maximum size being referenced is the one that the server passed
us in the qSupported packet.

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

Agreed.  It would be interesting to see if we could derive the required
size from the tdesc as Pedro suggested.

Thanks,
Andrew

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

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