[4/4] Change target_read_string API

Message ID 20200612215356.22145-5-tromey@adacore.com
State New
Headers show
Series
  • Unify string-reading APIs
Related show

Commit Message

Tom Tromey June 12, 2020, 9:53 p.m.
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(-)

-- 
2.21.3

Comments

Simon Marchi June 13, 2020, 2:40 p.m. | #1
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

Patch

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';