Re: [PATCH 2/3] gdb: Add per-remote target variables for memory read and write config

[Andrew Burgess <aburgess@redhat.com>]

* Schimpe, Christina via Gdb-patches <gdb-patches@sourceware.org> [2022-01-24 15:06:31 +0000]:

> Hi Andrew,
> 
> Thanks a lot for your feedback.
> Before sending a V2 for this patch, there is still one point I would like to discuss
> (refer to my answer to your comment below).
> 
> I also noticed that I missed an adaption in the function
> static void
> show_memory_packet_size (struct memory_packet_config *config)
> 
> at 
> if (remote != NULL)
>   printf_filtered (_("Packets are limited to %ld bytes.\n"),
> 		 remote->get_memory_packet_size (config));
> 
> In the function call get_memory_packet_size the global configuration is passed,
> but the current target's configuration should be used.
> I will correct this with the next version of this patch. 
> 
> > > The former global variables for that configuration are still available
> > > to allow the command line configuration for all future remote
> > > connections.  Similar to the command line configuration of the per-
> > > remote target feature array, the commands
> > >
> > > - set remotewritesize (deprecated)
> > > - set remote memory-read-packet-size
> > > - set remote memory-write-packet-size
> > >
> > > will configure the current target (if available) and future remote
> > > connections. The show command will display the global configuration
> > > and the packet size of the current remote target, if available.
> > >
> > > It is required to adapt the test gdb.base/remote.exp which is failing
> > > for --target_board=native-extended-gdbserver.  With that board GDB
> > > connects to gdbserver at gdb start time.  Due to this patch two
> > > loggings "The target may not be able to.." are shown if the command
> > > 'set remote memory-write-packet-size fixed' is executed while a target
> > > is connected for the current inferior.  To fix this, the clean_restart
> > > command is moved to a later time point of the test.  It is sufficient
> > > to be connected to the server when "runto_main" is executed.  Now the
> > > connection time is similar to a testrun with
> > > --target_board=native-gdbserver.
> > >
> > > To allow the user to distinguish between the packet-size configuration
> > > for future connections and for the currently selected target, the
> > > logging of the command 'set remote memory-write-packet-size fixed' is
> > > adapted.
> > > ---
> > >  gdb/remote.c                      | 94 +++++++++++++++++++------------
> > >  gdb/testsuite/gdb.base/remote.exp |  7 ++-
> > >  2 files changed, 61 insertions(+), 40 deletions(-)
> > 
> > This needs a NEWS entry and a docs update.
> > 
> > Like with the previous patch, I think we should consider having the set/show
> > commands print more informative messages about which targets are being
> > changed or displayed.
> > 
> 
> Yes, I agree, this could be improved.  I was wondering if we should then adapt
> the show command such that it behaves similar to the previous patch e.g. 
> "The show command always displays the current remote target's
> configuration.  If no remote target is selected the default
> configuration for future connections is shown.".

That sounds reasonable to me, and inline with the previous patch.

> For the GDB user it might be less confusing if we keep the behaviour of the
> "show remote"  commands  consistent.

I agree that being consistent across commands, especially related
commands is super important.

Thanks,
Andrew



> 
> > >
> > > +  memory_packet_config m_memory_read_packet_config;
> > > + memory_packet_config m_memory_write_packet_config;
> > 
> > Comment new fields please.
> 
> Yes, I will adapt it.
> 
> > > @@ -1956,9 +1971,16 @@ set_memory_packet_size (const char *args,
> > struct memory_packet_config *config)
> > >  			 ? DEFAULT_MAX_MEMORY_PACKET_SIZE_FIXED
> > >  			 : size);
> > >
> > > -      if (! query (_("The target may not be able to correctly handle a %s\n"
> > > -		   "of %ld bytes. Change the packet size? "),
> > > -		   config->name, query_size))
> > > +      if (target_connected
> > > +	  && ! query (_("The target may not be able to correctly handle a
> > %s\n"
> > 
> > The space after '!' is not gdb style, could you remove it please.
> > Also the two below please.
> > 
> > Thanks,
> > Andrew
> 
> Thank you, I will adapt it.
> 
> Best Regards,
> Christina
> 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
>