Re: [PATCHv7 07/11] gdb: parse pending breakpoint thread/task immediately

[Andrew Burgess <aburgess@redhat.com>]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> Andrew> As a result of this commit the breakpoints 'extra_string' field is now
> Andrew> only used by bp_dprintf type breakpoints to hold the printf format and
> Andrew> arguments.  This string should always be empty for other breakpoint
> Andrew> types.  This allows me to clean up some code in
> Andrew> print_breakpoint_location.
>
> I wonder if this member can be pushed into struct dprintf_breakpoint.
> (I don't think you should do that in this series, you've been waiting
> long enough.)
>
> Andrew>   2. The handling of '-force-condition' is odd, if this flag appears
> Andrew>   immediately after a condition then it will be treated as part of the
> Andrew>   condition, e.g.:
>
> We probably should have required this to appear before the linespec.
>
> Maybe also thread/task/etc.

So Baris also asked about this change on V6.  Apparently when the
-force-condition flag was first added there was a couple of requests
that the implementation change to use a more GDB option like approach,
and I think that makes sense.

I looked into this a little, and it's a slightly more complex change.

The existing breakpoint options are not handled like "normal" options,
but are tied up with location parsing.  And I don't think it makes sense
to handle -force-condition (or thread/task/etc) there -- these options
are related to the condition, not the location.

So my thinking is that what should happen is that the location parsing
code should export the gdb::option::* related data structures needed to
parse the location related options.

The condition related code should export the gdb::option::* related data
structures needed to parse the condition related options.

Then in breakpoint.c we'd create a gdb::option::option_def_group that
can parse all of the various options.

And finally, we can send those parsed options from breakpoint.c down
into the various parts of GDB (the location parsing and condition
parsing).

However.... one of the breakpoint options is a filename, and that
option currently supports filename completion.

So, to move to the gdb::option::* approach we need a new
gdb::option::filename_option_def, which we don't currently have.
Actually we're going to need a couple of new *_option_def types, but I
think filenames are the hardest.

I'm aware of a couple of attempts to add a filename_option_def; Pedro
has a branch `filename-options` on his github, and also Lancelot posted
this a while back:

  https://inbox.sourceware.org/gdb-patches/20210213220752.32581-1-lsix@lancelotsix.com/

I prefer Pedro's approach, though I haven't finished re-reading
Lancelot's branch (apparently I did review it when it was originally
posted, but I don't remember it), so maybe Lancelot's work offers
something Pedro's is missing.  However, Pedro never posted his work, so
I can't post anything based on that branch yet (Copyright), but I've
reached out to ask for permission.

Anyway.... the summary of all this, is that I have a multi-step plan to
change how -force-condition (and possibly thread/task/etc, we'll see)
are handled, but it requires a bunch of setup, so I don't plan to
combine these tasks (unless that becomes a requirement).

>
> Andrew> +/* Return true if STR starts with PREFIX, otherwise return false.  */
> Andrew> +
> Andrew> +static bool
> Andrew> +startswith (const char *str, const token &prefix)
> Andrew> +{
> Andrew> +  return strncmp (str, prefix.data (), prefix.length ()) == 0;
> Andrew> +}
>
> I wouldn't mind this in common-utils.h alongside the other startswith,
> but it's fine here too.
>
> Andrew> +  test ("  if blah  ", "blah");
> Andrew> +  test (" if blah thread 1", "blah", global_thread_num);
> Andrew> +  test (" if blah inferior 1", "blah", -1, 1);
> Andrew> +  test (" if blah thread 1  ", "blah", global_thread_num);
>
> It seems like there are some confounding cases, where gdb would previous
> (rightly) give a syntax error, but where now they may be passed through
> without notice.
>
> I thought of some... maybe in the "don't care" bucket.  I mean, it would
> be nice if we could do a little better, since I guess the user could
> typo and then wind up with a breakpoint that's never really useful,
> like:
>
>     break some_future_function -force-condition if x == thread 5
>
> Will this ever yield an error?  Maybe the user accidentally left out the
> right-hand-side of that 'thread'.  Or maybe meant to write "thread thread".
>
> Probably the worst cases involve deliberately confounding macro
> definitions, but these IMO are definitely "don't care".  Maybe advising
> the user to surround the 'if' in parens and that's enough, assuming:
>
>     break func if ( x == thread )
>
> ... does the right thing.

I'll add some new tests for these cases and dig into how they behave.

Thank,
Andrew


>
> Tom