[PATCH] linux_qxfer_libraries_svr4: Use std::string

[PATCH] linux_qxfer_libraries_svr4: Use std::string

[Simon Marchi]

Use std::string, removing some manual memory management.

gdb/gdbserver/ChangeLog:

	* linux-low.c (linux_qxfer_libraries_svr4): Use std::string.
---
 gdb/gdbserver/linux-low.c | 46 ++++++++++++----------------------------------
 1 file changed, 12 insertions(+), 34 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 00385ce4c9..c8bb1f5785 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -6988,8 +6988,6 @@ linux_qxfer_libraries_svr4 (const char *annex, unsigned char *readbuf,
 			    unsigned const char *writebuf,
 			    CORE_ADDR offset, int len)
 {
-  char *document;
-  unsigned document_len;
   struct process_info_private *const priv = current_process ()->priv;
   char filename[PATH_MAX];
   int pid, is_elf64;
@@ -7019,8 +7017,6 @@ linux_qxfer_libraries_svr4 (const char *annex, unsigned char *readbuf,
   unsigned int machine;
   int ptr_size;
   CORE_ADDR lm_addr = 0, lm_prev = 0;
-  int allocated = 1024;
-  char *p;
   CORE_ADDR l_name, l_addr, l_ld, l_next, l_prev;
   int header_done = 0;
 
@@ -7093,9 +7089,7 @@ linux_qxfer_libraries_svr4 (const char *annex, unsigned char *readbuf,
 	}
     }
 
-  document = (char *) xmalloc (allocated);
-  strcpy (document, "<library-list-svr4 version=\"1.0\"");
-  p = document + strlen (document);
+  std::string document = "<library-list-svr4 version=\"1.0\"";
 
   while (lm_addr
 	 && read_one_ptr (lm_addr + lmo->l_name_offset,
@@ -7125,10 +7119,7 @@ linux_qxfer_libraries_svr4 (const char *annex, unsigned char *readbuf,
 	 executable does not have PT_DYNAMIC present and this function already
 	 exited above due to failed get_r_debug.  */
       if (lm_prev == 0)
-	{
-	  sprintf (p, " main-lm=\"0x%lx\"", (unsigned long) lm_addr);
-	  p = p + strlen (p);
-	}
+	string_appendf (document, " main-lm=\"0x%lx\"", (unsigned long) lm_addr);
       else
 	{
 	  /* Not checking for error because reading may stop before
@@ -7138,31 +7129,19 @@ linux_qxfer_libraries_svr4 (const char *annex, unsigned char *readbuf,
 	  libname[sizeof (libname) - 1] = '\0';
 	  if (libname[0] != '\0')
 	    {
-	      /* 6x the size for xml_escape_text below.  */
-	      size_t len = 6 * strlen ((char *) libname);
-
 	      if (!header_done)
 		{
 		  /* Terminate `<library-list-svr4'.  */
-		  *p++ = '>';
+		  document += '>';
 		  header_done = 1;
 		}
 
-	      while (allocated < p - document + len + 200)
-		{
-		  /* Expand to guarantee sufficient storage.  */
-		  uintptr_t document_len = p - document;
-
-		  document = (char *) xrealloc (document, 2 * allocated);
-		  allocated *= 2;
-		  p = document + document_len;
-		}
-
 	      std::string name = xml_escape_text ((char *) libname);
-	      p += sprintf (p, "<library name=\"%s\" lm=\"0x%lx\" "
-			    "l_addr=\"0x%lx\" l_ld=\"0x%lx\"/>",
-			    name.c_str (), (unsigned long) lm_addr,
-			    (unsigned long) l_addr, (unsigned long) l_ld);
+	      string_appendf (document,
+			      "<library name=\"%s\" lm=\"0x%lx\" "
+			      "l_addr=\"0x%lx\" l_ld=\"0x%lx\"/>",
+			      name.c_str (), (unsigned long) lm_addr,
+			      (unsigned long) l_addr, (unsigned long) l_ld);
 	    }
 	}
 
@@ -7173,12 +7152,12 @@ linux_qxfer_libraries_svr4 (const char *annex, unsigned char *readbuf,
   if (!header_done)
     {
       /* Empty list; terminate `<library-list-svr4'.  */
-      strcpy (p, "/>");
+      document += "/>";
     }
   else
-    strcpy (p, "</library-list-svr4>");
+    document += "</library-list-svr4>";
 
-  document_len = strlen (document);
+  int document_len = document.length ();
   if (offset < document_len)
     document_len -= offset;
   else
@@ -7186,8 +7165,7 @@ linux_qxfer_libraries_svr4 (const char *annex, unsigned char *readbuf,
   if (len > document_len)
     len = document_len;
 
-  memcpy (readbuf, document + offset, len);
-  xfree (document);
+  memcpy (readbuf, document.data () + offset, len);
 
   return len;
 }
-- 
2.16.1

[PATCH] Add xml_escape_text_append and use it

[Simon Marchi]

[This patch should go on top of "linux_qxfer_libraries_svr4: Use
 std::string", I should have sent them together as a series.]

I noticed that linux_qxfer_libraries_svr4 used xml_escape_text, which
returns an std::string.  That string is then copied into a larger
buffer.  It would be more efficient if we had a version of
xml_escape_text which appended to an existing string instead of
returning a new one.  This is what this patch does.

I manually verified that the output of linux_qxfer_libraries_svr4 didn't
change before/after the patch.

gdb/ChangeLog:

	* common/xml-utils.c (xml_escape_text): Move code to...
	(xml_escape_text_append): ... this new function.
	* common/xml-utils.h (xml_escape_text_append): New declaration.
	* unittests/xml-utils-selftests.c (test_xml_escape_text_append):
	New function.
	(_initialize_xml_utils): register test_xml_escape_text_append as
	a selftest.

gdb/gdbserver/ChangeLog:

	* linux-low.c (linux_qxfer_libraries_svr4): Use
	xml_escape_text_append.
---
 gdb/common/xml-utils.c              | 27 +++++++++++++++++----------
 gdb/common/xml-utils.h              |  9 +++++++--
 gdb/gdbserver/linux-low.c           | 10 +++++-----
 gdb/unittests/xml-utils-selftests.c | 14 ++++++++++++++
 4 files changed, 43 insertions(+), 17 deletions(-)

diff --git a/gdb/common/xml-utils.c b/gdb/common/xml-utils.c
index aba1722cc0..3e1c2bfa7d 100644
--- a/gdb/common/xml-utils.c
+++ b/gdb/common/xml-utils.c
@@ -20,37 +20,44 @@
 #include "common-defs.h"
 #include "xml-utils.h"
 
-/* Return a string with special characters from TEXT replaced by entity
-   references.  */
+/* See xml-utils.h.  */
 
 std::string
 xml_escape_text (const char *text)
 {
   std::string result;
 
+  xml_escape_text_append (&result, text);
+
+  return result;
+}
+
+/* See xml-utils.h.  */
+
+void
+xml_escape_text_append (std::string *result, const char *text)
+{
   /* Expand the result.  */
   for (int i = 0; text[i] != '\0'; i++)
     switch (text[i])
       {
       case '\'':
-	result += "'";
+	*result += "'";
 	break;
       case '\"':
-	result += """;
+	*result += """;
 	break;
       case '&':
-	result += "&";
+	*result += "&";
 	break;
       case '<':
-	result += "&lt;";
+	*result += "&lt;";
 	break;
       case '>':
-	result += "&gt;";
+	*result += "&gt;";
 	break;
       default:
-	result += text[i];
+	*result += text[i];
 	break;
       }
-
-  return result;
 }
diff --git a/gdb/common/xml-utils.h b/gdb/common/xml-utils.h
index b874016334..3427ab246d 100644
--- a/gdb/common/xml-utils.h
+++ b/gdb/common/xml-utils.h
@@ -20,9 +20,14 @@
 #ifndef XML_UTILS_H
 #define XML_UTILS_H
 
-/* Return a malloc allocated string with special characters from TEXT
-   replaced by entity references.  */
+/* Return a string with special characters from TEXT replaced by entity
+   references.  */
 
 extern std::string xml_escape_text (const char *text);
 
+/* Append TEXT to RESULT, with special characters replaced by entity
+   references.  */
+
+extern void xml_escape_text_append (std::string *result, const char *text);
+
 #endif
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 8a807576fc..4700475dd4 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -7137,12 +7137,12 @@ linux_qxfer_libraries_svr4 (const char *annex, unsigned char *readbuf,
 		  header_done = 1;
 		}
 
-	      std::string name = xml_escape_text ((char *) libname);
-	      string_appendf (document,
-			      "<library name=\"%s\" lm=\"0x%lx\" "
+	      string_appendf (document, "<library name=\"");
+	      xml_escape_text_append (&document, (char *) libname);
+	      string_appendf (document, "\" lm=\"0x%lx\" "
 			      "l_addr=\"0x%lx\" l_ld=\"0x%lx\"/>",
-			      name.c_str (), (unsigned long) lm_addr,
-			      (unsigned long) l_addr, (unsigned long) l_ld);
+			      (unsigned long) lm_addr, (unsigned long) l_addr,
+			      (unsigned long) l_ld);
 	    }
 	}
 
diff --git a/gdb/unittests/xml-utils-selftests.c b/gdb/unittests/xml-utils-selftests.c
index 2457494d8c..1412773e18 100644
--- a/gdb/unittests/xml-utils-selftests.c
+++ b/gdb/unittests/xml-utils-selftests.c
@@ -33,6 +33,18 @@ static void test_xml_escape_text ()
   SELF_CHECK (actual_output == expected_output);
 }
 
+static void test_xml_escape_text_append ()
+{
+  /* Make sure that we do indeed append.  */
+  std::string actual_output = "foo<xml>";
+  const char *input = "<this isn't=\"xml\"> &";
+  const char *expected_output
+    = "foo<xml>&lt;this isn&apos;t=&quot;xml&quot;&gt; &amp;";
+  xml_escape_text_append (&actual_output, input);
+
+  SELF_CHECK (actual_output == expected_output);
+}
+
 }
 }
 
@@ -41,4 +53,6 @@ _initialize_xml_utils ()
 {
   selftests::register_test ("xml_escape_text",
 			    selftests::xml_utils::test_xml_escape_text);
+  selftests::register_test ("xml_escape_text_append",
+			    selftests::xml_utils::test_xml_escape_text_append);
 }
-- 
2.16.1

Re: [PATCH] linux_qxfer_libraries_svr4: Use std::string

[Simon Marchi]

On 2018-03-02 22:02, Simon Marchi wrote:
> Use std::string, removing some manual memory management.
> 
> gdb/gdbserver/ChangeLog:
> 
> 	* linux-low.c (linux_qxfer_libraries_svr4): Use std::string.

I pushed this patch.

Simon

Re: [PATCH] Add xml_escape_text_append and use it

[Simon Marchi]

On 2018-03-02 22:23, Simon Marchi wrote:
> [This patch should go on top of "linux_qxfer_libraries_svr4: Use
>  std::string", I should have sent them together as a series.]
> 
> I noticed that linux_qxfer_libraries_svr4 used xml_escape_text, which
> returns an std::string.  That string is then copied into a larger
> buffer.  It would be more efficient if we had a version of
> xml_escape_text which appended to an existing string instead of
> returning a new one.  This is what this patch does.
> 
> I manually verified that the output of linux_qxfer_libraries_svr4 
> didn't
> change before/after the patch.
> 
> gdb/ChangeLog:
> 
> 	* common/xml-utils.c (xml_escape_text): Move code to...
> 	(xml_escape_text_append): ... this new function.
> 	* common/xml-utils.h (xml_escape_text_append): New declaration.
> 	* unittests/xml-utils-selftests.c (test_xml_escape_text_append):
> 	New function.
> 	(_initialize_xml_utils): register test_xml_escape_text_append as
> 	a selftest.
> 
> gdb/gdbserver/ChangeLog:
> 
> 	* linux-low.c (linux_qxfer_libraries_svr4): Use
> 	xml_escape_text_append.

I pushed this patch.

Simon