[PATCH] Fix i386 FPU register conversion code

[PATCH] Fix i386 FPU register conversion code

[Mark Kettenis]

Fixing the debug register number mapping scheme uncovered a bug in
i386-tdep.c:i386_register_convert_to_virtual().  Fixed with the
attached patch.

Mark


Index: ChangeLog
from  Mark Kettenis  <kettenis@gnu.org>

	* i386-tdep.c: Include "gdb_assert.h"
	(i386_register_convert_to_virtual): Fix such that it can handle
	conversion to any floating-point type.  Assert that we are dealing
	with a floating-point first.
	(i386_register_convert_to_raw): Assert that TYPE is a
	floating-point type with length 12.

Index: i386-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-tdep.c,v
retrieving revision 1.34
diff -u -p -r1.34 i386-tdep.c
--- i386-tdep.c 2001/07/15 20:10:02 1.34
+++ i386-tdep.c 2001/07/28 16:45:41
@@ -33,6 +33,8 @@
 #include "arch-utils.h"
 #include "regcache.h"
 
+#include "gdb_assert.h"
+
 /* i386_register_byte[i] is the offset into the register file of the
    start of register number i.  We initialize this from
    i386_register_raw_size.  */
@@ -972,27 +974,39 @@ i386_register_convertible (int regnum)
 }
 
 /* Convert data from raw format for register REGNUM in buffer FROM to
-   virtual format with type TYPE in buffer TO.  In principle both
-   formats are identical except that the virtual format has two extra
-   bytes appended that aren't used.  We set these to zero.  */
+   virtual format with type TYPE in buffer TO.  */
 
 void
 i386_register_convert_to_virtual (int regnum, struct type *type,
 				  char *from, char *to)
 {
-  /* Copy straight over, but take care of the padding.  */
-  memcpy (to, from, FPU_REG_RAW_SIZE);
-  memset (to + FPU_REG_RAW_SIZE, 0, TYPE_LENGTH (type) - FPU_REG_RAW_SIZE);
+  char buf[12];
+  DOUBLEST d;
+
+  /* We only support floating-point values.  */
+  gdb_assert (TYPE_CODE (type) == TYPE_CODE_FLT);
+
+  /* First add the necessary padding.  */
+  memcpy (buf, from, FPU_REG_RAW_SIZE);
+  memset (buf + FPU_REG_RAW_SIZE, 0, sizeof buf - FPU_REG_RAW_SIZE);
+
+  /* Convert to TYPE.  This should be a no-op, if TYPE is equivalent
+     to the extended floating-point format used by the FPU.  */
+  d = extract_floating (buf, sizeof buf);
+  store_floating (to, TYPE_LENGTH (type), d);
 }
 
 /* Convert data from virtual format with type TYPE in buffer FROM to
-   raw format for register REGNUM in buffer TO.  Simply omit the two
-   unused bytes.  */
+   raw format for register REGNUM in buffer TO.  */
 
 void
 i386_register_convert_to_raw (struct type *type, int regnum,
 			      char *from, char *to)
 {
+  gdb_assert (TYPE_CODE (type) == TYPE_CODE_FLT
+	      && TYPE_LENGTH (type) == 12);
+
+  /* Simply omit the two unused bytes.  */
   memcpy (to, from, FPU_REG_RAW_SIZE);
 }
 \f     

Re: [PATCH] Fix i386 FPU register conversion code

[Michael Snyder]

Mark Kettenis wrote:
> 
> Fixing the debug register number mapping scheme uncovered a bug in
> i386-tdep.c:i386_register_convert_to_virtual().  Fixed with the
> attached patch.
> 
> Mark

Isn't "gdb_assert" rather strong for this use?
Seems like simply returning without doing anything
would be sufficient in case we were called with an
integer register or what-not.  Calling gdb_assert
will result in the user being asked if he would like
GDB to abort and dump core (I think...)


> 
> Index: ChangeLog
> from  Mark Kettenis  <kettenis@gnu.org>
> 
>         * i386-tdep.c: Include "gdb_assert.h"
>         (i386_register_convert_to_virtual): Fix such that it can handle
>         conversion to any floating-point type.  Assert that we are dealing
>         with a floating-point first.
>         (i386_register_convert_to_raw): Assert that TYPE is a
>         floating-point type with length 12.
> 
> Index: i386-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/i386-tdep.c,v
> retrieving revision 1.34
> diff -u -p -r1.34 i386-tdep.c
> --- i386-tdep.c 2001/07/15 20:10:02 1.34
> +++ i386-tdep.c 2001/07/28 16:45:41
> @@ -33,6 +33,8 @@
>  #include "arch-utils.h"
>  #include "regcache.h"
> 
> +#include "gdb_assert.h"
> +
>  /* i386_register_byte[i] is the offset into the register file of the
>     start of register number i.  We initialize this from
>     i386_register_raw_size.  */
> @@ -972,27 +974,39 @@ i386_register_convertible (int regnum)
>  }
> 
>  /* Convert data from raw format for register REGNUM in buffer FROM to
> -   virtual format with type TYPE in buffer TO.  In principle both
> -   formats are identical except that the virtual format has two extra
> -   bytes appended that aren't used.  We set these to zero.  */
> +   virtual format with type TYPE in buffer TO.  */
> 
>  void
>  i386_register_convert_to_virtual (int regnum, struct type *type,
>                                   char *from, char *to)
>  {
> -  /* Copy straight over, but take care of the padding.  */
> -  memcpy (to, from, FPU_REG_RAW_SIZE);
> -  memset (to + FPU_REG_RAW_SIZE, 0, TYPE_LENGTH (type) - FPU_REG_RAW_SIZE);
> +  char buf[12];
> +  DOUBLEST d;
> +
> +  /* We only support floating-point values.  */
> +  gdb_assert (TYPE_CODE (type) == TYPE_CODE_FLT);
> +
> +  /* First add the necessary padding.  */
> +  memcpy (buf, from, FPU_REG_RAW_SIZE);
> +  memset (buf + FPU_REG_RAW_SIZE, 0, sizeof buf - FPU_REG_RAW_SIZE);
> +
> +  /* Convert to TYPE.  This should be a no-op, if TYPE is equivalent
> +     to the extended floating-point format used by the FPU.  */
> +  d = extract_floating (buf, sizeof buf);
> +  store_floating (to, TYPE_LENGTH (type), d);
>  }
> 
>  /* Convert data from virtual format with type TYPE in buffer FROM to
> -   raw format for register REGNUM in buffer TO.  Simply omit the two
> -   unused bytes.  */
> +   raw format for register REGNUM in buffer TO.  */
> 
>  void
>  i386_register_convert_to_raw (struct type *type, int regnum,
>                               char *from, char *to)
>  {
> +  gdb_assert (TYPE_CODE (type) == TYPE_CODE_FLT
> +             && TYPE_LENGTH (type) == 12);
> +
> +  /* Simply omit the two unused bytes.  */
>    memcpy (to, from, FPU_REG_RAW_SIZE);
>  }
>

Re: [PATCH] Fix i386 FPU register conversion code

[Mark Kettenis]

   Date: Sat, 28 Jul 2001 12:48:12 -0700
   From: Michael Snyder <msnyder@redhat.com>

   Mark Kettenis wrote:
   > 
   > Fixing the debug register number mapping scheme uncovered a bug in
   > i386-tdep.c:i386_register_convert_to_virtual().  Fixed with the
   > attached patch.
   > 
   > Mark

   Isn't "gdb_assert" rather strong for this use?
   Seems like simply returning without doing anything
   would be sufficient in case we were called with an
   integer register or what-not.  Calling gdb_assert
   will result in the user being asked if he would like
   GDB to abort and dump core (I think...)

Hmm, when I added the assertion, I was under the impression that if
the virtual type wasn't a floating-point type it would be a GDB
internal error, hence the gdb_assert.  However, this is probably not
entirely true, since I now think that bogus debug information (e.g. a
stab that says that  an integer variable that lives in a
floating-point register) might trip the assertion.  Printing a warning
and returning without doing anything is probably better.

The assertion in i386_register_convert_to_raw() still seems
appropriate though.

Mark

Re: [PATCH] Fix i386 FPU register conversion code

[Michael Snyder]

Mark Kettenis wrote:
> 
>    Date: Sat, 28 Jul 2001 12:48:12 -0700
>    From: Michael Snyder <msnyder@redhat.com>
> 
>    Mark Kettenis wrote:
>    >
>    > Fixing the debug register number mapping scheme uncovered a bug in
>    > i386-tdep.c:i386_register_convert_to_virtual().  Fixed with the
>    > attached patch.
>    >
>    > Mark
> 
>    Isn't "gdb_assert" rather strong for this use?
>    Seems like simply returning without doing anything
>    would be sufficient in case we were called with an
>    integer register or what-not.  Calling gdb_assert
>    will result in the user being asked if he would like
>    GDB to abort and dump core (I think...)
> 
> Hmm, when I added the assertion, I was under the impression that if
> the virtual type wasn't a floating-point type it would be a GDB
> internal error, hence the gdb_assert.  However, this is probably not
> entirely true, since I now think that bogus debug information (e.g. a
> stab that says that  an integer variable that lives in a
> floating-point register) might trip the assertion.  Printing a warning
> and returning without doing anything is probably better.

Even if it is an internal error, it isn't necessarily a 
fatal one.  Assert is a fatal error, isn't it?  I just
think that some errors are recoverable or ignorable, 
and might just warant a warning (or a silent ignore).

Re: [PATCH] Fix i386 FPU register conversion code

[Michael Snyder]

Andrew Cagney wrote:
> 
> > Hmm, when I added the assertion, I was under the impression that if
> > the virtual type wasn't a floating-point type it would be a GDB
> > internal error, hence the gdb_assert.  However, this is probably not
> > entirely true, since I now think that bogus debug information (e.g. a
> > stab that says that  an integer variable that lives in a
> > floating-point register) might trip the assertion.  Printing a warning
> > and returning without doing anything is probably better.
> 
> By ``nothing'' I guess you mean do something like zero the destination
> buffer :-)

Right.  Silently fix it up, rather than call abort.
Some errors don't warrant terminating a possibly long
debug session.

Re: [PATCH] Fix i386 FPU register conversion code

[Mark Kettenis]

   Date: Mon, 30 Jul 2001 10:32:44 -0700
   From: Michael Snyder <msnyder@cygnus.com>

   Andrew Cagney wrote:
   > 
   > > Hmm, when I added the assertion, I was under the impression that if
   > > the virtual type wasn't a floating-point type it would be a GDB
   > > internal error, hence the gdb_assert.  However, this is probably not
   > > entirely true, since I now think that bogus debug information (e.g. a
   > > stab that says that  an integer variable that lives in a
   > > floating-point register) might trip the assertion.  Printing a warning
   > > and returning without doing anything is probably better.
   > 
   > By ``nothing'' I guess you mean do something like zero the destination
   > buffer :-)

   Right.  Silently fix it up, rather than call abort.
   Some errors don't warrant terminating a possibly long
   debug session.

I attached should address the concerns of both of you.  Checked in on
head and branch.

Mark


Index: ChangeLog
from  Mark Kettenis  <kettenis@gnu.org>

	* i386-tdep.c (i386_register_convert_to_virtual): Replace
	assertion with a warning if we're asked to convert towards a
	non-floating-point type.  Zero out the the buffer where the data
	is supposed to be stored in that case.

Index: i386-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-tdep.c,v
retrieving revision 1.36
diff -u -p -r1.36 i386-tdep.c
--- i386-tdep.c 2001/07/28 17:03:38 1.36
+++ i386-tdep.c 2001/07/30 20:54:33
@@ -1046,7 +1046,13 @@ i386_register_convert_to_virtual (int re
   DOUBLEST d;
 
   /* We only support floating-point values.  */
-  gdb_assert (TYPE_CODE (type) == TYPE_CODE_FLT);
+  if (TYPE_CODE (type) != TYPE_CODE_FLT)
+    {
+      warning ("Cannot convert floating-point register value "
+	       "to non-floating-point type.");
+      memset (to, 0, TYPE_LENGTH (type));
+      return;
+    }
 
   /* First add the necessary padding.  */
   memcpy (buf, from, FPU_REG_RAW_SIZE);