[patch] gdbserver <library-list> and its #FIXED version="1.0"

[patch] gdbserver <library-list> and its #FIXED version="1.0"

[Jan Kratochvil]

Hi,

while reimplementing <library-list/> I found from expat-2.0.1-11.fc15.x86_64:

warning: while parsing target library list (at line 1): Required attribute "version" of <library-list-svr4> not specified

I believe the same bug has to apply for existing FSF gdbserver but I do not
have any <library-list/> platform to test it (I did not try to build MinGW).

features/library-list.dtd:
<!ATTLIST library-list  version CDATA   #FIXED  "1.0">

http://www.xml.com/pub/a/98/10/guide0.html?page=3 says:

#FIXED
	In this case, the attribute is not required, but if it occurs, it must
	have the specified value.

Which would suggest gdbserver is right but solib-target.c is wrong.  One could
also make gdbserver explicit for the version (if those 14 bytes are not of
a concern).

Not going to check it in without a comment/approval.


Thanks,
Jan


gdb/
2011-11-03  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* solib-target.c (library_list_start_list): Do not dereference
	variable version in its initialization.  Make the VERSION check handle
	NULL.
	(library_list_attributes): Make "version" GDB_XML_AF_OPTIONAL.

gdb/gdbserver/
2011-11-03  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* server.c (handle_qxfer_libraries): Set `version' attribute for
	<library-list>.

--- a/gdb/solib-target.c
+++ b/gdb/solib-target.c
@@ -150,12 +150,18 @@ library_list_start_list (struct gdb_xml_parser *parser,
 			 const struct gdb_xml_element *element,
 			 void *user_data, VEC(gdb_xml_value_s) *attributes)
 {
-  char *version = xml_find_attribute (attributes, "version")->value;
+  struct gdb_xml_value *version = xml_find_attribute (attributes, "version");
 
-  if (strcmp (version, "1.0") != 0)
-    gdb_xml_error (parser,
-		   _("Library list has unsupported version \"%s\""),
-		   version);
+  /* #FIXED attribute may be omitted, Expat returns NULL in such case.  */
+  if (version)
+    {
+      const char *string = version->value;
+
+      if (strcmp (string, "1.0") != 0)
+	gdb_xml_error (parser,
+		       _("Library list has unsupported version \"%s\""),
+		       version);
+    }
 }
 
 /* Discard the constructed library list.  */
@@ -214,7 +220,7 @@ static const struct gdb_xml_element library_list_children[] = {
 };
 
 static const struct gdb_xml_attribute library_list_attributes[] = {
-  { "version", GDB_XML_AF_NONE, NULL, NULL },
+  { "version", GDB_XML_AF_OPTIONAL, NULL, NULL },
   { NULL, GDB_XML_AF_NONE, NULL, NULL }
 };
 
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -952,7 +952,7 @@ handle_qxfer_libraries (const char *annex,
   if (document == NULL)
     return -1;
 
-  strcpy (document, "<library-list>\n");
+  strcpy (document, "<library-list version=\"1.0\">\n");
   p = document + strlen (document);
 
   for (dll_ptr = all_dlls.head; dll_ptr != NULL; dll_ptr = dll_ptr->next)

ping: [patch] gdbserver <library-list> and its #FIXED version="1.0"

[Jan Kratochvil]

Hi,

this patch was never replied:
	[patch] gdbserver <library-list> and its #FIXED version="1.0"
	https://sourceware.org/ml/gdb-patches/2011-11/msg00099.html
	Message-ID: <20111103191640.GA27298@host1.jankratochvil.net>

but in this patch Tom Tromey has IIUC approved it:
	Re: [PATCH v5 6/8] gdbserver build-id attribute generator
	https://sourceware.org/ml/gdb-patches/2014-05/msg00490.html
	Message-ID: <87lhtvm634.fsf@fleche.redhat.com>

I guess I should check it in the first referenced patch but given all the years
have passed and maintainers changed I am rather pinging it for a new opinion.


Jan

Re: ping: [patch] gdbserver <library-list> and its #FIXED version="1.0"

[Gary Benson]

Jan Kratochvil wrote:
> this patch was never replied:
> 	[patch] gdbserver <library-list> and its #FIXED version="1.0"
> 	https://sourceware.org/ml/gdb-patches/2011-11/msg00099.html
> 	Message-ID: <20111103191640.GA27298@host1.jankratochvil.net>
> 
> but in this patch Tom Tromey has IIUC approved it:
> 	Re: [PATCH v5 6/8] gdbserver build-id attribute generator
> 	https://sourceware.org/ml/gdb-patches/2014-05/msg00490.html
> 	Message-ID: <87lhtvm634.fsf@fleche.redhat.com>
> 
> I guess I should check it in the first referenced patch but given all the years
> have passed and maintainers changed I am rather pinging it for a new opinion.

The "if (version)" should likely be "if (version != NULL)".
Otherwise it seems ok.

Cheers,
Gary

-- 
http://gbenson.net/

[commit] [patch] gdbserver <library-list> and its #FIXED version="1.0"

[Jan Kratochvil]

On Mon, 08 Jun 2015 11:07:56 +0200, Gary Benson wrote:
> The "if (version)" should likely be "if (version != NULL)".

On Wed, 10 Jun 2015 18:17:43 +0200, Pedro Alves wrote:
> Otherwise looks fine.

Checked in:
	24c05f46059182f0c8768c6ebbb66b4ca3233ecc


Jan

[commit#2] [patch] gdbserver <library-list> and its #FIXED version="1.0"

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 236 bytes --]

On Wed, 10 Jun 2015 18:31:21 +0200, Jan Kratochvil wrote:
> On Mon, 08 Jun 2015 11:07:56 +0200, Gary Benson wrote:
> > The "if (version)" should likely be "if (version != NULL)".

I forgot about this part.  Checked in as obvious.


Jan

[-- Attachment #2: Type: message/rfc822, Size: 1402 bytes --]

From: Jan Kratochvil <jan.kratochvil@redhat.com>
Subject: [PATCH] Code cleanup: Use explicit NULL comparison
Date: Wed, 10 Jun 2015 19:37:19 +0200

gdb/ChangeLog
2015-06-10  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Code cleanup.
	* solib-target.c (library_list_start_list): Use explicit NULL
	comparison.
---
 gdb/ChangeLog      | 6 ++++++
 gdb/solib-target.c | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 83845f4..5b19052 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,11 @@
 2015-06-10  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
+	Code cleanup.
+	* solib-target.c (library_list_start_list): Use explicit NULL
+	comparison.
+
+2015-06-10  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
 	* solib-target.c (library_list_start_list): Do not dereference
 	variable version in its initialization.  Make the VERSION check handle
 	NULL.
diff --git a/gdb/solib-target.c b/gdb/solib-target.c
index 891e572..f14363a 100644
--- a/gdb/solib-target.c
+++ b/gdb/solib-target.c
@@ -149,7 +149,7 @@ library_list_start_list (struct gdb_xml_parser *parser,
   struct gdb_xml_value *version = xml_find_attribute (attributes, "version");
 
   /* #FIXED attribute may be omitted, Expat returns NULL in such case.  */
-  if (version)
+  if (version != NULL)
     {
       const char *string = version->value;
 
-- 
2.1.0

Re: [patch] gdbserver <library-list> and its #FIXED version="1.0"

[Pedro Alves]

On 11/03/2011 07:16 PM, Jan Kratochvil wrote:

> I believe the same bug has to apply for existing FSF gdbserver but I do not
> have any <library-list/> platform to test it (I did not try to build MinGW).
> 
> features/library-list.dtd:
> <!ATTLIST library-list  version CDATA   #FIXED  "1.0">
> 
> http://www.xml.com/pub/a/98/10/guide0.html?page=3 says:
> 
> #FIXED
> 	In this case, the attribute is not required, but if it occurs, it must
> 	have the specified value.
> 
> Which would suggest gdbserver is right but solib-target.c is wrong.  One could
> also make gdbserver explicit for the version (if those 14 bytes are not of
> a concern).

Yeah, I couldn't find any other place that handled "version"
optionally.  Looking around, it looks like we end up passing
an explicit version number for all xml files but this one.

> +  /* #FIXED attribute may be omitted, Expat returns NULL in such case.  */
> +  if (version)

version != NULL

Otherwise looks fine.

Thanks,
Pedro Alves