GDBserver: Fix "Cond. jump or move depends on uninit value" in x87 code

Running gdbserver under Valgrind I get:

  ==26925== Conditional jump or move depends on uninitialised value(s)
  ==26925==    at 0x473E7F: i387_cache_to_xsave(regcache*, void*) (i387-fp.c:579)
  ==26925==    by 0x46E3ED: x86_fill_xstateregset(regcache*, void*) (linux-x86-low.c:418)
  ==26925==    by 0x45E747: regsets_store_inferior_registers(regsets_info*, regcache*) (linux-low.c:5456)
  ==26925==    by 0x45EEF8: linux_store_registers(regcache*, int) (linux-low.c:5731)
  ==26925==    by 0x426441: regcache_invalidate_thread(thread_info*) (regcache.c:89)
  ==26925==    by 0x45CCAF: linux_resume_one_lwp_throw(lwp_info*, int, int, siginfo_t*) (linux-low.c:4447)
  ==26925==    by 0x45CE2A: linux_resume_one_lwp(lwp_info*, int, int, siginfo_t*) (linux-low.c:4519)
  ==26925==    by 0x45E17C: proceed_one_lwp(thread_info*, lwp_info*) (linux-low.c:5216)
  ==26925==    by 0x45DC81: linux_resume_one_thread(thread_info*, bool) (linux-low.c:5031)
  ==26925==    by 0x45DD34: linux_resume(thread_resume*, unsigned long)::{lambda(thread_info*)#2}::operator()(thread_info*) const (linux-low.c:5095)
  ==26925==    by 0x462907: void for_each_thread<linux_resume(thread_resume*, unsigned long)::{lambda(thread_info*)#2}>(linux_resume(thread_resume*, unsigned long)::{lambda(thread_info*)#2}) (gdbthread.h:150)
  ==26925==    by 0x45DE62: linux_resume(thread_resume*, unsigned long) (linux-low.c:5093)
  ==26925==
  ==26925== Conditional jump or move depends on uninitialised value(s)
  ==26925==    at 0x473EBD: i387_cache_to_xsave(regcache*, void*) (i387-fp.c:586)
  ==26925==    by 0x46E3ED: x86_fill_xstateregset(regcache*, void*) (linux-x86-low.c:418)
  ==26925==    by 0x45E747: regsets_store_inferior_registers(regsets_info*, regcache*) (linux-low.c:5456)
  ==26925==    by 0x45EEF8: linux_store_registers(regcache*, int) (linux-low.c:5731)
  ==26925==    by 0x426441: regcache_invalidate_thread(thread_info*) (regcache.c:89)
  ==26925==    by 0x45CCAF: linux_resume_one_lwp_throw(lwp_info*, int, int, siginfo_t*) (linux-low.c:4447)
  ==26925==    by 0x45CE2A: linux_resume_one_lwp(lwp_info*, int, int, siginfo_t*) (linux-low.c:4519)
  ==26925==    by 0x45E17C: proceed_one_lwp(thread_info*, lwp_info*) (linux-low.c:5216)
  ==26925==    by 0x45DC81: linux_resume_one_thread(thread_info*, bool) (linux-low.c:5031)
  ==26925==    by 0x45DD34: linux_resume(thread_resume*, unsigned long)::{lambda(thread_info*)#2}::operator()(thread_info*) const (linux-low.c:5095)
  ==26925==    by 0x462907: void for_each_thread<linux_resume(thread_resume*, unsigned long)::{lambda(thread_info*)#2}>(linux_resume(thread_resume*, unsigned long)::{lambda(thread_info*)#2}) (gdbthread.h:150)
  ==26925==    by 0x45DE62: linux_resume(thread_resume*, unsigned long) (linux-low.c:5093)

The problem is a type/width mismatch in code like this, in
gdbserver/i387-fp.c:

  /* Some registers are 16-bit.  */
  collect_register_by_name (regcache, "fctrl", &val);
  fp->fctrl = val;

In the above code:

 #1 - 'val' is a 64-bit unsigned long.

 #2 - "fctrl" is 32-bit in the register cache, thus half of 'val' is
      left uninitialized by collect_register_by_name, which works with
      an untyped raw buffer output (i.e., void*).

 #3 - fp->fctrl is an unsigned short (16-bit).  For some such
      registers we're masking off the uninitialized bits with 0xffff,
      but not in all cases.

We end up in such a fragile situation because
collect_registers_by_name works with an untyped output buffer pointer,
making it easy to pass a pointer to a variable of the wrong size.

Fix this by using regcache_raw_get_unsigned instead (actually a new
regcache_raw_get_unsigned_by_name wrapper), which always returns a
zero-extended ULONGEST register value.  It ends up simplifying the
i387-tdep.c code a bit, even.

gdb/gdbserver/ChangeLog:
2018-07-11  Pedro Alves  <palves@redhat.com>

	* i387-fp.c (i387_cache_to_fsave, cache_to_fxsave)
	(i387_cache_to_xsave): Use regcache_raw_get_unsigned_by_name
	instead of collect_register_by_name.
	* regcache.c (regcache_raw_get_unsigned_by_name): New.
	* regcache.h (regcache_raw_get_unsigned_by_name): New.

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index ac644ceb7fbea6afdc795185894a9f79db874eec..7c482b0ddfc2d468000a44c321c4923445c70568 100644 (file)
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,11 @@
+2018-07-11  Pedro Alves  <palves@redhat.com>
+
+       * i387-fp.c (i387_cache_to_fsave, cache_to_fxsave)
+       (i387_cache_to_xsave): Use regcache_raw_get_unsigned_by_name
+       instead of collect_register_by_name.
+       * regcache.c (regcache_raw_get_unsigned_by_name): New.
+       * regcache.h (regcache_raw_get_unsigned_by_name): New.
+
 2018-07-04  Vyacheslav Barinov  <v.barinov@samsung.com>
            Pedro Alves  <palves@redhat.com>
 

diff --git a/gdb/gdbserver/i387-fp.c b/gdb/gdbserver/i387-fp.c
index bee1af9d8c6a8683ae689f8cc481ed5ba146e74b..b06079ab42343e2cdd816ddbd4be59b9f4314171 100644 (file)
--- a/gdb/gdbserver/i387-fp.c
+++ b/gdb/gdbserver/i387-fp.c
@@ -149,38 +149,25 @@ i387_cache_to_fsave (struct regcache *regcache, void *buf)
   struct i387_fsave *fp = (struct i387_fsave *) buf;
   int i;
   int st0_regnum = find_regno (regcache->tdesc, "st0");
-  unsigned long val, val2;
+  unsigned long val2;
 
   for (i = 0; i < 8; i++)
     collect_register (regcache, i + st0_regnum,
                      ((char *) &fp->st_space[0]) + i * 10);
 
-  collect_register_by_name (regcache, "fioff", &fp->fioff);
-  collect_register_by_name (regcache, "fooff", &fp->fooff);
-  
+  fp->fioff = regcache_raw_get_unsigned_by_name (regcache, "fioff");
+  fp->fooff = regcache_raw_get_unsigned_by_name (regcache, "fooff");
+
   /* This one's 11 bits... */
-  collect_register_by_name (regcache, "fop", &val2);
+  val2 = regcache_raw_get_unsigned_by_name (regcache, "fop");
   fp->fop = (val2 & 0x7FF) | (fp->fop & 0xF800);
 
   /* Some registers are 16-bit.  */
-  collect_register_by_name (regcache, "fctrl", &val);
-  fp->fctrl = val;
-
-  collect_register_by_name (regcache, "fstat", &val);
-  val &= 0xFFFF;
-  fp->fstat = val;
-
-  collect_register_by_name (regcache, "ftag", &val);
-  val &= 0xFFFF;
-  fp->ftag = val;
-
-  collect_register_by_name (regcache, "fiseg", &val);
-  val &= 0xFFFF;
-  fp->fiseg = val;
-
-  collect_register_by_name (regcache, "foseg", &val);
-  val &= 0xFFFF;
-  fp->foseg = val;
+  fp->fctrl = regcache_raw_get_unsigned_by_name (regcache, "fctrl");
+  fp->fstat = regcache_raw_get_unsigned_by_name (regcache, "fstat");
+  fp->ftag = regcache_raw_get_unsigned_by_name (regcache, "ftag");
+  fp->fiseg = regcache_raw_get_unsigned_by_name (regcache, "fiseg");
+  fp->foseg = regcache_raw_get_unsigned_by_name (regcache, "foseg");
 }
 
 void
@@ -237,24 +224,20 @@ i387_cache_to_fxsave (struct regcache *regcache, void *buf)
     collect_register (regcache, i + xmm0_regnum,
                      ((char *) &fp->xmm_space[0]) + i * 16);
 
-  collect_register_by_name (regcache, "fioff", &fp->fioff);
-  collect_register_by_name (regcache, "fooff", &fp->fooff);
-  collect_register_by_name (regcache, "mxcsr", &fp->mxcsr);
+  fp->fioff = regcache_raw_get_unsigned_by_name (regcache, "fioff");
+  fp->fooff = regcache_raw_get_unsigned_by_name (regcache, "fooff");
+  fp->mxcsr = regcache_raw_get_unsigned_by_name (regcache, "mxcsr");
 
   /* This one's 11 bits... */
-  collect_register_by_name (regcache, "fop", &val2);
+  val2 = regcache_raw_get_unsigned_by_name (regcache, "fop");
   fp->fop = (val2 & 0x7FF) | (fp->fop & 0xF800);
 
   /* Some registers are 16-bit.  */
-  collect_register_by_name (regcache, "fctrl", &val);
-  fp->fctrl = val;
-
-  collect_register_by_name (regcache, "fstat", &val);
-  fp->fstat = val;
+  fp->fctrl = regcache_raw_get_unsigned_by_name (regcache, "fctrl");
+  fp->fstat = regcache_raw_get_unsigned_by_name (regcache, "fstat");
 
   /* Convert to the simplifed tag form stored in fxsave data.  */
-  collect_register_by_name (regcache, "ftag", &val);
-  val &= 0xFFFF;
+  val = regcache_raw_get_unsigned_by_name (regcache, "ftag");
   val2 = 0;
   for (i = 7; i >= 0; i--)
     {
@@ -265,11 +248,8 @@ i387_cache_to_fxsave (struct regcache *regcache, void *buf)
     }
   fp->ftag = val2;
 
-  collect_register_by_name (regcache, "fiseg", &val);
-  fp->fiseg = val;
-
-  collect_register_by_name (regcache, "foseg", &val);
-  fp->foseg = val;
+  fp->fiseg = regcache_raw_get_unsigned_by_name (regcache, "fiseg");
+  fp->foseg = regcache_raw_get_unsigned_by_name (regcache, "foseg");
 }
 
 void
@@ -566,7 +546,7 @@ i387_cache_to_xsave (struct regcache *regcache, void *buf)
        }
 
       /* This one's 11 bits... */
-      collect_register_by_name (regcache, "fop", &val2);
+      val2 = regcache_raw_get_unsigned_by_name (regcache, "fop");
       val2 = (val2 & 0x7FF) | (fp->fop & 0xF800);
       if (fp->fop != val2)
        {
@@ -575,14 +555,14 @@ i387_cache_to_xsave (struct regcache *regcache, void *buf)
        }
 
       /* Some registers are 16-bit.  */
-      collect_register_by_name (regcache, "fctrl", &val);
+      val = regcache_raw_get_unsigned_by_name (regcache, "fctrl");
       if (fp->fctrl != val)
        {
          xstate_bv |= X86_XSTATE_X87;
          fp->fctrl = val;
        }
 
-      collect_register_by_name (regcache, "fstat", &val);
+      val = regcache_raw_get_unsigned_by_name (regcache, "fstat");
       if (fp->fstat != val)
        {
          xstate_bv |= X86_XSTATE_X87;
@@ -590,8 +570,7 @@ i387_cache_to_xsave (struct regcache *regcache, void *buf)
        }
 
       /* Convert to the simplifed tag form stored in fxsave data.  */
-      collect_register_by_name (regcache, "ftag", &val);
-      val &= 0xFFFF;
+      val = regcache_raw_get_unsigned_by_name (regcache, "ftag");
       val2 = 0;
       for (i = 7; i >= 0; i--)
        {
@@ -606,14 +585,14 @@ i387_cache_to_xsave (struct regcache *regcache, void *buf)
          fp->ftag = val2;
        }
 
-      collect_register_by_name (regcache, "fiseg", &val);
+      val = regcache_raw_get_unsigned_by_name (regcache, "fiseg");
       if (fp->fiseg != val)
        {
          xstate_bv |= X86_XSTATE_X87;
          fp->fiseg = val;
        }
 
-      collect_register_by_name (regcache, "foseg", &val);
+      val = regcache_raw_get_unsigned_by_name (regcache, "foseg");
       if (fp->foseg != val)
        {
          xstate_bv |= X86_XSTATE_X87;

diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c
index 735ce7bccfc7a058f8604620928f3894d4655887..0ffec534c3674913b428b7b7d133e26824a22a41 100644 (file)
--- a/gdb/gdbserver/regcache.c
+++ b/gdb/gdbserver/regcache.c
@@ -448,6 +448,16 @@ regcache_raw_read_unsigned (struct regcache *regcache, int regnum,
 
 #ifndef IN_PROCESS_AGENT
 
+/* See regcache.h.  */
+
+ULONGEST
+regcache_raw_get_unsigned_by_name (struct regcache *regcache,
+                                  const char *name)
+{
+  return regcache_raw_get_unsigned (regcache,
+                                   find_regno (regcache->tdesc, name));
+}
+
 void
 collect_register_as_string (struct regcache *regcache, int n, char *buf)
 {

diff --git a/gdb/gdbserver/regcache.h b/gdb/gdbserver/regcache.h
index b4c4c20ebd368f56f14af758301ff7d55fd16dae..a3d8922bb109e17cc6c53b53627729b508c4e8ad 100644 (file)
--- a/gdb/gdbserver/regcache.h
+++ b/gdb/gdbserver/regcache.h
@@ -131,4 +131,11 @@ void collect_register_as_string (struct regcache *regcache, int n, char *buf);
 void collect_register_by_name (struct regcache *regcache,
                               const char *name, void *buf);
 
+/* Read a raw register as an unsigned integer.  Convenience wrapper
+   around regcache_raw_get_unsigned that takes a register name instead
+   of a register number.  */
+
+ULONGEST regcache_raw_get_unsigned_by_name (struct regcache *regcache,
+                                           const char *name);
+
 #endif /* REGCACHE_H */