Re: RFA/remote: compare-sections

[Keith Seitz <keiths at redhat dot com>]

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