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

Giuseppe MONTALTO
2012-06-12T14:13:25Z

Thanks.

Incidentally, while fixing it I found another couple of similar formatting issues.

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Abid, Hafiz
> Sent: Tuesday, June 12, 2012 12:01 PM
> To: Giuseppe MONTALTO; Tom Tromey; gdb-patches@sourceware.org
> Subject: RE: [PATCH] new MI command for pattern filling of memory
> regions
>
> 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