[PATCH] Harden tests that deal with memory regions

[PATCH] Harden tests that deal with memory regions

[Luis Machado]

Exercising aarch64-elf with a custom debug stub i noticed a few failures in
both gdb.base/breakpoint-in-ro-region.exp and gdb.base/memattr.exp:

FAIL: gdb.base/breakpoint-in-ro-region.exp: create read-only mem region covering main
FAIL: gdb.base/breakpoint-in-ro-region.exp: writing to read-only memory fails
FAIL: gdb.base/breakpoint-in-ro-region.exp: inserting software breakpoint in read-only memory fails

FAIL: gdb.base/memattr.exp: create mem region 1
FAIL: gdb.base/memattr.exp: create mem region 2
FAIL: gdb.base/memattr.exp: create mem region 3
FAIL: gdb.base/memattr.exp: create mem region 4
FAIL: gdb.base/memattr.exp: create mem region 5
FAIL: gdb.base/memattr.exp: info mem (1)
FAIL: gdb.base/memattr.exp: mem1 cannot be read
FAIL: gdb.base/memattr.exp: mem2 cannot be written
FAIL: gdb.base/memattr.exp: mem2 can be read
FAIL: gdb.base/memattr.exp: disable mem 1
FAIL: gdb.base/memattr.exp: mem 1 was disabled
FAIL: gdb.base/memattr.exp: enable mem 1
FAIL: gdb.base/memattr.exp: mem 1 was enabled
FAIL: gdb.base/memattr.exp: disable mem 2 4
FAIL: gdb.base/memattr.exp: mem 2 and 4 were disabled
FAIL: gdb.base/memattr.exp: enable mem 2-4
FAIL: gdb.base/memattr.exp: mem 2-4 were enabled
FAIL: gdb.base/memattr.exp: mem 1 to 5 were disabled
FAIL: gdb.base/memattr.exp: mem 1 to 5 were enabled
FAIL: gdb.base/memattr.exp: delete mem 1
FAIL: gdb.base/memattr.exp: mem 1 was deleted
FAIL: gdb.base/memattr.exp: delete mem 2 4
FAIL: gdb.base/memattr.exp: mem 2 and 4 were deleted
FAIL: gdb.base/memattr.exp: mem 2-4 were deleted

These failures don't show up with gdbserver or native gdb on Linux because
they don't export any memory maps, therefore the vector of memory regions is
empty.

Outside of that scenario, we can't guarantee the absence of memory regions
reported by the target upon a connection. In our particular target, we
provide a memory map and the memory regions vector ceases to be empty.

With a non-empty memory regions vector, manipulating memory regions will cause
gdb to be more verbose and output text. For example:

memattr.c:require_user_regions

  /* Otherwise, let the user know how to get back.  */
  if (from_tty)
    warning (_("Switching to manual control of memory regions; use "
	       "\"mem auto\" to fetch regions from the target again."));

memattr.c:create_mem_region

      if ((lo >= n->lo && (lo < n->hi || n->hi == 0))
	  || (hi > n->lo && (hi <= n->hi || n->hi == 0))
	  || (lo <= n->lo && ((hi >= n->hi && n->hi != 0) || hi == 0)))
	{
	  printf_unfiltered (_("overlapping memory region\n"));
	  return;
	}

In my particular case i got both of the above messages.

In order to fix this, i've moved the delete_memory proc from
gdb.base/memattr.exp to a new file lib/gdb-memory.exp and made lib/gdb.exp
load that file.

For both gdb.base/breakpoint-in-ro-region.exp and gdb.base/memattr.exp the
patch clears all existing memory regions after running to main. That way we
are guaranteed to have a clean state for memory regions so the tests can
exercise whatever they want and have an expected output pattern.

Regression checked on x86-64/Ubuntu 16.04.

OK?

gdb/testsuite/ChangeLog:

2017-01-23  Luis Machado  <lgustavo@codesourcery.com>

	* lib/gdb-memory.exp: New file.
	* lib/gdb.exp: Load gdb-memory.exp
	* gdb.base/memattr.exp (delete_memory): Move proc to
	lib/gdb-memory.exp and rename to delete_memory_regions.
	Replace delete_memory with delete_memory_regions.
	Cleanup memory regions before tests.
	* gdb.base/breakpoint-in-ro-region.exp: Cleanup memory regions
	before tests.
---
 gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp |  3 ++
 gdb/testsuite/gdb.base/memattr.exp                 | 19 +++--------
 gdb/testsuite/lib/gdb-memory.exp                   | 38 ++++++++++++++++++++++
 gdb/testsuite/lib/gdb.exp                          |  1 +
 4 files changed, 47 insertions(+), 14 deletions(-)
 create mode 100644 gdb/testsuite/lib/gdb-memory.exp

diff --git a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
index 7c3bd46..e0c1a37 100644
--- 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
+
 # Manually create a read-only memory region that covers 'main'.
 gdb_test_no_output "mem $main_lo $main_hi ro" \
     "create read-only mem region covering main"
diff --git a/gdb/testsuite/gdb.base/memattr.exp b/gdb/testsuite/gdb.base/memattr.exp
index 5e37187..74f5ce6 100644
--- a/gdb/testsuite/gdb.base/memattr.exp
+++ b/gdb/testsuite/gdb.base/memattr.exp
@@ -25,6 +25,9 @@ if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
 
 runto main
 
+# Delete all memory regions.
+delete_memory_regions
+
 set mem1start -1
 set mem2start -1
 set mem3start -1
@@ -487,18 +490,6 @@ gdb_test "delete mem 8" "No memory region number 8." \
 # Test overlapping checking
 #
 
-proc delete_memory {} {
-    global gdb_prompt
-
-    gdb_test_multiple "delete mem" "delete mem" {
-       -re "Delete all memory regions.*y or n.*$" {
-           send_gdb "y\n"
-           exp_continue
-       }
-       -re "$gdb_prompt $" { }
-    }
-}
-
 # Create a region that doesn't overlap (a PASS in the table).
 
 proc region_pass { region } {
@@ -530,7 +521,7 @@ proc region_fail { region } {
 #                  |--|       PASS
 #                        |--- PASS
 
-delete_memory
+delete_memory_regions
 gdb_test_no_output "mem 0x30 0x60 ro"
 with_test_prefix "0x30 0x60" {
     region_fail "0x20 0x40"
@@ -562,7 +553,7 @@ with_test_prefix "0x30 0x60" {
 #         |--|                PASS
 #   |--|                      PASS
 
-delete_memory
+delete_memory_regions
 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.
+
+# 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)
+
+# 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.
+
+# This file holds functions and data dealing with memory regions manipulation.
+
+# Deletes all the memory regions GDB currently knows about.
+
+proc delete_memory_regions {} {
+    global gdb_prompt
+
+    gdb_test_multiple "delete mem" "delete mem" {
+       -re "Delete all memory regions.*y or n.*$" {
+           send_gdb "y\n"
+           exp_continue
+       }
+       -re "$gdb_prompt $" { }
+    }
+}
+
+
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 2ff5c22..8c64a28 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -28,6 +28,7 @@ if {$tool == ""} {
 load_lib libgloss.exp
 load_lib cache.exp
 load_lib gdb-utils.exp
+load_lib gdb-memory.exp
 
 global GDB
 
-- 
2.7.4

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

[Pedro Alves]

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.

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

Missing period.

> --- 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.

>  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)

> +
> +# 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.

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

Thanks,
Pedro Alves

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

[Luis Machado]

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.

[PATCH,v2] Harden tests that deal with memory regions

[Luis Machado]

Changes in v2:

- Addressed comments by Pedro.
- Removed stale comments, fixed copyright year.
- Updated comment for proc delete_memory_regions.

Exercising aarch64-elf with a custom debug stub i noticed a few failures in
both gdb.base/breakpoint-in-ro-region.exp and gdb.base/memattr.exp:

FAIL: gdb.base/breakpoint-in-ro-region.exp: create read-only mem region covering main
FAIL: gdb.base/breakpoint-in-ro-region.exp: writing to read-only memory fails
FAIL: gdb.base/breakpoint-in-ro-region.exp: inserting software breakpoint in read-only memory fails

FAIL: gdb.base/memattr.exp: create mem region 1
FAIL: gdb.base/memattr.exp: create mem region 2
FAIL: gdb.base/memattr.exp: create mem region 3
FAIL: gdb.base/memattr.exp: create mem region 4
FAIL: gdb.base/memattr.exp: create mem region 5
FAIL: gdb.base/memattr.exp: info mem (1)
FAIL: gdb.base/memattr.exp: mem1 cannot be read
FAIL: gdb.base/memattr.exp: mem2 cannot be written
FAIL: gdb.base/memattr.exp: mem2 can be read
FAIL: gdb.base/memattr.exp: disable mem 1
FAIL: gdb.base/memattr.exp: mem 1 was disabled
FAIL: gdb.base/memattr.exp: enable mem 1
FAIL: gdb.base/memattr.exp: mem 1 was enabled
FAIL: gdb.base/memattr.exp: disable mem 2 4
FAIL: gdb.base/memattr.exp: mem 2 and 4 were disabled
FAIL: gdb.base/memattr.exp: enable mem 2-4
FAIL: gdb.base/memattr.exp: mem 2-4 were enabled
FAIL: gdb.base/memattr.exp: mem 1 to 5 were disabled
FAIL: gdb.base/memattr.exp: mem 1 to 5 were enabled
FAIL: gdb.base/memattr.exp: delete mem 1
FAIL: gdb.base/memattr.exp: mem 1 was deleted
FAIL: gdb.base/memattr.exp: delete mem 2 4
FAIL: gdb.base/memattr.exp: mem 2 and 4 were deleted
FAIL: gdb.base/memattr.exp: mem 2-4 were deleted

These failures don't show up with gdbserver or native gdb on Linux because
they don't export any memory maps, therefore the vector of memory regions is
empty.

Outside of that scenario, we can't guarantee the absence of memory regions
reported by the target upon a connection. In our particular target, we
provide a memory map and the memory regions vector ceases to be empty.

With a non-empty memory regions vector, manipulating memory regions will cause
gdb to be more verbose and output text. For example:

memattr.c:require_user_regions

  /* Otherwise, let the user know how to get back.  */
  if (from_tty)
    warning (_("Switching to manual control of memory regions; use "
	       "\"mem auto\" to fetch regions from the target again."));

memattr.c:create_mem_region

      if ((lo >= n->lo && (lo < n->hi || n->hi == 0))
	  || (hi > n->lo && (hi <= n->hi || n->hi == 0))
	  || (lo <= n->lo && ((hi >= n->hi && n->hi != 0) || hi == 0)))
	{
	  printf_unfiltered (_("overlapping memory region\n"));
	  return;
	}

In my particular case i got both of the above messages.

In order to fix this, i've moved the delete_memory proc from
gdb.base/memattr.exp to a new file lib/gdb-memory.exp and made lib/gdb.exp
load that file.

For both gdb.base/breakpoint-in-ro-region.exp and gdb.base/memattr.exp the
patch clears all existing memory regions after running to main. That way we
are guaranteed to have a clean state for memory regions so the tests can
exercise whatever they want and have an expected output pattern.

Regression checked on x86-64/Ubuntu 16.04.

OK?

gdb/testsuite/ChangeLog:

2017-01-26  Luis Machado  <lgustavo@codesourcery.com>

	* lib/gdb-memory.exp: New file.
	* lib/gdb.exp: Load gdb-memory.exp.
	* gdb.base/memattr.exp (delete_memory): Move proc to
	lib/gdb-memory.exp and rename to delete_memory_regions.
	Replace delete_memory with delete_memory_regions.
	Cleanup memory regions before tests.
	* gdb.base/breakpoint-in-ro-region.exp: Cleanup memory regions
	before tests.
---
 gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp |  2 ++
 gdb/testsuite/gdb.base/memattr.exp                 | 18 +++---------
 gdb/testsuite/lib/gdb-memory.exp                   | 33 ++++++++++++++++++++++
 gdb/testsuite/lib/gdb.exp                          |  1 +
 4 files changed, 40 insertions(+), 14 deletions(-)
 create mode 100644 gdb/testsuite/lib/gdb-memory.exp

diff --git a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
index 7c3bd46..be7903b 100644
--- a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
+++ b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
@@ -137,6 +137,8 @@ if ![get_function_bounds "main" main_lo main_hi] {
     return -1
 }
 
+delete_memory_regions
+
 # Manually create a read-only memory region that covers 'main'.
 gdb_test_no_output "mem $main_lo $main_hi ro" \
     "create read-only mem region covering main"
diff --git a/gdb/testsuite/gdb.base/memattr.exp b/gdb/testsuite/gdb.base/memattr.exp
index 5e37187..df91829 100644
--- a/gdb/testsuite/gdb.base/memattr.exp
+++ b/gdb/testsuite/gdb.base/memattr.exp
@@ -25,6 +25,8 @@ if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
 
 runto main
 
+delete_memory_regions
+
 set mem1start -1
 set mem2start -1
 set mem3start -1
@@ -487,18 +489,6 @@ gdb_test "delete mem 8" "No memory region number 8." \
 # Test overlapping checking
 #
 
-proc delete_memory {} {
-    global gdb_prompt
-
-    gdb_test_multiple "delete mem" "delete mem" {
-       -re "Delete all memory regions.*y or n.*$" {
-           send_gdb "y\n"
-           exp_continue
-       }
-       -re "$gdb_prompt $" { }
-    }
-}
-
 # Create a region that doesn't overlap (a PASS in the table).
 
 proc region_pass { region } {
@@ -530,7 +520,7 @@ proc region_fail { region } {
 #                  |--|       PASS
 #                        |--- PASS
 
-delete_memory
+delete_memory_regions
 gdb_test_no_output "mem 0x30 0x60 ro"
 with_test_prefix "0x30 0x60" {
     region_fail "0x20 0x40"
@@ -562,7 +552,7 @@ with_test_prefix "0x30 0x60" {
 #         |--|                PASS
 #   |--|                      PASS
 
-delete_memory
+delete_memory_regions
 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..c8b3380
--- /dev/null
+++ b/gdb/testsuite/lib/gdb-memory.exp
@@ -0,0 +1,33 @@
+# Copyright 2012 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/>.
+
+# This file holds functions and data dealing with memory checks and other memory
+# manipulation routines.
+
+# Deletes all the target-supplied memory regions GDB currently knows about.
+
+proc delete_memory_regions {} {
+    global gdb_prompt
+
+    gdb_test_multiple "delete mem" "delete mem" {
+       -re "Delete all memory regions.*y or n.*$" {
+           send_gdb "y\n"
+           exp_continue
+       }
+       -re "$gdb_prompt $" { }
+    }
+}
+
+
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 2ff5c22..8c64a28 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -28,6 +28,7 @@ if {$tool == ""} {
 load_lib libgloss.exp
 load_lib cache.exp
 load_lib gdb-utils.exp
+load_lib gdb-memory.exp
 
 global GDB
 
-- 
2.7.4

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

[Pedro Alves]

On 01/26/2017 05:37 PM, Luis Machado wrote:
> 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).

So can you drop it?

> 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.

Sure, I'm not arguing against modularizing.  Only against calling it "memory",
but describing it as "memory ranges".  I'm arguing for picking one
and being consistent throughout.

> 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?

I don't think so.  The important detail is that you call it
_here_, right after starting the target to get rid of
any target-supplied memory region.  While the procedure could
be called at any other point, to delete user-defined regions,
even.

>>> +
>>> +# 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.

That doesn't sound right.  The procedure deletes all memory
regions, either target-supplied, or user defined.

Thanks,
Pedro Alves

[PATCH,v3] Harden tests that deal with memory regions

[Luis Machado]

Changes in v3:

- Fixed up comments.
- Renamed lib/gdb-memory.exp to lib/memory.exp.
- Moved memory region deletion closer to where we run to main in
  gdb.base/breakpoint-in-ro-regions.exp.

Changes in v2:

- Addressed comments by Pedro.
- Removed stale comments, fixed copyright year.
- Updated comment for proc delete_memory_regions.

Exercising aarch64-elf with a custom debug stub i noticed a few failures in
both gdb.base/breakpoint-in-ro-region.exp and gdb.base/memattr.exp:

FAIL: gdb.base/breakpoint-in-ro-region.exp: create read-only mem region covering main
FAIL: gdb.base/breakpoint-in-ro-region.exp: writing to read-only memory fails
FAIL: gdb.base/breakpoint-in-ro-region.exp: inserting software breakpoint in read-only memory fails

FAIL: gdb.base/memattr.exp: create mem region 1
FAIL: gdb.base/memattr.exp: create mem region 2
FAIL: gdb.base/memattr.exp: create mem region 3
FAIL: gdb.base/memattr.exp: create mem region 4
FAIL: gdb.base/memattr.exp: create mem region 5
FAIL: gdb.base/memattr.exp: info mem (1)
FAIL: gdb.base/memattr.exp: mem1 cannot be read
FAIL: gdb.base/memattr.exp: mem2 cannot be written
FAIL: gdb.base/memattr.exp: mem2 can be read
FAIL: gdb.base/memattr.exp: disable mem 1
FAIL: gdb.base/memattr.exp: mem 1 was disabled
FAIL: gdb.base/memattr.exp: enable mem 1
FAIL: gdb.base/memattr.exp: mem 1 was enabled
FAIL: gdb.base/memattr.exp: disable mem 2 4
FAIL: gdb.base/memattr.exp: mem 2 and 4 were disabled
FAIL: gdb.base/memattr.exp: enable mem 2-4
FAIL: gdb.base/memattr.exp: mem 2-4 were enabled
FAIL: gdb.base/memattr.exp: mem 1 to 5 were disabled
FAIL: gdb.base/memattr.exp: mem 1 to 5 were enabled
FAIL: gdb.base/memattr.exp: delete mem 1
FAIL: gdb.base/memattr.exp: mem 1 was deleted
FAIL: gdb.base/memattr.exp: delete mem 2 4
FAIL: gdb.base/memattr.exp: mem 2 and 4 were deleted
FAIL: gdb.base/memattr.exp: mem 2-4 were deleted

These failures don't show up with gdbserver or native gdb on Linux because
they don't export any memory maps, therefore the vector of memory regions is
empty.

Outside of that scenario, we can't guarantee the absence of memory regions
reported by the target upon a connection. In our particular target, we
provide a memory map and the memory regions vector ceases to be empty.

With a non-empty memory regions vector, manipulating memory regions will cause
gdb to be more verbose and output text. For example:

memattr.c:require_user_regions

  /* Otherwise, let the user know how to get back.  */
  if (from_tty)
    warning (_("Switching to manual control of memory regions; use "
	       "\"mem auto\" to fetch regions from the target again."));

memattr.c:create_mem_region

      if ((lo >= n->lo && (lo < n->hi || n->hi == 0))
	  || (hi > n->lo && (hi <= n->hi || n->hi == 0))
	  || (lo <= n->lo && ((hi >= n->hi && n->hi != 0) || hi == 0)))
	{
	  printf_unfiltered (_("overlapping memory region\n"));
	  return;
	}

In my particular case i got both of the above messages.

In order to fix this, i've moved the delete_memory proc from
gdb.base/memattr.exp to a new file lib/memory.exp and made lib/gdb.exp
load that file.

For both gdb.base/breakpoint-in-ro-region.exp and gdb.base/memattr.exp the
patch clears all existing memory regions after running to main. That way we
are guaranteed to have a clean state for memory regions so the tests can
exercise whatever they want and have an expected output pattern.

Regression checked on x86-64/Ubuntu 16.04.

OK?

gdb/testsuite/ChangeLog:

2017-01-26  Luis Machado  <lgustavo@codesourcery.com>

	* lib/memory.exp: New file.
	* lib/gdb.exp: Load memory.exp.
	* gdb.base/memattr.exp (delete_memory): Move proc to
	lib/memory.exp and rename to delete_memory_regions.
	Replace delete_memory with delete_memory_regions.
	Cleanup memory regions before tests.
	* gdb.base/breakpoint-in-ro-region.exp: Cleanup memory regions
	before tests.
---
 gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp |  3 ++
 gdb/testsuite/gdb.base/memattr.exp                 | 21 +++++---------
 gdb/testsuite/lib/gdb.exp                          |  1 +
 gdb/testsuite/lib/memory.exp                       | 33 ++++++++++++++++++++++
 4 files changed, 44 insertions(+), 14 deletions(-)
 create mode 100644 gdb/testsuite/lib/memory.exp

diff --git a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
index 7c3bd46..122b04d 100644
--- a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
+++ b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
@@ -25,6 +25,9 @@ if ![runto main] {
     return -1
 }
 
+# Delete all target-supplied memory regions.
+delete_memory_regions
+
 delete_breakpoints
 
 # Probe for hardware stepping.
diff --git a/gdb/testsuite/gdb.base/memattr.exp b/gdb/testsuite/gdb.base/memattr.exp
index 5e37187..48f0496 100644
--- a/gdb/testsuite/gdb.base/memattr.exp
+++ b/gdb/testsuite/gdb.base/memattr.exp
@@ -25,6 +25,9 @@ if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
 
 runto main
 
+# Delete all target-supplied memory regions.
+delete_memory_regions
+
 set mem1start -1
 set mem2start -1
 set mem3start -1
@@ -487,18 +490,6 @@ gdb_test "delete mem 8" "No memory region number 8." \
 # Test overlapping checking
 #
 
-proc delete_memory {} {
-    global gdb_prompt
-
-    gdb_test_multiple "delete mem" "delete mem" {
-       -re "Delete all memory regions.*y or n.*$" {
-           send_gdb "y\n"
-           exp_continue
-       }
-       -re "$gdb_prompt $" { }
-    }
-}
-
 # Create a region that doesn't overlap (a PASS in the table).
 
 proc region_pass { region } {
@@ -530,7 +521,8 @@ proc region_fail { region } {
 #                  |--|       PASS
 #                        |--- PASS
 
-delete_memory
+# Clear the memory regions list.
+delete_memory_regions
 gdb_test_no_output "mem 0x30 0x60 ro"
 with_test_prefix "0x30 0x60" {
     region_fail "0x20 0x40"
@@ -562,7 +554,8 @@ with_test_prefix "0x30 0x60" {
 #         |--|                PASS
 #   |--|                      PASS
 
-delete_memory
+# Clear the memory regions list.
+delete_memory_regions
 gdb_test_no_output "mem 0x30 0x0 ro"
 with_test_prefix "0x30 0x0" {
     region_fail "0x20 0x50"
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 2ff5c22..48bd7ee 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -28,6 +28,7 @@ if {$tool == ""} {
 load_lib libgloss.exp
 load_lib cache.exp
 load_lib gdb-utils.exp
+load_lib memory.exp
 
 global GDB
 
diff --git a/gdb/testsuite/lib/memory.exp b/gdb/testsuite/lib/memory.exp
new file mode 100644
index 0000000..d41954c
--- /dev/null
+++ b/gdb/testsuite/lib/memory.exp
@@ -0,0 +1,33 @@
+# Copyright 2012 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/>.
+
+# This file holds functions and data dealing with memory checks and other memory
+# manipulation routines.
+
+# Deletes all memory regions GDB currently knows about.
+
+proc delete_memory_regions {} {
+    global gdb_prompt
+
+    gdb_test_multiple "delete mem" "delete mem" {
+       -re "Delete all memory regions.*y or n.*$" {
+           send_gdb "y\n"
+           exp_continue
+       }
+       -re "$gdb_prompt $" { }
+    }
+}
+
+
-- 
2.7.4

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

[Luis Machado]

On 01/26/2017 11:47 AM, Pedro Alves wrote:
> On 01/26/2017 05:37 PM, Luis Machado wrote:
>> 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).
>
> So can you drop it?
>
>> 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.
>
> Sure, I'm not arguing against modularizing.  Only against calling it "memory",
> but describing it as "memory ranges".  I'm arguing for picking one
> and being consistent throughout.
>

Great. I've picked lib/memory.exp as the name in v3.

>> On second thought, i've pulled these comments from the test files. The
>> updated proc documentation should be enough. What do you think?
>
> I don't think so.  The important detail is that you call it
> _here_, right after starting the target to get rid of
> any target-supplied memory region.  While the procedure could
> be called at any other point, to delete user-defined regions,
> even.
>

I've reverted this in v3 and adjusted the comments to be more sensible 
depending on the context of the call.

>>>> +
>>>> +# 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.
>
> That doesn't sound right.  The procedure deletes all memory
> regions, either target-supplied, or user defined.
>
> Thanks,
> Pedro Alves
>

Fixed.

Thanks,
Luis

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

[Pedro Alves]

OK with ...

> Changes in v3:
> 
> - Fixed up comments.
> - Renamed lib/gdb-memory.exp to lib/memory.exp.
> - Moved memory region deletion closer to where we run to main in
>   gdb.base/breakpoint-in-ro-regions.exp.
> 
> Changes in v2:
> 
> - Addressed comments by Pedro.
> - Removed stale comments, fixed copyright year.
> - Updated comment for proc delete_memory_regions.
> 

... these removed from the final commit log ...

> 
> OK?

... and this too, and ...


> +++ b/gdb/testsuite/lib/memory.exp
> @@ -0,0 +1,33 @@
> +# Copyright 2012 Free Software Foundation, Inc.

2012-2017

Thanks,
Pedro Alves

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

[Luis Machado]

On 01/26/2017 01:32 PM, Pedro Alves wrote:
> OK with ...
>
>> Changes in v3:
>>
>> - Fixed up comments.
>> - Renamed lib/gdb-memory.exp to lib/memory.exp.
>> - Moved memory region deletion closer to where we run to main in
>>   gdb.base/breakpoint-in-ro-regions.exp.
>>
>> Changes in v2:
>>
>> - Addressed comments by Pedro.
>> - Removed stale comments, fixed copyright year.
>> - Updated comment for proc delete_memory_regions.
>>
>
> ... these removed from the final commit log ...
>
>>
>> OK?
>
> ... and this too, and ...
>
>
>> +++ b/gdb/testsuite/lib/memory.exp
>> @@ -0,0 +1,33 @@
>> +# Copyright 2012 Free Software Foundation, Inc.
>
> 2012-2017

Thanks! Pushed to master as
e309aa6524f8becadf6f1b75060a74be4c221899 with the above addressed.