[PATCH] Really fix Windows gdbserver segment registers

[PATCH] Really fix Windows gdbserver segment registers

[Tom Tromey]

My earlier attempt to mask the segment registers in gdbserver for
Windows introduced some bugs.  That patch is here:

https://sourceware.org/pipermail/gdb-patches/2023-December/205306.html

The problem turned out to be that these fields in the Windows
'CONTEXT' type are just 16 bits, so while masking the values on read
is fine, writing the truncated values back then causes zeros to be
written to some subsequent field.

This patch cleans this up by arranging never to write too much data to
a field.

I also noticed that two register numbers here were never updated for
the 64-bit port.  This patch fixes this as well, and renames the "FCS"
register to follow gdb.

Finally, this patch applies the same treatment to windows-nat.c.
---
 gdb/windows-nat.c           | 44 +++++++++++++------
 gdbserver/win32-i386-low.cc | 85 +++++++++++++++++++++++++++++++++----
 2 files changed, 107 insertions(+), 22 deletions(-)

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 28f142c1c6a..6fdd1f3a151 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -662,23 +662,19 @@ windows_fetch_one_register (struct regcache *regcache,
   gdb_assert (gdbarch_pc_regnum (gdbarch) >= 0);
   gdb_assert (!gdbarch_write_pc_p (gdbarch));
 
-  if (r == I387_FISEG_REGNUM (tdep))
+  /* GDB treats some registers as 32-bit, where they are in fact only
+     16 bits long.  These cases must be handled specially to avoid
+     reading extraneous bits from the context.  */
+  if (r == I387_FISEG_REGNUM (tdep) || windows_process.segment_register_p (r))
     {
-      long l = *((long *) context_offset) & 0xffff;
-      regcache->raw_supply (r, (char *) &l);
+      gdb_byte bytes[4] = {};
+      memcpy (bytes, context_offset, 2);
+      regcache->raw_supply (r, bytes);
     }
   else if (r == I387_FOP_REGNUM (tdep))
     {
       long l = (*((long *) context_offset) >> 16) & ((1 << 11) - 1);
-      regcache->raw_supply (r, (char *) &l);
-    }
-  else if (windows_process.segment_register_p (r))
-    {
-      /* GDB treats segment registers as 32bit registers, but they are
-	 in fact only 16 bits long.  Make sure we do not read extra
-	 bits from our source buffer.  */
-      long l = *((long *) context_offset) & 0xffff;
-      regcache->raw_supply (r, (char *) &l);
+      regcache->raw_supply (r, &l);
     }
   else
     {
@@ -799,7 +795,29 @@ windows_store_one_register (const struct regcache *regcache,
     context_ptr = (char *) &th->wow64_context;
 #endif
 
-  regcache->raw_collect (r, context_ptr + windows_process.mappings[r]);
+  struct gdbarch *gdbarch = regcache->arch ();
+  i386_gdbarch_tdep *tdep = gdbarch_tdep<i386_gdbarch_tdep> (gdbarch);
+
+  /* GDB treats some registers as 32-bit, where they are in fact only
+     16 bits long.  These cases must be handled specially to avoid
+     overwriting other registers in the context.  */
+  if (r == I387_FISEG_REGNUM (tdep) || windows_process.segment_register_p (r))
+    {
+      gdb_byte bytes[4];
+      regcache->raw_collect (r, bytes);
+      memcpy (context_ptr + windows_process.mappings[r], bytes, 2);
+    }
+  else if (r == I387_FOP_REGNUM (tdep))
+    {
+      gdb_byte bytes[4];
+      regcache->raw_collect (r, bytes);
+      /* The value of FOP occupies the top two bytes in the context,
+	 so write the two low-order bytes from the cache into the
+	 appropriate spot.  */
+      memcpy (context_ptr + windows_process.mappings[r] + 2, bytes, 2);
+    }
+  else
+    regcache->raw_collect (r, context_ptr + windows_process.mappings[r]);
 }
 
 /* Store a new register value into the context of the thread tied to
diff --git a/gdbserver/win32-i386-low.cc b/gdbserver/win32-i386-low.cc
index 783e61f6f33..44490300b69 100644
--- a/gdbserver/win32-i386-low.cc
+++ b/gdbserver/win32-i386-low.cc
@@ -32,8 +32,17 @@ using namespace windows_nat;
 #define CONTEXT_EXTENDED_REGISTERS 0
 #endif
 
-#define FCS_REGNUM 27
-#define FOP_REGNUM 31
+#define I386_FISEG_REGNUM 27
+#define I386_FOP_REGNUM 31
+
+#define I386_CS_REGNUM 10
+#define I386_GS_REGNUM 15
+
+#define AMD64_FISEG_REGNUM 35
+#define AMD64_FOP_REGNUM 39
+
+#define AMD64_CS_REGNUM 18
+#define AMD64_GS_REGNUM 23
 
 #define FLAG_TRACE_BIT 0x100
 
@@ -459,6 +468,42 @@ static const int amd64_mappings[] =
 
 #endif /* __x86_64__ */
 
+/* Return true if R is the FISEG register.  */
+static bool
+is_fiseg_register (int r)
+{
+#ifdef __x86_64__
+  if (!windows_process.wow64_process)
+    return r == AMD64_FISEG_REGNUM;
+  else
+#endif
+    return r == I386_FISEG_REGNUM;
+}
+
+/* Return true if R is the FOP register.  */
+static bool
+is_fop_register (int r)
+{
+#ifdef __x86_64__
+  if (!windows_process.wow64_process)
+    return r == AMD64_FOP_REGNUM;
+  else
+#endif
+    return r == I386_FOP_REGNUM;
+}
+
+/* Return true if R is a segment register.  */
+static bool
+is_segment_register (int r)
+{
+#ifdef __x86_64__
+  if (!windows_process.wow64_process)
+    return r >= AMD64_CS_REGNUM && r <= AMD64_GS_REGNUM;
+  else
+#endif
+    return r >= I386_CS_REGNUM && r <= I386_GS_REGNUM;
+}
+
 /* Fetch register from gdbserver regcache data.  */
 static void
 i386_fetch_inferior_register (struct regcache *regcache,
@@ -480,15 +525,18 @@ i386_fetch_inferior_register (struct regcache *regcache,
 #endif
     context_offset = (char *) &th->context + mappings[r];
 
-  long l;
-  if (r == FCS_REGNUM)
+  /* GDB treats some registers as 32-bit, where they are in fact only
+     16 bits long.  These cases must be handled specially to avoid
+     reading extraneous bits from the context.  */
+  if (is_fiseg_register (r) || is_segment_register (r))
     {
-      l = *((long *) context_offset) & 0xffff;
-      supply_register (regcache, r, (char *) &l);
+      gdb_byte bytes[4] = {};
+      memcpy (bytes, context_offset, 2);
+      supply_register (regcache, r, bytes);
     }
-  else if (r == FOP_REGNUM)
+  else if (is_fop_register (r))
     {
-      l = (*((long *) context_offset) >> 16) & ((1 << 11) - 1);
+      long l = (*((long *) context_offset) >> 16) & ((1 << 11) - 1);
       supply_register (regcache, r, (char *) &l);
     }
   else
@@ -516,7 +564,26 @@ i386_store_inferior_register (struct regcache *regcache,
 #endif
     context_offset = (char *) &th->context + mappings[r];
 
-  collect_register (regcache, r, context_offset);
+  /* GDB treats some registers as 32-bit, where they are in fact only
+     16 bits long.  These cases must be handled specially to avoid
+     overwriting other registers in the context.  */
+  if (is_fiseg_register (r) || is_segment_register (r))
+    {
+      gdb_byte bytes[4];
+      collect_register (regcache, r, bytes);
+      memcpy (context_offset, bytes, 2);
+    }
+  else if (is_fop_register (r))
+    {
+      gdb_byte bytes[4];
+      collect_register (regcache, r, bytes);
+      /* The value of FOP occupies the top two bytes in the context,
+	 so write the two low-order bytes from the cache into the
+	 appropriate spot.  */
+      memcpy (context_offset + 2, bytes, 2);
+    }
+  else
+    collect_register (regcache, r, context_offset);
 }
 
 static const unsigned char i386_win32_breakpoint = 0xcc;
-- 
2.43.0

Re: [PATCH] Really fix Windows gdbserver segment registers

[John Baldwin]

On 1/24/24 7:00 AM, Tom Tromey wrote:
> My earlier attempt to mask the segment registers in gdbserver for
> Windows introduced some bugs.  That patch is here:
> 
> https://sourceware.org/pipermail/gdb-patches/2023-December/205306.html
> 
> The problem turned out to be that these fields in the Windows
> 'CONTEXT' type are just 16 bits, so while masking the values on read
> is fine, writing the truncated values back then causes zeros to be
> written to some subsequent field.
> 
> This patch cleans this up by arranging never to write too much data to
> a field.
> 
> I also noticed that two register numbers here were never updated for
> the 64-bit port.  This patch fixes this as well, and renames the "FCS"
> register to follow gdb.
> 
> Finally, this patch applies the same treatment to windows-nat.c.

LGTM as far as I understand it.

Reviewed-by: John Baldwin <jhb@FreeBSD.org>

-- 
John Baldwin

Re: [PATCH] Really fix Windows gdbserver segment registers

[Tom Tromey]

>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:

John> LGTM as far as I understand it.

John> Reviewed-by: John Baldwin <jhb@FreeBSD.org>

Thank you.  I'm going to check this in.  Also, FWIW, it's been in
nightly testing internally at AdaCore for a while now.

Tom