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

["Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com>]

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