Re: [PATCH] Fix memory-region overlapping checking

[Pedro Alves <palves@redhat.com>]

On 08/24/2012 12:47 PM, Wei-cheng Wang wrote:
> Hi Pedro Alves,
> 
>   The line number in my previous mail was wrong (s/45/58/),
>   so it cannot be fully applied. Please try this again.
>   Sorry for the inconvenience

Ah, thanks.  The previous patch had applied, but with fuzz.

The fix is okay.  I caught a few nits, and when trying to grok
the test, ended up extending it some more.  See below.  If you
don't have further comments, I'll put it in.  Thanks for this,
and thanks to Yao too.

> +	  || (lo <= n->lo && ((hi >= n->hi && n->hi != 0)|| hi == 0)))

Missing space before second '||'.

> 2012-08-08  Wei-cheng Wang  <cole945@gmail.com>
>
> 	* memattr.c: Fix memory region overlapping checking

Missing function that changed in parens, and period at end of sentence.

> 2012-08-08  Wei-cheng Wang  <cole945@gmail.com>
>             Yao Qi <yao@codesourcery.com>
> 
>         * gdb.base/memattr.exp: Add cases for overlapping checking.

Ditto.

> +# Test overlapping checking
> +#
> +#       lo'        hi'
> +#        |---------|
> +#  10 20 30 40 50 60 70 80
> +#      |----|             FAIL
> +#        |--|             FAIL
> +#           |---|         FAIL
> +#               |--|      FAIL
> +#               |----|    FAIL
> +#        |---------|      FAIL
> +#      |-------------|    FAIL
> +#      |----------------- FAIL
> +#           |------------ FAIL
> +#                    |--- FAIL
> +#   |----|                PASS
> +#                  |----| PASS

This table doesn't match 1 on 1 with the tests below.
And I think we can be even more thorough.  We should test
all the '==' cases for the '>=' conditions in the code.
IOW, when lo == n->lo, etc.

> +
> +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 $" { }
> +    }
> +}
> +
> +# Test normal case (upper != 0)
> +delete_memory
> +gdb_test_no_output "mem 0x30 0x60 ro"
> +gdb_test "mem 0x20 0x40 ro" "overlapping memory region" "mem 0x20 0x40 1"
> +gdb_test "mem 0x30 0x40 ro" "overlapping memory region" "mem 0x30 0x40 1"
> +gdb_test "mem 0x40 0x50 ro" "overlapping memory region" "mem 0x40 0x50 1"
> +gdb_test "mem 0x50 0x60 ro" "overlapping memory region" "mem 0x50 0x60 1"
> +gdb_test "mem 0x30 0x60 ro" "overlapping memory region" "mem 0x30 0x60 1"
> +gdb_test "mem 0x50 0x70 ro" "overlapping memory region" "mem 0x50 0x70 1"
> +gdb_test "mem 0x20 0x0 ro" "overlapping memory region" "mem 0x20 0x0 1"
> +gdb_test "mem 0x30 0x0 ro" "overlapping memory region" "mem 0x30 0x0 1"
> +gdb_test_no_output "mem 0x10 0x30 ro" "mem 0x10 0x30 1"
> +gdb_test_no_output "mem 0x60 0x80 ro" "mem 0x60 0x80 1"
> +gdb_test_no_output "mem 0x80 0x0 ro" "mem 0x80 0x0 1"
> +
> +# Test special case (upper == 0)
> +delete_memory

Since I was looking, I thought these could use some neat ascii art too.

> +gdb_test_no_output "mem 0x30 0x0 ro"
> +gdb_test "mem 0x20 0x40 ro" "overlapping memory region" "mem 0x20 0x40 2"
> +gdb_test "mem 0x30 0x40 ro" "overlapping memory region" "mem 0x30 0x40 2"
> +gdb_test "mem 0x20 0x0 ro" "overlapping memory region" "mem 0x20 0x0 2"
> +gdb_test "mem 0x30 0x0 ro" "overlapping memory region" "mem 0x30 0x0 2"
> +gdb_test_no_output "mem 0x10 0x30 ro" "mem 0x10 0x30 2"

I also took the liberty of adding a couple of convenience functions that make
writing these lines, and eyeballing against the tables easier (I got confused
easily).

WDYT?

2012-08-24  Wei-cheng Wang  <cole945@gmail.com>

	* memattr.c (create_mem_region): Fix memory region overlapping
	checking.

2012-08-24  Wei-cheng Wang  <cole945@gmail.com>
            Yao Qi <yao@codesourcery.com>
            Pedro Alves <palves@redhat.com>

        * gdb.base/memattr.exp (delete_memory, region_pass, region_fail):
	New procedures.
	(top level): Add overlap checking tests.
---

 gdb/memattr.c                      |    2 -
 gdb/testsuite/gdb.base/memattr.exp |   94 ++++++++++++++++++++++++++++++++++++
 2 files changed, 95 insertions(+), 1 deletion(-)

diff --git a/gdb/memattr.c b/gdb/memattr.c
index ec7deb5..bd92f1d 100644
--- a/gdb/memattr.c
+++ b/gdb/memattr.c
@@ -207,7 +207,7 @@ create_mem_region (CORE_ADDR lo, CORE_ADDR hi,

       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 || hi == 0)))
+	  || (lo <= n->lo && ((hi >= n->hi && n->hi != 0) || hi == 0)))
 	{
 	  printf_unfiltered (_("overlapping memory region\n"));
 	  return;
diff --git a/gdb/testsuite/gdb.base/memattr.exp b/gdb/testsuite/gdb.base/memattr.exp
index 4065808..a83ca73 100644
--- a/gdb/testsuite/gdb.base/memattr.exp
+++ b/gdb/testsuite/gdb.base/memattr.exp
@@ -448,3 +448,97 @@ gdb_test_multiple "info mem" "mem 2-4 were deleted" {

 gdb_test "delete mem 8" "No memory region number 8." \
     "delete non-existant region"
+
+#
+# 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 } {
+    gdb_test_no_output "mem $region ro" "$region: no-overlap"
+}
+
+# Try to create a region that overlaps (a FAIL in the table).
+
+proc region_fail { region } {
+    gdb_test "mem $region ro" \
+	"overlapping memory region" \
+	"$region: overlap"
+}
+
+# Test normal case (upper != 0)
+#
+#        lo'       hi'
+#         |--------|
+#  10 20 30 40 50 60 70 80 90
+#      |-----|                FAIL
+#         |--|                FAIL
+#            |--|             FAIL
+#               |--|          FAIL
+#               |-----|       FAIL
+#         |--------|          FAIL
+#      |--------------|       FAIL
+#      |-----------------     FAIL
+#         |--------------     FAIL
+#            |-----------     FAIL
+#      |--|                   PASS
+#                  |--|       PASS
+#                        |--- PASS
+
+delete_memory
+gdb_test_no_output "mem 0x30 0x60 ro"
+with_test_prefix "0x30 0x60" {
+    region_fail "0x20 0x40"
+    region_fail "0x30 0x40"
+    region_fail "0x40 0x50"
+    region_fail "0x50 0x60"
+    region_fail "0x50 0x70"
+    region_fail "0x30 0x60"
+    region_fail "0x20 0x70"
+    region_fail "0x20 0x0"
+    region_fail "0x30 0x0"
+    region_fail "0x40 0x0"
+    region_pass "0x20 0x30"
+    region_pass "0x60 0x70"
+    region_pass "0x80 0x0"
+}
+
+# Test special case (upper == 0)
+#
+#           lo'             hi'
+#            |---------------
+#  00 10 20 30 40 50 60 70 80
+#         |--------|          FAIL
+#            |-----|          FAIL
+#               |--|          FAIL
+#         |------------------ FAIL
+#            |--------------- FAIL
+#               |------------ FAIL
+#         |--|                PASS
+#   |--|                      PASS
+
+delete_memory
+gdb_test_no_output "mem 0x30 0x0 ro"
+with_test_prefix "0x30 0x0" {
+    region_fail "0x20 0x50"
+    region_fail "0x30 0x50"
+    region_fail "0x40 0x50"
+    region_fail "0x20 0x0"
+    region_fail "0x30 0x0"
+    region_fail "0x40 0x0"
+    region_pass "0x20 0x30"
+    region_pass "0x00 0x10"
+}