i387-tdep.c:i387_supply_xsave: Assert the xsave section buffer, is not NULL.

[Pedro Alves <palves@redhat.com>]

On 01/26/2012 07:07 PM, Pedro Alves wrote:

> 
> I have a doubt in the xsave-in-corefile support bits.  There's code in place to
> handle a NULL regs (as in no xsave contents to work with), so I'm handling it
> as presently:
> 
> +
> +     Note however, the case when REGS is NULL is a different case.
> +     That case means we do not have access to the x87 states, so we
> +     should mark the registers as unavailable (by supplying NULL).  */
> +
> 
> but I can't figure out how would we ever get a NULL REGS there.  Is there a
> convoluted path I missed?  amd64-linux-tdep.c unconditionally installs
> amd64_linux_regset_sections as gdbarch_core_regset_sections
> callback, and this includes the .reg-xstate section.
> However, corelow.c:get_core_register_section bails early if
> a section is not found in the core, never reaching regset->supply_regset
> with a NULL `contents'.


On 02/27/2012 06:48 PM, Pedro Alves wrote:
> I'll post a separate RFC patch proposing to remove the NULL REGS handling

Here's what I'd like to check in, given the analysis quoted above.

2012-03-05  Pedro Alves  <palves@redhat.com>

	* i387-tdep.c (i387_supply_xsave): Assert the xsave section buffer
	is not NULL, and remove resulting dead code.

---

 gdb/i387-tdep.c |   33 +++++++++++----------------------
 1 files changed, 11 insertions(+), 22 deletions(-)

diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c
index 3fc344d..26e4e50 100644
--- a/gdb/i387-tdep.c
+++ b/gdb/i387-tdep.c
@@ -790,6 +790,7 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
       all = x87 | sse | avxh
     } regclass;

+  gdb_assert (regs != NULL);
   gdb_assert (tdep->st0_regnum >= I386_ST0_REGNUM);
   gdb_assert (tdep->num_xmm_regs > 0);

@@ -807,7 +808,7 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
   else
     regclass = none;

-  if (regs != NULL && regclass != none)
+  if (regclass != none)
     {
       /* Get `xstat_bv'.  */
       const gdb_byte *xstate_bv_p = XSAVE_XSTATE_BV_ADDR (regs);
@@ -826,11 +827,7 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
      time in a program.  This means that from the user-space programs'
      perspective, it's the same as if the registers have always been
      zero from the start of the program.  Therefore, the debugger
-     should provide the same illusion to the user.
-
-     Note however, the case when REGS is NULL is a different case.
-     That case means we do not have access to the x87 states, so we
-     should mark the registers as unavailable (by supplying NULL).  */
+     should provide the same illusion to the user.  */

   switch (regclass)
     {
@@ -839,7 +836,7 @@ i387_supply_xsave (struct regcache *regcache, int regnum,

     case avxh:
       if ((clear_bv & I386_XSTATE_AVX))
-	regcache_raw_supply (regcache, regnum, regs == NULL ? NULL : zero);
+	regcache_raw_supply (regcache, regnum, zero);
       else
 	regcache_raw_supply (regcache, regnum,
 			     XSAVE_AVXH_ADDR (tdep, regs, regnum));
@@ -847,7 +844,7 @@ i387_supply_xsave (struct regcache *regcache, int regnum,

     case sse:
       if ((clear_bv & I386_XSTATE_SSE))
-	regcache_raw_supply (regcache, regnum, regs == NULL ? NULL : zero);
+	regcache_raw_supply (regcache, regnum, zero);
       else
 	regcache_raw_supply (regcache, regnum,
 			     FXSAVE_ADDR (tdep, regs, regnum));
@@ -855,7 +852,7 @@ i387_supply_xsave (struct regcache *regcache, int regnum,

     case x87:
       if ((clear_bv & I386_XSTATE_X87))
-	regcache_raw_supply (regcache, regnum, regs == NULL ? NULL : zero);
+	regcache_raw_supply (regcache, regnum, zero);
       else
 	regcache_raw_supply (regcache, regnum,
 			     FXSAVE_ADDR (tdep, regs, regnum));
@@ -870,7 +867,7 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
 	      for (i = I387_YMM0H_REGNUM (tdep);
 		   i < I387_YMMENDH_REGNUM (tdep);
 		   i++)
-		regcache_raw_supply (regcache, i, regs == NULL ? NULL : zero);
+		regcache_raw_supply (regcache, i, zero);
 	    }
 	  else
 	    {
@@ -890,7 +887,7 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
 	      for (i = I387_XMM0_REGNUM (tdep);
 		   i < I387_MXCSR_REGNUM (tdep);
 		   i++)
-		regcache_raw_supply (regcache, i, regs == NULL ? NULL : zero);
+		regcache_raw_supply (regcache, i, zero);
 	    }
 	  else
 	    {
@@ -909,7 +906,7 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
 	      for (i = I387_ST0_REGNUM (tdep);
 		   i < I387_FCTRL_REGNUM (tdep);
 		   i++)
-		regcache_raw_supply (regcache, i, regs == NULL ? NULL : zero);
+		regcache_raw_supply (regcache, i, zero);
 	    }
 	  else
 	    {
@@ -926,12 +923,6 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
   for (i = I387_FCTRL_REGNUM (tdep); i < I387_XMM0_REGNUM (tdep); i++)
     if (regnum == -1 || regnum == i)
       {
-	if (regs == NULL)
-	  {
-	    regcache_raw_supply (regcache, i, NULL);
-	    continue;
-	  }
-
 	/* Most of the FPU control registers occupy only 16 bits in
 	   the xsave extended state.  Give those a special treatment.  */
 	if (i != I387_FIOFF_REGNUM (tdep)
@@ -982,10 +973,8 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
       }

   if (regnum == I387_MXCSR_REGNUM (tdep) || regnum == -1)
-    {
-      p = regs == NULL ? NULL : FXSAVE_MXCSR_ADDR (regs);
-      regcache_raw_supply (regcache, I387_MXCSR_REGNUM (tdep), p);
-    }
+    regcache_raw_supply (regcache, I387_MXCSR_REGNUM (tdep),
+			 FXSAVE_MXCSR_ADDR (regs));
 }

 /* Similar to i387_collect_fxsave, but use XSAVE extended state.  */