From: "Gerlicher, Klaus" <klaus.gerlicher@intel.com>
To: Pedro Alves <pedro@palves.net>,
Andrew Burgess <aburgess@redhat.com>,
Simon Marchi <simon.marchi@polymtl.ca>,
"gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Subject: RE: [PATCH 1/1] gdb, gdbserver: replace PBUFSIZ with a target op
Date: Thu, 21 Sep 2023 06:02:31 +0000 [thread overview]
Message-ID: <SN7PR11MB709111C497251D1F4D78C8ECE8F8A@SN7PR11MB7091.namprd11.prod.outlook.com> (raw)
In-Reply-To: <24534cd7-9c80-19da-5a9c-17962ae46fb3@palves.net>
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
next prev parent reply other threads:[~2023-09-21 6:02 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-19 5:45 [PATCH 0/1] " Klaus Gerlicher
2023-09-19 5:45 ` [PATCH 1/1] gdb, gdbserver: " Klaus Gerlicher
2023-09-19 14:07 ` Simon Marchi
2023-09-20 6:21 ` Gerlicher, Klaus
2023-09-20 12:59 ` Andrew Burgess
2023-09-20 16:32 ` Pedro Alves
2023-09-21 6:02 ` Gerlicher, Klaus [this message]
2023-09-21 14:02 ` Andrew Burgess
2023-09-21 14:07 ` Pedro Alves
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=SN7PR11MB709111C497251D1F4D78C8ECE8F8A@SN7PR11MB7091.namprd11.prod.outlook.com \
--to=klaus.gerlicher@intel.com \
--cc=aburgess@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=pedro@palves.net \
--cc=simon.marchi@polymtl.ca \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).