[PATCH 0/9] Windows thread names

[PATCH 0/9] Windows thread names

[Tom Tromey]

I noticed that newer versions of Windows have a nicer API to setting
and fetching a thread's name.  Whereas in earlier versions this was
done by throwing and catching a special SEH exception, that the
debugger could intercept, now there is a simple API call.

I also noticed that, while gdb implements the older method already,
gdbserver did not.

This patch refactors some code a little bit so that the old method can
be shared between gdb and gdbserver.  Then it implements the new
method as well, and also changes gdb to set the names of its own
worker threads using the new approach.

I ran this through the AdaCore test suite without problems, and also
tested it by hand on a simple program that sets the current thread's
name.

Tom

[PATCH 1/9] Fix possible Cygwin build problem

[Tom Tromey]

I noticed that nat/windows-nat.c checks __USEWIDE, but nothing sets it
there -- I forgot to copy over the definition when making this file.
This patch tries to fix the problem.  I don't have a Cygwin setup, so
I don't know whether this is sufficient, but it's probably necessary.
---
 gdb/nat/windows-nat.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gdb/nat/windows-nat.c b/gdb/nat/windows-nat.c
index c890ee13c6c..e6d73485c24 100644
--- a/gdb/nat/windows-nat.c
+++ b/gdb/nat/windows-nat.c
@@ -20,6 +20,10 @@
 #include "nat/windows-nat.h"
 #include "gdbsupport/common-debug.h"
 
+#ifdef __CYGWIN__
+#define __USEWIDE
+#endif
+
 namespace windows_nat
 {
 
-- 
2.34.1

[PATCH 2/9] Don't call QUIT in read_string

[Tom Tromey]

read_string does not need to call QUIT, because target_read_memory
already does.  This change is needed to make string-reading usable by
gdbserver.
---
 gdb/valprint.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/gdb/valprint.c b/gdb/valprint.c
index 9990d432e35..65b85cfb038 100644
--- a/gdb/valprint.c
+++ b/gdb/valprint.c
@@ -2096,7 +2096,6 @@ read_string (CORE_ADDR addr, int len, int width, unsigned int fetchlimit,
 
       do
 	{
-	  QUIT;
 	  nfetch = std::min ((unsigned long) chunksize, fetchlimit - bufsize);
 
 	  if (*buffer == NULL)
@@ -2152,8 +2151,6 @@ read_string (CORE_ADDR addr, int len, int width, unsigned int fetchlimit,
      consider part of the string (including a '\0' which ends the string).  */
   *bytes_read = bufptr - buffer->get ();
 
-  QUIT;
-
   return errcode;
 }
 
-- 
2.34.1

[PATCH 3/9] Rename read_string

[Tom Tromey]

This renames read_string to be an overload of target_read_string.
This makes it more consistent for the eventual merger with gdbserver.
---
 gdb/c-lang.c   |  4 ++--
 gdb/target.c   |  4 ++--
 gdb/valprint.c | 12 +++++++-----
 gdb/valprint.h | 10 +++++-----
 4 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/gdb/c-lang.c b/gdb/c-lang.c
index 014dbefb8e2..a7ecf8f91da 100644
--- a/gdb/c-lang.c
+++ b/gdb/c-lang.c
@@ -352,8 +352,8 @@ c_get_string (struct value *value, gdb::unique_xmalloc_ptr<gdb_byte> *buffer,
       if (*length > 0)
 	fetchlimit = UINT_MAX;
 
-      err = read_string (addr, *length, width, fetchlimit,
-			 byte_order, buffer, length);
+      err = target_read_string (addr, *length, width, fetchlimit,
+				byte_order, buffer, length);
       if (err != 0)
 	memory_error (TARGET_XFER_E_IO, addr);
     }
diff --git a/gdb/target.c b/gdb/target.c
index f33bf345cfa..6542305f0d0 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1406,8 +1406,8 @@ target_read_string (CORE_ADDR memaddr, int len, int *bytes_read)
     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);
+  int errcode = target_read_string (memaddr, -1, 1, len, BFD_ENDIAN_LITTLE,
+				    &buffer, bytes_read);
   if (errcode != 0)
     return {};
 
diff --git a/gdb/valprint.c b/gdb/valprint.c
index 65b85cfb038..a4c0f7b343d 100644
--- a/gdb/valprint.c
+++ b/gdb/valprint.c
@@ -2050,9 +2050,11 @@ partial_memory_read (CORE_ADDR memaddr, gdb_byte *myaddr,
    failure happened.  Check BYTES_READ to recognize this situation.  */
 
 int
-read_string (CORE_ADDR addr, int len, int width, unsigned int fetchlimit,
-	     enum bfd_endian byte_order, gdb::unique_xmalloc_ptr<gdb_byte> *buffer,
-	     int *bytes_read)
+target_read_string (CORE_ADDR addr, int len, int width,
+		    unsigned int fetchlimit,
+		    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.  */
@@ -2731,8 +2733,8 @@ val_print_string (struct type *elttype, const char *encoding,
   fetchlimit = (len == -1 ? options->print_max : std::min ((unsigned) len,
 							   options->print_max));
 
-  err = read_string (addr, len, width, fetchlimit, byte_order,
-		     &buffer, &bytes_read);
+  err = target_read_string (addr, len, width, fetchlimit, byte_order,
+			    &buffer, &bytes_read);
 
   addr += bytes_read;
 
diff --git a/gdb/valprint.h b/gdb/valprint.h
index 0586836f9e6..2f4a5022b3e 100644
--- a/gdb/valprint.h
+++ b/gdb/valprint.h
@@ -165,11 +165,11 @@ extern void print_function_pointer_address (const struct value_print_options *op
 					    CORE_ADDR address,
 					    struct ui_file *stream);
 
-extern int read_string (CORE_ADDR addr, int len, int width,
-			unsigned int fetchlimit,
-			enum bfd_endian byte_order,
-			gdb::unique_xmalloc_ptr<gdb_byte> *buffer,
-			int *bytes_read);
+extern int target_read_string (CORE_ADDR addr, int len, int width,
+			       unsigned int fetchlimit,
+			       enum bfd_endian byte_order,
+			       gdb::unique_xmalloc_ptr<gdb_byte> *buffer,
+			       int *bytes_read);
 
 /* Helper function to check the validity of some bits of a value.
 
-- 
2.34.1

[PATCH 4/9] Remove the byte order parameter to target_read_string

[Tom Tromey]

target_read_string takes a byte order parameter, but only uses this to
check whether a given character is zero.  This is readily done without
requiring the parameter, so remove it.
---
 gdb/c-lang.c   |  2 +-
 gdb/target.c   |  3 +--
 gdb/valprint.c | 12 +++++++-----
 gdb/valprint.h |  1 -
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/gdb/c-lang.c b/gdb/c-lang.c
index a7ecf8f91da..be9ee073f58 100644
--- a/gdb/c-lang.c
+++ b/gdb/c-lang.c
@@ -353,7 +353,7 @@ c_get_string (struct value *value, gdb::unique_xmalloc_ptr<gdb_byte> *buffer,
 	fetchlimit = UINT_MAX;
 
       err = target_read_string (addr, *length, width, fetchlimit,
-				byte_order, buffer, length);
+				buffer, length);
       if (err != 0)
 	memory_error (TARGET_XFER_E_IO, addr);
     }
diff --git a/gdb/target.c b/gdb/target.c
index 6542305f0d0..5a416d5ee3a 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1406,8 +1406,7 @@ target_read_string (CORE_ADDR memaddr, int len, int *bytes_read)
     bytes_read = &ignore;
 
   /* Note that the endian-ness does not matter here.  */
-  int errcode = target_read_string (memaddr, -1, 1, len, BFD_ENDIAN_LITTLE,
-				    &buffer, bytes_read);
+  int errcode = target_read_string (memaddr, -1, 1, len, &buffer, bytes_read);
   if (errcode != 0)
     return {};
 
diff --git a/gdb/valprint.c b/gdb/valprint.c
index a4c0f7b343d..8f9954778c5 100644
--- a/gdb/valprint.c
+++ b/gdb/valprint.c
@@ -2052,7 +2052,6 @@ partial_memory_read (CORE_ADDR memaddr, gdb_byte *myaddr,
 int
 target_read_string (CORE_ADDR addr, int len, int width,
 		    unsigned int fetchlimit,
-		    enum bfd_endian byte_order,
 		    gdb::unique_xmalloc_ptr<gdb_byte> *buffer,
 		    int *bytes_read)
 {
@@ -2122,12 +2121,15 @@ target_read_string (CORE_ADDR addr, int len, int width,
 	  limit = bufptr + nfetch * width;
 	  while (bufptr < limit)
 	    {
-	      unsigned long c;
+	      bool found_nonzero = false;
+
+	      for (int i = 0; !found_nonzero && i < width; ++i)
+		if (bufptr[i] != 0)
+		  found_nonzero = true;
 
-	      c = extract_unsigned_integer (bufptr, width, byte_order);
 	      addr += width;
 	      bufptr += width;
-	      if (c == 0)
+	      if (!found_nonzero)
 		{
 		  /* We don't care about any error which happened after
 		     the NUL terminator.  */
@@ -2733,7 +2735,7 @@ val_print_string (struct type *elttype, const char *encoding,
   fetchlimit = (len == -1 ? options->print_max : std::min ((unsigned) len,
 							   options->print_max));
 
-  err = target_read_string (addr, len, width, fetchlimit, byte_order,
+  err = target_read_string (addr, len, width, fetchlimit,
 			    &buffer, &bytes_read);
 
   addr += bytes_read;
diff --git a/gdb/valprint.h b/gdb/valprint.h
index 2f4a5022b3e..b12495b10c1 100644
--- a/gdb/valprint.h
+++ b/gdb/valprint.h
@@ -167,7 +167,6 @@ extern void print_function_pointer_address (const struct value_print_options *op
 
 extern int target_read_string (CORE_ADDR addr, int len, int width,
 			       unsigned int fetchlimit,
-			       enum bfd_endian byte_order,
 			       gdb::unique_xmalloc_ptr<gdb_byte> *buffer,
 			       int *bytes_read);
 
-- 
2.34.1

[PATCH 5/9] Move target_read_string to target/target.c

[Tom Tromey]

This moves the two overloads of target_read_string to a new file,
target/target.c, and updates both gdb and gdbserver to build this.
---
 gdb/Makefile.in       |   2 +-
 gdb/target.c          |  19 -----
 gdb/target.h          |   8 --
 gdb/target/target.c   | 190 ++++++++++++++++++++++++++++++++++++++++++
 gdb/target/target.h   |  31 +++++++
 gdb/valprint.c        | 172 --------------------------------------
 gdb/valprint.h        |   5 --
 gdbserver/Makefile.in |   2 +
 8 files changed, 224 insertions(+), 205 deletions(-)
 create mode 100644 gdb/target/target.c

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 872fbe1f6df..c8e140b5544 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -487,7 +487,7 @@ SELFTESTS_SRCS = \
 
 SELFTESTS_OBS = $(patsubst %.c,%.o,$(SELFTESTS_SRCS))
 
-SUBDIR_TARGET_SRCS = target/waitstatus.c
+SUBDIR_TARGET_SRCS = target/target.c target/waitstatus.c
 SUBDIR_TARGET_OBS = $(patsubst %.c,%.o,$(SUBDIR_TARGET_SRCS))
 
 
diff --git a/gdb/target.c b/gdb/target.c
index 5a416d5ee3a..7d291fd4d0d 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1394,25 +1394,6 @@ target_xfer_status_to_string (enum target_xfer_status status)
 };
 
 
-/* See target.h.  */
-
-gdb::unique_xmalloc_ptr<char>
-target_read_string (CORE_ADDR memaddr, int len, 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 = target_read_string (memaddr, -1, 1, len, &buffer, bytes_read);
-  if (errcode != 0)
-    return {};
-
-  return gdb::unique_xmalloc_ptr<char> ((char *) buffer.release ());
-}
-
 const target_section_table *
 target_get_section_table (struct target_ops *target)
 {
diff --git a/gdb/target.h b/gdb/target.h
index c9791e850ac..093178af0bc 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1567,14 +1567,6 @@ extern void target_dumpcore (const char *filename);
 
 extern bool target_can_run_breakpoint_commands ();
 
-/* 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.  */
 
 extern int target_read_raw_memory (CORE_ADDR memaddr, gdb_byte *myaddr,
diff --git a/gdb/target/target.c b/gdb/target/target.c
new file mode 100644
index 00000000000..0b165bc05fe
--- /dev/null
+++ b/gdb/target/target.c
@@ -0,0 +1,190 @@
+/* String reading
+
+   Copyright (C) 2022 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "gdbsupport/common-defs.h"
+#include "target/target.h"
+
+/* Read LEN bytes of target memory at address MEMADDR, placing the
+   results in GDB's memory at MYADDR.  Returns a count of the bytes
+   actually read, and optionally a target_xfer_status value in the
+   location pointed to by ERRPTR if ERRPTR is non-null.  */
+
+static int
+partial_memory_read (CORE_ADDR memaddr, gdb_byte *myaddr,
+		     int len, int *errptr)
+{
+  int nread;			/* Number of bytes actually read.  */
+  int errcode;			/* Error from last read.  */
+
+  /* First try a complete read.  */
+  errcode = target_read_memory (memaddr, myaddr, len);
+  if (errcode == 0)
+    {
+      /* Got it all.  */
+      nread = len;
+    }
+  else
+    {
+      /* Loop, reading one byte at a time until we get as much as we can.  */
+      for (errcode = 0, nread = 0; len > 0 && errcode == 0; nread++, len--)
+	{
+	  errcode = target_read_memory (memaddr++, myaddr++, 1);
+	}
+      /* If an error, the last read was unsuccessful, so adjust count.  */
+      if (errcode != 0)
+	{
+	  nread--;
+	}
+    }
+  if (errptr != NULL)
+    {
+      *errptr = errcode;
+    }
+  return (nread);
+}
+
+/* See target/target.h.  */
+
+int
+target_read_string (CORE_ADDR addr, int len, int width,
+		    unsigned int fetchlimit,
+		    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.  */
+
+  /* Loop until we either have all the characters, or we encounter
+     some error, such as bumping into the end of the address space.  */
+
+  buffer->reset (nullptr);
+
+  if (len > 0)
+    {
+      /* We want fetchlimit chars, so we might as well read them all in
+	 one operation.  */
+      unsigned int fetchlen = std::min ((unsigned) len, fetchlimit);
+
+      buffer->reset ((gdb_byte *) xmalloc (fetchlen * width));
+      bufptr = buffer->get ();
+
+      nfetch = partial_memory_read (addr, bufptr, fetchlen * width, &errcode)
+	/ width;
+      addr += nfetch * width;
+      bufptr += nfetch * width;
+    }
+  else if (len == -1)
+    {
+      unsigned long bufsize = 0;
+      unsigned int chunksize;	/* Size of each fetch, in chars.  */
+      int found_nul;		/* Non-zero if we found the nul char.  */
+      gdb_byte *limit;		/* First location past end of fetch buffer.  */
+
+      found_nul = 0;
+      /* We are looking for a NUL terminator to end the fetching, so we
+	 might as well read in blocks that are large enough to be efficient,
+	 but not so large as to be slow if fetchlimit happens to be large.
+	 So we choose the minimum of 8 and fetchlimit.  We used to use 200
+	 instead of 8 but 200 is way too big for remote debugging over a
+	  serial line.  */
+      chunksize = std::min (8u, fetchlimit);
+
+      do
+	{
+	  nfetch = std::min ((unsigned long) chunksize, fetchlimit - bufsize);
+
+	  if (*buffer == NULL)
+	    buffer->reset ((gdb_byte *) xmalloc (nfetch * width));
+	  else
+	    buffer->reset ((gdb_byte *) xrealloc (buffer->release (),
+						  (nfetch + bufsize) * width));
+
+	  bufptr = buffer->get () + bufsize * width;
+	  bufsize += nfetch;
+
+	  /* Read as much as we can.  */
+	  nfetch = partial_memory_read (addr, bufptr, nfetch * width, &errcode)
+		    / width;
+
+	  /* Scan this chunk for the null character that terminates the string
+	     to print.  If found, we don't need to fetch any more.  Note
+	     that bufptr is explicitly left pointing at the next character
+	     after the null character, or at the next character after the end
+	     of the buffer.  */
+
+	  limit = bufptr + nfetch * width;
+	  while (bufptr < limit)
+	    {
+	      bool found_nonzero = false;
+
+	      for (int i = 0; !found_nonzero && i < width; ++i)
+		if (bufptr[i] != 0)
+		  found_nonzero = true;
+
+	      addr += width;
+	      bufptr += width;
+	      if (!found_nonzero)
+		{
+		  /* We don't care about any error which happened after
+		     the NUL terminator.  */
+		  errcode = 0;
+		  found_nul = 1;
+		  break;
+		}
+	    }
+	}
+      while (errcode == 0	/* no error */
+	     && 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->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->get ();
+
+  return errcode;
+}
+
+/* See target/target.h.  */
+
+gdb::unique_xmalloc_ptr<char>
+target_read_string (CORE_ADDR memaddr, int len, 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 = target_read_string (memaddr, -1, 1, len, &buffer, bytes_read);
+  if (errcode != 0)
+    return {};
+
+  return gdb::unique_xmalloc_ptr<char> ((char *) buffer.release ());
+}
diff --git a/gdb/target/target.h b/gdb/target/target.h
index 818c8c6232f..a5b0dd3ed1a 100644
--- a/gdb/target/target.h
+++ b/gdb/target/target.h
@@ -48,6 +48,37 @@ extern int target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr,
 
 extern int target_read_uint32 (CORE_ADDR memaddr, uint32_t *result);
 
+/* 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);
+
+/* 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
+   (including eventual NULs in the middle or end of the string).
+
+   If LEN is -1, stops at the first null character (not necessarily
+   the first null byte) up to a maximum of FETCHLIMIT characters.  Set
+   FETCHLIMIT to UINT_MAX to read as many characters as possible from
+   the string.
+
+   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.  */
+
+extern int target_read_string (CORE_ADDR addr, int len, int width,
+			       unsigned int fetchlimit,
+			       gdb::unique_xmalloc_ptr<gdb_byte> *buffer,
+			       int *bytes_read);
+
 /* Write LEN bytes from MYADDR to target memory at address MEMADDR.
    Return zero for success, nonzero if any error occurs.  This
    function must be provided by the client.  Implementations of this
diff --git a/gdb/valprint.c b/gdb/valprint.c
index 8f9954778c5..47114676934 100644
--- a/gdb/valprint.c
+++ b/gdb/valprint.c
@@ -85,9 +85,6 @@ struct cmd_list_element *showprintrawlist;
 
 /* Prototypes for local functions */
 
-static int partial_memory_read (CORE_ADDR memaddr, gdb_byte *myaddr,
-				int len, int *errptr);
-
 static void set_input_radix_1 (int, unsigned);
 
 static void set_output_radix_1 (int, unsigned);
@@ -1989,175 +1986,6 @@ value_print_array_elements (struct value *val, struct ui_file *stream,
     }
 }
 
-/* Read LEN bytes of target memory at address MEMADDR, placing the
-   results in GDB's memory at MYADDR.  Returns a count of the bytes
-   actually read, and optionally a target_xfer_status value in the
-   location pointed to by ERRPTR if ERRPTR is non-null.  */
-
-/* FIXME: cagney/1999-10-14: Only used by val_print_string.  Can this
-   function be eliminated.  */
-
-static int
-partial_memory_read (CORE_ADDR memaddr, gdb_byte *myaddr,
-		     int len, int *errptr)
-{
-  int nread;			/* Number of bytes actually read.  */
-  int errcode;			/* Error from last read.  */
-
-  /* First try a complete read.  */
-  errcode = target_read_memory (memaddr, myaddr, len);
-  if (errcode == 0)
-    {
-      /* Got it all.  */
-      nread = len;
-    }
-  else
-    {
-      /* Loop, reading one byte at a time until we get as much as we can.  */
-      for (errcode = 0, nread = 0; len > 0 && errcode == 0; nread++, len--)
-	{
-	  errcode = target_read_memory (memaddr++, myaddr++, 1);
-	}
-      /* If an error, the last read was unsuccessful, so adjust count.  */
-      if (errcode != 0)
-	{
-	  nread--;
-	}
-    }
-  if (errptr != NULL)
-    {
-      *errptr = errcode;
-    }
-  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, 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
-   (including eventual NULs in the middle or end of the string).
-
-   If LEN is -1, stops at the first null character (not necessarily
-   the first null byte) up to a maximum of FETCHLIMIT characters.  Set
-   FETCHLIMIT to UINT_MAX to read as many characters as possible from
-   the string.
-
-   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.  */
-
-int
-target_read_string (CORE_ADDR addr, int len, int width,
-		    unsigned int fetchlimit,
-		    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.  */
-
-  /* Loop until we either have all the characters, or we encounter
-     some error, such as bumping into the end of the address space.  */
-
-  buffer->reset (nullptr);
-
-  if (len > 0)
-    {
-      /* We want fetchlimit chars, so we might as well read them all in
-	 one operation.  */
-      unsigned int fetchlen = std::min ((unsigned) len, fetchlimit);
-
-      buffer->reset ((gdb_byte *) xmalloc (fetchlen * width));
-      bufptr = buffer->get ();
-
-      nfetch = partial_memory_read (addr, bufptr, fetchlen * width, &errcode)
-	/ width;
-      addr += nfetch * width;
-      bufptr += nfetch * width;
-    }
-  else if (len == -1)
-    {
-      unsigned long bufsize = 0;
-      unsigned int chunksize;	/* Size of each fetch, in chars.  */
-      int found_nul;		/* Non-zero if we found the nul char.  */
-      gdb_byte *limit;		/* First location past end of fetch buffer.  */
-
-      found_nul = 0;
-      /* We are looking for a NUL terminator to end the fetching, so we
-	 might as well read in blocks that are large enough to be efficient,
-	 but not so large as to be slow if fetchlimit happens to be large.
-	 So we choose the minimum of 8 and fetchlimit.  We used to use 200
-	 instead of 8 but 200 is way too big for remote debugging over a
-	  serial line.  */
-      chunksize = std::min (8u, fetchlimit);
-
-      do
-	{
-	  nfetch = std::min ((unsigned long) chunksize, fetchlimit - bufsize);
-
-	  if (*buffer == NULL)
-	    buffer->reset ((gdb_byte *) xmalloc (nfetch * width));
-	  else
-	    buffer->reset ((gdb_byte *) xrealloc (buffer->release (),
-						  (nfetch + bufsize) * width));
-
-	  bufptr = buffer->get () + bufsize * width;
-	  bufsize += nfetch;
-
-	  /* Read as much as we can.  */
-	  nfetch = partial_memory_read (addr, bufptr, nfetch * width, &errcode)
-		    / width;
-
-	  /* Scan this chunk for the null character that terminates the string
-	     to print.  If found, we don't need to fetch any more.  Note
-	     that bufptr is explicitly left pointing at the next character
-	     after the null character, or at the next character after the end
-	     of the buffer.  */
-
-	  limit = bufptr + nfetch * width;
-	  while (bufptr < limit)
-	    {
-	      bool found_nonzero = false;
-
-	      for (int i = 0; !found_nonzero && i < width; ++i)
-		if (bufptr[i] != 0)
-		  found_nonzero = true;
-
-	      addr += width;
-	      bufptr += width;
-	      if (!found_nonzero)
-		{
-		  /* We don't care about any error which happened after
-		     the NUL terminator.  */
-		  errcode = 0;
-		  found_nul = 1;
-		  break;
-		}
-	    }
-	}
-      while (errcode == 0	/* no error */
-	     && 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->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->get ();
-
-  return errcode;
-}
-
 /* Return true if print_wchar can display W without resorting to a
    numeric escape, false otherwise.  */
 
diff --git a/gdb/valprint.h b/gdb/valprint.h
index b12495b10c1..ff536fbe4f0 100644
--- a/gdb/valprint.h
+++ b/gdb/valprint.h
@@ -165,11 +165,6 @@ extern void print_function_pointer_address (const struct value_print_options *op
 					    CORE_ADDR address,
 					    struct ui_file *stream);
 
-extern int target_read_string (CORE_ADDR addr, int len, int width,
-			       unsigned int fetchlimit,
-			       gdb::unique_xmalloc_ptr<gdb_byte> *buffer,
-			       int *bytes_read);
-
 /* Helper function to check the validity of some bits of a value.
 
    If TYPE represents some aggregate type (e.g., a structure), return 1.
diff --git a/gdbserver/Makefile.in b/gdbserver/Makefile.in
index 47648b8d962..b9a687c9231 100644
--- a/gdbserver/Makefile.in
+++ b/gdbserver/Makefile.in
@@ -223,6 +223,7 @@ SFILES = \
 	$(srcdir)/../gdb/nat/ppc-linux.c \
 	$(srcdir)/../gdb/nat/riscv-linux-tdesc.c \
 	$(srcdir)/../gdb/nat/fork-inferior.c \
+	$(srcdir)/../gdb/target/target.c \
 	$(srcdir)/../gdb/target/waitstatus.c
 
 DEPFILES = @GDBSERVER_DEPFILES@
@@ -250,6 +251,7 @@ OBS = \
 	tracepoint.o \
 	utils.o \
 	version.o \
+	target/target.o \
 	target/waitstatus.o \
 	$(DEPFILES) \
 	$(LIBOBJS) \
-- 
2.34.1

[PATCH 6/9] Share handle_ms_vc_exception with gdbserver

[Tom Tromey]

Currently, gdb's native Windows target implements the exception-based
approach for setting thread names, but gdbserver does not.  This patch
moves handle_ms_vc_exception to the shared nat/windows-nat.c code, as
preparation for adding this support to gdbserver.
---
 gdb/nat/windows-nat.c  | 40 ++++++++++++++++++++++++++++++++++++++++
 gdb/nat/windows-nat.h  | 20 +++++++++-----------
 gdb/windows-nat.c      | 40 ----------------------------------------
 gdbserver/win32-low.cc |  9 ---------
 4 files changed, 49 insertions(+), 60 deletions(-)

diff --git a/gdb/nat/windows-nat.c b/gdb/nat/windows-nat.c
index e6d73485c24..9defbd71d14 100644
--- a/gdb/nat/windows-nat.c
+++ b/gdb/nat/windows-nat.c
@@ -19,6 +19,7 @@
 #include "gdbsupport/common-defs.h"
 #include "nat/windows-nat.h"
 #include "gdbsupport/common-debug.h"
+#include "target/target.h"
 
 #ifdef __CYGWIN__
 #define __USEWIDE
@@ -162,6 +163,45 @@ get_image_name (HANDLE h, void *address, int unicode)
   return buf;
 }
 
+/* See nat/windows-nat.h.  */
+
+bool
+windows_process_info::handle_ms_vc_exception (const EXCEPTION_RECORD *rec)
+{
+  if (rec->NumberParameters >= 3
+      && (rec->ExceptionInformation[0] & 0xffffffff) == 0x1000)
+    {
+      DWORD named_thread_id;
+      windows_thread_info *named_thread;
+      CORE_ADDR thread_name_target;
+
+      thread_name_target = rec->ExceptionInformation[1];
+      named_thread_id = (DWORD) (0xffffffff & rec->ExceptionInformation[2]);
+
+      if (named_thread_id == (DWORD) -1)
+	named_thread_id = current_event.dwThreadId;
+
+      named_thread = thread_rec (ptid_t (current_event.dwProcessId,
+					 named_thread_id, 0),
+				 DONT_INVALIDATE_CONTEXT);
+      if (named_thread != NULL)
+	{
+	  int thread_name_len;
+	  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';
+	      named_thread->name = std::move (thread_name);
+	    }
+	}
+
+      return true;
+    }
+
+  return false;
+}
+
 /* The exception thrown by a program to tell the debugger the name of
    a thread.  The exception record contains an ID of a thread and a
    name to give it.  This exception has no documented name, but MSDN
diff --git a/gdb/nat/windows-nat.h b/gdb/nat/windows-nat.h
index 7f76ba0e58f..522e267176d 100644
--- a/gdb/nat/windows-nat.h
+++ b/gdb/nat/windows-nat.h
@@ -92,7 +92,7 @@ struct windows_thread_info
      adjustments if the registers are read multiple times.  */
   bool pc_adjusted = false;
 
-  /* The name of the thread, allocated by xmalloc.  */
+  /* The name of the thread.  */
   gdb::unique_xmalloc_ptr<char> name;
 };
 
@@ -213,16 +213,6 @@ struct windows_process_info
 
   void handle_unload_dll ();
 
-  /* Handle MS_VC_EXCEPTION when processing a stop.  MS_VC_EXCEPTION is
-     somewhat undocumented but is used to tell the debugger the name of
-     a thread.
-
-     Return true if the exception was handled; return false otherwise.
-
-     This function must be supplied by the embedding application.  */
-
-  bool handle_ms_vc_exception (const EXCEPTION_RECORD *rec);
-
   /* When EXCEPTION_ACCESS_VIOLATION is processed, we give the embedding
      application a chance to change it to be considered "unhandled".
      This function must be supplied by the embedding application.  If it
@@ -256,6 +246,14 @@ struct windows_process_info
 
 private:
 
+  /* Handle MS_VC_EXCEPTION when processing a stop.  MS_VC_EXCEPTION is
+     somewhat undocumented but is used to tell the debugger the name of
+     a thread.
+
+     Return true if the exception was handled; return false otherwise.  */
+
+  bool handle_ms_vc_exception (const EXCEPTION_RECORD *rec);
+
   /* Iterate over all DLLs currently mapped by our inferior, looking for
      a DLL which is loaded at LOAD_ADDR.  If found, add the DLL to our
      list of solibs; otherwise do nothing.  LOAD_ADDR NULL means add all
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 646ddda7328..581eb47e7ba 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -1054,46 +1054,6 @@ display_selectors (const char * args, int from_tty)
 
 /* See nat/windows-nat.h.  */
 
-bool
-windows_nat::windows_process_info::handle_ms_vc_exception
-     (const EXCEPTION_RECORD *rec)
-{
-  if (rec->NumberParameters >= 3
-      && (rec->ExceptionInformation[0] & 0xffffffff) == 0x1000)
-    {
-      DWORD named_thread_id;
-      windows_thread_info *named_thread;
-      CORE_ADDR thread_name_target;
-
-      thread_name_target = rec->ExceptionInformation[1];
-      named_thread_id = (DWORD) (0xffffffff & rec->ExceptionInformation[2]);
-
-      if (named_thread_id == (DWORD) -1)
-	named_thread_id = current_event.dwThreadId;
-
-      named_thread = thread_rec (ptid_t (current_event.dwProcessId,
-					 named_thread_id, 0),
-				 DONT_INVALIDATE_CONTEXT);
-      if (named_thread != NULL)
-	{
-	  int thread_name_len;
-	  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';
-	      named_thread->name = std::move (thread_name);
-	    }
-	}
-
-      return true;
-    }
-
-  return false;
-}
-
-/* See nat/windows-nat.h.  */
-
 bool
 windows_nat::windows_process_info::handle_access_violation
      (const EXCEPTION_RECORD *rec)
diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
index 8437c69e1cc..8fde3e95b2d 100644
--- a/gdbserver/win32-low.cc
+++ b/gdbserver/win32-low.cc
@@ -1041,15 +1041,6 @@ fake_breakpoint_event (void)
 
 /* See nat/windows-nat.h.  */
 
-bool
-windows_nat::windows_process_info::handle_ms_vc_exception
-     (const EXCEPTION_RECORD *rec)
-{
-  return false;
-}
-
-/* See nat/windows-nat.h.  */
-
 bool
 windows_nat::windows_process_info::handle_access_violation
      (const EXCEPTION_RECORD *rec)
-- 
2.34.1

[PATCH 7/9] Implement thread_name for gdbserver

[Tom Tromey]

This changes gdbserver to implement thread_name method.
---
 gdbserver/win32-low.cc | 9 +++++++++
 gdbserver/win32-low.h  | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
index 8fde3e95b2d..16c13f32d77 100644
--- a/gdbserver/win32-low.cc
+++ b/gdbserver/win32-low.cc
@@ -1502,6 +1502,15 @@ win32_process_target::write_pc (struct regcache *regcache, CORE_ADDR pc)
   return (*the_low_target.set_pc) (regcache, pc);
 }
 
+const char *
+win32_process_target::thread_name (ptid_t thread)
+{
+  windows_thread_info *th
+    = windows_process.thread_rec (current_thread_ptid (),
+				  DONT_INVALIDATE_CONTEXT);
+  return th->name.get ();
+}
+
 /* The win32 target ops object.  */
 
 static win32_process_target the_win32_target;
diff --git a/gdbserver/win32-low.h b/gdbserver/win32-low.h
index 8856a84baa3..a1d74573cc9 100644
--- a/gdbserver/win32-low.h
+++ b/gdbserver/win32-low.h
@@ -158,6 +158,8 @@ class win32_process_target : public process_stratum_target
   bool stopped_by_sw_breakpoint () override;
 
   bool supports_stopped_by_sw_breakpoint () override;
+
+  const char *thread_name (ptid_t thread) override;
 };
 
 /* Retrieve the context for this thread, if not already retrieved.  */
-- 
2.34.1

[PATCH 8/9] Set the worker thread name on Windows

[Tom Tromey]

This patch is a bit different from the rest of the series, in that it
is a change to gdb's behavior on the host.  It changes gdb's thread
pool to try to set the thread name on Windows, if SetThreadDescription
is available.

This is part of PR win32/29050.

This patch isn't likely to be useful to many people in the short term,
because the Windows port of the libstdc++ thread code is not upstream.
(AdaCore uses it, and sent it upstream, but it did not land, I don't
know why.)  However, if that patch does ever go in, or presumably if
you build using some other C++ runtime library, then this will be
useful.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29050
---
 gdbsupport/thread-pool.cc | 72 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 64 insertions(+), 8 deletions(-)

diff --git a/gdbsupport/thread-pool.cc b/gdbsupport/thread-pool.cc
index efd8b799713..9d98529231c 100644
--- a/gdbsupport/thread-pool.cc
+++ b/gdbsupport/thread-pool.cc
@@ -46,14 +46,15 @@
    difference.  */
 
 ATTRIBUTE_UNUSED static void
-set_thread_name (int (*set_name) (pthread_t, const char *, void *),
-				  const char *name)
+do_set_thread_name (int (*set_name) (pthread_t, const char *, void *),
+		    const char *name)
 {
   set_name (pthread_self (), "%s", const_cast<char *> (name));
 }
 
 ATTRIBUTE_UNUSED static void
-set_thread_name (int (*set_name) (pthread_t, const char *), const char *name)
+do_set_thread_name (int (*set_name) (pthread_t, const char *),
+		    const char *name)
 {
   set_name (pthread_self (), name);
 }
@@ -61,12 +62,69 @@ set_thread_name (int (*set_name) (pthread_t, const char *), const char *name)
 /* The macOS man page says that pthread_setname_np returns "void", but
    the headers actually declare it returning "int".  */
 ATTRIBUTE_UNUSED static void
-set_thread_name (int (*set_name) (const char *), const char *name)
+do_set_thread_name (int (*set_name) (const char *), const char *name)
 {
   set_name (name);
 }
 
-#endif	/* USE_PTHREAD_SETNAME_NP */
+static void
+set_thread_name (const char *name)
+{
+  do_set_thread_name (pthread_setname_np, name);
+}
+
+#elif defined (USE_WIN32API)
+
+#include <windows.h>
+
+typedef HRESULT WINAPI (SetThreadDescription_ftype) (HANDLE, PCWSTR);
+static SetThreadDescription_ftype *dyn_SetThreadDescription;
+static bool initialized;
+
+static void
+init_windows ()
+{
+  initialized = true;
+
+  HMODULE hm = LoadLibrary (TEXT ("kernel32.dll"));
+  if (hm)
+    dyn_SetThreadDescription
+      = (SetThreadDescription_ftype *) GetProcAddress (hm,
+						       "SetThreadDescription");
+
+  /* On some versions of Windows, this function is only available in
+     KernelBase.dll, not kernel32.dll.  */
+  if (dyn_SetThreadDescription == nullptr)
+    {
+      hm = LoadLibrary (TEXT ("KernelBase.dll"));
+      if (hm)
+	dyn_SetThreadDescription
+	  = (SetThreadDescription_ftype *) GetProcAddress (hm,
+							   "SetThreadDescription");
+    }
+}
+
+static void
+do_set_thread_name (const wchar_t *name)
+{
+  if (!initialized)
+    init_windows ();
+
+  if (dyn_SetThreadDescription != nullptr)
+    dyn_SetThreadDescription (GetCurrentThread (), name);
+}
+
+#define set_thread_name(NAME) do_set_thread_name (L ## NAME)
+
+#else /* USE_WIN32API */
+
+static void
+set_thread_name (const char *name)
+{
+}
+
+#endif
+
 #endif /* CXX_STD_THREAD */
 
 namespace gdb
@@ -159,11 +217,9 @@ thread_pool::do_post_task (std::packaged_task<void ()> &&func)
 void
 thread_pool::thread_function ()
 {
-#ifdef USE_PTHREAD_SETNAME_NP
   /* This must be done here, because on macOS one can only set the
      name of the current thread.  */
-  set_thread_name (pthread_setname_np, "gdb worker");
-#endif
+  set_thread_name ("gdb worker");
 
   /* Ensure that SIGSEGV is delivered to an alternate signal
      stack.  */
-- 
2.34.1

[PATCH 9/9] Use GetThreadDescription on Windows

[Tom Tromey]

Windows 10 introduced SetThreadDescription and GetThreadDescription, a
simpler way to set a thread's name.  This changes gdb and gdbserver to
use this convention when it is available.

This is part of PR win32/29050.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29050
---
 gdb/nat/windows-nat.c  | 38 ++++++++++++++++++++++++++++++++++++++
 gdb/nat/windows-nat.h  |  5 +++++
 gdb/windows-nat.c      |  6 ++++--
 gdbserver/win32-low.cc |  2 +-
 4 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/gdb/nat/windows-nat.c b/gdb/nat/windows-nat.c
index 9defbd71d14..677525d1629 100644
--- a/gdb/nat/windows-nat.c
+++ b/gdb/nat/windows-nat.c
@@ -58,6 +58,10 @@ Wow64GetThreadSelectorEntry_ftype *Wow64GetThreadSelectorEntry;
 #endif
 GenerateConsoleCtrlEvent_ftype *GenerateConsoleCtrlEvent;
 
+#define GetThreadDescription dyn_GetThreadDescription
+typedef HRESULT WINAPI (GetThreadDescription_ftype) (HANDLE, PWSTR *);
+static GetThreadDescription_ftype *GetThreadDescription;
+
 /* Note that 'debug_events' must be locally defined in the relevant
    functions.  */
 #define DEBUG_EVENTS(fmt, ...) \
@@ -106,6 +110,30 @@ windows_thread_info::resume ()
   suspended = 0;
 }
 
+const char *
+windows_thread_info::thread_name ()
+{
+  if (GetThreadDescription != nullptr)
+    {
+      PWSTR value;
+      HRESULT result = GetThreadDescription (h, &value);
+      if (SUCCEEDED (result))
+	{
+	  size_t needed;
+	  if (wcstombs_s (&needed, nullptr, 0, value, _TRUNCATE) == 0)
+	    {
+	      name.reset ((char *) xmalloc (needed));
+	      if (wcstombs_s (&needed, name.get (), needed,
+			      value, needed - 1) != 0)
+		name.reset ();
+	    }
+	  LocalFree (value);
+	}
+    }
+
+  return name.get ();
+}
+
 /* Return the name of the DLL referenced by H at ADDRESS.  UNICODE
    determines what sort of string is read from the inferior.  Returns
    the name of the DLL, or NULL on error.  If a name is returned, it
@@ -658,6 +686,7 @@ initialize_loadable ()
       GPA (hm, Wow64GetThreadSelectorEntry);
 #endif
       GPA (hm, GenerateConsoleCtrlEvent);
+      GPA (hm, GetThreadDescription);
     }
 
   /* Set variables to dummy versions of these processes if the function
@@ -714,6 +743,15 @@ initialize_loadable ()
 	OpenProcessToken = bad;
     }
 
+  /* On some versions of Windows, this function is only available in
+     KernelBase.dll, not kernel32.dll.  */
+  if (GetThreadDescription == nullptr)
+    {
+      hm = LoadLibrary (TEXT ("KernelBase.dll"));
+      if (hm)
+	GPA (hm, GetThreadDescription);
+    }
+
 #undef GPA
 
   return result;
diff --git a/gdb/nat/windows-nat.h b/gdb/nat/windows-nat.h
index 522e267176d..29fd0a3a69b 100644
--- a/gdb/nat/windows-nat.h
+++ b/gdb/nat/windows-nat.h
@@ -51,6 +51,11 @@ struct windows_thread_info
   /* Resume the thread if it has been suspended.  */
   void resume ();
 
+  /* Return the thread's name, or nullptr if not known.  The name is
+     stored in this thread and is guaranteed to live until at least
+     the next call.  */
+  const char *thread_name ();
+
   /* The Win32 thread identifier.  */
   DWORD tid;
 
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 581eb47e7ba..1068558cd21 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -2997,8 +2997,10 @@ windows_nat_target::get_ada_task_ptid (long lwp, ULONGEST thread)
 const char *
 windows_nat_target::thread_name (struct thread_info *thr)
 {
-  return windows_process.thread_rec (thr->ptid,
-				     DONT_INVALIDATE_CONTEXT)->name.get ();
+  windows_thread_info *th
+    = windows_process.thread_rec (thr->ptid,
+				  DONT_INVALIDATE_CONTEXT);
+  return th->thread_name ();
 }
 
 
diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
index 16c13f32d77..afeed1a9881 100644
--- a/gdbserver/win32-low.cc
+++ b/gdbserver/win32-low.cc
@@ -1508,7 +1508,7 @@ win32_process_target::thread_name (ptid_t thread)
   windows_thread_info *th
     = windows_process.thread_rec (current_thread_ptid (),
 				  DONT_INVALIDATE_CONTEXT);
-  return th->name.get ();
+  return th->thread_name ();
 }
 
 /* The win32 target ops object.  */
-- 
2.34.1

Re: [PATCH 9/9] Use GetThreadDescription on Windows

[Eli Zaretskii]

> Date: Wed, 13 Apr 2022 13:17:56 -0600
> From: Tom Tromey via Gdb-patches <gdb-patches@sourceware.org>
> Cc: Tom Tromey <tromey@adacore.com>
> 
> Windows 10 introduced SetThreadDescription and GetThreadDescription, a
> simpler way to set a thread's name.  This changes gdb and gdbserver to
> use this convention when it is available.

Thanks for doing this.

> +const char *
> +windows_thread_info::thread_name ()
> +{
> +  if (GetThreadDescription != nullptr)
> +    {
> +      PWSTR value;
> +      HRESULT result = GetThreadDescription (h, &value);
> +      if (SUCCEEDED (result))
> +	{
> +	  size_t needed;
> +	  if (wcstombs_s (&needed, nullptr, 0, value, _TRUNCATE) == 0)
> +	    {
> +	      name.reset ((char *) xmalloc (needed));
> +	      if (wcstombs_s (&needed, name.get (), needed,
> +			      value, needed - 1) != 0)
> +		name.reset ();
> +	    }
> +	  LocalFree (value);
> +	}
> +    }
> +
> +  return name.get ();
> +}

AFAIK, wcstombs_s is only available since Vista, so I think we need a
configure-time test for that, and use wcstombs as fallback.

Re: [PATCH 9/9] Use GetThreadDescription on Windows

[Eli Zaretskii]

> Date: Wed, 13 Apr 2022 22:33:07 +0300
> From: Eli Zaretskii via Gdb-patches <gdb-patches@sourceware.org>
> Cc: gdb-patches@sourceware.org
> 
> AFAIK, wcstombs_s is only available since Vista, so I think we need a
> configure-time test for that, and use wcstombs as fallback.

On second thought, it should be a run-time test, so that the binary
doesn't depend too much on the system where it was built.  Or maybe
just use wcstombs always?

Thanks.

Re: [PATCH 9/9] Use GetThreadDescription on Windows

[Tom Tromey]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

>> AFAIK, wcstombs_s is only available since Vista, so I think we need a
>> configure-time test for that, and use wcstombs as fallback.

Eli> On second thought, it should be a run-time test, so that the binary
Eli> doesn't depend too much on the system where it was built.  Or maybe
Eli> just use wcstombs always?

Yeah, I'm going to do that.

Is pre-Vista support needed?  I don't really know what versions of
Windows gdb is intended to support, but the web tells me that Vista
itself was de-supported by MS 5 years ago.  So surely pre-Vista stuff
must be extra obsolete.  Wikipedia says Vista is in use by 0.18% of PCs.

Tom

Re: [PATCH 9/9] Use GetThreadDescription on Windows

[Eli Zaretskii]

> From: Tom Tromey <tromey@adacore.com>
> Cc: tromey@adacore.com,  gdb-patches@sourceware.org
> Date: Thu, 14 Apr 2022 07:17:13 -0600
> 
> >>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> AFAIK, wcstombs_s is only available since Vista, so I think we need a
> >> configure-time test for that, and use wcstombs as fallback.
> 
> Eli> On second thought, it should be a run-time test, so that the binary
> Eli> doesn't depend too much on the system where it was built.  Or maybe
> Eli> just use wcstombs always?
> 
> Yeah, I'm going to do that.
> 
> Is pre-Vista support needed?  I don't really know what versions of
> Windows gdb is intended to support, but the web tells me that Vista
> itself was de-supported by MS 5 years ago.  So surely pre-Vista stuff
> must be extra obsolete.  Wikipedia says Vista is in use by 0.18% of PCs.

MS obsoleted Vista, but whether and when we do is up to us.  I think
we still want to support XP, but maybe I'm wrong, or maybe people want
to change that.

Re: [PATCH 0/9] Windows thread names

[Pedro Alves]

On 2022-04-13 20:17, Tom Tromey via Gdb-patches wrote:
> I noticed that newer versions of Windows have a nicer API to setting
> and fetching a thread's name.  Whereas in earlier versions this was
> done by throwing and catching a special SEH exception, that the
> debugger could intercept, now there is a simple API call.
> 
> I also noticed that, while gdb implements the older method already,
> gdbserver did not.
> 
> This patch refactors some code a little bit so that the old method can
> be shared between gdb and gdbserver.  Then it implements the new
> method as well, and also changes gdb to set the names of its own
> worker threads using the new approach.
> 
> I ran this through the AdaCore test suite without problems, and also
> tested it by hand on a simple program that sets the current thread's
> name.
> 

I read this and it looks fine to me.

Thanks,
Pedro Alves

Re: [PATCH 9/9] Use GetThreadDescription on Windows

[Tom Tromey]

Eli> On second thought, it should be a run-time test, so that the binary
Eli> doesn't depend too much on the system where it was built.  Or maybe
Eli> just use wcstombs always?

Tom> Yeah, I'm going to do that.

I changed this to use wcstombs and re-tested it.
I'm checking in the updated series now.

Tom

Re: [PATCH 9/9] Use GetThreadDescription on Windows

[Tom Tromey]

Eli> MS obsoleted Vista, but whether and when we do is up to us.  I think
Eli> we still want to support XP, but maybe I'm wrong, or maybe people want
Eli> to change that.

I don't really know what the tradeoffs are.  Wikipedia says that
globally XP is barely used but that in certain countries that isn't
true -- e.g., it mentions Armenia having ~50% usage.

The wcstombs change just gave me pause since it's marked as deprecated
in the Windows docs.  But maybe that doesn't really matter.

Tom

Re: [PATCH 1/9] Fix possible Cygwin build problem

[Jon Turney]

On 13/04/2022 20:17, Tom Tromey via Gdb-patches wrote:
> I noticed that nat/windows-nat.c checks __USEWIDE, but nothing sets it
> there -- I forgot to copy over the definition when making this file.
> This patch tries to fix the problem.  I don't have a Cygwin setup, so
> I don't know whether this is sufficient, but it's probably necessary.

Yes, this is necessary, but not sufficient.  I've posted a patch with 
the other needed pieces today.

Otoh, I'm not sure if the !__USEWIDE case has much value anymore, unless 
someone is still building gdb for Windows 9x...

Re: [PATCH 1/9] Fix possible Cygwin build problem

[Eli Zaretskii]

> Date: Sat, 21 May 2022 12:18:45 +0100
> From: Jon Turney <jon.turney@dronecode.org.uk>
> 
> Otoh, I'm not sure if the !__USEWIDE case has much value anymore, unless 
> someone is still building gdb for Windows 9x...

__USEWIDE in native Windows programming assumes the use of wchar_t for
character and character strings, something that GDB sources aren't
equipped for doing, at least not easily.  AFAIK, Cygwin alleviates
that by using UTF-8 encoded strings, but native Windows programming
cannot (yet) do that safely enough on all supported Windows versions.

As long as MinGW builds of GDB support only the so-called "ANSI"
encoding (i.e. the current system codepage), there's no reason to use
__USEWIDE in the MinGW build of GDB, and every reason not to use it.

Re: [PATCH 1/9] Fix possible Cygwin build problem

[Tom Tromey]

>>>>> "Eli" == Eli Zaretskii via Gdb-patches <gdb-patches@sourceware.org> writes:

Eli> __USEWIDE in native Windows programming assumes the use of wchar_t for
Eli> character and character strings, something that GDB sources aren't
Eli> equipped for doing, at least not easily.  AFAIK, Cygwin alleviates
Eli> that by using UTF-8 encoded strings, but native Windows programming
Eli> cannot (yet) do that safely enough on all supported Windows versions.

It looks like windows-nat.c defines __USEWIDE only for Cygwin, and then
is careful to use the *W forms of various Windows APIs.  So maybe it
works ok in practice?

Eli> As long as MinGW builds of GDB support only the so-called "ANSI"
Eli> encoding (i.e. the current system codepage), there's no reason to use
Eli> __USEWIDE in the MinGW build of GDB, and every reason not to use it.

Yeah, I think __USEWIDE is not used for mingw.  However I wonder if we
could remove it for Cygwin as well... that would clean up some code.
Not a huge amount, but every Cygwin divergence is a bit of a pain.

Tom

Re: [PATCH 1/9] Fix possible Cygwin build problem

[Eli Zaretskii]

> From: Tom Tromey <tromey@adacore.com>
> Cc: Jon Turney <jon.turney@dronecode.org.uk>,  Eli Zaretskii <eliz@gnu.org>,
>   tromey@adacore.com
> Date: Thu, 26 May 2022 11:00:45 -0600
> 
> >>>>> "Eli" == Eli Zaretskii via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Eli> __USEWIDE in native Windows programming assumes the use of wchar_t for
> Eli> character and character strings, something that GDB sources aren't
> Eli> equipped for doing, at least not easily.  AFAIK, Cygwin alleviates
> Eli> that by using UTF-8 encoded strings, but native Windows programming
> Eli> cannot (yet) do that safely enough on all supported Windows versions.
> 
> It looks like windows-nat.c defines __USEWIDE only for Cygwin, and then
> is careful to use the *W forms of various Windows APIs.  So maybe it
> works ok in practice?

I doubt it, in general, because GDB uses 'char *' for strings, and
those won't work in *W APIs without an explicit conversion to wchar_t.

However, I see only 2 calls to *W APIs in windows-nat.c, and they are
both under the __CYGWIN__ condition.  So I think we are good.

> Eli> As long as MinGW builds of GDB support only the so-called "ANSI"
> Eli> encoding (i.e. the current system codepage), there's no reason to use
> Eli> __USEWIDE in the MinGW build of GDB, and every reason not to use it.
> 
> Yeah, I think __USEWIDE is not used for mingw.  However I wonder if we
> could remove it for Cygwin as well... that would clean up some code.
> Not a huge amount, but every Cygwin divergence is a bit of a pain.

This will need a Cygwin expert, which I'm not.

Re: [PATCH 1/9] Fix possible Cygwin build problem

[Jon Turney]

On 26/05/2022 20:04, Eli Zaretskii wrote:
>> From: Tom Tromey <tromey@adacore.com>
>> Cc: Jon Turney <jon.turney@dronecode.org.uk>,  Eli Zaretskii <eliz@gnu.org>,
>>    tromey@adacore.com
>> Date: Thu, 26 May 2022 11:00:45 -0600
>>
>>>>>>> "Eli" == Eli Zaretskii via Gdb-patches <gdb-patches@sourceware.org> writes:
>>
>> Eli> __USEWIDE in native Windows programming assumes the use of wchar_t for
>> Eli> character and character strings, something that GDB sources aren't
>> Eli> equipped for doing, at least not easily.  AFAIK, Cygwin alleviates
>> Eli> that by using UTF-8 encoded strings, but native Windows programming
>> Eli> cannot (yet) do that safely enough on all supported Windows versions.
>>
>> It looks like windows-nat.c defines __USEWIDE only for Cygwin, and then
>> is careful to use the *W forms of various Windows APIs.  So maybe it
>> works ok in practice?
> 
> I doubt it, in general, because GDB uses 'char *' for strings, and
> those won't work in *W APIs without an explicit conversion to wchar_t.
> 
> However, I see only 2 calls to *W APIs in windows-nat.c, and they are
> both under the __CYGWIN__ condition.  So I think we are good.
> 
>> Eli> As long as MinGW builds of GDB support only the so-called "ANSI"
>> Eli> encoding (i.e. the current system codepage), there's no reason to use
>> Eli> __USEWIDE in the MinGW build of GDB, and every reason not to use it.
>>
>> Yeah, I think __USEWIDE is not used for mingw.  However I wonder if we
>> could remove it for Cygwin as well... that would clean up some code.
>> Not a huge amount, but every Cygwin divergence is a bit of a pain.
> 
> This will need a Cygwin expert, which I'm not.

I think that would be a step backwards.

The reasons for this code seem to have been discussed, back when it was 
added [1].

I think we must use the *W() API variants on cygwin, so that the paths 
can be losslessly converted char * encoded in cygwin's conception of the 
locale (which is not the Windows codepage, and may not be representable 
as one).

The only way I can think of this to be unified is to always use the *W() 
API variants, keep that data which is internal to windows-nat in WCHAR, 
and convert to a displayable name for the rest of gdb using wcstombs() 
(on cygwin) or WideCharToMultiByte() on Windows.

[1] https://sourceware.org/pipermail/gdb-patches/2010-February/072173.html

Re: [PATCH 1/9] Fix possible Cygwin build problem

[Eli Zaretskii]

> Date: Thu, 2 Jun 2022 15:12:48 +0100
> From: Jon Turney <jon.turney@dronecode.org.uk>
> 
> >> Yeah, I think __USEWIDE is not used for mingw.  However I wonder if we
> >> could remove it for Cygwin as well... that would clean up some code.
> >> Not a huge amount, but every Cygwin divergence is a bit of a pain.
> > 
> > This will need a Cygwin expert, which I'm not.
> 
> I think that would be a step backwards.
> 
> The reasons for this code seem to have been discussed, back when it was 
> added [1].
> 
> I think we must use the *W() API variants on cygwin, so that the paths 
> can be losslessly converted char * encoded in cygwin's conception of the 
> locale (which is not the Windows codepage, and may not be representable 
> as one).

I don't think Tom was suggesting to remove the *W APIs, I think he was
asking about removing the __USEWIDE thing.  Is it really necessary for
Cygwin to use the *W APIs?