diff mbox

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

Message ID 1510269503-12483-1-git-send-email-simon.marchi@ericsson.com
State New, archived
Headers

Commit Message

Simon Marchi Nov. 9, 2017, 11:18 p.m. UTC 
  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(-)

Comments

Eli Zaretskii Nov. 10, 2017, 8:01 a.m. UTC | #1 
> 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.
Simon Marchi Nov. 10, 2017, 9:01 p.m. UTC | #2 
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
Simon Marchi Nov. 24, 2017, 4:30 p.m. UTC | #3 
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
Joel Brobecker Nov. 24, 2017, 9:30 p.m. UTC | #4 
> 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...
Simon Marchi Nov. 24, 2017, 9:34 p.m. UTC | #5 
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
Joel Brobecker Nov. 24, 2017, 9:57 p.m. UTC | #6 
> 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?
Simon Marchi Nov. 24, 2017, 10:15 p.m. UTC | #7 
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
Joel Brobecker Nov. 24, 2017, 10:23 p.m. UTC | #8 
> So yes, you can updated the file online now.  Thanks a lot!

Sure thing. Done!
diff mbox

Patch

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