[PATCH 2/2] Fix mem region parsing regression and add test

[PATCH 2/2] Fix mem region parsing regression and add test

[Simon Marchi]

In my patch

  Get rid of VEC (mem_region)
  a664f67e50eff30198097d51cec0ec4690abb2a1

I introduced a regression, where the length of the memory region is
assigned to the "hi" field.  It should obviously be computed as "start +
length".  To my defense, no test had caught this :).  As a penance, I
wrote one.

gdb/ChangeLog:

	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
	memory-map-selftests.c.
	(SUBDIR_UNITTESTS_OBS): Add memory-map-selftests.o.
	* memory-map.c (memory_map_start_memory): Fix computation of hi
	address.
	* unittests/memory-map-selftests.c: New file.
---
 gdb/Makefile.in                      |  2 +
 gdb/memory-map.c                     |  2 +-
 gdb/unittests/memory-map-selftests.c | 81 ++++++++++++++++++++++++++++++++++++
 3 files changed, 84 insertions(+), 1 deletion(-)
 create mode 100644 gdb/unittests/memory-map-selftests.c

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 5e01816..a3bfbf9 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -531,6 +531,7 @@ SUBDIR_UNITTESTS_SRCS = \
 	unittests/environ-selftests.c \
 	unittests/function-view-selftests.c \
 	unittests/lookup_name_info-selftests.c \
+	unittests/memory-map-selftests.c \
 	unittests/memrange-selftests.c \
 	unittests/offset-type-selftests.c \
 	unittests/optional-selftests.c \
@@ -544,6 +545,7 @@ SUBDIR_UNITTESTS_OBS = \
 	environ-selftests.o \
 	function-view-selftests.o \
 	lookup_name_info-selftests.o \
+	memory-map-selftests.o \
 	memrange-selftests.o \
 	offset-type-selftests.o \
 	optional-selftests.o \
diff --git a/gdb/memory-map.c b/gdb/memory-map.c
index 9582ceb..e098209 100644
--- a/gdb/memory-map.c
+++ b/gdb/memory-map.c
@@ -71,7 +71,7 @@ memory_map_start_memory (struct gdb_xml_parser *parser,
   type_p
     = (ULONGEST *) xml_find_attribute (attributes, "type")->value;
 
-  data->memory_map->emplace_back (*start_p, *length_p,
+  data->memory_map->emplace_back (*start_p, *start_p + *length_p,
 				  (enum mem_access_mode) *type_p);
 }
 
diff --git a/gdb/unittests/memory-map-selftests.c b/gdb/unittests/memory-map-selftests.c
new file mode 100644
index 0000000..3b282b5
--- /dev/null
+++ b/gdb/unittests/memory-map-selftests.c
@@ -0,0 +1,81 @@
+/* Self tests for memory-map for GDB, the GNU debugger.
+
+   Copyright (C) 2017 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+#include "defs.h"
+#include "selftest.h"
+#include "memory-map.h"
+
+namespace selftests {
+namespace memory_map_tests {
+
+/* A simple valid test input for parse_memory_map.  */
+
+static const char *valid_mem_map = R"(<?xml version="1.0"?>
+<!DOCTYPE memory-map
+          PUBLIC "+//IDN gnu.org//DTD GDB Memory Map V1.0//EN"
+                 "http://sourceware.org/gdb/gdb-memory-map.dtd">
+<memory-map>
+  <memory type="ram" start="0" length="4096" />
+  <memory type="rom" start="65536" length="256" />
+  <memory type="flash" start="131072" length="65536">
+    <property name="blocksize">1024</property>
+  </memory>
+</memory-map>
+)";
+
+/* Validate memory region R against some expected values (the other
+   parameters).  */
+
+static void
+check_mem_region (const mem_region &r, CORE_ADDR lo, CORE_ADDR hi,
+		  mem_access_mode mode, int blocksize)
+{
+  SELF_CHECK (r.lo == lo);
+  SELF_CHECK (r.hi == hi);
+  SELF_CHECK (r.enabled_p);
+
+  SELF_CHECK (r.attrib.mode == mode);
+  SELF_CHECK (r.attrib.blocksize == blocksize);
+
+}
+
+/* Test the parse_memory_map function.  */
+
+static void
+parse_memory_map_tests ()
+{
+  std::vector<mem_region> regions = parse_memory_map (valid_mem_map);
+
+  SELF_CHECK (regions.size () == 3);
+
+  check_mem_region (regions[0], 0, 0 + 4096, MEM_RW, -1);
+  check_mem_region (regions[1], 65536, 65536 + 256, MEM_RO, -1);
+  check_mem_region (regions[2], 131072, 131072 + 65536, MEM_FLASH, 1024);
+}
+
+} /* namespace memory_map_tests */
+} /* namespace selftests */
+
+void
+_initialize_memory_map_selftests ()
+{
+  selftests::register_test
+    ("parse_memory_map",
+     selftests::memory_map_tests::parse_memory_map_tests);
+}
-- 
2.7.4

[PATCH 1/2] Fix issues with gdb-memory-map.dtd

[Simon Marchi]

While writing a unit test for parse_memory_map, I tried to validate my
test input against gdb-memory-map.dtd, and found a few problems with it.
This doesn't influence how gdb parses it (AFAIK it doesn't use the
linked dtd), but if you edit the xml file in an editor that supports
dtds, you'll get plenty of errors.

  - The <memory-map> element accepts exactly one <memory> OR <property>
    as a child.  This is a problem because you can't have multiple
    <memory> elements and you shouldn't be able to have <property> elements
    as direct children of <memory-map>.
  - The <memory> element wants exactly one <property> child.  This is
    wrong, since you could have zero or more (even though we only
    support one kind of property currently).
  - I have no idea wht the device attribute of <memory> is, GDB doesn't
    read that.  I searched back in time a bit but couldn't find a trace
    of it.

I took the opportunity to tighten what is accepted as a value of the
memory type and property name attributes.  We currently accept any
string, but we could restrict them to the values GDB really accepts (and
which are documented).

AFAIK, this "file" only exists in the documentation, in gdb.texinfo, so
this is what I modified.  However, it's also available at
http://sourceware.org/gdb/gdb-memory-map.dtd.  This one should be
updated too, but I don't know how that should be done.

gdb/doc/ChangeLog:

	* gdb.texinfo (Memory Map Format): Update gdb-memory-map.dtd.
---
 gdb/doc/gdb.texinfo | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 29d4789..37d3e3d 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -40807,18 +40807,17 @@ The formal DTD for memory map format is given below:
 <!-- .................................... .............. -->
 <!-- memory-map.dtd -->
 <!-- memory-map: Root element with versioning -->
-<!ELEMENT memory-map (memory | property)>
+<!ELEMENT memory-map (memory)*>
 <!ATTLIST memory-map    version CDATA   #FIXED  "1.0.0">
-<!ELEMENT memory (property)>
+<!ELEMENT memory (property)*>
 <!-- memory: Specifies a memory region,
              and its type, or device. -->
-<!ATTLIST memory        type    CDATA   #REQUIRED
+<!ATTLIST memory        type    (ram|rom|flash) #REQUIRED
                         start   CDATA   #REQUIRED
-                        length  CDATA   #REQUIRED
-                        device  CDATA   #IMPLIED>
+                        length  CDATA   #REQUIRED>
 <!-- property: Generic attribute tag -->
 <!ELEMENT property (#PCDATA | property)*>
-<!ATTLIST property      name    CDATA   #REQUIRED>
+<!ATTLIST property      name    (blocksize) #REQUIRED>
 @end smallexample
 
 @node Thread List Format
-- 
2.7.4

Re: [PATCH 1/2] Fix issues with gdb-memory-map.dtd

[Eli Zaretskii]

> From: Simon Marchi <simon.marchi@ericsson.com>
> CC: Simon Marchi <simon.marchi@ericsson.com>
> Date: Thu, 9 Nov 2017 18:18:22 -0500
> 
> gdb/doc/ChangeLog:
> 
> 	* gdb.texinfo (Memory Map Format): Update gdb-memory-map.dtd.

It's technically a documentation patch, but I don't think I have
anything intelligent to say about it.  Would someone else please
review this in my stead?

Texinfo-wise, there are no problems in the patch.

Re: [PATCH 1/2] Fix issues with gdb-memory-map.dtd

[Simon Marchi]

On 2017-11-10 03:01, Eli Zaretskii wrote:
>> From: Simon Marchi <simon.marchi@ericsson.com>
>> CC: Simon Marchi <simon.marchi@ericsson.com>
>> Date: Thu, 9 Nov 2017 18:18:22 -0500
>> 
>> gdb/doc/ChangeLog:
>> 
>> 	* gdb.texinfo (Memory Map Format): Update gdb-memory-map.dtd.
> 
> It's technically a documentation patch, but I don't think I have
> anything intelligent to say about it.  Would someone else please
> review this in my stead?
> 
> Texinfo-wise, there are no problems in the patch.

Thanks for taking a look.

Simon

Re: [PATCH 2/2] Fix mem region parsing regression and add test

[Simon Marchi]

On 2017-11-09 18:18, Simon Marchi wrote:
> In my patch
> 
>   Get rid of VEC (mem_region)
>   a664f67e50eff30198097d51cec0ec4690abb2a1
> 
> I introduced a regression, where the length of the memory region is
> assigned to the "hi" field.  It should obviously be computed as "start 
> +
> length".  To my defense, no test had caught this :).  As a penance, I
> wrote one.

I have pushed this patch (2/2), since it's a bug fix and I don't want to 
forget it.  Patch 1/2 is still up for review, especially the part about 
updating the file on the website.

Simon

Re: [PATCH 1/2] Fix issues with gdb-memory-map.dtd

[Simon Marchi]

On 2017-11-09 06:18 PM, Simon Marchi wrote:
> While writing a unit test for parse_memory_map, I tried to validate my
> test input against gdb-memory-map.dtd, and found a few problems with it.
> This doesn't influence how gdb parses it (AFAIK it doesn't use the
> linked dtd), but if you edit the xml file in an editor that supports
> dtds, you'll get plenty of errors.
> 
>   - The <memory-map> element accepts exactly one <memory> OR <property>
>     as a child.  This is a problem because you can't have multiple
>     <memory> elements and you shouldn't be able to have <property> elements
>     as direct children of <memory-map>.
>   - The <memory> element wants exactly one <property> child.  This is
>     wrong, since you could have zero or more (even though we only
>     support one kind of property currently).
>   - I have no idea wht the device attribute of <memory> is, GDB doesn't
>     read that.  I searched back in time a bit but couldn't find a trace
>     of it.
> 
> I took the opportunity to tighten what is accepted as a value of the
> memory type and property name attributes.  We currently accept any
> string, but we could restrict them to the values GDB really accepts (and
> which are documented).
> 
> AFAIK, this "file" only exists in the documentation, in gdb.texinfo, so
> this is what I modified.  However, it's also available at
> http://sourceware.org/gdb/gdb-memory-map.dtd.  This one should be
> updated too, but I don't know how that should be done.
> 
> gdb/doc/ChangeLog:
> 
> 	* gdb.texinfo (Memory Map Format): Update gdb-memory-map.dtd.
> ---
>  gdb/doc/gdb.texinfo | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 29d4789..37d3e3d 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -40807,18 +40807,17 @@ The formal DTD for memory map format is given below:
>  <!-- .................................... .............. -->
>  <!-- memory-map.dtd -->
>  <!-- memory-map: Root element with versioning -->
> -<!ELEMENT memory-map (memory | property)>
> +<!ELEMENT memory-map (memory)*>
>  <!ATTLIST memory-map    version CDATA   #FIXED  "1.0.0">
> -<!ELEMENT memory (property)>
> +<!ELEMENT memory (property)*>
>  <!-- memory: Specifies a memory region,
>               and its type, or device. -->
> -<!ATTLIST memory        type    CDATA   #REQUIRED
> +<!ATTLIST memory        type    (ram|rom|flash) #REQUIRED
>                          start   CDATA   #REQUIRED
> -                        length  CDATA   #REQUIRED
> -                        device  CDATA   #IMPLIED>
> +                        length  CDATA   #REQUIRED>
>  <!-- property: Generic attribute tag -->
>  <!ELEMENT property (#PCDATA | property)*>
> -<!ATTLIST property      name    CDATA   #REQUIRED>
> +<!ATTLIST property      name    (blocksize) #REQUIRED>
>  @end smallexample
>  
>  @node Thread List Format
> 

Hi Joel,

You are probably the person that has the most chance to know how to
update this file:

  http://sourceware.org/gdb/gdb-memory-map.dtd

Any idea?

Simon

Re: [PATCH 1/2] Fix issues with gdb-memory-map.dtd

[Joel Brobecker]

> You are probably the person that has the most chance to know how to
> update this file:
> 
>   http://sourceware.org/gdb/gdb-memory-map.dtd
> 
> Any idea?

I can absolutely do that. But I'm wondering whether we might
just want to delete the file instead? Why keep a copy on
the website? I couldn't find a reference to it anywhere
in any of the webpages...

-- 
Joel

Re: [PATCH 1/2] Fix issues with gdb-memory-map.dtd

[Simon Marchi]

On 2017-11-24 16:30, Joel Brobecker wrote:
>> You are probably the person that has the most chance to know how to
>> update this file:
>> 
>>   http://sourceware.org/gdb/gdb-memory-map.dtd
>> 
>> Any idea?
> 
> I can absolutely do that. But I'm wondering whether we might
> just want to delete the file instead? Why keep a copy on
> the website? I couldn't find a reference to it anywhere
> in any of the webpages...

The XML files that follow this dtd can point to it.  For example, look 
at the example in the doc:

https://sourceware.org/gdb/onlinedocs/gdb/Memory-Map-Format.html

This allows a tool to validate that the XML respects the dtd, and XML 
editors can provide assistance when hand-writing it, giving 
auto-complete based on the possible options.

Simon

Re: [PATCH 1/2] Fix issues with gdb-memory-map.dtd

[Joel Brobecker]

> The XML files that follow this dtd can point to it.  For example, look at
> the example in the doc:
> 
> https://sourceware.org/gdb/onlinedocs/gdb/Memory-Map-Format.html
> 
> This allows a tool to validate that the XML respects the dtd, and XML
> editors can provide assistance when hand-writing it, giving auto-complete
> based on the possible options.

Thanks for explaining. I can update the file. Would you like me
to do it now, or should I wait for your patch to get in first?

-- 
Joel

Re: [PATCH 1/2] Fix issues with gdb-memory-map.dtd

[Simon Marchi]

On 2017-11-24 04:57 PM, Joel Brobecker wrote:
>> The XML files that follow this dtd can point to it.  For example, look at
>> the example in the doc:
>>
>> https://sourceware.org/gdb/onlinedocs/gdb/Memory-Map-Format.html
>>
>> This allows a tool to validate that the XML respects the dtd, and XML
>> editors can provide assistance when hand-writing it, giving auto-complete
>> based on the possible options.
>
> Thanks for explaining. I can update the file. Would you like me
> to do it now, or should I wait for your patch to get in first?

I was hoping to have at least someone with a reasonable level of confidence
the dtd changes, but I guess if it hasn't happened by now it won't happen :)
I tested the file by validating various files with xmllint, and by editing
them in the Eclipse XML editor, so I'm fairly confident that it's ok.  I
pushed it just now, but comments are still welcome.

So yes, you can updated the file online now.  Thanks a lot!

Simon

Re: [PATCH 1/2] Fix issues with gdb-memory-map.dtd

[Joel Brobecker]

> So yes, you can updated the file online now.  Thanks a lot!

Sure thing. Done!

-- 
Joel