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

[Hui Zhu <teawater at gmail dot com>]

On Tue, Mar 26, 2013 at 2:09 AM, Keith Seitz <keiths@redhat.com> wrote:
> 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:

This function have parse_condition_and_thread.  It already choice
which part it will get the cond_string from, why we still need this
check?
Also it didn't handle this pending breakpoints.

>
> 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.
>

And if we want add comments on this function to let people don't do
that.  We need also tell they, the pending breakpoints's extra_string
will be dropped.

>
>> 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