Re: [RFA v3] enable/disable sub breakpoint range

[Philippe Waroquiers <philippe.waroquiers@skynet.be>]

On Fri, 2017-10-06 at 10:54 +0200, Xavier Roirand wrote:
> Le 10/3/17 à 6:02 PM, Pedro Alves a écrit :
> > We'd do the same to breakpoint commands, i.e., commands that take
> > an breakpoint/location list would xref the description of
> > breakpoint
> > lists.
> > 
> > See commit 5d5658a1d3c3 ("Per-inferior/Inferior-qualified thread
> > IDs")
> > for how that looked like before support for '*' ranges was added.
> > 
> > (And now I wonder whether it'd make sense to model the breakpoint
> > number parsing on a simplified version of the thread ID number
> > parsing.  See gdb/tid-parse.h / tid_range_parser.)
> > 
> > Thanks,
> > Pedro Alves >
> 
> We have several ways for achieving this, especially when taking the 
> C++ization of the code in account. What do you think would be the
> best 
> approach:
> 
> - commit patch I've done as a first step, then propose a new patch 
> including '.*' support ?
> 
> - write a breakpoint location range parser similar to the tid (for 
> thread) one in pure C style including the '.*' support ? In that
> case 
> what about the C++ization ? Would it be done in the future ?
> 
> - Change the patch I've proposed to integrate '.*' support in the
> new 
> C++ style function I wrote ?
> 
> Something else ?
IMO, if there are a number of breakpoint related commands that benefits
from breakpoints range and/or breakpoints location range, a C++ parser
inspired from tid parser looks a good approach.

Today, however, it looks like only the enable/disable commands
are accepting a bp loc.

We might envisage to convert other commands to work on bp loc
rather than on bp, but that looks like a much bigger work.
For example, 'commands' today associates the list of commands to
run with the bp, and not with each bp loc.
Similarly, 'enable once' is on the bp, as the hit count is maintained
per bp, and not per bp loc. 
Same for 'condition', 'delete', 'enable count/delete/once'

So, with the current state of the breakpoints related commands,
IIUC, only enable/disable are ready to work with bp loc.

When another breakpoint related command is converted to work on bp
loc, then a "general bp set/bp range/bp loc/bp loc range' parser would
be interesting.

In the meantime, it looks to me that 'perfect is the enemy of good'
and that the current V3 patch looks good enough for the moment.

My 2 cents ...

Philippe
Note: at my day job, I am a (paying) Adacore customer, and I suggested
to Adacore this enhancement of enable/disable to support bp loc ranges.
So, the above 2 cents opinion that the patch is good enough might be
somewhat biased.