RE: [PATCH] enhancement of mi_cmd_data_write_memory_bytes for filling memory regions (was [PATCH] new MI command for pattern filling of memory regions)

[Giuseppe MONTALTO <giuseppe.montalto@st.com>]

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

Hi Tom,

Thanks for your comments: I've reworked the patch taking them into account.
I have also updated it, so that it should now fit the current HEAD 

Please, find it attached.

Regards,
	Giuseppe.

> -----Original Message-----
> From: Tom Tromey [mailto:tromey@redhat.com]
> Sent: Friday, September 14, 2012 6:31 PM
> To: Giuseppe MONTALTO
> Cc: Abid, Hafiz; gdb-patches@sourceware.org
> Subject: Re: [PATCH] enhancement of mi_cmd_data_write_memory_bytes
> for filling memory regions (was [PATCH] new MI command for pattern filling
> of memory regions)
> 
> >>>>> "Giuseppe" == Giuseppe MONTALTO <giuseppe.montalto@st.com>
> writes:
> 
> Giuseppe> New version of the patch.
> 
> I'm sorry about the delay on this.
> 
> I know it feels uncomfortable, but it really is good to ping patches -- say every
> week or so.  Otherwise they tend to slide off the radar.
> 
> Giuseppe> +  int len, r, i, steps, reminder;  long int count, j;
> 
> I think now size_t is preferred.  (This changed since your patch
> submission.)
> 
> Typo: should be "remainder", not "reminder".
> 
> Giuseppe> +    count = strtol (argv[2], NULL, 10);
> 
> I think strtoul instead.
> 
> Giuseppe> +  if (len < count)
> Giuseppe> +    {
> Giuseppe> +      /* pattern is made of less bytes than count:
> Giuseppe> +         repeat pattern to fill memory.  */
> 
> Comments should start with an uppercase letter.
> 
> Giuseppe> +  else
> Giuseppe> +    {
> Giuseppe> +      /* pattern is longer than (or equal to) count:
> Giuseppe> +         just copy len bytes.  */
> 
> Likewise.
> 
> Giuseppe> +  xfree (databuf);
> 
> It seems like you could do less work for the usual case where len==count.
> That is, avoid the extra malloc and memcpy.
> 
> Giuseppe> +set testfile "mi-read-memory"
> Giuseppe> +set srcfile ${testfile}.c
> Giuseppe> +set binfile ${objdir}/${subdir}/${testfile}
> 
> Use standard_testfile now.
> 
> Giuseppe> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}"
> executable {debug additional_flags=-DFAKEARGV}] != "" } {
> Giuseppe> +     untested mi-read-memory.exp
> 
> Use build_executable instead and don't call untested.
> 
> Giuseppe> +return 0
> 
> No need for a return at the end of a .exp file.
> 
> Tom

[-- Attachment #2: mi_cmd_data_write_memory_bytes.patch --]
[-- Type: application/octet-stream, Size: 6806 bytes --]

 gdb/ChangeLog                           |    5 ++
 gdb/mi/mi-main.c                        |   61 ++++++++++++++++++++++-----
 gdb/testsuite/ChangeLog                 |    4 ++
 gdb/testsuite/gdb.mi/mi-fill-memory.exp |   68 +++++++++++++++++++++++++++++++
 4 files changed, 127 insertions(+), 11 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 4ab15c4..3a0268e 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2012-09-18  Giuseppe Montalto  <giuseppe.montalto@st.com>
+
+	* mi/mi-main.c  (mi_cmd_data_write_memory): additional
+	parameter for pattern filling of memory regions
+
 2012-09-17  Mike Wrighton  <wrighton@codesourcery.com>
 
 	* MAINTAINERS (Write After Approval): Add "Mike Wrighton".
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index f1d21bc..5f670c2 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -1656,7 +1656,8 @@ mi_cmd_data_write_memory (char *command, char **argv, int argc)
 /* Implementation of the -data-write-memory-bytes command.
 
    ADDR: start address
-   DATA: string of bytes to write at that address.  */
+   DATA: string of bytes to write at that address
+   COUNT: number of bytes to be filled (decimal integer).  */
 
 void
 mi_cmd_data_write_memory_bytes (char *command, char **argv, int argc)
@@ -1664,30 +1665,68 @@ mi_cmd_data_write_memory_bytes (char *command, char **argv, int argc)
   CORE_ADDR addr;
   char *cdata;
   gdb_byte *data;
-  int len, r, i;
+  gdb_byte *databuf;
+  size_t len, r, i, steps, remainder;
+  long int count, j;
   struct cleanup *back_to;
 
-  if (argc != 2)
-    error (_("Usage: ADDR DATA."));
+  if (argc != 2 && argc != 3)
+    error (_("Usage: ADDR DATA [COUNT]."));
 
   addr = parse_and_eval_address (argv[0]);
   cdata = argv[1];
   len = strlen (cdata)/2;
+  if (argc == 3)
+    count = strtoul (argv[2], NULL, 10);
+  else
+    count = len;
 
-  data = xmalloc (len);
-  back_to = make_cleanup (xfree, data);
+  databuf = xmalloc (len * sizeof (gdb_byte));
 
   for (i = 0; i < len; ++i)
     {
       int x;
       sscanf (cdata + i * 2, "%02x", &x);
-      data[i] = (gdb_byte) x;
+      databuf[i] = (gdb_byte) x;
     }
 
-  r = target_write_memory (addr, data, len);
+  if (len < count)
+    {
+      /* Pattern is made of less bytes than count: 
+         repeat pattern to fill memory.  */
+      data = xmalloc (count);
+      back_to = make_cleanup (xfree, data);
+    
+      steps = count / len;
+      remainder = count % len; /* there may be some spare bytes.  */
+      for (j = 0; j < steps; j++)
+        memcpy (data + j * len, databuf, len);
+
+      if (remainder > 0) /* copy spare bytes too.  */
+        memcpy (data + steps * len, databuf, remainder);
+    }
+  else if (len > count)
+    {
+      /* Pattern is longer than count: 
+         just copy len bytes.  */
+      data = xmalloc (len);
+      back_to = make_cleanup (xfree, data);
+      memcpy (data, databuf, len);
+    }
+  else
+    {
+      /* Pattern is equal to count: 
+         we can just use databuf.  */
+      back_to = make_cleanup (xfree, data);
+      data = databuf;
+    }
+
+  r = target_write_memory (addr, data, count);
   if (r != 0)
     error (_("Could not write memory"));
 
+  xfree (databuf);
+
   do_cleanups (back_to);
 }
 
@@ -2097,10 +2136,10 @@ mi_cmd_execute (struct mi_parse *parse)
 
   current_context = parse;
 
-  if (parse->cmd->suppress_notification != NULL)
+  if (strncmp (parse->command, "break-", sizeof ("break-") - 1 ) == 0)
     {
-      make_cleanup_restore_integer (parse->cmd->suppress_notification);
-      *parse->cmd->suppress_notification = 1;
+      make_cleanup_restore_integer (&mi_suppress_breakpoint_notifications);
+      mi_suppress_breakpoint_notifications = 1;
     }
 
   if (parse->cmd->argv_func != NULL)
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index b458aba..42f66be 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2012-09-18  Giuseppe Montalto  <giuseppe.montalto@st.com>
+
+	* gdb.mi/mi-fill-memory.exp: New test.
+
 2012-09-17  Yao Qi  <yao@codesourcery.com>
 
 	* gdb.base/list.exp (set_listsize): Don't set arg to "unlimited"
diff --git a/gdb/testsuite/gdb.mi/mi-fill-memory.exp b/gdb/testsuite/gdb.mi/mi-fill-memory.exp
new file mode 100644
index 0000000..3df6a96
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-fill-memory.exp
@@ -0,0 +1,68 @@
+# Copyright (C) 2012 Free Software Foundation, Inc.
+# Copyright (C) 2012 STMicroelectronics
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+#
+# test basic Machine interface (MI) operations
+#
+# Verify that, using the MI, we can load a program and do
+# other basic things that are used by all test files through  mi_gdb_exit,
+# mi_gdb_start, mi_delete_breakpoints, mi_gdb_reinitialize_dir and
+# mi_gdb_load, so we can safely use those.
+#
+# The goal is not to test gdb functionality, which is done by other tests,
+# but the command syntax and correct output response to MI operations.
+# 
+# added for testing the -data-write-memory-bytes MI command enhancements
+#
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+gdb_exit
+if [mi_gdb_start] {
+    continue
+}
+
+standard_testfile "mi-read-memory"
+ 
+if {[build_executable ${testfile}.exp ${binfile} ${srcfile}.c {debug additional_flags=-DFAKEARGV}] == -1} {
+    return -1
+}
+
+mi_run_to_main
+mi_next_to "main" "" "mi-read-memory.c" "20" "next at main"
+
+mi_gdb_test "1-data-write-memory-bytes"\
+	"1\\\^error,msg=\"Usage: ADDR DATA \\\[COUNT\\\]\.\""\
+	"no arguments"
+
+mi_gdb_test "2-data-write-memory-bytes 8"\
+	"2\\\^error,msg=\"Usage: ADDR DATA \\\[COUNT\\\]\.\""\
+	"one argument missing"
+
+mi_gdb_test "3-data-write-memory-bytes \$pc ab"\
+	"3\\\^done" \
+	"memory successfully written"
+
+mi_gdb_test "4-data-write-memory-bytes \$pc ab 8"\
+	"4\\\^done" \
+	"memory successfully filled (8 bytes)"
+
+mi_gdb_test "5-interpreter-exec console \"x \$pc\"" \
+    ".*0xabababab.*" \
+    "pattern correctly read from memory"
+
+mi_gdb_exit