Re: [PATCH] Harden tests that deal with memory regions

[Luis Machado <lgustavo at codesourcery dot com>]

On 01/26/2017 07:17 AM, Pedro Alves wrote:
> On 01/23/2017 09:24 PM, Luis Machado wrote:
>
> > 2017-01-23  Luis Machado  <lgustavo@codesourcery.com>
> >
> > 	* lib/gdb-memory.exp: New file.
>
> Do we need "gdb-" in the file name?
>
> What other procedures to you envision being placed here?  Should
> this have "regions" in the file name, like "memory-regions.exp"?
> The file's intro comment talks about memory regions.

I guess we don't really need the gdb prefix. I originally envisioned 
this particular file storing all proc's dealing with memory checks and 
manipulation (though i ended up describing it in a different way).

I wanted to avoid having to add more helper functions to lib/gdb.exp. 
But maybe it wouldn't be a big problem? My instinct is to modularize it.

Either way is fine with me though, lib/gdb.exp or lib/memory.exp.

> > 	* lib/gdb.exp: Load gdb-memory.exp
>
> Missing period.

Thanks.

> > --- a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
> > +++ b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
> > @@ -137,6 +137,9 @@ if ![get_function_bounds "main" main_lo main_hi] {
> >      return -1
> >  }
> >
> > +# Delete all memory regions.
> > +delete_memory_regions
> > +
>
> The comment as-is practically just reads the function name
> in English.  The important detail missing here
> is "target-supplied".  So:
>
>  # Delete all target-supplied memory regions.
>  delete_memory_regions
>
> Likewise in the other spot.

On second thought, i've pulled these comments from the test files. The 
updated proc documentation should be enough. What do you think?

> >  gdb_test_no_output "mem 0x30 0x0 ro"
> >  with_test_prefix "0x30 0x0" {
> >      region_fail "0x20 0x50"
> > diff --git a/gdb/testsuite/lib/gdb-memory.exp b/gdb/testsuite/lib/gdb-memory.exp
> > new file mode 100644
> > index 0000000..3377011
> > --- /dev/null
> > +++ b/gdb/testsuite/lib/gdb-memory.exp
> > @@ -0,0 +1,38 @@
> > +# Copyright 2017 Free Software Foundation, Inc.
>
> The file's non-boilerplate code is copyright 2012, so
> preserve that.  (git show 1591a1e8)

Done.

> > +
> > +# 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/>.
> > +
> > +# This file was written by Fred Fish. (fnf@cygnus.com)
>
> No it wasn't.
>
> > +
> > +# Generic gdb subroutines that should work for any target.  If these
> > +# need to be modified for any target, it can be done with a variable
> > +# or by passing arguments.
>
> Stale comment.

Thanks copy/paste. Fixed.

> > +
> > +# This file holds functions and data dealing with memory regions manipulation.
> > +
> > +# Deletes all the memory regions GDB currently knows about.
> > +
> > +proc delete_memory_regions {} {

I've added the target-supplied bit to this as well.