RE: [PATCH] new MI command for pattern filling of memory regions

["Abid, Hafiz" <Hafiz_Abid@mentor.com>]

I don't know much about this area of the code. Just a formatting comment.


-----Original Message-----
From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Giuseppe MONTALTO
Sent: Tuesday, June 12, 2012 10:07 AM
To: Tom Tromey; gdb-patches@sourceware.org
Cc: Giuseppe MONTALTO
Subject: RE: [PATCH] new MI command for pattern filling of memory regions

Hi!

Sorry to bother you, but I received no feedback on the patch after three weeks; is there any chance 
for it to be reviewed on time for 7.5?

> -----Original Message-----
> From: Giuseppe MONTALTO
> Sent: Friday, May 25, 2012 4:35 PM
> To: 'Tom Tromey'
> Cc: 'gdb-patches@sourceware.org'
> Subject: RE: [PATCH] new MI command for pattern filling of memory
> regions
> 
> I reworked the patch, including the test script;
> Accordingly with your feedback, the sscanf cycle is now performed only
> once.
> 
> Here's the new patch, please let me know if there is anything more to
> change.
> 
> Regards,
> 	Giuseppe
> 
> gdb/ChangeLog                           |    5 ++
>  gdb/mi/mi-main.c                        |   50 +++++++++++++++++----
>  gdb/testsuite/ChangeLog                 |    4 ++
>  gdb/testsuite/gdb.mi/mi-fill-memory.exp |   72
> +++++++++++++++++++++++++++++++
>  4 files changed, 122 insertions(+), 9 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 309473e..9ec8882 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,8 @@
> +2012-05-09  Giuseppe Montalto  <giuseppe.montalto@st.com>
> +
> +	* mi/mi-main.c  (mi_cmd_data_write_memory): additional
> +	parameter for pattern filling of memory regions
> +
>  2012-05-09  Pedro Alves  <palves@redhat.com>
> 
>  	* target.c (set_maintenance_target_async_permitted): Rename to
> ...
> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index 90fb624..7315848 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -1658,7 +1658,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)
> @@ -1666,27 +1667,58 @@ 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;
> +  int len, r, i, steps, reminder;
> +  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;
> +  len = strlen (cdata) / 2;
> +  if (argc == 3)
> +    count = strtol(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;
> +      reminder = count % len; /* there may be some spare bytes.  */
> +      for (j = 0; j < steps; j++)
> +        memcpy (data + j * len, databuf, (size_t) len);
> +
> +      if (reminder > 0) /* copy spare bytes too.  */
> +        memcpy (data + steps * len, databuf, (size_t) reminder);
> +    }
> +  else
> +    {
> +      /* pattern is longer than (or equal to) count:
> +         just copy len bytes.  */
> +      data = xmalloc (len);
> +      back_to = make_cleanup (xfree, data);
> +      memcpy (data, databuf, (size_t) len);
> +    }
> +
> +  xfree(databuf);

Please add a space before the opening parenthesis. 
> +
> +  r = target_write_memory (addr, data, count);
>    if (r != 0)
>      error (_("Could not write memory"));
> 
> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
> index 43549bf..a49274f 100644
> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,3 +1,7 @@
> +2012-05-09  Giuseppe Montalto  <giuseppe.montalto@st.com>
> +
> +	* gdb.mi/mi-fill-memory.exp: New test.
> +
>  2012-05-08  Maciej W. Rozycki  <macro@codesourcery.com>
> 
>  	* gdb.mi/mi-var-display.exp: Check for the existence of $fp
> 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..b6961b1
> --- /dev/null
> +++ b/gdb/testsuite/gdb.mi/mi-fill-memory.exp
> @@ -0,0 +1,72 @@
> +# 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
> +}
> +
> +set testfile "mi-read-memory"
> +set srcfile ${testfile}.c
> +set binfile ${objdir}/${subdir}/${testfile}
> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}"
> executable {debug additional_flags=-DFAKEARGV}] != "" } {
> +     untested mi-read-memory.exp
> +     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
> +return 0
> 
> 
> 
> > -----Original Message-----
> > From: Giuseppe MONTALTO
> > Sent: Friday, May 11, 2012 6:06 PM
> > To: 'Tom Tromey'
> > Cc: gdb-patches@sourceware.org
> > Subject: RE: [PATCH] new MI command for pattern filling of memory
> > regions
> >
> > Thanks,
> >
> > my answers are interleaved below.
> >
> > > -----Original Message-----
> > > From: Tom Tromey [mailto:tromey@redhat.com]
> > > Sent: Friday, May 11, 2012 4:14 PM
> > > To: Giuseppe MONTALTO
> > > Cc: gdb-patches@sourceware.org
> > > Subject: Re: [PATCH] new MI command for pattern filling of memory
> > > regions
> > >
> > > The indentation is wrong here and elsewhere in the patch.
> > > There are missing spaces as well.
> > > See the GNU Coding Standards for details.
> >
> > I'll try to better format the code.
> >
> > >
> > > I don't think the cast here, or other casts in the patch, are
> > > necessary.
> >
> > Consider them gone.
> >
> > >
> > > Giuseppe> +	  /* pattern is made of less bytes than count:
> > > Giuseppe> +	     repeat pattern to fill memory.  */
> > > Giuseppe> +	  data = xmalloc (count/len);
> > >
> > > This allocates count/len bytes...
> > >
> > > Giuseppe> +	  steps = count/len;
> > > Giuseppe> +	  for (j = 0; j < steps; j++)
> > > Giuseppe> +		{
> > > Giuseppe> +		  for (i = 0; i < len; ++i)
> > > Giuseppe> +			{
> > > Giuseppe> +			  int x;
> > > Giuseppe> +			  sscanf (cdata + i * 2, "%02x", &x);
> > > Giuseppe> +			  data[i + j * len] = (gdb_byte)x;
> > >
> > > ... but writes 'count' bytes.
> >
> > you're right; I was supposed to allocate 'count' bytes.
> >
> > Incidentally, there's a worse error there:
> > is that I'm counting the cycles basing on the result of an integer
> > division, hence not taking into account the spare bytes!
> >
> > I'll fix that too...
> >
> > 	Giuseppe