[PATCH] remove download_write_size variable

[PATCH] remove download_write_size variable

[Vladimir Prus]

[-- Attachment #1: Type: text/plain, Size: 690 bytes --]


This patch removes the 'download_write_size'. At the moment, is has pretty low
value, it's not clear why we need a separate option for just one command, and 
the new qSupported packet allows to negotiate packet size with the stub.

So, I don't see any reason why this variable is needed, and given that flash 
patches will seriously change symfile.c anyway, I suggest that we 
"garbage-collect" this variable.

Comments?

- Volodya

2006-07-18  Vladimir Prus  <vladimir@codesourcery.com>

	* symfile.c (download_write_size): Remove.
	(show_download_write_size): Remove. 
	(load_section_callback): Don't use download_write_size.
	(_initialize_symfile): Don't register download_write_size.


[-- Attachment #2: 4_download_write_size.diff --]
[-- Type: text/x-diff, Size: 2193 bytes --]

--- symfile.c	(revision 176)
+++ symfile.c	(revision 177)
@@ -1521,15 +1521,6 @@ load_command (char *arg, int from_tty)
    we don't want to run a subprocess.  On the other hand, I'm not sure how
    performance compares.  */
 
-static int download_write_size = 512;
-static void
-show_download_write_size (struct ui_file *file, int from_tty,
-			  struct cmd_list_element *c, const char *value)
-{
-  fprintf_filtered (file, _("\
-The write size used when downloading a program is %s.\n"),
-		    value);
-}
 static int validate_download = 0;
 
 /* Callback service function for generic_load (bfd_map_over_sections).  */
@@ -1570,11 +1561,6 @@ load_section_callback (bfd *abfd, asecti
 	  const char *sect_name = bfd_get_section_name (abfd, asec);
 	  bfd_size_type sent;
 
-	  if (download_write_size > 0 && size > download_write_size)
-	    block_size = download_write_size;
-	  else
-	    block_size = size;
-
 	  buffer = xmalloc (size);
 	  old_chain = make_cleanup (xfree, buffer);
 
@@ -1591,8 +1577,6 @@ load_section_callback (bfd *abfd, asecti
 	      int len;
 	      bfd_size_type this_transfer = size - sent;
 
-	      if (this_transfer >= block_size)
-		this_transfer = block_size;
 	      len = target_write_memory_partial (lma, buffer,
 						 this_transfer, &err);
 	      if (err)
@@ -3806,19 +3790,6 @@ Usage: set extension-language .foo bar")
   add_info ("extensions", info_ext_lang_command,
 	    _("All filename extensions associated with a source language."));
 
-  add_setshow_integer_cmd ("download-write-size", class_obscure,
-			   &download_write_size, _("\
-Set the write size used when downloading a program."), _("\
-Show the write size used when downloading a program."), _("\
-Only used when downloading a program onto a remote\n\
-target. Specify zero, or a negative value, to disable\n\
-blocked writes. The actual size of each transfer is also\n\
-limited by the size of the target packet and the memory\n\
-cache."),
-			   NULL,
-			   show_download_write_size,
-			   &setlist, &showlist);
-
   debug_file_directory = xstrdup (DEBUGDIR);
   add_setshow_optional_filename_cmd ("debug-file-directory", class_support,
 				     &debug_file_directory, _("\

Re: [PATCH] remove download_write_size variable

[Daniel Jacobowitz]

On Tue, Jul 18, 2006 at 04:48:44PM +0400, Vladimir Prus wrote:
> 
> This patch removes the 'download_write_size'. At the moment, is has pretty low
> value, it's not clear why we need a separate option for just one command, and 
> the new qSupported packet allows to negotiate packet size with the stub.
> 
> So, I don't see any reason why this variable is needed, and given that flash 
> patches will seriously change symfile.c anyway, I suggest that we 
> "garbage-collect" this variable.
> 
> Comments?
> 
> - Volodya
> 
> 2006-07-18  Vladimir Prus  <vladimir@codesourcery.com>
> 
> 	* symfile.c (download_write_size): Remove.
> 	(show_download_write_size): Remove. 
> 	(load_section_callback): Don't use download_write_size.
> 	(_initialize_symfile): Don't register download_write_size.

I can find references to download-write-size on the Internet, e.g. in
FAQs suggesting how to get better write performance.  They all seem to
push it as high as memory-write-packet-size, though, which suggests we
do not need both limits.

I think it's reasonable to remove it.  However, you'll have to also
remove it from the manual (it's a documented command).  And I believe
it should be added to NEWS as a removed feature.

I'm going to let this patch sit a little while, in case anyone else can
think of a reason to keep "set download-write-size".

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] remove download_write_size variable

[Vladimir Prus]

[-- Attachment #1: Type: text/plain, Size: 1999 bytes --]

On Wednesday 19 July 2006 00:42, Daniel Jacobowitz wrote:
> On Tue, Jul 18, 2006 at 04:48:44PM +0400, Vladimir Prus wrote:
> > This patch removes the 'download_write_size'. At the moment, is has
> > pretty low value, it's not clear why we need a separate option for just
> > one command, and the new qSupported packet allows to negotiate packet
> > size with the stub.
> >
> > So, I don't see any reason why this variable is needed, and given that
> > flash patches will seriously change symfile.c anyway, I suggest that we
> > "garbage-collect" this variable.
> >
> > Comments?
> >
> > - Volodya
> >
> > 2006-07-18  Vladimir Prus  <vladimir@codesourcery.com>
> >
> > 	* symfile.c (download_write_size): Remove.
> > 	(show_download_write_size): Remove.
> > 	(load_section_callback): Don't use download_write_size.
> > 	(_initialize_symfile): Don't register download_write_size.
>
> I can find references to download-write-size on the Internet, e.g. in
> FAQs suggesting how to get better write performance.  They all seem to
> push it as high as memory-write-packet-size, though, which suggests we
> do not need both limits.
>
> I think it's reasonable to remove it.  However, you'll have to also
> remove it from the manual (it's a documented command).  

Are you sure? Grep does not find any occurrence in docs, on current mainline.

> And I believe 
> it should be added to NEWS as a removed feature.

Done.

> I'm going to let this patch sit a little while, in case anyone else can
> think of a reason to keep "set download-write-size".

The patch was sitting for quite some time already, OK to commit now? I'm 
attaching it again for reference.

- Volodya

2006-07-31  Vladimir Prus  <vladimir@codesourcery.com>

        * symfile.c (download_write_size): Remove.
        (show_download_write_size): Remove.
        (load_section_callback): Don't use download_write_size.
        (_initialize_symfile): Don't register download_write_size.
        * NEWS: Mention 'download_write_size' removal.

[-- Attachment #2: download_write_size__gdb.diff --]
[-- Type: text/x-diff, Size: 2568 bytes --]

=== gdb/symfile.c
==================================================================
--- gdb/symfile.c	(/mirrors/gdb)	(revision 315)
+++ gdb/symfile.c	(/patches/download_write_size/gdb)	(revision 315)
@@ -1521,15 +1521,6 @@
    we don't want to run a subprocess.  On the other hand, I'm not sure how
    performance compares.  */
 
-static int download_write_size = 512;
-static void
-show_download_write_size (struct ui_file *file, int from_tty,
-			  struct cmd_list_element *c, const char *value)
-{
-  fprintf_filtered (file, _("\
-The write size used when downloading a program is %s.\n"),
-		    value);
-}
 static int validate_download = 0;
 
 /* Callback service function for generic_load (bfd_map_over_sections).  */
@@ -1570,11 +1561,6 @@
 	  const char *sect_name = bfd_get_section_name (abfd, asec);
 	  bfd_size_type sent;
 
-	  if (download_write_size > 0 && size > download_write_size)
-	    block_size = download_write_size;
-	  else
-	    block_size = size;
-
 	  buffer = xmalloc (size);
 	  old_chain = make_cleanup (xfree, buffer);
 
@@ -1591,8 +1577,6 @@
 	      int len;
 	      bfd_size_type this_transfer = size - sent;
 
-	      if (this_transfer >= block_size)
-		this_transfer = block_size;
 	      len = target_write_memory_partial (lma, buffer,
 						 this_transfer, &err);
 	      if (err)
@@ -3806,19 +3790,6 @@
   add_info ("extensions", info_ext_lang_command,
 	    _("All filename extensions associated with a source language."));
 
-  add_setshow_integer_cmd ("download-write-size", class_obscure,
-			   &download_write_size, _("\
-Set the write size used when downloading a program."), _("\
-Show the write size used when downloading a program."), _("\
-Only used when downloading a program onto a remote\n\
-target. Specify zero, or a negative value, to disable\n\
-blocked writes. The actual size of each transfer is also\n\
-limited by the size of the target packet and the memory\n\
-cache."),
-			   NULL,
-			   show_download_write_size,
-			   &setlist, &showlist);
-
   debug_file_directory = xstrdup (DEBUGDIR);
   add_setshow_optional_filename_cmd ("debug-file-directory", class_support,
 				     &debug_file_directory, _("\
=== gdb/NEWS
==================================================================
--- gdb/NEWS	(/mirrors/gdb)	(revision 315)
+++ gdb/NEWS	(/patches/download_write_size/gdb)	(revision 315)
@@ -18,6 +18,8 @@
 Kernel Object Display, an embedded debugging feature which only worked with
 an obsolete version of Cisco IOS.
 
+The 'download_write_size' variable.
+
 * New remote packets
 
 qSupported:

Re: [PATCH] remove download_write_size variable

[Eli Zaretskii]

> From: Vladimir Prus <vladimir@codesourcery.com>
> Date: Mon, 31 Jul 2006 16:22:57 +0400
> Cc: gdb-patches@sources.redhat.com
> 
> > > 	* symfile.c (download_write_size): Remove.
> > > 	(show_download_write_size): Remove.
> > > 	(load_section_callback): Don't use download_write_size.
> > > 	(_initialize_symfile): Don't register download_write_size.
> >
> > I can find references to download-write-size on the Internet, e.g. in
> > FAQs suggesting how to get better write performance.  They all seem to
> > push it as high as memory-write-packet-size, though, which suggests we
> > do not need both limits.
> >
> > I think it's reasonable to remove it.  However, you'll have to also
> > remove it from the manual (it's a documented command).  
> 
> Are you sure? Grep does not find any occurrence in docs, on current mainline.

It's in the node called "Target Commands".

> === gdb/NEWS
> ==================================================================
> --- gdb/NEWS	(/mirrors/gdb)	(revision 315)
> +++ gdb/NEWS	(/patches/download_write_size/gdb)	(revision 315)
> @@ -18,6 +18,8 @@
>  Kernel Object Display, an embedded debugging feature which only worked with
>  an obsolete version of Cisco IOS.
>  
> +The 'download_write_size' variable.
> +

This variable is not user-visible.  You need to mention the command,
not the C variable.

Re: [PATCH] remove download_write_size variable

[Vladimir Prus]

[-- Attachment #1: Type: text/plain, Size: 1751 bytes --]

On Monday 31 July 2006 21:16, Eli Zaretskii wrote:
> > From: Vladimir Prus <vladimir@codesourcery.com>
> > Date: Mon, 31 Jul 2006 16:22:57 +0400
> > Cc: gdb-patches@sources.redhat.com
> >
> > > > 	* symfile.c (download_write_size): Remove.
> > > > 	(show_download_write_size): Remove.
> > > > 	(load_section_callback): Don't use download_write_size.
> > > > 	(_initialize_symfile): Don't register download_write_size.
> > >
> > > I can find references to download-write-size on the Internet, e.g. in
> > > FAQs suggesting how to get better write performance.  They all seem to
> > > push it as high as memory-write-packet-size, though, which suggests we
> > > do not need both limits.
> > >
> > > I think it's reasonable to remove it.  However, you'll have to also
> > > remove it from the manual (it's a documented command).
> >
> > Are you sure? Grep does not find any occurrence in docs, on current
> > mainline.
>
> It's in the node called "Target Commands".

Thanks. I was searching with underscores in names, not dashes ;-)

>
> > === gdb/NEWS
> > ==================================================================
> > --- gdb/NEWS	(/mirrors/gdb)	(revision 315)
> > +++ gdb/NEWS	(/patches/download_write_size/gdb)	(revision 315)
> > @@ -18,6 +18,8 @@
> >  Kernel Object Display, an embedded debugging feature which only worked
> > with an obsolete version of Cisco IOS.
> >
> > +The 'download_write_size' variable.
> > +
>
> This variable is not user-visible.  You need to mention the command,
> not the C variable.

Please find the new patch attached. Doc dir changelog is:

2006-08-01  Vladimir Prus  <vladimir@codesourcery.com>

	* gdb.texinfo (Target Commands): Remove 
	'set download-write-size' and 'show download-write-size'.


- Volodya

[-- Attachment #2: download_write_size__gdb.diff --]
[-- Type: text/x-diff, Size: 3785 bytes --]

=== gdb/doc/gdb.texinfo
==================================================================
--- gdb/doc/gdb.texinfo	(/mirrors/gdb)	(revision 338)
+++ gdb/doc/gdb.texinfo	(/patches/download_write_size/gdb)	(revision 338)
@@ -12133,23 +12133,10 @@
 
 Many remote targets require you to download the executable's code once
 you've successfully established a connection.  You may wish to control
-various aspects of this process, such as the size of the data chunks
-used by @value{GDBN} to download program parts to the remote target.
+various aspects of this process.
 
 @table @code
-@kindex set download-write-size
-@item set download-write-size @var{size}
-Set the write size used when downloading a program.  Only used when
-downloading a program onto a remote target.  Specify zero or a
-negative value to disable blocked writes.  The actual size of each
-transfer is also limited by the size of the target packet and the
-memory cache.
 
-@kindex show download-write-size
-@item show download-write-size
-@kindex show download-write-size
-Show the current value of the write size.
-
 @item set hash
 @kindex set hash@r{, for remote monitors}
 @cindex hash mark while downloading
=== gdb/symfile.c
==================================================================
--- gdb/symfile.c	(/mirrors/gdb)	(revision 338)
+++ gdb/symfile.c	(/patches/download_write_size/gdb)	(revision 338)
@@ -1521,15 +1521,6 @@
    we don't want to run a subprocess.  On the other hand, I'm not sure how
    performance compares.  */
 
-static int download_write_size = 512;
-static void
-show_download_write_size (struct ui_file *file, int from_tty,
-			  struct cmd_list_element *c, const char *value)
-{
-  fprintf_filtered (file, _("\
-The write size used when downloading a program is %s.\n"),
-		    value);
-}
 static int validate_download = 0;
 
 /* Callback service function for generic_load (bfd_map_over_sections).  */
@@ -1570,11 +1561,6 @@
 	  const char *sect_name = bfd_get_section_name (abfd, asec);
 	  bfd_size_type sent;
 
-	  if (download_write_size > 0 && size > download_write_size)
-	    block_size = download_write_size;
-	  else
-	    block_size = size;
-
 	  buffer = xmalloc (size);
 	  old_chain = make_cleanup (xfree, buffer);
 
@@ -1591,8 +1577,6 @@
 	      int len;
 	      bfd_size_type this_transfer = size - sent;
 
-	      if (this_transfer >= block_size)
-		this_transfer = block_size;
 	      len = target_write_memory_partial (lma, buffer,
 						 this_transfer, &err);
 	      if (err)
@@ -3806,19 +3790,6 @@
   add_info ("extensions", info_ext_lang_command,
 	    _("All filename extensions associated with a source language."));
 
-  add_setshow_integer_cmd ("download-write-size", class_obscure,
-			   &download_write_size, _("\
-Set the write size used when downloading a program."), _("\
-Show the write size used when downloading a program."), _("\
-Only used when downloading a program onto a remote\n\
-target. Specify zero, or a negative value, to disable\n\
-blocked writes. The actual size of each transfer is also\n\
-limited by the size of the target packet and the memory\n\
-cache."),
-			   NULL,
-			   show_download_write_size,
-			   &setlist, &showlist);
-
   debug_file_directory = xstrdup (DEBUGDIR);
   add_setshow_optional_filename_cmd ("debug-file-directory", class_support,
 				     &debug_file_directory, _("\
=== gdb/NEWS
==================================================================
--- gdb/NEWS	(/mirrors/gdb)	(revision 338)
+++ gdb/NEWS	(/patches/download_write_size/gdb)	(revision 338)
@@ -18,6 +18,8 @@
 Kernel Object Display, an embedded debugging feature which only worked with
 an obsolete version of Cisco IOS.
 
+The 'set download-write-size' and 'show download-write-size' commands.
+
 * New remote packets
 
 qSupported:

Re: [PATCH] remove download_write_size variable

[Eli Zaretskii]

> From: Vladimir Prus <vladimir@codesourcery.com>
> Date: Tue, 1 Aug 2006 09:33:58 +0400
> Cc: gdb-patches@sources.redhat.com
> 
> Please find the new patch attached. Doc dir changelog is:
> 
> 2006-08-01  Vladimir Prus  <vladimir@codesourcery.com>
> 
> 	* gdb.texinfo (Target Commands): Remove 
> 	'set download-write-size' and 'show download-write-size'.

The patches for gdb.texinfo and for NEWS are approved.

Thanks.

Re: [PATCH] remove download_write_size variable

[Daniel Jacobowitz]

On Wed, Aug 02, 2006 at 06:12:40AM +0300, Eli Zaretskii wrote:
> > From: Vladimir Prus <vladimir@codesourcery.com>
> > Date: Tue, 1 Aug 2006 09:33:58 +0400
> > Cc: gdb-patches@sources.redhat.com
> > 
> > Please find the new patch attached. Doc dir changelog is:
> > 
> > 2006-08-01  Vladimir Prus  <vladimir@codesourcery.com>
> > 
> > 	* gdb.texinfo (Target Commands): Remove 
> > 	'set download-write-size' and 'show download-write-size'.
> 
> The patches for gdb.texinfo and for NEWS are approved.

Thanks.  While Vladimir is away, I've committed this on his behalf,
along with these testsuite fixups (not many testing configurations
trigger this code).

-- 
Daniel Jacobowitz
CodeSourcery

2006-08-08  Daniel Jacobowitz  <dan@codesourcery.com>

	* config/monitor.exp (gdb_load): Remove support for obsolete
	download-write-size.
	* gdb.base/remote.exp: Likewise.  Update all callers of
	gdb_timed_load.

Index: testsuite/config/monitor.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/config/monitor.exp,v
retrieving revision 1.5
diff -u -p -r1.5 monitor.exp
--- testsuite/config/monitor.exp	15 Mar 2001 21:46:57 -0000	1.5
+++ testsuite/config/monitor.exp	8 Aug 2006 15:46:06 -0000
@@ -140,17 +140,6 @@ proc gdb_load { arg } {
     global timeout
     global last_gdb_file;
 
-    if [target_info exists gdb_download_size] {
-	send_gdb "set download-write-size [target_info gdb_download_size]\n";
-	gdb_expect 30 {
-	    -re "$gdb_prompt $" { }
-	    default {
-		perror "Setting download-write-size for target failed";
-		return -1;
-	    }
-	}
-    }
-
     if { $arg == "" } {
 	if [info exists last_gdb_file] {
 	    set arg $last_gdb_file;
Index: testsuite/gdb.base/remote.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/remote.exp,v
retrieving revision 1.6
diff -u -p -r1.6 remote.exp
--- testsuite/gdb.base/remote.exp	4 Jul 2006 09:40:28 -0000	1.6
+++ testsuite/gdb.base/remote.exp	8 Aug 2006 15:46:06 -0000
@@ -46,17 +46,6 @@ if {$result != "" } then {
 # Part ONE: Check the down load commands
 #
 
-gdb_test "show download-write-size" \
-	"The write size used when downloading a program is 512." \
-	"download limit default"
-
-gdb_test "set download-write-size" "Argument required.*"
-
-gdb_test "set download-write-size 0" ""
-gdb_test "show download-write-size" \
-	"The write size used when downloading a program is unlimited." \
-	"set download limit - unlimited"
-
 gdb_test "show remote memory-write-packet-size" \
 	"The memory-write-packet-size is 0. Packets are limited to \[0-9\]+ bytes." \
 	"write-packet default"
@@ -79,21 +68,14 @@ gdb_test "show remote memory-write-packe
 # Part TWO: Check the download behavour
 #
 
-proc gdb_load_timed {executable downloadsize class writesize} {
+proc gdb_load_timed {executable class writesize} {
     global test gdb_prompt
-    set test "timed download `[file tail $executable]' - $downloadsize, $class, $writesize"
+    set test "timed download `[file tail $executable]' - $class, $writesize"
 
     if {$writesize != ""} then {
 	gdb_test "set remote memory-write-packet-size $writesize" \
 		"" "$test - set packet size"
-    }
-
-    if {$downloadsize != ""} then {
-	gdb_test "set download-write-size $downloadsize" \
-		"" "$test - set download size"
-    }
 
-    if {$downloadsize != ""} then {
 	send_gdb "set remote memory-write-packet-size $class\n"
 	gdb_expect 5 {
 	    -re ".*Change the packet size.*$" {
@@ -127,18 +109,18 @@ proc gdb_load_timed {executable download
     pass $test
 }
 
-gdb_load_timed $binfile {} "" {}
+gdb_load_timed $binfile "" {}
 
 # Typically about 400-1 bytes can be downloaded
-gdb_load_timed $binfile 0 "limit" 398
-gdb_load_timed $binfile 0 "limit" 400
+gdb_load_timed $binfile "limit" 398
+gdb_load_timed $binfile "limit" 400
 
 # Absolute max is 16384
-gdb_load_timed $binfile 0 "fixed" 0
-gdb_load_timed $binfile 0 "fixed" 16385
+gdb_load_timed $binfile "fixed" 0
+gdb_load_timed $binfile "fixed" 16385
 
 # fall back to the default
-gdb_load_timed $binfile 0 "limit" 0
+gdb_load_timed $binfile "limit" 0
 
 # Get size of data uploaded