RE: [MI] Segfault using 'interpreter-exec mi'

[Marc Khouzam <marc dot khouzam at ericsson dot com>]

> -----Original Message-----
> From: Tom Tromey [mailto:tromey@redhat.com] 
> Sent: Thursday, December 02, 2010 11:35 AM
> To: Marc Khouzam
> Cc: 'gdb-patches@sourceware.org'
> Subject: Re: [MI] Segfault using 'interpreter-exec mi'
> 
> >>>>> "Marc" == Marc Khouzam <marc.khouzam@ericsson.com> writes:
> 
> Marc> I got a segfault when using 'interpreter-exec mi' and getting an
> Marc> error result.  I believe I tracked it down to mi_parse().  From
> Marc> what I can see, we cannot call error() from mi_parse() 
> because it
> Marc> does not catch exceptions.
> 
> Marc> Note that the segfault does not happen in full MI mode, 
> I think because
> Marc> we are in the correct interpreter for output, however, 
> the MI command
> Marc> does not get the proper ^error and requires the user to 
> enter a new line
> Marc> to get the ^done.
> 
> Thanks for the patch.
> 
> Marc> The below patch removes the calls to error() and uses 
> fprintf_unfiltered.
> Marc> Because of the comment
> Marc> /* FIXME: This should be a function call. */
> Marc> I took the opportunity to make a method mi_parse_error().
> 
> I don't mind this approach, but I think it is probably better to just
> change mi_parse to use exceptions like the rest of gdb.  Then 
> the caller
> can handle them, just like it does for exceptions occurring in the
> actual MI command.
> 
> The reason I think this is better is that a rule like "this 
> code cannot
> call error" is reasonably difficult to enforce in gdb.
> 
> What do you think of that?

I agree with you and I started coding such a patch.  Things
got a bit more complex than I expected though.  Mostly because
the error output of mi_parse() has to use some of the data that 
was actually parsed in mi_parse(), which lead me to have to 
play around with return values while handling exceptions, or
having to free some memory _after_ calling error(), which 
I didn't know how to do.

After a while, it started to feel a bit complicated of a fix for 
something that I'm hoping to push to the 7_2 branch.  Currently, 
not getting the proper ^error message from MI could lead the 
frontend into a pretty bad situation, which is why I think this
should be fixed in 7_2.

Could we have the brute force fix now, and leave the improvement
for later?

> A quick nit about the patch itself.
> 
> Marc> +void
> Marc> +vmi_parse_error (struct mi_parse *parse, const char 
> *format, va_list args)
> 
> The new functions need introductory comments.
> 
> I would have made them both static.

Here is the same patch incorporating your two comments.

Thanks

Marc


2010-12-06  Marc Khouzam  <marc.khouzam@ericsson.com>

	* mi/mi-parse.c (vmi_parse_error, mi_parse_error): Added.
	(mi_parse): Call mi_parse_error instead of error.


### Eclipse Workspace Patch 1.0
#P src
Index: gdb/mi/mi-parse.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-parse.c,v
retrieving revision 1.22
diff -u -r1.22 mi-parse.c
--- gdb/mi/mi-parse.c   6 Dec 2010 14:16:43 -0000       1.22
+++ gdb/mi/mi-parse.c   6 Dec 2010 21:33:39 -0000
@@ -223,6 +223,26 @@
   xfree (parse);
 }
 
+/* Output the error message from the mi_parse method using
+   a variable parameter list, and deallocate the struct parse */
+static void
+vmi_parse_error (struct mi_parse *parse, const char *format, va_list args)
+{
+  vfprintf_unfiltered (raw_stdout, format, args);
+  mi_parse_free (parse);
+}
+
+/* Output an error message for the mi_parse method,
+   and deallocate the struct parse */
+static void
+mi_parse_error (struct mi_parse *parse, const char *format, ...) 
+{
+  va_list args;
+
+  va_start (args, format);
+  vmi_parse_error (parse, format, args);
+  va_end (args);
+}
 
 struct mi_parse *
 mi_parse (char *cmd)
@@ -272,12 +292,10 @@
   parse->cmd = mi_lookup (parse->command);
   if (parse->cmd == NULL)
     {
-      /* FIXME: This should be a function call. */
-      fprintf_unfiltered
-       (raw_stdout,
+      mi_parse_error 
+       (parse,
         "%s^error,msg=\"Undefined MI command: %s\"\n",
         parse->token, parse->command);
-      mi_parse_free (parse);
       return NULL;
     }
 
@@ -312,24 +330,51 @@
       if (strncmp (chp, "--thread-group ", tgs) == 0)
        {
          if (parse->thread_group != -1)
-           error (_("Duplicate '--thread-group' option"));
+           {   
+             mi_parse_error
+               (parse,
+                "%s^error,msg=\"Duplicate '--thread-group' option\"\n",
+                parse->token);
+             return NULL;
+           }   
+
          chp += tgs;
          if (*chp != 'i')
-           error (_("Invalid thread group id"));
+           {   
+             mi_parse_error
+               (parse,
+                "%s^error,msg=\"Invalid thread group id\"\n",
+                parse->token);
+             return NULL;
+           }
          chp += 1;
          parse->thread_group = strtol (chp, &chp, 10);
        }
       else if (strncmp (chp, "--thread ", ts) == 0)
        {
          if (parse->thread != -1)
-           error (_("Duplicate '--thread' option"));
+           {   
+             mi_parse_error
+               (parse,
+                "%s^error,msg=\"Duplicate '--thread' option\"\n",
+                parse->token);
+             return NULL;
+           }   
+
          chp += ts;
          parse->thread = strtol (chp, &chp, 10);
        }
       else if (strncmp (chp, "--frame ", fs) == 0)
        {
          if (parse->frame != -1)
-           error (_("Duplicate '--frame' option"));
+           {   
+             mi_parse_error
+               (parse,
+                "%s^error,msg=\"Duplicate '--frame' option\"\n",
+                parse->token);
+             return NULL;
+           }
+
          chp += fs;
          parse->frame = strtol (chp, &chp, 10);
        }
@@ -337,8 +382,14 @@
        break;
 
       if (*chp != '\0' && !isspace (*chp))
-       error (_("Invalid value for the '%s' option"),
-              start[2] == 't' ? "--thread" : "--frame");
+       {
+         mi_parse_error
+           (parse,
+            "%s^error,msg=\"Invalid value for the '%s' option\"\n",
+            parse->token, start[2] == 't' ? "--thread" : "--frame");
+         return NULL;
+       }
+
       while (isspace (*chp))
        chp++;
     }
@@ -350,12 +401,10 @@
       mi_parse_argv (chp, parse);
       if (parse->argv == NULL)
        {
-         /* FIXME: This should be a function call. */
-         fprintf_unfiltered
-           (raw_stdout,
+         mi_parse_error
+           (parse,
             "%s^error,msg=\"Problem parsing arguments: %s %s\"\n",
             parse->token, parse->command, chp);
-         mi_parse_free (parse);
          return NULL;
        }
     }