[patch] handle nested exceptions

[patch] handle nested exceptions

[Doug Evans]

Hi.

We ran into a case where gdb was using a previously freed copy of e.message.
It's not enough to keep just last_message.

This patch creates a stack of messages.

I will check this in in a few days if there are no objections.

2011-12-14  Doug Evans  <dje@google.com>

	* exceptions.c (catcher_list_size): New function.
	(last_message): Delete.
	(exception_messages, exception_messages_size): New static globals.
	(throw_it): Use exception_messages array to handle nested calls.

Index: exceptions.c
===================================================================
RCS file: /cvs/src/src/gdb/exceptions.c,v
retrieving revision 1.48
diff -u -p -r1.48 exceptions.c
--- exceptions.c	26 Aug 2011 21:45:22 -0000	1.48
+++ exceptions.c	9 Dec 2011 19:54:36 -0000
@@ -68,6 +68,22 @@ struct catcher
 /* Where to go for throw_exception().  */
 static struct catcher *current_catcher;
 
+/* Return length of current_catcher list.  */
+
+static int
+catcher_list_size (void)
+{
+  int size;
+  struct catcher *catcher;
+
+  for (size = 0, catcher = current_catcher;
+       catcher != NULL;
+       catcher = catcher->prev)
+    ++size;
+
+  return size;
+}
+
 EXCEPTIONS_SIGJMP_BUF *
 exceptions_state_mc_init (volatile struct gdb_exception *exception,
 			  return_mask mask)
@@ -220,8 +236,6 @@ throw_exception (struct gdb_exception ex
   EXCEPTIONS_SIGLONGJMP (current_catcher->buf, exception.reason);
 }
 
-static char *last_message;
-
 void
 deprecated_throw_reason (enum return_reason reason)
 {
@@ -359,23 +373,48 @@ print_any_exception (struct ui_file *fil
     }
 }
 
+/* A stack of exception messages.
+   This is needed to handle nested calls to throw_it: we don't want to
+   xfree space for a message before it's used.
+   It is indexed by the size of the current_catcher list.
+   This is a dynamically allocated array so that we don't care how deeply
+   GDB nests its TRY_CATCHs.  */
+static char **exception_messages;
+/* The number of currently allocated entries in exception_messages.  */
+static int exception_messages_size;
+
 static void ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (3, 0)
 throw_it (enum return_reason reason, enum errors error, const char *fmt,
 	  va_list ap)
 {
   struct gdb_exception e;
   char *new_message;
+  int depth = catcher_list_size ();
+
+  gdb_assert (depth > 0);
 
-  /* Save the message.  Create the new message before deleting the
-     old, the new message may include the old message text.  */
+  /* Note: The new message may use an old message's text.  */
   new_message = xstrvprintf (fmt, ap);
-  xfree (last_message);
-  last_message = new_message;
+
+  if (depth > exception_messages_size)
+    {
+      int old_size = exception_messages_size;
+
+      exception_messages_size = depth + 10;
+      exception_messages = (char **) xrealloc (exception_messages,
+					       exception_messages_size
+					       * sizeof (char *));
+      memset (exception_messages + old_size, 0,
+	      (exception_messages_size - old_size) * sizeof (char *));
+    }
+
+  xfree (exception_messages[depth - 1]);
+  exception_messages[depth - 1] = new_message;
 
   /* Create the exception.  */
   e.reason = reason;
   e.error = error;
-  e.message = last_message;
+  e.message = new_message;
 
   /* Throw the exception.  */
   throw_exception (e);

Re: [patch] handle nested exceptions

[Tom Tromey]

>>>>> "Doug" == Doug Evans <dje@google.com> writes:

Doug> I will check this in in a few days if there are no objections.
Doug> 2011-12-14  Doug Evans  <dje@google.com>

Doug> 	* exceptions.c (catcher_list_size): New function.
Doug> 	(last_message): Delete.
Doug> 	(exception_messages, exception_messages_size): New static globals.
Doug> 	(throw_it): Use exception_messages array to handle nested calls.

It seems good to me.

It was a little hard to understand why this was the right approach.
Actually I am not 100% sure I have convinced myself.
I think a comment explaining the motivating scenario would help.

Tom

Re: [patch] handle nested exceptions

[Doug Evans]

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

On Thu, Dec 15, 2011 at 6:58 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Doug" == Doug Evans <dje@google.com> writes:
>
> Doug> I will check this in in a few days if there are no objections.
> Doug> 2011-12-14  Doug Evans  <dje@google.com>
>
> Doug>   * exceptions.c (catcher_list_size): New function.
> Doug>   (last_message): Delete.
> Doug>   (exception_messages, exception_messages_size): New static globals.
> Doug>   (throw_it): Use exception_messages array to handle nested calls.
>
> It seems good to me.
>
> It was a little hard to understand why this was the right approach.
> Actually I am not 100% sure I have convinced myself.
> I think a comment explaining the motivating scenario would help.

The motivating scenario is when we call throw_it instead a cleanup due
to an earlier throw_it.
[Actually, for the case at hand there were multiple calls to throw_it
during the cleanup.]

How's this?

2011-12-15  Doug Evans  <dje@google.com>

        * exceptions.c (catcher_list_size): New function.
        (last_message): Delete.
        (exception_messages, exception_messages_size): New static globals.
        (throw_it): Use exception_messages array to handle nested calls.

[-- Attachment #2: gdb-111215-nested-throw-it-3.patch.txt --]
[-- Type: text/plain, Size: 3258 bytes --]

2011-12-15  Doug Evans  <dje@google.com>

	* exceptions.c (catcher_list_size): New function.
	(last_message): Delete.
	(exception_messages, exception_messages_size): New static globals.
	(throw_it): Use exception_messages array to handle nested calls.

Index: exceptions.c
===================================================================
RCS file: /cvs/src/src/gdb/exceptions.c,v
retrieving revision 1.48
diff -u -p -r1.48 exceptions.c
--- exceptions.c	26 Aug 2011 21:45:22 -0000	1.48
+++ exceptions.c	15 Dec 2011 17:58:51 -0000
@@ -68,6 +68,22 @@ struct catcher
 /* Where to go for throw_exception().  */
 static struct catcher *current_catcher;
 
+/* Return length of current_catcher list.  */
+
+static int
+catcher_list_size (void)
+{
+  int size;
+  struct catcher *catcher;
+
+  for (size = 0, catcher = current_catcher;
+       catcher != NULL;
+       catcher = catcher->prev)
+    ++size;
+
+  return size;
+}
+
 EXCEPTIONS_SIGJMP_BUF *
 exceptions_state_mc_init (volatile struct gdb_exception *exception,
 			  return_mask mask)
@@ -220,8 +236,6 @@ throw_exception (struct gdb_exception ex
   EXCEPTIONS_SIGLONGJMP (current_catcher->buf, exception.reason);
 }
 
-static char *last_message;
-
 void
 deprecated_throw_reason (enum return_reason reason)
 {
@@ -359,23 +373,53 @@ print_any_exception (struct ui_file *fil
     }
 }
 
+/* A stack of exception messages.
+   This is needed to handle nested calls to throw_it: we don't want to
+   xfree space for a message before it's used.
+   This can happen if we throw an exception during a cleanup:
+   An outer TRY_CATCH may have an exception message it wants to print,
+   but while doing cleanups further calls to throw_it are made.
+
+   This is indexed by the size of the current_catcher list.
+   It is a dynamically allocated array so that we don't care how deeply
+   GDB nests its TRY_CATCHs.  */
+static char **exception_messages;
+
+/* The number of currently allocated entries in exception_messages.  */
+static int exception_messages_size;
+
 static void ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (3, 0)
 throw_it (enum return_reason reason, enum errors error, const char *fmt,
 	  va_list ap)
 {
   struct gdb_exception e;
   char *new_message;
+  int depth = catcher_list_size ();
 
-  /* Save the message.  Create the new message before deleting the
-     old, the new message may include the old message text.  */
+  gdb_assert (depth > 0);
+
+  /* Note: The new message may use an old message's text.  */
   new_message = xstrvprintf (fmt, ap);
-  xfree (last_message);
-  last_message = new_message;
+
+  if (depth > exception_messages_size)
+    {
+      int old_size = exception_messages_size;
+
+      exception_messages_size = depth + 10;
+      exception_messages = (char **) xrealloc (exception_messages,
+					       exception_messages_size
+					       * sizeof (char *));
+      memset (exception_messages + old_size, 0,
+	      (exception_messages_size - old_size) * sizeof (char *));
+    }
+
+  xfree (exception_messages[depth - 1]);
+  exception_messages[depth - 1] = new_message;
 
   /* Create the exception.  */
   e.reason = reason;
   e.error = error;
-  e.message = last_message;
+  e.message = new_message;
 
   /* Throw the exception.  */
   throw_exception (e);

Re: [patch] handle nested exceptions

[Tom Tromey]

>>>>> "Doug" == Doug Evans <dje@google.com> writes:

Doug> How's this?
Doug> 2011-12-15  Doug Evans  <dje@google.com>

Doug>         * exceptions.c (catcher_list_size): New function.
Doug>         (last_message): Delete.
Doug>         (exception_messages, exception_messages_size): New static globals.
Doug>         (throw_it): Use exception_messages array to handle nested calls.

Looks good to me.  Thanks for updating that.

Tom