Re: [PATCH] Fix create pending breakpoint handle extra_string issue if not parse_condition_and_thread

[Keith Seitz <keiths@redhat.com>]

On 03/25/2013 10:14 AM, Tom Tromey wrote:
>>>>>> "Hui" == Hui Zhu <teawater@gmail.com> writes:
>
> Hui> I am sorry that what you care about is the issue that affect the mi.
> Hui> But my patch is for the issue inside the function create_breakpoint.

Actually I /was/ talking about create_breakpoint. As you stated, the 
only way to demonstrate the problem is via MI, so that's what I used to 
demonstrate how I think the situation should be handled.

Here's a patch which does exactly what I consider the "right" way to 
react to having both cond_string and a condition inside arg:

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.747
diff -u -p -r1.747 breakpoint.c
--- breakpoint.c	20 Mar 2013 22:17:18 -0000	1.747
+++ breakpoint.c	25 Mar 2013 17:59:36 -0000
@@ -9659,6 +9659,11 @@ create_breakpoint (struct gdbarch *gdbar
                  extra_string = xstrdup (extra_string);
                  make_cleanup (xfree, extra_string);
  	      }
+	    else if (*arg != '\000')
+	      {
+		extra_string = xstrdup (arg);
+		make_cleanup (xfree, extra_string);
+	      }
          }

        ops->create_breakpoints_sal (gdbarch, &canonical, lsal,

> In this case, it seems to me that the API must be a bad one.

Yes, that API extension was a horribly implemented (quick and dirty), 
but create_breakpoint is a bit of a mess, since it not only has to deal 
with setting breakpoints (of various varieties), it also has to deal 
with parsing user input. I'm not a fan of this (too common) paradigm.

> Can't we just tell callers, "don't do that"?
> To me it seems like a pathological case.

We can certainly enforce this, as my patchlet above demonstrates:

-break-insert -c "argc > 1" "main if argc > 2"
^error,msg="Garbage 'if argc > 2' at end of command"

Keith