RFA/remote: compare-sections

RFA/remote: compare-sections

[David Taylor]

Motivation:

When connecting to a remote system, we use the compare-sections command
to verify that the box is running the code that we think it is running.
Since the system is up and running and *NOT* 'freshly downloaded without
yet executing anything', read-write sections, of course, differ from
what they were in the executable file.

Comparing read-write sections takes time and more importantly the
MIS-MATCHED output is confusing to some users.

The compare-sections command compares all loadable sections including
read-write sections.  This patch gives the user the option to compare
just the loadable read-only sections.

For gdb/ChangeLog:

2014-03-26  David Taylor  <dtaylor@emc.com>

	* remote.c (compare_sections_command): Add -r option to compare
	all loadable read-only sections.

For gdb/doc/ChangeLog:

2014-03-26  David Taylor  <dtaylor@emc.com>

	* gdb.texinfo (compare-sections): Document the new -r (read-only)
	option.

I'm not sure that this patch is big enough to require a copyright
assignment, but regardless, EMC has a copyright assignment on file for
GDB (and GCC and BINUTILS as well).

If it is approved, I will need someone else to commit it as I don't have
write access.

Patch:

Index: gdb/remote.c
===================================================================
RCS file: /home/cvsroot/GDB/gdb/remote.c,v
retrieving revision 1.8
diff -u -r1.8 remote.c
--- gdb/remote.c	26 Mar 2014 14:12:34 -0000	1.8
+++ gdb/remote.c	26 Mar 2014 15:49:40 -0000
@@ -8664,6 +8664,7 @@
   int matched = 0;
   int mismatched = 0;
   int res;
+  int read_only = 0;
 
   if (!exec_bfd)
     error (_("command cannot be used without an exec file"));
@@ -8671,11 +8672,20 @@
   /* Make sure the remote is pointing at the right process.  */
   set_general_process ();
 
+  if (args && (strcmp (args, "-r") == 0))
+    {
+      read_only = 1;
+      args = NULL;
+    }
+
   for (s = exec_bfd->sections; s; s = s->next)
     {
       if (!(s->flags & SEC_LOAD))
 	continue;		/* Skip non-loadable section.  */
 
+      if (read_only && ((s->flags & SEC_READONLY) == 0))
+	continue;		/* Skip writeable sections */
+
       size = bfd_get_section_size (s);
       if (size == 0)
 	continue;		/* Skip zero-length section.  */
@@ -12046,7 +12056,8 @@
 
   add_cmd ("compare-sections", class_obscure, compare_sections_command, _("\
 Compare section data on target to the exec file.\n\
-Argument is a single section name (default: all loaded sections)."),
+Argument is a single section name (default: all loaded sections).\n\
+To compare only read-only loaded sections, specify the -r option."),
 	   &cmdlist);
 
   add_cmd ("packet", class_maintenance, packet_command, _("\
Index: gdb/doc/gdb.texinfo
===================================================================
RCS file: /home/cvsroot/GDB/gdb/doc/gdb.texinfo,v
retrieving revision 1.1.1.2
diff -u -r1.1.1.2 gdb.texinfo
--- gdb/doc/gdb.texinfo	18 Feb 2014 15:36:03 -0000	1.1.1.2
+++ gdb/doc/gdb.texinfo	26 Mar 2014 15:49:40 -0000
@@ -8760,11 +8760,12 @@
 
 @table @code
 @kindex compare-sections
-@item compare-sections @r{[}@var{section-name}@r{]}
+@item compare-sections @r{[}@var{section-name}@r{|}@code{-r}@r{]}
 Compare the data of a loadable section @var{section-name} in the
 executable file of the program being debugged with the same section in
 the remote machine's memory, and report any mismatches.  With no
-arguments, compares all loadable sections.  This command's
+arguments, compares all loadable sections.  With an argument of
+@code{-r}, compares all loadable read-only sections.  This command's
 availability depends on the target's support for the @code{"qCRC"}
 remote request.
 @end table

Re: RFA/remote: compare-sections

[Eli Zaretskii]

> From: David Taylor <dtaylor@emc.com>
> Date: Wed, 26 Mar 2014 12:00:10 -0400
> 
> For gdb/ChangeLog:
> 
> 2014-03-26  David Taylor  <dtaylor@emc.com>
> 
> 	* remote.c (compare_sections_command): Add -r option to compare
> 	all loadable read-only sections.
> 
> For gdb/doc/ChangeLog:
> 
> 2014-03-26  David Taylor  <dtaylor@emc.com>
> 
> 	* gdb.texinfo (compare-sections): Document the new -r (read-only)
> 	option.

OK for the documentation part, but I think this warrants a NEWS entry.

Thanks.

Re: RFA/remote: compare-sections

[David Taylor]

ping.

David Taylor <dtaylor@emc.com> wrote:

> Motivation:
> 
> When connecting to a remote system, we use the compare-sections command
> to verify that the box is running the code that we think it is running.
> Since the system is up and running and *NOT* 'freshly downloaded without
> yet executing anything', read-write sections, of course, differ from
> what they were in the executable file.
> 
> Comparing read-write sections takes time and more importantly the
> MIS-MATCHED output is confusing to some users.
> 
> The compare-sections command compares all loadable sections including
> read-write sections.  This patch gives the user the option to compare
> just the loadable read-only sections.
> 
> For gdb/ChangeLog:
> 
> 2014-03-26  David Taylor  <dtaylor@emc.com>
> 
> 	* remote.c (compare_sections_command): Add -r option to compare
> 	all loadable read-only sections.
> 
> For gdb/doc/ChangeLog:
> 
> 2014-03-26  David Taylor  <dtaylor@emc.com>
> 
> 	* gdb.texinfo (compare-sections): Document the new -r (read-only)
> 	option.
> 
> I'm not sure that this patch is big enough to require a copyright
> assignment, but regardless, EMC has a copyright assignment on file for
> GDB (and GCC and BINUTILS as well).
> 
> If it is approved, I will need someone else to commit it as I don't have
> write access.
> 
> Patch:
> 
> Index: gdb/remote.c
> ===================================================================
> RCS file: /home/cvsroot/GDB/gdb/remote.c,v
> retrieving revision 1.8
> diff -u -r1.8 remote.c
> --- gdb/remote.c	26 Mar 2014 14:12:34 -0000	1.8
> +++ gdb/remote.c	26 Mar 2014 15:49:40 -0000
> @@ -8664,6 +8664,7 @@
>    int matched = 0;
>    int mismatched = 0;
>    int res;
> +  int read_only = 0;
>  
>    if (!exec_bfd)
>      error (_("command cannot be used without an exec file"));
> @@ -8671,11 +8672,20 @@
>    /* Make sure the remote is pointing at the right process.  */
>    set_general_process ();
>  
> +  if (args && (strcmp (args, "-r") == 0))
> +    {
> +      read_only = 1;
> +      args = NULL;
> +    }
> +
>    for (s = exec_bfd->sections; s; s = s->next)
>      {
>        if (!(s->flags & SEC_LOAD))
>  	continue;		/* Skip non-loadable section.  */
>  
> +      if (read_only && ((s->flags & SEC_READONLY) == 0))
> +	continue;		/* Skip writeable sections */
> +
>        size = bfd_get_section_size (s);
>        if (size == 0)
>  	continue;		/* Skip zero-length section.  */
> @@ -12046,7 +12056,8 @@
>  
>    add_cmd ("compare-sections", class_obscure, compare_sections_command, _("\
>  Compare section data on target to the exec file.\n\
> -Argument is a single section name (default: all loaded sections)."),
> +Argument is a single section name (default: all loaded sections).\n\
> +To compare only read-only loaded sections, specify the -r option."),
>  	   &cmdlist);
>  
>    add_cmd ("packet", class_maintenance, packet_command, _("\
> Index: gdb/doc/gdb.texinfo
> ===================================================================
> RCS file: /home/cvsroot/GDB/gdb/doc/gdb.texinfo,v
> retrieving revision 1.1.1.2
> diff -u -r1.1.1.2 gdb.texinfo
> --- gdb/doc/gdb.texinfo	18 Feb 2014 15:36:03 -0000	1.1.1.2
> +++ gdb/doc/gdb.texinfo	26 Mar 2014 15:49:40 -0000
> @@ -8760,11 +8760,12 @@
>  
>  @table @code
>  @kindex compare-sections
> -@item compare-sections @r{[}@var{section-name}@r{]}
> +@item compare-sections @r{[}@var{section-name}@r{|}@code{-r}@r{]}
>  Compare the data of a loadable section @var{section-name} in the
>  executable file of the program being debugged with the same section in
>  the remote machine's memory, and report any mismatches.  With no
> -arguments, compares all loadable sections.  This command's
> +arguments, compares all loadable sections.  With an argument of
> +@code{-r}, compares all loadable read-only sections.  This command's
>  availability depends on the target's support for the @code{"qCRC"}
>  remote request.
>  @end table
> 

Re: RFA/remote: compare-sections

[Eli Zaretskii]

> Date: Thu, 03 Apr 2014 16:38:49 -0400
> From: David Taylor <dtaylor@emc.com>
> 
> ping.

I already approved the documentation part.

Re: RFA/remote: compare-sections

[David Taylor]

Eli Zaretskii <eliz@gnu.org> wrote:

> > Date: Thu, 03 Apr 2014 16:38:49 -0400
> > From: David Taylor <dtaylor@emc.com>
> > 
> > ping.
> 
> I already approved the documentation part.

Yes, thank you.  I have never heard from anyone about the
non-documentation parts.

Re: RFA/remote: compare-sections

[David Taylor]

Final ping.  I initially posted this three weeks ago.  Eli Zaretskii
immediately approved the doc part.  The remainder has been ignored.
I pinged two weeks ago.  The remainder continues to be unreviewed.

We (at EMC) find it useful.  I will not ping further on it.  I hope it
goes in eventually, but have resigned myself to having to maintain it
as a local change.  Sigh.

EMC has a copyright assignment on file with the FSF (plus this is small
enought that I'm not sure it is copyrightable), so anyone who wishes to
do so should feel free to incorporate it into their local versions of
GDB.

David Taylor <dtaylor@emc.com> wrote:
 
> ping.
> 
> David Taylor <dtaylor@emc.com> wrote:
> 
> > Motivation:
> > 
> > When connecting to a remote system, we use the compare-sections command
> > to verify that the box is running the code that we think it is running.
> > Since the system is up and running and *NOT* 'freshly downloaded without
> > yet executing anything', read-write sections, of course, differ from
> > what they were in the executable file.
> > 
> > Comparing read-write sections takes time and more importantly the
> > MIS-MATCHED output is confusing to some users.
> > 
> > The compare-sections command compares all loadable sections including
> > read-write sections.  This patch gives the user the option to compare
> > just the loadable read-only sections.
> > 
> > For gdb/ChangeLog:
> > 
> > 2014-03-26  David Taylor  <dtaylor@emc.com>
> > 
> > 	* remote.c (compare_sections_command): Add -r option to compare
> > 	all loadable read-only sections.
> > 
> > For gdb/doc/ChangeLog:
> > 
> > 2014-03-26  David Taylor  <dtaylor@emc.com>
> > 
> > 	* gdb.texinfo (compare-sections): Document the new -r (read-only)
> > 	option.
> > 
> > I'm not sure that this patch is big enough to require a copyright
> > assignment, but regardless, EMC has a copyright assignment on file for
> > GDB (and GCC and BINUTILS as well).
> > 
> > If it is approved, I will need someone else to commit it as I don't have
> > write access.
> > 
> > Patch:
> > 
> > Index: gdb/remote.c
> > ===================================================================
> > RCS file: /home/cvsroot/GDB/gdb/remote.c,v
> > retrieving revision 1.8
> > diff -u -r1.8 remote.c
> > --- gdb/remote.c	26 Mar 2014 14:12:34 -0000	1.8
> > +++ gdb/remote.c	26 Mar 2014 15:49:40 -0000
> > @@ -8664,6 +8664,7 @@
> >    int matched = 0;
> >    int mismatched = 0;
> >    int res;
> > +  int read_only = 0;
> >  
> >    if (!exec_bfd)
> >      error (_("command cannot be used without an exec file"));
> > @@ -8671,11 +8672,20 @@
> >    /* Make sure the remote is pointing at the right process.  */
> >    set_general_process ();
> >  
> > +  if (args && (strcmp (args, "-r") == 0))
> > +    {
> > +      read_only = 1;
> > +      args = NULL;
> > +    }
> > +
> >    for (s = exec_bfd->sections; s; s = s->next)
> >      {
> >        if (!(s->flags & SEC_LOAD))
> >  	continue;		/* Skip non-loadable section.  */
> >  
> > +      if (read_only && ((s->flags & SEC_READONLY) == 0))
> > +	continue;		/* Skip writeable sections */
> > +
> >        size = bfd_get_section_size (s);
> >        if (size == 0)
> >  	continue;		/* Skip zero-length section.  */
> > @@ -12046,7 +12056,8 @@
> >  
> >    add_cmd ("compare-sections", class_obscure, compare_sections_command, _("\
> >  Compare section data on target to the exec file.\n\
> > -Argument is a single section name (default: all loaded sections)."),
> > +Argument is a single section name (default: all loaded sections).\n\
> > +To compare only read-only loaded sections, specify the -r option."),
> >  	   &cmdlist);
> >  
> >    add_cmd ("packet", class_maintenance, packet_command, _("\
> > Index: gdb/doc/gdb.texinfo
> > ===================================================================
> > RCS file: /home/cvsroot/GDB/gdb/doc/gdb.texinfo,v
> > retrieving revision 1.1.1.2
> > diff -u -r1.1.1.2 gdb.texinfo
> > --- gdb/doc/gdb.texinfo	18 Feb 2014 15:36:03 -0000	1.1.1.2
> > +++ gdb/doc/gdb.texinfo	26 Mar 2014 15:49:40 -0000
> > @@ -8760,11 +8760,12 @@
> >  
> >  @table @code
> >  @kindex compare-sections
> > -@item compare-sections @r{[}@var{section-name}@r{]}
> > +@item compare-sections @r{[}@var{section-name}@r{|}@code{-r}@r{]}
> >  Compare the data of a loadable section @var{section-name} in the
> >  executable file of the program being debugged with the same section in
> >  the remote machine's memory, and report any mismatches.  With no
> > -arguments, compares all loadable sections.  This command's
> > +arguments, compares all loadable sections.  With an argument of
> > +@code{-r}, compares all loadable read-only sections.  This command's
> >  availability depends on the target's support for the @code{"qCRC"}
> >  remote request.
> >  @end table
> > 
> 

Re: RFA/remote: compare-sections

[Keith Seitz]

On 04/16/2014 09:43 AM, David Taylor wrote:
> Final ping.

Hi, David,

I am really sorry that patch reviewing is going so slowly right now. It 
appears the few active maintainers we have are (extremely) busy with 
"work." As it is, I think maintainers are probably several /months/ 
behind on patch reviews. There are neither enough people to review 
patches nor enough maintainers to approve patches after random folk 
(like me) make recommendations.

So please don't feel that you've been singled out. It is a growing pain 
that gdb is experiencing right now.

>>> Motivation:
>>>
>>> When connecting to a remote system, we use the compare-sections command
>>> to verify that the box is running the code that we think it is running.
>>> Since the system is up and running and *NOT* 'freshly downloaded without
>>> yet executing anything', read-write sections, of course, differ from
>>> what they were in the executable file.
>>>
>>> Comparing read-write sections takes time and more importantly the
>>> MIS-MATCHED output is confusing to some users.
>>>
>>> The compare-sections command compares all loadable sections including
>>> read-write sections.  This patch gives the user the option to compare
>>> just the loadable read-only sections.

This is excellent. Thank you for providing this information. I found it 
very useful.

>>> Index: gdb/remote.c
>>> ===================================================================
>>> RCS file: /home/cvsroot/GDB/gdb/remote.c,v
>>> retrieving revision 1.8
>>> diff -u -r1.8 remote.c
>>> --- gdb/remote.c	26 Mar 2014 14:12:34 -0000	1.8
>>> +++ gdb/remote.c	26 Mar 2014 15:49:40 -0000
>>> @@ -8664,6 +8664,7 @@
>>>     int matched = 0;
>>>     int mismatched = 0;
>>>     int res;
>>> +  int read_only = 0;
>>>
>>>     if (!exec_bfd)
>>>       error (_("command cannot be used without an exec file"));
>>> @@ -8671,11 +8672,20 @@
>>>     /* Make sure the remote is pointing at the right process.  */
>>>     set_general_process ();
>>>
>>> +  if (args && (strcmp (args, "-r") == 0))
>>> +    {
>>> +      read_only = 1;
>>> +      args = NULL;
>>> +    }

Two (and one-half) things:

1) It was recently agreed that we would now explicitly check pointers 
against NULL.

2) I think it better/more consistent to use check_for_argument (in 
cli/cli-utils.[ch]) for this:

   if (args != NULL
       && check_for_argument (&args, "-r", sizeof ("-r") - 1))
     read_only = 1;

2.5) I'm a testing freak, so I'd really like to see is a test case. 
Alas, I see that there are no tests /at all/ for compare-sections, so 
that may be heaping more hardship onto this than is either necessary or 
plausible. I am not going to ask you to provide one because of this, but 
a global maintainer might.

In any case, with the two changes above, I recommend that this patch be 
approved by a maintainer.

Keith

>>> +
>>>     for (s = exec_bfd->sections; s; s = s->next)
>>>       {
>>>         if (!(s->flags & SEC_LOAD))
>>>   	continue;		/* Skip non-loadable section.  */
>>>
>>> +      if (read_only && ((s->flags & SEC_READONLY) == 0))
>>> +	continue;		/* Skip writeable sections */
>>> +
>>>         size = bfd_get_section_size (s);
>>>         if (size == 0)
>>>   	continue;		/* Skip zero-length section.  */
>>> @@ -12046,7 +12056,8 @@
>>>
>>>     add_cmd ("compare-sections", class_obscure, compare_sections_command, _("\
>>>   Compare section data on target to the exec file.\n\
>>> -Argument is a single section name (default: all loaded sections)."),
>>> +Argument is a single section name (default: all loaded sections).\n\
>>> +To compare only read-only loaded sections, specify the -r option."),
>>>   	   &cmdlist);
>>>
>>>     add_cmd ("packet", class_maintenance, packet_command, _("\
>>> Index: gdb/doc/gdb.texinfo
>>> ===================================================================
>>> RCS file: /home/cvsroot/GDB/gdb/doc/gdb.texinfo,v
>>> retrieving revision 1.1.1.2
>>> diff -u -r1.1.1.2 gdb.texinfo
>>> --- gdb/doc/gdb.texinfo	18 Feb 2014 15:36:03 -0000	1.1.1.2
>>> +++ gdb/doc/gdb.texinfo	26 Mar 2014 15:49:40 -0000
>>> @@ -8760,11 +8760,12 @@
>>>
>>>   @table @code
>>>   @kindex compare-sections
>>> -@item compare-sections @r{[}@var{section-name}@r{]}
>>> +@item compare-sections @r{[}@var{section-name}@r{|}@code{-r}@r{]}
>>>   Compare the data of a loadable section @var{section-name} in the
>>>   executable file of the program being debugged with the same section in
>>>   the remote machine's memory, and report any mismatches.  With no
>>> -arguments, compares all loadable sections.  This command's
>>> +arguments, compares all loadable sections.  With an argument of
>>> +@code{-r}, compares all loadable read-only sections.  This command's
>>>   availability depends on the target's support for the @code{"qCRC"}
>>>   remote request.
>>>   @end table
>>>
>>

[PATCH, doc RFA] Make compare-sections work against all targets; add, compare-sections [-r] tests

[Pedro Alves]

On 04/16/2014 11:30 PM, Keith Seitz wrote:

> 2.5) I'm a testing freak, so I'd really like to see is a test case. 
> Alas, I see that there are no tests /at all/ for compare-sections, so 
> that may be heaping more hardship onto this than is either necessary or 
> plausible. I am not going to ask you to provide one because of this, but 
> a global maintainer might.

Yeah, I ended up not requesting one, as it could reasonable be argued
that it'd not be that fair, but I wrote one now.
While at it, I made "compare-sections" work against all targets...
Let me know what you think.

Eli, docs OK?

--------------
From 9e373fa2e67b1f2e63e17e0c2872a479158c8842 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 2 May 2014 00:23:12 +0100
Subject: [PATCH] Make compare-sections work against all targets; add
 compare-sections [-r] tests

This does two things:

1. Adds a test.

Recently compare-sections got a new "-r" switch, but given no test
existed for compare-sections, the patch was allowed in with no
testsuite addition.  Adds now adds a test for both compare-sections
and compare-sections -r.

2. Makes the compare-sections command work against all targets.

Currently, it only works with remote targets, and only those that
support the qCRC packet.  The way this works is that if the target
doesn't support accelerating memory verification, then GDB falls back
to comparing memory itself.  This is of course slower, but it's better
than nothing, IMO.  While testing against extended-remote gdbserver I
noticed that we send the qCRC request to the target if we're connect,
but not running a program.  That can't work of course.  The patch
fixes that.  This also goes in the direction of bridging the
local/remote parity gap.

I didn't decouple 1. from 2., because that would mean that the test
would need to handle the case of the target not supporting the
command.

Tested on x86_64 Fedora 17, native, remote gdbserver, and
extended-remote gdbserver.

gdb/doc/
2014-05-02  Pedro Alves  <palves@redhat.com>

	* gdb.texinfo (Memory) <compare-sections>: Generalize comments to
	not be remote specific.  Add cross reference to the qCRC packet.
	(Separate Debug Files): Update cross reference to the qCRC packet.
	(General Query Packets) <qCRC packet>: Add anchor.

gdb/
2014-05-02  Pedro Alves  <palves@redhat.com>

	* NEWS: Mention that compare-sections now works with all targets.

	* remote.c (PACKET_qCRC): New enum value.
	(remote_verify_memory): Don't send qCRC if the target has no
	execution.  Use packet_support/packet_ok.  If the target doesn't
	support the qCRC packet, fallback to a deep memory copy.
	(compare_sections_command): Say "target image" instead of "remote
	executable".
	(_initialize_remote): Add PACKET_qCRC to the list of config
	packets that have no associated command.  Extend comment.
	* target.c (simple_verify_memory, default_verify_memory): New
	function.
	* target.h (struct target_ops) <to_verify_memory>: Default to
	default_verify_memory.
	(simple_verify_memory): New declaration.
	* target-delegates.c: Regenerate.

gdb/testsuite/
2014-05-02  Pedro Alves  <palves@redhat.com>

	* gdb.base/compare-sections.c: New file.
	* gdb.base/compare-sections.exp: New file.
---
 gdb/NEWS                                    |   3 +
 gdb/doc/gdb.texinfo                         |  31 +++---
 gdb/remote.c                                |  52 ++++++----
 gdb/target-delegates.c                      |   8 +-
 gdb/target.c                                |  47 +++++++++
 gdb/target.h                                |  10 +-
 gdb/testsuite/gdb.base/compare-sections.c   |  26 +++++
 gdb/testsuite/gdb.base/compare-sections.exp | 152 ++++++++++++++++++++++++++++
 8 files changed, 289 insertions(+), 40 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/compare-sections.c
 create mode 100644 gdb/testsuite/gdb.base/compare-sections.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index d0c44ea..d8799f2 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -89,6 +89,9 @@ maint ada show ignore-descriptive-types
 
 * The "catch syscall" command now works on s390*-linux* targets.
 
+* The "compare-sections" command is no longer specific to target
+  remote.  It now works with all targets.
+
 * New remote packets
 
 qXfer:btrace:read's annex
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 7f7650d..ef93403 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -8763,23 +8763,28 @@ are from the last memory unit printed; this is not the same as the last
 address printed if several units were printed on the last line of output.
 
 @cindex remote memory comparison
+@cindex target memory comparison
 @cindex verify remote memory image
+@cindex verify target memory image
 When you are debugging a program running on a remote target machine
-(@pxref{Remote Debugging}), you may wish to verify the program's image in the
-remote machine's memory against the executable file you downloaded to
-the target.  The @code{compare-sections} command is provided for such
-situations.
+(@pxref{Remote Debugging}), you may wish to verify the program's image
+in the remote machine's memory against the executable file you
+downloaded to the target.  Or, on any target, you may want to check
+whether the program has corrupted its own read-only sections.  The
+@code{compare-sections} command is provided for such situations.
 
 @table @code
 @kindex compare-sections
 @item compare-sections @r{[}@var{section-name}@r{|}@code{-r}@r{]}
 Compare the data of a loadable section @var{section-name} in the
 executable file of the program being debugged with the same section in
-the remote machine's memory, and report any mismatches.  With no
+the target machine's memory, and report any mismatches.  With no
 arguments, compares all loadable sections.  With an argument of
-@code{-r}, compares all loadable read-only sections.  This command's
-availability depends on the target's support for the @code{"qCRC"}
-remote request.
+@code{-r}, compares all loadable read-only sections.
+
+Note: for remote targets, this command can be accelerated if the
+target supports computing the CRC checksum of a block of memory
+(@pxref{qCRC packet}).
 @end table
 
 @node Auto Display
@@ -17577,11 +17582,10 @@ the final result is inverted to ensure trailing zeros also affect the
 CRC.
 
 @emph{Note:} This is the same CRC polynomial as used in handling the
-@dfn{Remote Serial Protocol} @code{qCRC} packet (@pxref{Remote Protocol,
-, @value{GDBN} Remote Serial Protocol}).  However in the
-case of the Remote Serial Protocol, the CRC is computed @emph{most}
-significant bit first, and the result is not inverted, so trailing
-zeros have no effect on the CRC value.
+@dfn{Remote Serial Protocol} @code{qCRC} packet (@pxref{qCRC packet}).
+However in the case of the Remote Serial Protocol, the CRC is computed
+@emph{most} significant bit first, and the result is not inverted, so
+trailing zeros have no effect on the CRC value.
 
 To complete the description, we show below the code of the function
 which produces the CRC used in @code{.gnu_debuglink}.  Inverting the
@@ -34720,6 +34724,7 @@ Any other reply implies the old thread ID.
 @item qCRC:@var{addr},@var{length}
 @cindex CRC of memory block, remote request
 @cindex @samp{qCRC} packet
+@anchor{qCRC packet}
 Compute the CRC checksum of a block of memory using CRC-32 defined in
 IEEE 802.3.  The CRC is computed byte at a time, taking the most
 significant bit of each byte first.  The initial pattern code
diff --git a/gdb/remote.c b/gdb/remote.c
index ba04d0c..964bd41 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1273,6 +1273,7 @@ enum {
   PACKET_qTStatus,
   PACKET_QPassSignals,
   PACKET_QProgramSignals,
+  PACKET_qCRC,
   PACKET_qSearch_memory,
   PACKET_vAttach,
   PACKET_vRun,
@@ -8441,29 +8442,40 @@ remote_verify_memory (struct target_ops *ops,
   unsigned long host_crc, target_crc;
   char *tmp;
 
-  /* Make sure the remote is pointing at the right process.  */
-  set_general_process ();
+  /* It doesn't make sense to use qCRC if the remote target is
+     connected but not running.  */
+  if (target_has_execution && packet_support (PACKET_qCRC) != PACKET_DISABLE)
+    {
+      enum packet_result result;
 
-  /* FIXME: assumes lma can fit into long.  */
-  xsnprintf (rs->buf, get_remote_packet_size (), "qCRC:%lx,%lx",
-	     (long) lma, (long) size);
-  putpkt (rs->buf);
+      /* Make sure the remote is pointing at the right process.  */
+      set_general_process ();
 
-  /* Be clever; compute the host_crc before waiting for target
-     reply.  */
-  host_crc = xcrc32 (data, size, 0xffffffff);
+      /* FIXME: assumes lma can fit into long.  */
+      xsnprintf (rs->buf, get_remote_packet_size (), "qCRC:%lx,%lx",
+		 (long) lma, (long) size);
+      putpkt (rs->buf);
 
-  getpkt (&rs->buf, &rs->buf_size, 0);
-  if (rs->buf[0] == 'E')
-    return -1;
+      /* Be clever; compute the host_crc before waiting for target
+	 reply.  */
+      host_crc = xcrc32 (data, size, 0xffffffff);
+
+      getpkt (&rs->buf, &rs->buf_size, 0);
 
-  if (rs->buf[0] != 'C')
-    error (_("remote target does not support this operation"));
+      result = packet_ok (rs->buf,
+			  &remote_protocol_packets[PACKET_qCRC]);
+      if (result == PACKET_ERROR)
+	return -1;
+      else if (result == PACKET_OK)
+	{
+	  for (target_crc = 0, tmp = &rs->buf[1]; *tmp; tmp++)
+	    target_crc = target_crc * 16 + fromhex (*tmp);
 
-  for (target_crc = 0, tmp = &rs->buf[1]; *tmp; tmp++)
-    target_crc = target_crc * 16 + fromhex (*tmp);
+	  return (host_crc == target_crc);
+	}
+    }
 
-  return (host_crc == target_crc);
+  return simple_verify_memory (ops, data, lma, size);
 }
 
 /* compare-sections command
@@ -8542,7 +8554,7 @@ compare_sections_command (char *args, int from_tty)
       do_cleanups (old_chain);
     }
   if (mismatched > 0)
-    warning (_("One or more sections of the remote executable does not match\n\
+    warning (_("One or more sections of the target image does not match\n\
 the loaded file\n"));
   if (args && !matched)
     printf_filtered (_("No loaded section named '%s'.\n"), args);
@@ -12097,7 +12109,9 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
 	  case PACKET_tracenz_feature:
 	  case PACKET_DisconnectedTracing_feature:
 	  case PACKET_augmented_libraries_svr4_read_feature:
-	    /* Additions to this list need to be well justified.  */
+	  case PACKET_qCRC:
+	    /* Additions to this list need to be well justified:
+	       pre-existing packets are OK; new packets are not.  */
 	    excepted = 1;
 	    break;
 	  default:
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index 87ce0d1..0e95cf0 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -1249,12 +1249,6 @@ delegate_verify_memory (struct target_ops *self, const gdb_byte *arg1, CORE_ADDR
 }
 
 static int
-tdefault_verify_memory (struct target_ops *self, const gdb_byte *arg1, CORE_ADDR arg2, ULONGEST arg3)
-{
-  tcomplain ();
-}
-
-static int
 delegate_get_tib_address (struct target_ops *self, ptid_t arg1, CORE_ADDR *arg2)
 {
   self = self->beneath;
@@ -2003,7 +1997,7 @@ install_dummy_methods (struct target_ops *ops)
   ops->to_set_trace_buffer_size = tdefault_set_trace_buffer_size;
   ops->to_set_trace_notes = tdefault_set_trace_notes;
   ops->to_core_of_thread = tdefault_core_of_thread;
-  ops->to_verify_memory = tdefault_verify_memory;
+  ops->to_verify_memory = default_verify_memory;
   ops->to_get_tib_address = tdefault_get_tib_address;
   ops->to_set_permissions = tdefault_set_permissions;
   ops->to_static_tracepoint_marker_at = tdefault_static_tracepoint_marker_at;
diff --git a/gdb/target.c b/gdb/target.c
index 1b48f79..7c73f73 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -73,6 +73,10 @@ static int default_search_memory (struct target_ops *ops,
 				  ULONGEST pattern_len,
 				  CORE_ADDR *found_addrp);
 
+static int default_verify_memory (struct target_ops *self,
+				  const gdb_byte *data,
+				  CORE_ADDR memaddr, ULONGEST size);
+
 static void tcomplain (void) ATTRIBUTE_NORETURN;
 
 static int return_zero (struct target_ops *);
@@ -3261,6 +3265,49 @@ target_core_of_thread (ptid_t ptid)
 }
 
 int
+simple_verify_memory (struct target_ops *ops,
+		      const gdb_byte *data, CORE_ADDR lma, ULONGEST size)
+{
+  LONGEST total_xfered = 0;
+
+  while (total_xfered < size)
+    {
+      ULONGEST xfered_len;
+      enum target_xfer_status status;
+      gdb_byte buf[1024];
+      ULONGEST howmuch = min (sizeof (buf), size - total_xfered);
+
+      status = target_xfer_partial (ops, TARGET_OBJECT_MEMORY, NULL,
+				    buf, NULL, lma + total_xfered, howmuch,
+				    &xfered_len);
+
+      if (status == TARGET_XFER_EOF)
+	return 0;
+      else if (status == TARGET_XFER_OK)
+	{
+	  if (memcmp (data + total_xfered, buf, xfered_len) != 0)
+	    return 0;
+	  total_xfered += xfered_len;
+	  QUIT;
+	}
+      else
+	return 0;
+    }
+  return 1;
+}
+
+/* Default implementation of memory verification.  */
+
+static int
+default_verify_memory (struct target_ops *self,
+		       const gdb_byte *data, CORE_ADDR memaddr, ULONGEST size)
+{
+  /* Start over from the top of the target stack.  */
+  return simple_verify_memory (current_target.beneath,
+			       data, memaddr, size);
+}
+
+int
 target_verify_memory (const gdb_byte *data, CORE_ADDR memaddr, ULONGEST size)
 {
   int retval = current_target.to_verify_memory (&current_target,
diff --git a/gdb/target.h b/gdb/target.h
index 492ecb5..23a7566 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -941,7 +941,7 @@ struct target_ops
        encountered while reading memory.  */
     int (*to_verify_memory) (struct target_ops *, const gdb_byte *data,
 			     CORE_ADDR memaddr, ULONGEST size)
-      TARGET_DEFAULT_NORETURN (tcomplain ());
+      TARGET_DEFAULT_FUNC (default_verify_memory);
 
     /* Return the address of the start of the Thread Information Block
        a Windows OS specific feature.  */
@@ -2005,6 +2005,14 @@ extern const struct frame_unwind *target_get_unwinder (void);
 /* See to_get_tailcall_unwinder in struct target_ops.  */
 extern const struct frame_unwind *target_get_tailcall_unwinder (void);
 
+/* This implements basic memory verification, reading target memory
+   and performing the comparison here (as opposed to accelerated
+   verification making use of the qCRC packet, for example).  */
+
+extern int simple_verify_memory (struct target_ops* ops,
+				 const gdb_byte *data,
+				 CORE_ADDR memaddr, ULONGEST size);
+
 /* Verify that the memory in the [MEMADDR, MEMADDR+SIZE) range matches
    the contents of [DATA,DATA+SIZE).  Returns 1 if there's a match, 0
    if there's a mismatch, and -1 if an error is encountered while
diff --git a/gdb/testsuite/gdb.base/compare-sections.c b/gdb/testsuite/gdb.base/compare-sections.c
new file mode 100644
index 0000000..1a75dd3
--- /dev/null
+++ b/gdb/testsuite/gdb.base/compare-sections.c
@@ -0,0 +1,26 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2011-2014 Free Software Foundation, Inc.
+
+   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/>.  */
+
+int bss_variable = 0;
+int data_variable = 1;
+const int const_variable = 2;
+
+int
+main (void)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/compare-sections.exp b/gdb/testsuite/gdb.base/compare-sections.exp
new file mode 100644
index 0000000..cec21fd
--- /dev/null
+++ b/gdb/testsuite/gdb.base/compare-sections.exp
@@ -0,0 +1,152 @@
+# Copyright 2014 Free Software Foundation, Inc.
+
+# 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 the compare-sections command.
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug}]} {
+    return -1
+}
+
+# Run the compare-sections command along with any options as specified
+# by OPTIONS, and check that no mismatch is found.
+proc compare_sections { {options ""} } {
+    global gdb_prompt
+
+    if {$options != ""} {
+	set command "compare-sections $options"
+    } else {
+	set command "compare-sections"
+    }
+    set test $command
+    gdb_test_multiple $command $test {
+	-re "MIS-MATCHED.*$gdb_prompt $" {
+	    fail $test
+	}
+	-re "warning.*One or more sections.*does not match.*loaded file.*$gdb_prompt $" {
+	    fail $test
+	}
+	-re "Section.*matched.*$gdb_prompt $" {
+	    pass $test
+	}
+    }
+}
+
+gdb_file_cmd $binfile
+
+with_test_prefix "after file" {
+    # Before starting the target, we're just comparing the
+    # executable's sections against themselves...  This should never
+    # find a mismatch.
+    compare_sections
+    compare_sections "-r"
+}
+
+# Load the file into the target.
+gdb_reload
+
+with_test_prefix "after reload" {
+    # We're presumabily still stopped at the entry point.  All
+    # sections should match.
+    compare_sections
+    compare_sections "-r"
+}
+
+# Try comparing just one section.  Assume there's a .text section,
+# which is a pretty safe bet.
+set command "compare-sections .text"
+set command_re [string_to_regexp $command]
+set test $command
+gdb_test_multiple $command $test {
+    -re "^$command_re\r\nSection .text, range $hex -- $hex. matched\.\r\n$gdb_prompt $" {
+	pass $test
+    }
+}
+
+# Now get past startup code.
+if ![runto_main] then {
+    fail "Can't run to main"
+    return 0
+}
+
+with_test_prefix "after run to main" {
+    # Assume all targets' startup code changes some loaded variable.
+    gdb_test "compare-sections" \
+	"MIS-MATCHED.*warning.*One or more sections.*does not match.*loaded file"
+
+    # Assume startup code doesn't change read-only sections.
+    compare_sections "-r"
+}
+
+# Now test that "compare-sections -r" works as expected.  Look for an
+# address in a read-only section, patch it, and check that
+# "compare-sections -r" detects a mismatch.
+with_test_prefix "read-only" {
+    set ro_address 0
+    set has_ro_sections 0
+    set test "list read-only sections"
+    gdb_test_multiple "maint info sections READONLY" $test {
+	-re "($hex)->$hex at $hex. \[^\r\n\]* READONLY.*$gdb_prompt $" {
+	    set ro_address $expect_out(1,string)
+	    set has_ro_sections 1
+	    pass $test
+	}
+	-re "$gdb_prompt $" {
+	    pass $test
+	}
+    }
+
+    if {!$has_ro_sections} {
+	unsupported "no read-only sections"
+	return -1;
+    }
+
+    set orig -1
+
+    set test "get value of read-only section"
+    gdb_test_multiple "print /d *(unsigned char *) $ro_address" "$test" {
+	-re " = (\[0-9\]*).*$gdb_prompt $" {
+	    set orig $expect_out(1,string)
+	    pass "$test"
+	}
+    }
+
+    if {$orig == -1} {
+	untested "couldn't read address of read-only section"
+	return -1
+    }
+
+    # Come up with different value.
+    set patch [expr 255 - $orig]
+
+    # Write PATCH to memory.
+    set written -1
+    set test "corrupt read-only section"
+    gdb_test_multiple "print /d *(unsigned char *) $ro_address = $patch" "$test" {
+	-re " = (\[0-9\]*).*$gdb_prompt $" {
+	    set written $expect_out(1,string)
+	    pass "$test"
+	}
+    }
+
+    if { $written != $patch } {
+	unsupported "can't patch read-only section"
+	return -1
+    }
+
+    gdb_test "compare-sections -r" \
+	"MIS-MATCHED.*warning.*One or more sections.*does not match.*loaded file.*"
+}
-- 
1.7.11.7

Re: [PATCH, doc RFA] Make compare-sections work against all targets; add, compare-sections [-r] tests

[Eli Zaretskii]

> Date: Fri, 02 May 2014 01:50:54 +0100
> From: Pedro Alves <palves@redhat.com>
> CC: David Taylor <dtaylor@emc.com>,        "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
> 
> On 04/16/2014 11:30 PM, Keith Seitz wrote:
> 
> > 2.5) I'm a testing freak, so I'd really like to see is a test case. 
> > Alas, I see that there are no tests /at all/ for compare-sections, so 
> > that may be heaping more hardship onto this than is either necessary or 
> > plausible. I am not going to ask you to provide one because of this, but 
> > a global maintainer might.
> 
> Yeah, I ended up not requesting one, as it could reasonable be argued
> that it'd not be that fair, but I wrote one now.
> While at it, I made "compare-sections" work against all targets...
> Let me know what you think.
> 
> Eli, docs OK?

Yes, thanks.

Re: [PATCH, doc RFA] Make compare-sections work against all targets; add, compare-sections [-r] tests

[Pedro Alves]

On 05/02/2014 08:22 AM, Eli Zaretskii wrote:

>> Eli, docs OK?
> 
> Yes, thanks.

Thanks.  Now pushed.

-- 
Pedro Alves

Re: RFA/remote: compare-sections

[Pedro Alves]

On 04/16/2014 05:43 PM, David Taylor wrote:
> Final ping.  I initially posted this three weeks ago.  Eli Zaretskii
> immediately approved the doc part.  The remainder has been ignored.
> I pinged two weeks ago.  The remainder continues to be unreviewed.

As Keith said.  Please don't take it personally.

In his quick reply, Eli also mentioned that this warrants
a NEWS entry.  I agree.  But, I didn't see an updated patch.
Would you like to contribute one?

>>> 2014-03-26  David Taylor  <dtaylor@emc.com>
>>>
>>> 	* remote.c (compare_sections_command): Add -r option to compare
>>> 	all loadable read-only sections.

This is OK.  I've pushed it in, as below.  I took the liberty of
using an explicit NULL check, and removed the redundant parens.

Thank you.

----------
From 95cf3b38cd2fae4a53c8ff12bef21b2ae3fdd0cb Mon Sep 17 00:00:00 2001
From: David Taylor <dtaylor@emc.com>
Date: Thu, 1 May 2014 18:09:43 +0100
Subject: [PATCH] compare-sections: New -r option.

When connecting to a remote system, we use the compare-sections
command to verify that the box is running the code that we think it is
running.  Since the system is up and running and *NOT* 'freshly
downloaded without yet executing anything', read-write sections, of
course, differ from what they were in the executable file.

Comparing read-write sections takes time and more importantly the
MIS-MATCHED output is confusing to some users.

The compare-sections command compares all loadable sections including
read-write sections.  This patch gives the user the option to compare
just the loadable read-only sections.

gdb/
2014-05-01  David Taylor  <dtaylor@emc.com>

	* remote.c (compare_sections_command): Add -r option to compare
	all loadable read-only sections.

gdb/doc/
2014-05-01  David Taylor  <dtaylor@emc.com>

	* gdb.texinfo (compare-sections): Document the new -r (read-only)
	option.
---
 gdb/ChangeLog       |  5 +++++
 gdb/doc/ChangeLog   |  5 +++++
 gdb/doc/gdb.texinfo |  5 +++--
 gdb/remote.c        | 13 ++++++++++++-
 4 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7e5b290..eac6037 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2014-05-01  David Taylor  <dtaylor@emc.com>
+
+	* remote.c (compare_sections_command): Add -r option to compare
+	all loadable read-only sections.
+
 2014-04-30  Siva Chandra Reddy  <sivachandra@google.com>
 
 	* dwarf2loc.c (dwarf2_locexpr_baton_eval,
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index 2b0f686..1e60575 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,8 @@
+2014-05-01  David Taylor  <dtaylor@emc.com>
+
+	* gdb.texinfo (compare-sections): Document the new -r (read-only)
+	option.
+
 2014-04-24  Michael Sturm  <michael.sturm@mintel.com>
 	    Walfred Tedeschi  <walfred.tedeschi@intel.com>
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 9d91075..7f7650d 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -8772,11 +8772,12 @@ situations.
 
 @table @code
 @kindex compare-sections
-@item compare-sections @r{[}@var{section-name}@r{]}
+@item compare-sections @r{[}@var{section-name}@r{|}@code{-r}@r{]}
 Compare the data of a loadable section @var{section-name} in the
 executable file of the program being debugged with the same section in
 the remote machine's memory, and report any mismatches.  With no
-arguments, compares all loadable sections.  This command's
+arguments, compares all loadable sections.  With an argument of
+@code{-r}, compares all loadable read-only sections.  This command's
 availability depends on the target's support for the @code{"qCRC"}
 remote request.
 @end table
diff --git a/gdb/remote.c b/gdb/remote.c
index 4177b39..ba04d0c 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -8484,6 +8484,7 @@ compare_sections_command (char *args, int from_tty)
   int matched = 0;
   int mismatched = 0;
   int res;
+  int read_only = 0;
 
   if (!exec_bfd)
     error (_("command cannot be used without an exec file"));
@@ -8491,11 +8492,20 @@ compare_sections_command (char *args, int from_tty)
   /* Make sure the remote is pointing at the right process.  */
   set_general_process ();
 
+  if (args != NULL && strcmp (args, "-r") == 0)
+    {
+      read_only = 1;
+      args = NULL;
+    }
+
   for (s = exec_bfd->sections; s; s = s->next)
     {
       if (!(s->flags & SEC_LOAD))
 	continue;		/* Skip non-loadable section.  */
 
+      if (read_only && (s->flags & SEC_READONLY) == 0)
+	continue;		/* Skip writeable sections */
+
       size = bfd_get_section_size (s);
       if (size == 0)
 	continue;		/* Skip zero-length section.  */
@@ -11771,7 +11781,8 @@ the packets being used"),
 
   add_cmd ("compare-sections", class_obscure, compare_sections_command, _("\
 Compare section data on target to the exec file.\n\
-Argument is a single section name (default: all loaded sections)."),
+Argument is a single section name (default: all loaded sections).\n\
+To compare only read-only loaded sections, specify the -r option."),
 	   &cmdlist);
 
   add_cmd ("packet", class_maintenance, packet_command, _("\
-- 
1.7.11.7