[patch] Add proper error message instead of gdb_assert

[patch] Add proper error message instead of gdb_assert

[Markus Deuling]

[-- Attachment #1: Type: text/plain, Size: 1163 bytes --]

Hi,

when trying to put > 1 values into an array (fortran subrange) which
comes from a register, register_size is called with regnum == -1.

The following example comes from SPU architecture. Currently GDB exits 
with a gdb_assert going wrong:

(gdb) set $r0%v2_int64(0:1)=(1,2)
../../../gdb-6.7/gdb/regcache.c:177: internal-error: register_size: Assertion `regnum >= 0 && regnum < (gdbarch_num_regs (gdbarch) + gdbarch_num_pseudo_regs (gdbarch))' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) y

This patch replaces that gdb_assert by a proper error message before
exiting:

(gdb) set $r2%v2_int64(0:1)=(1,1)
../../../gdb-6.7/gdb/regcache.c:185: internal-error: invalid register -1
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) 

If this patch is ok it would be great to have it in gdb 6.8. Ok?

ChangeLog:

        * regcache.c (register_size): Replace gdb_assert by a proper error
        message.


Regards,
Markus


-- 
  Markus Deuling
  GNU Toolchain for Linux on Cell BE
  deuling@de.ibm.com


[-- Attachment #2: fix-assert --]
[-- Type: text/plain, Size: 858 bytes --]

diff -urpN gdb-6.7-orig/gdb/regcache.c gdb-6.7/gdb/regcache.c
--- gdb-6.7-orig/gdb/regcache.c	2008-03-03 19:30:54.000000000 +0100
+++ gdb-6.7/gdb/regcache.c	2008-03-03 20:30:10.000000000 +0100
@@ -172,11 +172,18 @@ register_size (struct gdbarch *gdbarch, 
 {
   struct regcache_descr *descr = regcache_descr (gdbarch);
   int size;
-  gdb_assert (regnum >= 0
-	      && regnum < (gdbarch_num_regs (gdbarch)
-			   + gdbarch_num_pseudo_regs (gdbarch)));
-  size = descr->sizeof_register[regnum];
-  return size;
+
+ 
+  if (regnum >= 0 
+      && regnum < (gdbarch_num_regs (gdbarch)
+	 + gdbarch_num_pseudo_regs (gdbarch)))
+    {
+      size = descr->sizeof_register[regnum];
+      return size;
+    }
+
+  internal_error (__FILE__, __LINE__, _("invalid register %d"), regnum);
+  return 0;
 }
 
 /* The register cache for storing raw register values.  */

Re: [patch] Add proper error message instead of gdb_assert

[Mark Kettenis]

> Date: Mon, 03 Mar 2008 20:36:18 +0100
> From: "Markus Deuling" <deuling@de.ibm.com>
> 
> Hi,
> 
> when trying to put > 1 values into an array (fortran subrange) which
> comes from a register, register_size is called with regnum =3D=3D -1.
> 
> The following example comes from SPU architecture. Currently GDB exits 
> with a gdb_assert going wrong:
> 
> (gdb) set $r0%v2_int64(0:1)=3D(1,2)
> .../../../gdb-6.7/gdb/regcache.c:177: internal-error: register_size: =
> Assertion `regnum >=3D 0 && regnum < (gdbarch_num_regs (gdbarch) + =
> gdbarch_num_pseudo_regs (gdbarch))' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n) y
> 
> This patch replaces that gdb_assert by a proper error message before
> exiting:
> 
> (gdb) set $r2%v2_int64(0:1)=3D(1,1)
> .../../../gdb-6.7/gdb/regcache.c:185: internal-error: invalid register -1
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n) 
> 
> If this patch is ok it would be great to have it in gdb 6.8. Ok?

Sorry, but I don't see why your error message is "proper".  The
gdb_assert() should never fail; the fact that it does means that you
have a bug elsewhere in gdb.

Re: [patch] Add proper error message instead of gdb_assert

[Ulrich Weigand]

Markus Deuling wrote:

> when trying to put > 1 values into an array (fortran subrange) which
> comes from a register, register_size is called with regnum == -1.

You need to fix the caller to not do this, then.

> This patch replaces that gdb_assert by a proper error message before
> exiting:

An "internal error" is neither more nor less "proper" than a failed
assertion.  Both say "there's a bug in GDB", and neither should be
triggerable by normal user interaction.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [patch] Add proper error message instead of gdb_assert

[Michael Snyder]

In general I don't like asserts, and especially I don't like them
when the error is something local as opposed to something
un-recoverable.

Whenever it is NOT an unrecoverable error, I think that
replacing an assert with an error message should be a 
good thing.

I'm inviting comment.   ;-)

On Mon, 2008-03-03 at 20:36 +0100, Markus Deuling wrote:
> Hi,
> 
> when trying to put > 1 values into an array (fortran subrange) which
> comes from a register, register_size is called with regnum == -1.
> 
> The following example comes from SPU architecture. Currently GDB exits 
> with a gdb_assert going wrong:
> 
> (gdb) set $r0%v2_int64(0:1)=(1,2)
> ../../../gdb-6.7/gdb/regcache.c:177: internal-error: register_size: Assertion `regnum >= 0 && regnum < (gdbarch_num_regs (gdbarch) + gdbarch_num_pseudo_regs (gdbarch))' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n) y
> 
> This patch replaces that gdb_assert by a proper error message before
> exiting:
> 
> (gdb) set $r2%v2_int64(0:1)=(1,1)
> ../../../gdb-6.7/gdb/regcache.c:185: internal-error: invalid register -1
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n) 
> 
> If this patch is ok it would be great to have it in gdb 6.8. Ok?
> 
> ChangeLog:
> 
>         * regcache.c (register_size): Replace gdb_assert by a proper error
>         message.
> 
> 
> Regards,
> Markus
> 
> 
> plain text document attachment (fix-assert)
> diff -urpN gdb-6.7-orig/gdb/regcache.c gdb-6.7/gdb/regcache.c
> --- gdb-6.7-orig/gdb/regcache.c	2008-03-03 19:30:54.000000000 +0100
> +++ gdb-6.7/gdb/regcache.c	2008-03-03 20:30:10.000000000 +0100
> @@ -172,11 +172,18 @@ register_size (struct gdbarch *gdbarch, 
>  {
>    struct regcache_descr *descr = regcache_descr (gdbarch);
>    int size;
> -  gdb_assert (regnum >= 0
> -	      && regnum < (gdbarch_num_regs (gdbarch)
> -			   + gdbarch_num_pseudo_regs (gdbarch)));
> -  size = descr->sizeof_register[regnum];
> -  return size;
> +
> + 
> +  if (regnum >= 0 
> +      && regnum < (gdbarch_num_regs (gdbarch)
> +	 + gdbarch_num_pseudo_regs (gdbarch)))
> +    {
> +      size = descr->sizeof_register[regnum];
> +      return size;
> +    }
> +
> +  internal_error (__FILE__, __LINE__, _("invalid register %d"), regnum);
> +  return 0;
>  }
>  
>  /* The register cache for storing raw register values.  */

Re: [patch] Add proper error message instead of gdb_assert

[Michael Snyder]

On Mon, 2008-03-03 at 20:55 +0100, Mark Kettenis wrote:

> 
> Sorry, but I don't see why your error message is "proper".  The
> gdb_assert() should never fail; the fact that it does means that you
> have a bug elsewhere in gdb.

Isn't that what the "internal error" call is for?

The fact that there's a bug somewhere else in gdb is not
necessarily a reason to abort the debugging session.
It COULD be that severe -- but I don't think we necessarily
need to assume it is.

If so, we might just about as well call abort from main, 
because there are *always* bugs somewhere else in gdb...
(1/2 <g>)

Re: [patch] Add proper error message instead of gdb_assert

[Michael Snyder]

On Mon, 2008-03-03 at 21:00 +0100, Ulrich Weigand wrote:
> Markus Deuling wrote:
> 
> > when trying to put > 1 values into an array (fortran subrange) which
> > comes from a register, register_size is called with regnum == -1.
> 
> You need to fix the caller to not do this, then.
> 
> > This patch replaces that gdb_assert by a proper error message before
> > exiting:
> 
> An "internal error" is neither more nor less "proper" than a failed
> assertion.  Both say "there's a bug in GDB", and neither should be
> triggerable by normal user interaction.

Yes, but they're not equivalent.  An assertion failure terminates
the session.  Shouldn't we reserve that for situations that we
can reasonably assume are un-recoverable?

Re: [patch] Add proper error message instead of gdb_assert

[Mark Kettenis]

> From: Michael Snyder <msnyder@specifix.com>
> Date: Mon, 03 Mar 2008 12:28:51 -0800
> 
> On Mon, 2008-03-03 at 20:55 +0100, Mark Kettenis wrote:
> 
> > 
> > Sorry, but I don't see why your error message is "proper".  The
> > gdb_assert() should never fail; the fact that it does means that you
> > have a bug elsewhere in gdb.
> 
> Isn't that what the "internal error" call is for?
> 
> The fact that there's a bug somewhere else in gdb is not
> necessarily a reason to abort the debugging session.
> It COULD be that severe -- but I don't think we necessarily
> need to assume it is.

Sure, that's why we don't use assert(), but have gdb_assert(), which
calls internal_error() instead of abort.  It's just that gdb_assert()
results in a slightly different error message (which hopefuly is more
helpful to the poor soul who actually will be fixing the bug).

Re: [patch] Add proper error message instead of gdb_assert

[Daniel Jacobowitz]

On Mon, Mar 03, 2008 at 12:28:51PM -0800, Michael Snyder wrote:
> On Mon, 2008-03-03 at 20:55 +0100, Mark Kettenis wrote:
> 
> > 
> > Sorry, but I don't see why your error message is "proper".  The
> > gdb_assert() should never fail; the fact that it does means that you
> > have a bug elsewhere in gdb.
> 
> Isn't that what the "internal error" call is for?

gdb_assert calls internal_error anyway.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Add proper error message instead of gdb_assert

[Markus Deuling]

Daniel Jacobowitz schrieb:
> On Mon, Mar 03, 2008 at 12:28:51PM -0800, Michael Snyder wrote:
>> On Mon, 2008-03-03 at 20:55 +0100, Mark Kettenis wrote:
>>
>>> Sorry, but I don't see why your error message is "proper".  The
>>> gdb_assert() should never fail; the fact that it does means that you
>>> have a bug elsewhere in gdb.
>> Isn't that what the "internal error" call is for?
> 
> gdb_assert calls internal_error anyway.
> 


In my special case this error is recoverable so an "error" would be ok, too (beside the fact that
the bug has to be fixed :-) ).

Are there situations where a GDB run is unrecoverable broken when register_size is given the wrong regnum?


-- 
  Markus Deuling
  GNU Toolchain for Linux on Cell BE
  deuling@de.ibm.com

Re: [patch] Add proper error message instead of gdb_assert

[Markus Deuling]

[-- Attachment #1: Type: text/plain, Size: 717 bytes --]

Daniel Jacobowitz schrieb:
> On Mon, Mar 03, 2008 at 12:28:51PM -0800, Michael Snyder wrote:
>> On Mon, 2008-03-03 at 20:55 +0100, Mark Kettenis wrote:
>>
>>> Sorry, but I don't see why your error message is "proper".  The
>>> gdb_assert() should never fail; the fact that it does means that you
>>> have a bug elsewhere in gdb.
>> Isn't that what the "internal error" call is for?
> 
> gdb_assert calls internal_error anyway.
> 

What about the attached patch? It gives an error message in value_assign if you try to
access lval_register < 0. I guess this is much better for the user than a crashed GDB session.

What do you think ?



-- 
  Markus Deuling
  GNU Toolchain for Linux on Cell BE
  deuling@de.ibm.com


[-- Attachment #2: fix-fortran-2 --]
[-- Type: text/plain, Size: 697 bytes --]

diff -urpN gdb-6.7-orig/gdb/valops.c gdb-6.7/gdb/valops.c
--- gdb-6.7-orig/gdb/valops.c	2007-08-23 20:08:46.000000000 +0200
+++ gdb-6.7/gdb/valops.c	2008-03-03 22:05:59.000000000 +0100
@@ -623,10 +623,12 @@ value_assign (struct value *toval, struc
 	struct frame_info *frame;
 	int value_reg;
 
-	/* Figure out which frame this is in currently.  */
-	frame = frame_find_by_id (VALUE_FRAME_ID (toval));
 	value_reg = VALUE_REGNUM (toval);
+	if (value_reg < 0)
+	  error (_("Invalid register %d"), value_reg);
 
+	/* Figure out which frame this is in currently.  */
+	frame = frame_find_by_id (VALUE_FRAME_ID (toval));
 	if (!frame)
 	  error (_("Value being assigned to is no longer active."));
 	

Re: [patch] Add proper error message instead of gdb_assert

[Daniel Jacobowitz]

On Mon, Mar 03, 2008 at 10:11:44PM +0100, Markus Deuling wrote:
> What about the attached patch? It gives an error message in value_assign if you try to
> access lval_register < 0. I guess this is much better for the user than a crashed GDB session.
>
> What do you think ?

No.  Both of these are internal errors in GDB.  The internal error
prompt already gives you the option to keep going if you want to.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Add proper error message instead of gdb_assert

[Eli Zaretskii]

> Date: Mon, 3 Mar 2008 21:35:35 +0100 (CET)
> From: Mark Kettenis <mark.kettenis@xs4all.nl>
> CC: deuling@de.ibm.com, gdb-patches@sourceware.org, uweigand@de.ibm.com
> 
> > From: Michael Snyder <msnyder@specifix.com>
> > Date: Mon, 03 Mar 2008 12:28:51 -0800
> > 
> > On Mon, 2008-03-03 at 20:55 +0100, Mark Kettenis wrote:
> > 
> > > 
> > > Sorry, but I don't see why your error message is "proper".  The
> > > gdb_assert() should never fail; the fact that it does means that you
> > > have a bug elsewhere in gdb.
> > 
> > Isn't that what the "internal error" call is for?
> > 
> > The fact that there's a bug somewhere else in gdb is not
> > necessarily a reason to abort the debugging session.
> > It COULD be that severe -- but I don't think we necessarily
> > need to assume it is.
> 
> Sure, that's why we don't use assert(), but have gdb_assert(), which
> calls internal_error() instead of abort.  It's just that gdb_assert()
> results in a slightly different error message (which hopefuly is more
> helpful to the poor soul who actually will be fixing the bug).

I agree with Michael and Markus: a proper error message in plain
English is better in this case.  If the text displayed by gdb_assert
is going to help (though personally I don't see how), we could add
some of it to the error message.