RE: [PATCHv6 06/10] gdb: parse pending breakpoint thread/task immediately

Aktemur, Tankut Baris
2023-12-27T12:24:38Z

On Wednesday, December 13, 2023 2:51 PM, Andrew Burgess wrote:
> >> + /* The "-force-condition" flag gets some special treatment. If this
> >> + token is immediately after the condition string then we treat this as
> >> + part of the condition string rather than a separate token. This is
> >> + just a quirk of how this token used to be parsed, and has been
> >> + retained for backwards compatibility. Maybe this should be updated
> >> + in the future. */
> >
> > (I was the author of the -force-condition patch.)
> > The quirks about the -force-condition flag were not a deliberate feature.
> > For whatever it's worth, I think it's OK to de-prioritize backwards
> > compatibility for this flag. I doubt that it would impact many users.
> >
> > In fact, in a post-merge comment, Pedro and Tom had suggested converting
> > the flag to an option rather than a keyword:
> >
> > https://sourceware.org/pipermail/gdb-patches/2020-October/172952.html
> > https://sourceware.org/pipermail/gdb-patches/2020-December/173802.html
> > https://sourceware.org/pipermail/gdb-patches/2020-December/173999.html
> >
> > I (embarrassingly) did not have the opportunity to get back to it. Maybe
> > it's now the correct time to address the concerns? I can gladly help.
> > What do you think?
>
> I think this sounds like a good idea, but ....
>
> ... I took a quick look at how this might be done,

Thanks for taking a look!

> and the problem I see
> is that the current 'break' option parsing is buried within
> string_to_explicit_location_spec, which is called from
> string_to_location_spec, which is called from multiple commands.
>
> Which makes sense, currently, all of the options for 'break' are about
> specifying the location.
>
> In contrast, -force-condition has nothing to do with the location, but
> relates (as you know) to the condition.
>
> We could handle -force-condition in string_to_explicit_location_spec,
> but then commands like 'edit', which take a location, but not a
> condition would accept -force-condition, and we'd have to have some
> mechanism to the 'force' flag true/false value back out of
> string_to_explicit_location_spec, either passing a container around, or
> storing the 'force' state within the locspec... this all sounds like the
> wrong approach.
>
> Better I think would be to pull the option parsing out of
> string_to_explicit_location_spec, and move it into the individual
> commands. What we'd ideally want is to convert the current bespoke
> option parsing (for location spec options) to use GDB's generic option
> parsing mechanism. It's possible to bind multiple
> gdb::option::option_def into a single gdb::option::option_def_group, so
> we can imagine that the 'break' command would use a common
> gdb::option::option_def from location.h, which defines all the location
> spec arguments, and then a separate gdb::option::option_def which adds
> the -force-condition flag. The only issue then is that we'd need to
> pass the location spec related arguments into
> string_to_explicit_location_spec somehow....
>
> Phew. Now the next problem is that the gdb::option::option_def contains
> a list of gdb::option::*_option_def objects, the precise type of which
> defines how the options are parsed. It we look at how the arguments to
> things like -function are parsed, these get special language specific
> handling, which the current option mechanism doesn't support, so I think
> we'd need to add that, which will mean at least extending the option
> process, or maybe even some reworking of the option handling...
>
> All that is to say that I agree with Pedro that this would be better
> done as an real option ... but I really don't want to tie this work to
> that refactoring if at all possible...

Yes, sure, given the non-trivial nature of that refactoring, this makes sense.

Thanks
-Baris

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928