[PATCH 0/4] Unify string-reading APIs

[PATCH 0/4] Unify string-reading APIs

[Tom Tromey]

An old comment notes that gdb has several different ways to read a
string from the target.  This series tries to simplify this situation.

read_memory_string is removed entirely.  Perhaps this is the better
name, in the end; but I didn't want to move read_string into
corelow.c.  Maybe we need a new file for the memory-reading functions
that are there?  (Also I wonder if any of the remaining ones are
redundant.)

target_read_string is first rewritten in terms of read_string.  A
subsequent patch changes its API to be simpler.

Perhaps further simplification could be done as well.  I'm open to
suggestions.

Regression tested on x86-64 Fedora 30.  I also built it using a mingw
cross, to make sure windows-nat.c still builds.

Tom

[PATCH 1/4] Remove read_memory_string

[Tom Tromey]

read_memory_string is redundant and only called in a couple of spots.
This patch removes it in favor of target_read_string.

gdb/ChangeLog
2020-06-02  Tom Tromey  <tromey@adacore.com>

	* corefile.c (read_memory_string): Remove.
	* ada-valprint.c (ada_value_print_ptr): Update.
	* ada-lang.h (ada_tag_name): Change return type.
	* ada-lang.c (type_from_tag): Update.
	(ada_tag_name_from_tsd): Change return type.  Use
	target_read_string.
	(ada_tag_name): Likewise.
	* gdbcore.h (read_memory_string): Don't declare.
---
 gdb/ChangeLog      | 11 +++++++++++
 gdb/ada-lang.c     | 34 ++++++++++++++++++++--------------
 gdb/ada-lang.h     |  2 +-
 gdb/ada-valprint.c |  4 ++--
 gdb/corefile.c     | 27 ---------------------------
 gdb/gdbcore.h      |  6 ------
 6 files changed, 34 insertions(+), 50 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index f7b973f36ee..3245758a1bd 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -6567,10 +6567,10 @@ value_tag_from_contents_and_address (struct type *type,
 static struct type *
 type_from_tag (struct value *tag)
 {
-  const char *type_name = ada_tag_name (tag);
+  gdb::unique_xmalloc_ptr<char> type_name = ada_tag_name (tag);
 
   if (type_name != NULL)
-    return ada_find_any_type (ada_encode (type_name));
+    return ada_find_any_type (ada_encode (type_name.get ()));
   return NULL;
 }
 
@@ -6721,34 +6721,38 @@ ada_get_tsd_from_tag (struct value *tag)
    The returned value is good until the next call.  May return NULL
    if we are unable to determine the tag name.  */
 
-static char *
+static gdb::unique_xmalloc_ptr<char>
 ada_tag_name_from_tsd (struct value *tsd)
 {
-  static char name[1024];
   char *p;
   struct value *val;
 
   val = ada_value_struct_elt (tsd, "expanded_name", 1);
   if (val == NULL)
     return NULL;
-  read_memory_string (value_as_address (val), name, sizeof (name) - 1);
-  for (p = name; *p != '\0'; p += 1)
+  gdb::unique_xmalloc_ptr<char> buffer;
+  int err;
+  if (target_read_string (value_as_address (val), &buffer, INT_MAX, &err) == 0
+      || err != 0)
+    return nullptr;
+
+  for (p = (char *) buffer.get (); *p != '\0'; p += 1)
     if (isalpha (*p))
       *p = tolower (*p);
-  return name;
+
+  return buffer;
 }
 
 /* The type name of the dynamic type denoted by the 'tag value TAG, as
    a C string.
 
    Return NULL if the TAG is not an Ada tag, or if we were unable to
-   determine the name of that tag.  The result is good until the next
-   call.  */
+   determine the name of that tag.  */
 
-const char *
+gdb::unique_xmalloc_ptr<char>
 ada_tag_name (struct value *tag)
 {
-  char *name = NULL;
+  gdb::unique_xmalloc_ptr<char> name;
 
   if (!ada_is_tag_type (value_type (tag)))
     return NULL;
@@ -12104,9 +12108,11 @@ ada_exception_message_1 (void)
   if (e_msg_len <= 0)
     return NULL;
 
-  gdb::unique_xmalloc_ptr<char> e_msg ((char *) xmalloc (e_msg_len + 1));
-  read_memory_string (value_address (e_msg_val), e_msg.get (), e_msg_len + 1);
-  e_msg.get ()[e_msg_len] = '\0';
+  gdb::unique_xmalloc_ptr<char> e_msg;
+  int err;
+  if (target_read_string (value_address (e_msg_val), &e_msg, INT_MAX, &err) == 0
+      || err != 0)
+    return nullptr;
 
   return e_msg;
 }
diff --git a/gdb/ada-lang.h b/gdb/ada-lang.h
index 5ba00518e6d..9be597942fd 100644
--- a/gdb/ada-lang.h
+++ b/gdb/ada-lang.h
@@ -260,7 +260,7 @@ extern int ada_is_tagged_type (struct type *, int);
 
 extern int ada_is_tag_type (struct type *);
 
-extern const char *ada_tag_name (struct value *);
+extern gdb::unique_xmalloc_ptr<char> ada_tag_name (struct value *);
 
 extern struct value *ada_tag_value_at_base_address (struct value *obj);
 
diff --git a/gdb/ada-valprint.c b/gdb/ada-valprint.c
index a36e7ca793a..61893d5cad3 100644
--- a/gdb/ada-valprint.c
+++ b/gdb/ada-valprint.c
@@ -754,10 +754,10 @@ ada_value_print_ptr (struct value *val,
   struct type *type = ada_check_typedef (value_type (val));
   if (ada_is_tag_type (type))
     {
-      const char *name = ada_tag_name (val);
+      gdb::unique_xmalloc_ptr<char> name = ada_tag_name (val);
 
       if (name != NULL)
-	fprintf_filtered (stream, " (%s)", name);
+	fprintf_filtered (stream, " (%s)", name.get ());
     }
 }
 
diff --git a/gdb/corefile.c b/gdb/corefile.c
index 996e5301b27..fed0e4fe8ad 100644
--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -328,33 +328,6 @@ read_code_unsigned_integer (CORE_ADDR memaddr, int len,
   return extract_unsigned_integer (buf, len, byte_order);
 }
 
-void
-read_memory_string (CORE_ADDR memaddr, char *buffer, int max_len)
-{
-  char *cp;
-  int i;
-  int cnt;
-
-  cp = buffer;
-  while (1)
-    {
-      if (cp - buffer >= max_len)
-	{
-	  buffer[max_len - 1] = '\0';
-	  break;
-	}
-      cnt = max_len - (cp - buffer);
-      if (cnt > 8)
-	cnt = 8;
-      read_memory (memaddr + (int) (cp - buffer), (gdb_byte *) cp, cnt);
-      for (i = 0; i < cnt && *cp; i++, cp++)
-	;			/* null body */
-
-      if (i < cnt && !*cp)
-	break;
-    }
-}
-
 CORE_ADDR
 read_memory_typed_address (CORE_ADDR addr, struct type *type)
 {
diff --git a/gdb/gdbcore.h b/gdb/gdbcore.h
index 24db21e462c..58566d58785 100644
--- a/gdb/gdbcore.h
+++ b/gdb/gdbcore.h
@@ -87,12 +87,6 @@ extern ULONGEST read_code_unsigned_integer (CORE_ADDR memaddr,
 					    int len,
 					    enum bfd_endian byte_order);
 
-/* Read a null-terminated string from the debuggee's memory, given
-   address, a buffer into which to place the string, and the maximum
-   available space.  */
-
-extern void read_memory_string (CORE_ADDR, char *, int);
-
 /* Read the pointer of type TYPE at ADDR, and return the address it
    represents.  */
 
-- 
2.21.3

Re: [PATCH 1/4] Remove read_memory_string

[Simon Marchi]

On 2020-06-12 5:53 p.m., Tom Tromey wrote:
> @@ -6721,34 +6721,38 @@ ada_get_tsd_from_tag (struct value *tag)
>     The returned value is good until the next call.  May return NULL
>     if we are unable to determine the tag name.  */
>  
> -static char *
> +static gdb::unique_xmalloc_ptr<char>
>  ada_tag_name_from_tsd (struct value *tsd)

The comment above needs to be updated.

>  {
> -  static char name[1024];
>    char *p;
>    struct value *val;
>  
>    val = ada_value_struct_elt (tsd, "expanded_name", 1);
>    if (val == NULL)
>      return NULL;
> -  read_memory_string (value_as_address (val), name, sizeof (name) - 1);
> -  for (p = name; *p != '\0'; p += 1)
> +  gdb::unique_xmalloc_ptr<char> buffer;
> +  int err;
> +  if (target_read_string (value_as_address (val), &buffer, INT_MAX, &err) == 0
> +      || err != 0)
> +    return nullptr;
> +
> +  for (p = (char *) buffer.get (); *p != '\0'; p += 1)

This cast is unnecessary.  I'd also change `p += 1` for p++ or ++p.

The enclosing for loop should use braces, normally.

Simon

Re: [PATCH 1/4] Remove read_memory_string

[Pedro Alves]

On 6/12/20 10:53 PM, Tom Tromey wrote:
> read_memory_string is redundant and only called in a couple of spots.
> This patch removes it in favor of target_read_string.
> 

There's a difference of behavior that is getting lost here -- read_memory_string
throws an error on memory read failure, while target_read_string does not.

Similarly to how we have read_memory throwing, vs target_read_memory not
throwing.  It would make sense to me to reimplement read_memory_string on top
of target_read_string and throw on error, for instance.

Did you go over the call paths, to check whether the behavior change
is desirable?

Thanks,
Pedro Alves

> gdb/ChangeLog
> 2020-06-02  Tom Tromey  <tromey@adacore.com>
> 
> 	* corefile.c (read_memory_string): Remove.
> 	* ada-valprint.c (ada_value_print_ptr): Update.
> 	* ada-lang.h (ada_tag_name): Change return type.
> 	* ada-lang.c (type_from_tag): Update.
> 	(ada_tag_name_from_tsd): Change return type.  Use
> 	target_read_string.
> 	(ada_tag_name): Likewise.
> 	* gdbcore.h (read_memory_string): Don't declare.
> ---
>  gdb/ChangeLog      | 11 +++++++++++
>  gdb/ada-lang.c     | 34 ++++++++++++++++++++--------------
>  gdb/ada-lang.h     |  2 +-
>  gdb/ada-valprint.c |  4 ++--
>  gdb/corefile.c     | 27 ---------------------------
>  gdb/gdbcore.h      |  6 ------
>  6 files changed, 34 insertions(+), 50 deletions(-)
> 
> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index f7b973f36ee..3245758a1bd 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -6567,10 +6567,10 @@ value_tag_from_contents_and_address (struct type *type,
>  static struct type *
>  type_from_tag (struct value *tag)
>  {
> -  const char *type_name = ada_tag_name (tag);
> +  gdb::unique_xmalloc_ptr<char> type_name = ada_tag_name (tag);
>  
>    if (type_name != NULL)
> -    return ada_find_any_type (ada_encode (type_name));
> +    return ada_find_any_type (ada_encode (type_name.get ()));
>    return NULL;
>  }
>  
> @@ -6721,34 +6721,38 @@ ada_get_tsd_from_tag (struct value *tag)
>     The returned value is good until the next call.  May return NULL
>     if we are unable to determine the tag name.  */
>  
> -static char *
> +static gdb::unique_xmalloc_ptr<char>
>  ada_tag_name_from_tsd (struct value *tsd)
>  {
> -  static char name[1024];
>    char *p;
>    struct value *val;
>  
>    val = ada_value_struct_elt (tsd, "expanded_name", 1);
>    if (val == NULL)
>      return NULL;
> -  read_memory_string (value_as_address (val), name, sizeof (name) - 1);
> -  for (p = name; *p != '\0'; p += 1)
> +  gdb::unique_xmalloc_ptr<char> buffer;
> +  int err;
> +  if (target_read_string (value_as_address (val), &buffer, INT_MAX, &err) == 0
> +      || err != 0)
> +    return nullptr;
> +
> +  for (p = (char *) buffer.get (); *p != '\0'; p += 1)
>      if (isalpha (*p))
>        *p = tolower (*p);
> -  return name;
> +
> +  return buffer;
>  }
>  
>  /* The type name of the dynamic type denoted by the 'tag value TAG, as
>     a C string.
>  
>     Return NULL if the TAG is not an Ada tag, or if we were unable to
> -   determine the name of that tag.  The result is good until the next
> -   call.  */
> +   determine the name of that tag.  */
>  
> -const char *
> +gdb::unique_xmalloc_ptr<char>
>  ada_tag_name (struct value *tag)
>  {
> -  char *name = NULL;
> +  gdb::unique_xmalloc_ptr<char> name;
>  
>    if (!ada_is_tag_type (value_type (tag)))
>      return NULL;
> @@ -12104,9 +12108,11 @@ ada_exception_message_1 (void)
>    if (e_msg_len <= 0)
>      return NULL;
>  
> -  gdb::unique_xmalloc_ptr<char> e_msg ((char *) xmalloc (e_msg_len + 1));
> -  read_memory_string (value_address (e_msg_val), e_msg.get (), e_msg_len + 1);
> -  e_msg.get ()[e_msg_len] = '\0';
> +  gdb::unique_xmalloc_ptr<char> e_msg;
> +  int err;
> +  if (target_read_string (value_address (e_msg_val), &e_msg, INT_MAX, &err) == 0
> +      || err != 0)
> +    return nullptr;
>  
>    return e_msg;
>  }
> diff --git a/gdb/ada-lang.h b/gdb/ada-lang.h
> index 5ba00518e6d..9be597942fd 100644
> --- a/gdb/ada-lang.h
> +++ b/gdb/ada-lang.h
> @@ -260,7 +260,7 @@ extern int ada_is_tagged_type (struct type *, int);
>  
>  extern int ada_is_tag_type (struct type *);
>  
> -extern const char *ada_tag_name (struct value *);
> +extern gdb::unique_xmalloc_ptr<char> ada_tag_name (struct value *);
>  
>  extern struct value *ada_tag_value_at_base_address (struct value *obj);
>  
> diff --git a/gdb/ada-valprint.c b/gdb/ada-valprint.c
> index a36e7ca793a..61893d5cad3 100644
> --- a/gdb/ada-valprint.c
> +++ b/gdb/ada-valprint.c
> @@ -754,10 +754,10 @@ ada_value_print_ptr (struct value *val,
>    struct type *type = ada_check_typedef (value_type (val));
>    if (ada_is_tag_type (type))
>      {
> -      const char *name = ada_tag_name (val);
> +      gdb::unique_xmalloc_ptr<char> name = ada_tag_name (val);
>  
>        if (name != NULL)
> -	fprintf_filtered (stream, " (%s)", name);
> +	fprintf_filtered (stream, " (%s)", name.get ());
>      }
>  }
>  
> diff --git a/gdb/corefile.c b/gdb/corefile.c
> index 996e5301b27..fed0e4fe8ad 100644
> --- a/gdb/corefile.c
> +++ b/gdb/corefile.c
> @@ -328,33 +328,6 @@ read_code_unsigned_integer (CORE_ADDR memaddr, int len,
>    return extract_unsigned_integer (buf, len, byte_order);
>  }
>  
> -void
> -read_memory_string (CORE_ADDR memaddr, char *buffer, int max_len)
> -{
> -  char *cp;
> -  int i;
> -  int cnt;
> -
> -  cp = buffer;
> -  while (1)
> -    {
> -      if (cp - buffer >= max_len)
> -	{
> -	  buffer[max_len - 1] = '\0';
> -	  break;
> -	}
> -      cnt = max_len - (cp - buffer);
> -      if (cnt > 8)
> -	cnt = 8;
> -      read_memory (memaddr + (int) (cp - buffer), (gdb_byte *) cp, cnt);
> -      for (i = 0; i < cnt && *cp; i++, cp++)
> -	;			/* null body */
> -
> -      if (i < cnt && !*cp)
> -	break;
> -    }
> -}
> -
>  CORE_ADDR
>  read_memory_typed_address (CORE_ADDR addr, struct type *type)
>  {
> diff --git a/gdb/gdbcore.h b/gdb/gdbcore.h
> index 24db21e462c..58566d58785 100644
> --- a/gdb/gdbcore.h
> +++ b/gdb/gdbcore.h
> @@ -87,12 +87,6 @@ extern ULONGEST read_code_unsigned_integer (CORE_ADDR memaddr,
>  					    int len,
>  					    enum bfd_endian byte_order);
>  
> -/* Read a null-terminated string from the debuggee's memory, given
> -   address, a buffer into which to place the string, and the maximum
> -   available space.  */
> -
> -extern void read_memory_string (CORE_ADDR, char *, int);
> -
>  /* Read the pointer of type TYPE at ADDR, and return the address it
>     represents.  */
>  
> 

[PATCH 2/4] Rewrite target_read_string

[Tom Tromey]

This rewrites target_read_string in terms of read_string.

gdb/ChangeLog
2020-06-02  Tom Tromey  <tromey@adacore.com>

	* valprint.c (read_string): Update comment.
	* target.c (MIN): Remove.
	(target_read_string): Rewrite.
---
 gdb/ChangeLog  |  6 +++++
 gdb/target.c   | 71 +++++++-------------------------------------------
 gdb/valprint.c |  8 +-----
 3 files changed, 16 insertions(+), 69 deletions(-)

diff --git a/gdb/target.c b/gdb/target.c
index 82c405a8491..14c494688e0 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -803,9 +803,6 @@ target_xfer_status_to_string (enum target_xfer_status status)
 };
 
 
-#undef	MIN
-#define MIN(A, B) (((A) <= (B)) ? (A) : (B))
-
 /* target_read_string -- read a null terminated string, up to LEN bytes,
    from MEMADDR in target.  Set *ERRNOP to the errno code, or 0 if successful.
    Set *STRING to a pointer to malloc'd memory containing the data; the caller
@@ -816,68 +813,18 @@ int
 target_read_string (CORE_ADDR memaddr, gdb::unique_xmalloc_ptr<char> *string,
 		    int len, int *errnop)
 {
-  int tlen, offset, i;
-  gdb_byte buf[4];
-  int errcode = 0;
-  char *buffer;
-  int buffer_allocated;
-  char *bufptr;
-  unsigned int nbytes_read = 0;
-
-  gdb_assert (string);
-
-  /* Small for testing.  */
-  buffer_allocated = 4;
-  buffer = (char *) xmalloc (buffer_allocated);
-  bufptr = buffer;
-
-  while (len > 0)
-    {
-      tlen = MIN (len, 4 - (memaddr & 3));
-      offset = memaddr & 3;
+  int bytes_read;
+  gdb::unique_xmalloc_ptr<gdb_byte> buffer;
 
-      errcode = target_read_memory (memaddr & ~3, buf, sizeof buf);
-      if (errcode != 0)
-	{
-	  /* The transfer request might have crossed the boundary to an
-	     unallocated region of memory.  Retry the transfer, requesting
-	     a single byte.  */
-	  tlen = 1;
-	  offset = 0;
-	  errcode = target_read_memory (memaddr, buf, 1);
-	  if (errcode != 0)
-	    goto done;
-	}
-
-      if (bufptr - buffer + tlen > buffer_allocated)
-	{
-	  unsigned int bytes;
+  /* Note that the endian-ness does not matter here.  */
+  int errcode = read_string (memaddr, -1, 1, len, BFD_ENDIAN_LITTLE,
+			     &buffer, &bytes_read);
 
-	  bytes = bufptr - buffer;
-	  buffer_allocated *= 2;
-	  buffer = (char *) xrealloc (buffer, buffer_allocated);
-	  bufptr = buffer + bytes;
-	}
-
-      for (i = 0; i < tlen; i++)
-	{
-	  *bufptr++ = buf[i + offset];
-	  if (buf[i + offset] == '\000')
-	    {
-	      nbytes_read += i + 1;
-	      goto done;
-	    }
-	}
-
-      memaddr += tlen;
-      len -= tlen;
-      nbytes_read += tlen;
-    }
-done:
-  string->reset (buffer);
-  if (errnop != NULL)
+  if (errnop != nullptr)
     *errnop = errcode;
-  return nbytes_read;
+
+  string->reset ((char *) buffer.release ());
+  return bytes_read;
 }
 
 struct target_section_table *
diff --git a/gdb/valprint.c b/gdb/valprint.c
index d5490898b9b..f2549805268 100644
--- a/gdb/valprint.c
+++ b/gdb/valprint.c
@@ -2027,13 +2027,7 @@ partial_memory_read (CORE_ADDR memaddr, gdb_byte *myaddr,
 
    Unless an exception is thrown, BUFFER will always be allocated, even on
    failure.  In this case, some characters might have been read before the
-   failure happened.  Check BYTES_READ to recognize this situation.
-
-   Note: There was a FIXME asking to make this code use target_read_string,
-   but this function is more general (can read past null characters, up to
-   given LEN).  Besides, it is used much more often than target_read_string
-   so it is more tested.  Perhaps callers of target_read_string should use
-   this function instead?  */
+   failure happened.  Check BYTES_READ to recognize this situation.  */
 
 int
 read_string (CORE_ADDR addr, int len, int width, unsigned int fetchlimit,
-- 
2.21.3

[PATCH 3/4] Remove a use of target_read_string

[Tom Tromey]

linux-tdep.c:dump_mapping_p uses target_read_string, but in a way that
does not really make sense.  It's better to use target_read_memory
here.

gdb/ChangeLog
2020-06-02  Tom Tromey  <tromey@adacore.com>

	* linux-tdep.c (dump_mapping_p): Use target_read_memory.
---
 gdb/ChangeLog    |  4 ++++
 gdb/linux-tdep.c | 12 +++---------
 gdb/target.c     |  1 +
 3 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index 0f9559355f1..2dcdc630769 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -701,22 +701,16 @@ dump_mapping_p (filter_flags filterflags, const struct smaps_vmflags *v,
   if (!dump_p && private_p && offset == 0
       && (filterflags & COREFILTER_ELF_HEADERS) != 0)
     {
-      /* Let's check if we have an ELF header.  */
-      gdb::unique_xmalloc_ptr<char> header;
-      int errcode;
-
       /* Useful define specifying the size of the ELF magical
 	 header.  */
 #ifndef SELFMAG
 #define SELFMAG 4
 #endif
 
-      /* Read the first SELFMAG bytes and check if it is ELFMAG.  */
-      if (target_read_string (addr, &header, SELFMAG, &errcode) == SELFMAG
-	  && errcode == 0)
+      /* Let's check if we have an ELF header.  */
+      gdb_byte h[SELFMAG];
+      if (target_read_memory (addr, h, SELFMAG) == 0)
 	{
-	  const char *h = header.get ();
-
 	  /* The EI_MAG* and ELFMAG* constants come from
 	     <elf/common.h>.  */
 	  if (h[EI_MAG0] == ELFMAG0 && h[EI_MAG1] == ELFMAG1
diff --git a/gdb/target.c b/gdb/target.c
index 14c494688e0..897b8fdd32b 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -50,6 +50,7 @@
 #include "terminal.h"
 #include <unordered_map>
 #include "target-connection.h"
+#include "valprint.h"
 
 static void generic_tls_error (void) ATTRIBUTE_NORETURN;
 
-- 
2.21.3

Re: [PATCH 3/4] Remove a use of target_read_string

[Sergio Durigan Junior]

On Friday, June 12 2020, Tom Tromey wrote:

> linux-tdep.c:dump_mapping_p uses target_read_string, but in a way that
> does not really make sense.  It's better to use target_read_memory
> here.

Thanks for catching this problem, Tom.

> gdb/ChangeLog
> 2020-06-02  Tom Tromey  <tromey@adacore.com>
>
> 	* linux-tdep.c (dump_mapping_p): Use target_read_memory.
> ---
>  gdb/ChangeLog    |  4 ++++
>  gdb/linux-tdep.c | 12 +++---------
>  gdb/target.c     |  1 +
>  3 files changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index 0f9559355f1..2dcdc630769 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -701,22 +701,16 @@ dump_mapping_p (filter_flags filterflags, const struct smaps_vmflags *v,
>    if (!dump_p && private_p && offset == 0
>        && (filterflags & COREFILTER_ELF_HEADERS) != 0)
>      {
> -      /* Let's check if we have an ELF header.  */
> -      gdb::unique_xmalloc_ptr<char> header;
> -      int errcode;
> -
>        /* Useful define specifying the size of the ELF magical
>  	 header.  */
>  #ifndef SELFMAG
>  #define SELFMAG 4
>  #endif
>  
> -      /* Read the first SELFMAG bytes and check if it is ELFMAG.  */
> -      if (target_read_string (addr, &header, SELFMAG, &errcode) == SELFMAG
> -	  && errcode == 0)
> +      /* Let's check if we have an ELF header.  */
> +      gdb_byte h[SELFMAG];
> +      if (target_read_memory (addr, h, SELFMAG) == 0)
>  	{
> -	  const char *h = header.get ();
> -
>  	  /* The EI_MAG* and ELFMAG* constants come from
>  	     <elf/common.h>.  */
>  	  if (h[EI_MAG0] == ELFMAG0 && h[EI_MAG1] == ELFMAG1

This looks OK to me.

> diff --git a/gdb/target.c b/gdb/target.c
> index 14c494688e0..897b8fdd32b 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -50,6 +50,7 @@
>  #include "terminal.h"
>  #include <unordered_map>
>  #include "target-connection.h"
> +#include "valprint.h"
>  
>  static void generic_tls_error (void) ATTRIBUTE_NORETURN;

This hunk looks unrelated.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
https://sergiodj.net/

Re: [PATCH 3/4] Remove a use of target_read_string

[Tom Tromey]

>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@sergiodj.net> writes:

>> diff --git a/gdb/target.c b/gdb/target.c
>> index 14c494688e0..897b8fdd32b 100644
>> --- a/gdb/target.c
>> +++ b/gdb/target.c
>> @@ -50,6 +50,7 @@
>> #include "terminal.h"
>> #include <unordered_map>
>> #include "target-connection.h"
>> +#include "valprint.h"
>> 
>> static void generic_tls_error (void) ATTRIBUTE_NORETURN;

Sergio> This hunk looks unrelated.

Oops, yeah -- this should be in one of the other patches.
I'll fix this.

Tom

[PATCH 4/4] Change target_read_string API

[Tom Tromey]

This simplifies the target_read_string API a bit.

Note that some code was using safe_strerror on the error codes
returned by target_read_string.  It seems to me that this is incorrect
(if it was ever correct, it must have been quite a long time ago).

gdb/ChangeLog
2020-06-12  Tom Tromey  <tromey@adacore.com>

	* windows-nat.c (windows_nat::handle_output_debug_string):
	Update.
	(windows_nat::handle_ms_vc_exception): Update.
	* target.h (target_read_string): Change API.
	* target.c (target_read_string): Change API.
	* solib-svr4.c (open_symbol_file_object, svr4_read_so_list):
	Update.
	* solib-frv.c (frv_current_sos): Update.
	* solib-dsbt.c (dsbt_current_sos): Update.
	* solib-darwin.c (darwin_current_sos): Update.
	* linux-thread-db.c (inferior_has_bug): Update.
	* expprint.c (print_subexp_standard): Update.
	* ada-lang.c (ada_main_name, ada_tag_name_from_tsd)
	(ada_exception_message_1): Update.
---
 gdb/ChangeLog         | 17 +++++++++++++++++
 gdb/ada-lang.c        | 22 +++++-----------------
 gdb/expprint.c        |  9 +++------
 gdb/linux-thread-db.c |  9 +++++----
 gdb/solib-darwin.c    |  6 ++----
 gdb/solib-dsbt.c      |  9 +++------
 gdb/solib-frv.c       |  9 +++------
 gdb/solib-svr4.c      | 17 ++++++-----------
 gdb/target.c          | 26 +++++++++++---------------
 gdb/target.h          |  9 +++++++--
 gdb/windows-nat.c     | 14 ++++++--------
 11 files changed, 68 insertions(+), 79 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 3245758a1bd..5c2ceb72f69 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -866,17 +866,12 @@ ada_main_name (void)
   if (msym.minsym != NULL)
     {
       CORE_ADDR main_program_name_addr;
-      int err_code;
 
       main_program_name_addr = BMSYMBOL_VALUE_ADDRESS (msym);
       if (main_program_name_addr == 0)
         error (_("Invalid address for Ada main program name."));
 
-      target_read_string (main_program_name_addr, &main_program_name,
-                          1024, &err_code);
-
-      if (err_code != 0)
-        return NULL;
+      main_program_name = target_read_string (main_program_name_addr, 1024);
       return main_program_name.get ();
     }
 
@@ -6730,10 +6725,9 @@ ada_tag_name_from_tsd (struct value *tsd)
   val = ada_value_struct_elt (tsd, "expanded_name", 1);
   if (val == NULL)
     return NULL;
-  gdb::unique_xmalloc_ptr<char> buffer;
-  int err;
-  if (target_read_string (value_as_address (val), &buffer, INT_MAX, &err) == 0
-      || err != 0)
+  gdb::unique_xmalloc_ptr<char> buffer
+    = target_read_string (value_as_address (val), INT_MAX);
+  if (buffer == nullptr)
     return nullptr;
 
   for (p = (char *) buffer.get (); *p != '\0'; p += 1)
@@ -12108,13 +12102,7 @@ ada_exception_message_1 (void)
   if (e_msg_len <= 0)
     return NULL;
 
-  gdb::unique_xmalloc_ptr<char> e_msg;
-  int err;
-  if (target_read_string (value_address (e_msg_val), &e_msg, INT_MAX, &err) == 0
-      || err != 0)
-    return nullptr;
-
-  return e_msg;
+  return target_read_string (value_address (e_msg_val), INT_MAX);
 }
 
 /* Same as ada_exception_message_1, except that all exceptions are
diff --git a/gdb/expprint.c b/gdb/expprint.c
index 026b775260d..86cffa890f9 100644
--- a/gdb/expprint.c
+++ b/gdb/expprint.c
@@ -247,12 +247,9 @@ print_subexp_standard (struct expression *exp, int *pos,
 	nargs = longest_to_int (exp->elts[pc + 2].longconst);
 	fprintf_unfiltered (stream, "[");
 	print_subexp (exp, pos, stream, PREC_SUFFIX);
-	if (0 == target_read_string (exp->elts[pc + 1].longconst,
-				     &selector, 1024, NULL))
-	  {
-	    error (_("bad selector"));
-	    return;
-	  }
+	selector = target_read_string (exp->elts[pc + 1].longconst, 1024);
+	if (selector == nullptr)
+	  error (_("bad selector"));
 	if (nargs)
 	  {
 	    char *s, *nextS;
diff --git a/gdb/linux-thread-db.c b/gdb/linux-thread-db.c
index ae29c51673a..b3cda05cd6e 100644
--- a/gdb/linux-thread-db.c
+++ b/gdb/linux-thread-db.c
@@ -472,16 +472,17 @@ inferior_has_bug (const char *ver_symbol, int ver_major_min, int ver_minor_min)
 {
   struct bound_minimal_symbol version_msym;
   CORE_ADDR version_addr;
-  gdb::unique_xmalloc_ptr<char> version;
-  int err, got, retval = 0;
+  int got, retval = 0;
 
   version_msym = lookup_minimal_symbol (ver_symbol, NULL, NULL);
   if (version_msym.minsym == NULL)
     return 0;
 
   version_addr = BMSYMBOL_VALUE_ADDRESS (version_msym);
-  got = target_read_string (version_addr, &version, 32, &err);
-  if (err == 0 && memchr (version.get (), 0, got) == version.get () + got - 1)
+  gdb::unique_xmalloc_ptr<char> version
+    = target_read_string (version_addr, 32, &got);
+  if (version != nullptr
+      && memchr (version.get (), 0, got) == version.get () + got - 1)
     {
       int major, minor;
 
diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
index ee0483d2c87..93eabf38678 100644
--- a/gdb/solib-darwin.c
+++ b/gdb/solib-darwin.c
@@ -252,7 +252,6 @@ darwin_current_sos (void)
       struct mach_o_header_external hdr;
       unsigned long hdr_val;
       gdb::unique_xmalloc_ptr<char> file_path;
-      int errcode;
 
       /* Read image info from inferior.  */
       if (target_read_memory (iinfo, buf, image_info_size))
@@ -275,9 +274,8 @@ darwin_current_sos (void)
       if (hdr_val == BFD_MACH_O_MH_EXECUTE)
         continue;
 
-      target_read_string (path_addr, &file_path,
-			  SO_NAME_MAX_PATH_SIZE - 1, &errcode);
-      if (errcode)
+      file_path = target_read_string (path_addr, SO_NAME_MAX_PATH_SIZE - 1);
+      if (file_path == nullptr)
 	break;
 
       /* Create and fill the new so_list element.  */
diff --git a/gdb/solib-dsbt.c b/gdb/solib-dsbt.c
index 2acad96b5d9..c676edf4f1c 100644
--- a/gdb/solib-dsbt.c
+++ b/gdb/solib-dsbt.c
@@ -681,7 +681,6 @@ dsbt_current_sos (void)
 	 this in the list of shared objects.  */
       if (dsbt_index != 0)
 	{
-	  int errcode;
 	  gdb::unique_xmalloc_ptr<char> name_buf;
 	  struct int_elf32_dsbt_loadmap *loadmap;
 	  struct so_list *sop;
@@ -703,12 +702,10 @@ dsbt_current_sos (void)
 	  addr = extract_unsigned_integer (lm_buf.l_name,
 					   sizeof (lm_buf.l_name),
 					   byte_order);
-	  target_read_string (addr, &name_buf, SO_NAME_MAX_PATH_SIZE - 1,
-			      &errcode);
+	  name_buf = target_read_string (addr, SO_NAME_MAX_PATH_SIZE - 1);
 
-	  if (errcode != 0)
-	    warning (_("Can't read pathname for link map entry: %s."),
-		     safe_strerror (errcode));
+	  if (name_buf == nullptr)
+	    warning (_("Can't read pathname for link map entry."));
 	  else
 	    {
 	      if (solib_dsbt_debug)
diff --git a/gdb/solib-frv.c b/gdb/solib-frv.c
index 62e7b05b490..fad4cc8f021 100644
--- a/gdb/solib-frv.c
+++ b/gdb/solib-frv.c
@@ -376,7 +376,6 @@ frv_current_sos (void)
 	 this in the list of shared objects.  */
       if (got_addr != mgot)
 	{
-	  int errcode;
 	  gdb::unique_xmalloc_ptr<char> name_buf;
 	  struct int_elf32_fdpic_loadmap *loadmap;
 	  struct so_list *sop;
@@ -404,16 +403,14 @@ frv_current_sos (void)
 	  addr = extract_unsigned_integer (lm_buf.l_name,
 					   sizeof (lm_buf.l_name),
 					   byte_order);
-	  target_read_string (addr, &name_buf, SO_NAME_MAX_PATH_SIZE - 1,
-			      &errcode);
+	  name_buf = target_read_string (addr, SO_NAME_MAX_PATH_SIZE - 1);
 
 	  if (solib_frv_debug)
 	    fprintf_unfiltered (gdb_stdlog, "current_sos: name = %s\n",
 	                        name_buf.get ());
 	  
-	  if (errcode != 0)
-	    warning (_("Can't read pathname for link map entry: %s."),
-		     safe_strerror (errcode));
+	  if (name_buf == nullptr)
+	    warning (_("Can't read pathname for link map entry."));
 	  else
 	    {
 	      strncpy (sop->so_name, name_buf.get (),
diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index 19d1105ae95..6c0cffe4a42 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -958,7 +958,6 @@ open_symbol_file_object (int from_tty)
 {
   CORE_ADDR lm, l_name;
   gdb::unique_xmalloc_ptr<char> filename;
-  int errcode;
   struct link_map_offsets *lmo = svr4_fetch_link_map_offsets ();
   struct type *ptr_type = builtin_type (target_gdbarch ())->builtin_data_ptr;
   int l_name_size = TYPE_LENGTH (ptr_type);
@@ -993,12 +992,11 @@ open_symbol_file_object (int from_tty)
     return 0;		/* No filename.  */
 
   /* Now fetch the filename from target memory.  */
-  target_read_string (l_name, &filename, SO_NAME_MAX_PATH_SIZE - 1, &errcode);
+  filename = target_read_string (l_name, SO_NAME_MAX_PATH_SIZE - 1);
 
-  if (errcode)
+  if (filename == nullptr)
     {
-      warning (_("failed to read exec filename from attached file: %s"),
-	       safe_strerror (errcode));
+      warning (_("failed to read exec filename from attached file"));
       return 0;
     }
 
@@ -1297,7 +1295,6 @@ svr4_read_so_list (svr4_info *info, CORE_ADDR lm, CORE_ADDR prev_lm,
 
   for (; lm != 0; prev_lm = lm, lm = next_lm)
     {
-      int errcode;
       gdb::unique_xmalloc_ptr<char> buffer;
 
       so_list_up newobj (XCNEW (struct so_list));
@@ -1330,17 +1327,15 @@ svr4_read_so_list (svr4_info *info, CORE_ADDR lm, CORE_ADDR prev_lm,
 	}
 
       /* Extract this shared object's name.  */
-      target_read_string (li->l_name, &buffer, SO_NAME_MAX_PATH_SIZE - 1,
-			  &errcode);
-      if (errcode != 0)
+      buffer = target_read_string (li->l_name, SO_NAME_MAX_PATH_SIZE - 1);
+      if (buffer == nullptr)
 	{
 	  /* If this entry's l_name address matches that of the
 	     inferior executable, then this is not a normal shared
 	     object, but (most likely) a vDSO.  In this case, silently
 	     skip it; otherwise emit a warning. */
 	  if (first_l_name == 0 || li->l_name != first_l_name)
-	    warning (_("Can't read pathname for load map: %s."),
-		     safe_strerror (errcode));
+	    warning (_("Can't read pathname for load map."));
 	  continue;
 	}
 
diff --git a/gdb/target.c b/gdb/target.c
index 897b8fdd32b..e8193b49fa0 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -804,28 +804,24 @@ target_xfer_status_to_string (enum target_xfer_status status)
 };
 
 
-/* target_read_string -- read a null terminated string, up to LEN bytes,
-   from MEMADDR in target.  Set *ERRNOP to the errno code, or 0 if successful.
-   Set *STRING to a pointer to malloc'd memory containing the data; the caller
-   is responsible for freeing it.  Return the number of bytes successfully
-   read.  */
+/* See target.h.  */
 
-int
-target_read_string (CORE_ADDR memaddr, gdb::unique_xmalloc_ptr<char> *string,
-		    int len, int *errnop)
+gdb::unique_xmalloc_ptr<char>
+target_read_string (CORE_ADDR memaddr, int len, int *bytes_read)
 {
-  int bytes_read;
   gdb::unique_xmalloc_ptr<gdb_byte> buffer;
 
+  int ignore;
+  if (bytes_read == nullptr)
+    bytes_read = &ignore;
+
   /* Note that the endian-ness does not matter here.  */
   int errcode = read_string (memaddr, -1, 1, len, BFD_ENDIAN_LITTLE,
-			     &buffer, &bytes_read);
-
-  if (errnop != nullptr)
-    *errnop = errcode;
+			     &buffer, bytes_read);
+  if (errcode != 0)
+    return {};
 
-  string->reset ((char *) buffer.release ());
-  return bytes_read;
+  return gdb::unique_xmalloc_ptr<char> ((char *) buffer.release ());
 }
 
 struct target_section_table *
diff --git a/gdb/target.h b/gdb/target.h
index 37bfb29882a..4e8d4cccd5c 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1505,8 +1505,13 @@ int target_supports_disable_randomization (void);
 #define target_can_run_breakpoint_commands() \
   (current_top_target ()->can_run_breakpoint_commands) ()
 
-extern int target_read_string (CORE_ADDR, gdb::unique_xmalloc_ptr<char> *,
-			       int, int *);
+/* Read a string from target memory at address MEMADDR.  The string
+   will be at most LEN bytes long (note that excess bytes may be read
+   in some cases -- but these will not be returned).  Returns nullptr
+   on error.  */
+
+extern gdb::unique_xmalloc_ptr<char> target_read_string
+  (CORE_ADDR memaddr, int len, int *bytes_read = nullptr);
 
 /* For target_read_memory see target/target.h.  */
 
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 3452f6c827f..2af14033296 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -988,10 +988,10 @@ windows_nat::handle_output_debug_string (struct target_waitstatus *ourstatus)
   gdb::unique_xmalloc_ptr<char> s;
   int retval = 0;
 
-  if (!target_read_string
-	((CORE_ADDR) (uintptr_t) current_event.u.DebugString.lpDebugStringData,
-	 &s, 1024, 0)
-      || !s || !*(s.get ()))
+  s = (target_read_string
+       ((CORE_ADDR) (uintptr_t) current_event.u.DebugString.lpDebugStringData,
+	1024));
+  if (s == nullptr || !*(s.get ()))
     /* nothing to do */;
   else if (!startswith (s.get (), _CYGWIN_SIGNAL_STRING))
     {
@@ -1216,10 +1216,8 @@ windows_nat::handle_ms_vc_exception (const EXCEPTION_RECORD *rec)
       if (named_thread != NULL)
 	{
 	  int thread_name_len;
-	  gdb::unique_xmalloc_ptr<char> thread_name;
-
-	  thread_name_len = target_read_string (thread_name_target,
-						&thread_name, 1025, NULL);
+	  gdb::unique_xmalloc_ptr<char> thread_name
+	    = target_read_string (thread_name_target, 1025, &thread_name_len);
 	  if (thread_name_len > 0)
 	    {
 	      thread_name.get ()[thread_name_len - 1] = '\0';
-- 
2.21.3

Re: [PATCH 4/4] Change target_read_string API

[Simon Marchi]

On 2020-06-12 5:53 p.m., Tom Tromey wrote:
> diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
> index 19d1105ae95..6c0cffe4a42 100644
> --- a/gdb/solib-svr4.c
> +++ b/gdb/solib-svr4.c
> @@ -958,7 +958,6 @@ open_symbol_file_object (int from_tty)
>  {
>    CORE_ADDR lm, l_name;
>    gdb::unique_xmalloc_ptr<char> filename;
> -  int errcode;
>    struct link_map_offsets *lmo = svr4_fetch_link_map_offsets ();
>    struct type *ptr_type = builtin_type (target_gdbarch ())->builtin_data_ptr;
>    int l_name_size = TYPE_LENGTH (ptr_type);
> @@ -993,12 +992,11 @@ open_symbol_file_object (int from_tty)
>      return 0;		/* No filename.  */
>  
>    /* Now fetch the filename from target memory.  */
> -  target_read_string (l_name, &filename, SO_NAME_MAX_PATH_SIZE - 1, &errcode);
> +  filename = target_read_string (l_name, SO_NAME_MAX_PATH_SIZE - 1);

Declare `filename` here?  There are a few more occurences of this in the patch that I
would fix, since that actually results in less generated code.

Simon

Re: [PATCH 0/4] Unify string-reading APIs

[Simon Marchi]

On 2020-06-12 5:53 p.m., Tom Tromey wrote:
> An old comment notes that gdb has several different ways to read a
> string from the target.  This series tries to simplify this situation.
> 
> read_memory_string is removed entirely.  Perhaps this is the better
> name, in the end; but I didn't want to move read_string into
> corelow.c.  Maybe we need a new file for the memory-reading functions
> that are there?  (Also I wonder if any of the remaining ones are
> redundant.)
> 
> target_read_string is first rewritten in terms of read_string.  A
> subsequent patch changes its API to be simpler.
> 
> Perhaps further simplification could be done as well.  I'm open to
> suggestions.
> 
> Regression tested on x86-64 Fedora 30.  I also built it using a mingw
> cross, to make sure windows-nat.c still builds.
> 
> Tom

Apart from the few stylistic comments I've made, it LGTM.

Thanks!

Simon

Re: [PATCH 0/4] Unify string-reading APIs

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> Apart from the few stylistic comments I've made, it LGTM.

I fixed these up, and I'm checking this in now.

Tom