[RFA] Use unique_xmalloc_ptr for read_string

Message ID 20180521204146.17553-1-tom@tromey.com
State New, archived
Headers

Commit Message

Tom Tromey May 21, 2018, 8:41 p.m. UTC
  This changes read_string's "buffer" out-parameter to be a
unique_xmalloc_ptr, then updates the users.  This allows for the
removal of some cleanups.

I chose unique_xmalloc_ptr rather than byte_vector here due to the way
Guile unwinding seems to work.

Tested by the buildbot.

ChangeLog
2018-05-21  Tom Tromey  <tom@tromey.com>

	* valprint.h (read_string): Update.
	* valprint.c (read_string): Change type of "buffer".
	(val_print_string): Update.
	* python/py-value.c (valpy_string): Update.
	* language.h (struct language_defn) <la_get_string>: Change
	type of "buffer".
	(default_get_string, c_get_string): Update.
	* language.c (default_get_string): Change type of "buffer".
	* guile/scm-value.c (gdbscm_value_to_string): Update.
	* c-lang.c (c_get_string): Change type of "buffer".
---
 gdb/ChangeLog         | 13 +++++++++++++
 gdb/c-lang.c          | 13 +++++--------
 gdb/guile/scm-value.c |  7 ++++---
 gdb/language.c        |  5 +++--
 gdb/language.h        | 18 ++++++++++++------
 gdb/python/py-value.c | 12 ++++--------
 gdb/valprint.c        | 48 +++++++++++++++++++++---------------------------
 gdb/valprint.h        |  3 ++-
 8 files changed, 64 insertions(+), 55 deletions(-)
  

Comments

Tom Tromey June 18, 2018, 2:44 p.m. UTC | #1
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> This changes read_string's "buffer" out-parameter to be a
Tom> unique_xmalloc_ptr, then updates the users.  This allows for the
Tom> removal of some cleanups.

Tom> I chose unique_xmalloc_ptr rather than byte_vector here due to the way
Tom> Guile unwinding seems to work.

Tom> Tested by the buildbot.

Ping.

Tom
  
Simon Marchi June 18, 2018, 6:28 p.m. UTC | #2
On 2018-05-21 16:41, Tom Tromey wrote:
> This changes read_string's "buffer" out-parameter to be a
> unique_xmalloc_ptr, then updates the users.  This allows for the
> removal of some cleanups.
> 
> I chose unique_xmalloc_ptr rather than byte_vector here due to the way
> Guile unwinding seems to work.

Hmm yeah I'm not too sure how Guile exceptions, work.  Would the 
destructor of a an object local to gdbscm_value_to_string run, if 
scm_from_stringn was to throw an exception, I guess that's the question?

In any case, your patch LGTM.

Simon
  
Tom Tromey June 18, 2018, 7:15 p.m. UTC | #3
>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> On 2018-05-21 16:41, Tom Tromey wrote:
>> This changes read_string's "buffer" out-parameter to be a
>> unique_xmalloc_ptr, then updates the users.  This allows for the
>> removal of some cleanups.
>> 
>> I chose unique_xmalloc_ptr rather than byte_vector here due to the way
>> Guile unwinding seems to work.

Simon> Hmm yeah I'm not too sure how Guile exceptions, work.  Would the
Simon> destructor of a an object local to gdbscm_value_to_string run, if
Simon> scm_from_stringn was to throw an exception, I guess that's the
Simon> question?

Yes - well, I think they would not be run, because guile exceptions are
longjmp based.

Also I think some replacement for
GDBSCM_HANDLE_GDB_EXCEPTION_WITH_CLEANUPS that uses destructors will need
to be found.

Tom
  

Patch

diff --git a/gdb/c-lang.c b/gdb/c-lang.c
index 15e633f8c8..8f5e880a92 100644
--- a/gdb/c-lang.c
+++ b/gdb/c-lang.c
@@ -233,7 +233,7 @@  c_printstr (struct ui_file *stream, struct type *type,
    target charset.  */
 
 void
-c_get_string (struct value *value, gdb_byte **buffer,
+c_get_string (struct value *value, gdb::unique_xmalloc_ptr<gdb_byte> *buffer,
 	      int *length, struct type **char_type,
 	      const char **charset)
 {
@@ -299,8 +299,8 @@  c_get_string (struct value *value, gdb_byte **buffer,
       /* I is now either a user-defined length, the number of non-null
  	 characters, or FETCHLIMIT.  */
       *length = i * width;
-      *buffer = (gdb_byte *) xmalloc (*length);
-      memcpy (*buffer, contents, *length);
+      buffer->reset ((gdb_byte *) xmalloc (*length));
+      memcpy (buffer->get (), contents, *length);
       err = 0;
     }
   else
@@ -325,10 +325,7 @@  c_get_string (struct value *value, gdb_byte **buffer,
       err = read_string (addr, *length, width, fetchlimit,
 			 byte_order, buffer, length);
       if (err != 0)
-	{
-	  xfree (*buffer);
-	  memory_error (TARGET_XFER_E_IO, addr);
-	}
+	memory_error (TARGET_XFER_E_IO, addr);
     }
 
   /* If the LENGTH is specified at -1, we want to return the string
@@ -338,7 +335,7 @@  c_get_string (struct value *value, gdb_byte **buffer,
   if (req_length == -1)
     /* If the last character is null, subtract it from LENGTH.  */
     if (*length > 0
- 	&& extract_unsigned_integer (*buffer + *length - width,
+	&& extract_unsigned_integer (buffer->get () + *length - width,
 				     width, byte_order) == 0)
       *length -= width;
   
diff --git a/gdb/guile/scm-value.c b/gdb/guile/scm-value.c
index 5a28d4d0ba..fccddfec41 100644
--- a/gdb/guile/scm-value.c
+++ b/gdb/guile/scm-value.c
@@ -1106,7 +1106,7 @@  gdbscm_value_to_string (SCM self, SCM rest)
   char *encoding = NULL;
   SCM errors = SCM_BOOL_F;
   int length = -1;
-  gdb_byte *buffer = NULL;
+  gdb::unique_xmalloc_ptr<gdb_byte> buffer;
   const char *la_encoding = NULL;
   struct type *char_type = NULL;
   SCM result;
@@ -1163,9 +1163,10 @@  gdbscm_value_to_string (SCM self, SCM rest)
   scm_dynwind_begin ((scm_t_dynwind_flags) 0);
 
   gdbscm_dynwind_xfree (encoding);
-  gdbscm_dynwind_xfree (buffer);
+  gdb_byte *buffer_contents = buffer.release ();
+  gdbscm_dynwind_xfree (buffer_contents);
 
-  result = scm_from_stringn ((const char *) buffer,
+  result = scm_from_stringn ((const char *) buffer_contents,
 			     length * TYPE_LENGTH (char_type),
 			     (encoding != NULL && *encoding != '\0'
 			      ? encoding
diff --git a/gdb/language.c b/gdb/language.c
index 22199e0c0d..1c908f5076 100644
--- a/gdb/language.c
+++ b/gdb/language.c
@@ -693,8 +693,9 @@  default_print_array_index (struct value *index_value, struct ui_file *stream,
 }
 
 void
-default_get_string (struct value *value, gdb_byte **buffer, int *length,
-		    struct type **char_type, const char **charset)
+default_get_string (struct value *value,
+		    gdb::unique_xmalloc_ptr<gdb_byte> *buffer,
+		    int *length, struct type **char_type, const char **charset)
 {
   error (_("Getting a string is unsupported in this language."));
 }
diff --git a/gdb/language.h b/gdb/language.h
index 029de4a7ab..dbe22d7b5b 100644
--- a/gdb/language.h
+++ b/gdb/language.h
@@ -370,8 +370,10 @@  struct language_defn
        characters, excluding any eventual terminating null character.
        Otherwise *LENGTH will include all characters - including any nulls.
        CHARSET will hold the encoding used in the string.  */
-    void (*la_get_string) (struct value *value, gdb_byte **buffer, int *length,
-			   struct type **chartype, const char **charset);
+    void (*la_get_string) (struct value *value,
+			   gdb::unique_xmalloc_ptr<gdb_byte> *buffer,
+			   int *length, struct type **chartype,
+			   const char **charset);
 
     /* Return an expression that can be used for a location
        watchpoint.  TYPE is a pointer type that points to the memory
@@ -631,8 +633,10 @@  int default_pass_by_reference (struct type *type);
 void default_print_typedef (struct type *type, struct symbol *new_symbol,
 			    struct ui_file *stream);
 
-void default_get_string (struct value *value, gdb_byte **buffer, int *length,
-			 struct type **char_type, const char **charset);
+void default_get_string (struct value *value,
+			 gdb::unique_xmalloc_ptr<gdb_byte> *buffer,
+			 int *length, struct type **char_type,
+			 const char **charset);
 
 /* Default name hashing function.  */
 
@@ -642,8 +646,10 @@  void default_get_string (struct value *value, gdb_byte **buffer, int *length,
    comparison operators hash to the same value.  */
 extern unsigned int default_search_name_hash (const char *search_name);
 
-void c_get_string (struct value *value, gdb_byte **buffer, int *length,
-		   struct type **char_type, const char **charset);
+void c_get_string (struct value *value,
+		   gdb::unique_xmalloc_ptr<gdb_byte> *buffer,
+		   int *length, struct type **char_type,
+		   const char **charset);
 
 /* The default implementation of la_symbol_name_matcher.  Matches with
    strncmp_iw.  */
diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c
index bba6d0b8a4..69863dea73 100644
--- a/gdb/python/py-value.c
+++ b/gdb/python/py-value.c
@@ -517,9 +517,8 @@  static PyObject *
 valpy_string (PyObject *self, PyObject *args, PyObject *kw)
 {
   int length = -1;
-  gdb_byte *buffer;
+  gdb::unique_xmalloc_ptr<gdb_byte> buffer;
   struct value *value = ((value_object *) self)->value;
-  PyObject *unicode;
   const char *encoding = NULL;
   const char *errors = NULL;
   const char *user_encoding = NULL;
@@ -542,12 +541,9 @@  valpy_string (PyObject *self, PyObject *args, PyObject *kw)
   END_CATCH
 
   encoding = (user_encoding && *user_encoding) ? user_encoding : la_encoding;
-  unicode = PyUnicode_Decode ((const char *) buffer,
-			      length * TYPE_LENGTH (char_type),
-			      encoding, errors);
-  xfree (buffer);
-
-  return unicode;
+  return PyUnicode_Decode ((const char *) buffer.get (),
+			   length * TYPE_LENGTH (char_type),
+			   encoding, errors);
 }
 
 /* A helper function that implements the various cast operators.  */
diff --git a/gdb/valprint.c b/gdb/valprint.c
index bed2cecf2c..e107b285f2 100644
--- a/gdb/valprint.c
+++ b/gdb/valprint.c
@@ -2096,10 +2096,10 @@  partial_memory_read (CORE_ADDR memaddr, gdb_byte *myaddr,
   return (nread);
 }
 
-/* Read a string from the inferior, at ADDR, with LEN characters of WIDTH bytes
-   each.  Fetch at most FETCHLIMIT characters.  BUFFER will be set to a newly
-   allocated buffer containing the string, which the caller is responsible to
-   free, and BYTES_READ will be set to the number of bytes read.  Returns 0 on
+/* Read a string from the inferior, at ADDR, with LEN characters of
+   WIDTH bytes each.  Fetch at most FETCHLIMIT characters.  BUFFER
+   will be set to a newly allocated buffer containing the string, and
+   BYTES_READ will be set to the number of bytes read.  Returns 0 on
    success, or a target_xfer_status on failure.
 
    If LEN > 0, reads the lesser of LEN or FETCHLIMIT characters
@@ -2122,20 +2122,18 @@  partial_memory_read (CORE_ADDR memaddr, gdb_byte *myaddr,
 
 int
 read_string (CORE_ADDR addr, int len, int width, unsigned int fetchlimit,
-	     enum bfd_endian byte_order, gdb_byte **buffer, int *bytes_read)
+	     enum bfd_endian byte_order, gdb::unique_xmalloc_ptr<gdb_byte> *buffer,
+	     int *bytes_read)
 {
   int errcode;			/* Errno returned from bad reads.  */
   unsigned int nfetch;		/* Chars to fetch / chars fetched.  */
   gdb_byte *bufptr;		/* Pointer to next available byte in
 				   buffer.  */
-  struct cleanup *old_chain = NULL;	/* Top of the old cleanup chain.  */
 
   /* Loop until we either have all the characters, or we encounter
      some error, such as bumping into the end of the address space.  */
 
-  *buffer = NULL;
-
-  old_chain = make_cleanup (free_current_contents, buffer);
+  buffer->reset (nullptr);
 
   if (len > 0)
     {
@@ -2143,8 +2141,8 @@  read_string (CORE_ADDR addr, int len, int width, unsigned int fetchlimit,
 	 one operation.  */
       unsigned int fetchlen = std::min ((unsigned) len, fetchlimit);
 
-      *buffer = (gdb_byte *) xmalloc (fetchlen * width);
-      bufptr = *buffer;
+      buffer->reset ((gdb_byte *) xmalloc (fetchlen * width));
+      bufptr = buffer->get ();
 
       nfetch = partial_memory_read (addr, bufptr, fetchlen * width, &errcode)
 	/ width;
@@ -2173,12 +2171,12 @@  read_string (CORE_ADDR addr, int len, int width, unsigned int fetchlimit,
 	  nfetch = std::min ((unsigned long) chunksize, fetchlimit - bufsize);
 
 	  if (*buffer == NULL)
-	    *buffer = (gdb_byte *) xmalloc (nfetch * width);
+	    buffer->reset ((gdb_byte *) xmalloc (nfetch * width));
 	  else
-	    *buffer = (gdb_byte *) xrealloc (*buffer,
-					     (nfetch + bufsize) * width);
+	    buffer->reset ((gdb_byte *) xrealloc (buffer->release (),
+						  (nfetch + bufsize) * width));
 
-	  bufptr = *buffer + bufsize * width;
+	  bufptr = buffer->get () + bufsize * width;
 	  bufsize += nfetch;
 
 	  /* Read as much as we can.  */
@@ -2210,24 +2208,23 @@  read_string (CORE_ADDR addr, int len, int width, unsigned int fetchlimit,
 	    }
 	}
       while (errcode == 0	/* no error */
-	     && bufptr - *buffer < fetchlimit * width	/* no overrun */
+	     && bufptr - buffer->get () < fetchlimit * width	/* no overrun */
 	     && !found_nul);	/* haven't found NUL yet */
     }
   else
     {				/* Length of string is really 0!  */
       /* We always allocate *buffer.  */
-      *buffer = bufptr = (gdb_byte *) xmalloc (1);
+      buffer->reset ((gdb_byte *) xmalloc (1));
+      bufptr = buffer->get ();
       errcode = 0;
     }
 
   /* bufptr and addr now point immediately beyond the last byte which we
      consider part of the string (including a '\0' which ends the string).  */
-  *bytes_read = bufptr - *buffer;
+  *bytes_read = bufptr - buffer->get ();
 
   QUIT;
 
-  discard_cleanups (old_chain);
-
   return errcode;
 }
 
@@ -2793,8 +2790,7 @@  val_print_string (struct type *elttype, const char *encoding,
   int found_nul;		/* Non-zero if we found the nul char.  */
   unsigned int fetchlimit;	/* Maximum number of chars to print.  */
   int bytes_read;
-  gdb_byte *buffer = NULL;	/* Dynamically growable fetch buffer.  */
-  struct cleanup *old_chain = NULL;	/* Top of the old cleanup chain.  */
+  gdb::unique_xmalloc_ptr<gdb_byte> buffer;	/* Dynamically growable fetch buffer.  */
   struct gdbarch *gdbarch = get_type_arch (elttype);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   int width = TYPE_LENGTH (elttype);
@@ -2812,7 +2808,6 @@  val_print_string (struct type *elttype, const char *encoding,
 
   err = read_string (addr, len, width, fetchlimit, byte_order,
 		     &buffer, &bytes_read);
-  old_chain = make_cleanup (xfree, buffer);
 
   addr += bytes_read;
 
@@ -2823,8 +2818,8 @@  val_print_string (struct type *elttype, const char *encoding,
   /* Determine found_nul by looking at the last character read.  */
   found_nul = 0;
   if (bytes_read >= width)
-    found_nul = extract_unsigned_integer (buffer + bytes_read - width, width,
-					  byte_order) == 0;
+    found_nul = extract_unsigned_integer (buffer.get () + bytes_read - width,
+					  width, byte_order) == 0;
   if (len == -1 && !found_nul)
     {
       gdb_byte *peekbuf;
@@ -2852,7 +2847,7 @@  val_print_string (struct type *elttype, const char *encoding,
      and then the error message.  */
   if (err == 0 || bytes_read > 0)
     {
-      LA_PRINT_STRING (stream, elttype, buffer, bytes_read / width,
+      LA_PRINT_STRING (stream, elttype, buffer.get (), bytes_read / width,
 		       encoding, force_ellipsis, options);
     }
 
@@ -2866,7 +2861,6 @@  val_print_string (struct type *elttype, const char *encoding,
     }
 
   gdb_flush (stream);
-  do_cleanups (old_chain);
 
   return (bytes_read / width);
 }
diff --git a/gdb/valprint.h b/gdb/valprint.h
index f005c31f87..93f3365eac 100644
--- a/gdb/valprint.h
+++ b/gdb/valprint.h
@@ -150,7 +150,8 @@  extern void print_function_pointer_address (const struct value_print_options *op
 
 extern int read_string (CORE_ADDR addr, int len, int width,
 			unsigned int fetchlimit,
-			enum bfd_endian byte_order, gdb_byte **buffer,
+			enum bfd_endian byte_order,
+			gdb::unique_xmalloc_ptr<gdb_byte> *buffer,
 			int *bytes_read);
 
 extern void val_print_optimized_out (const struct value *val,