RE: [PATCH 5/6] gdb: add inferior-specific breakpoints and watchpoints

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

On Thursday, January 19, 2023 8:14 PM, Andrew Burgess wrote:
> "Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com> writes:
> 
> > On Monday, November 28, 2022 12:26 PM, Andrew Burgess wrote:
> >> This commit extends the breakpoint mechanism to allow for inferior
> >> specific breakpoints (and watchpoints).
> >>
> >> As GDB gains better support for multiple connections, and so for
> >> running multiple (possibly unrelated) inferiors, then it is not hard
> >
> > Nit: IMHO, removing "then" makes the sentence sound better.
> >
> >> to imagine that a user might wish to create breakpoints that apply to
> >> any thread in a single inferior.  To achieve this currently, the user
> >> would need to create a condition possibly making use of the $_inferior
> >> convenience variable, which, though functional, isn't the most user
> >> friendly.
> >
> > An important difference of an inferior-specific breakpoint wrt using
> > conditions that contain the 'thread' keyword or the '$_inferior' variable
> > could be that the breakpoints would not be inserted at all on other inferiors.
> > For inferiors that have a large number of threads, this could save a
> > substantial amount of overhead of stopping, evaluating the condition, and
> > resuming.  IMHO, it is worth considering this for inferior-specific breakpoints.
> >
> > In a downstream debugger, we had included this feature:
> > https://github.com/intel/gdb/commit/7d87ac91308cd7a8984ba7b0e333a6689790972d
> > (Please see the modifications in 'create_breakpoint').
> 
> Neat.  I wasn't aware Intel had already worked on this feature.
> 
> I had also thought about not inserting breakpoints into non-matching
> inferiors.  In the end I decided to leave that for a follow up patch,
> but it's nice to see that Intel have been doing this for a few years now.

We should&could have submitted the patch to upstream earlier, but this was
delayed due to several factors.  Sorry about that.  This could have saved some
efforts.

Avoiding the insertion of the bp in other inferiors can certainly be done
in a follow-up, IMHO.

> >
> > With this perspective, I also think that allowing the use of both 'thread'
> > and 'inferior' clauses makes sense, because they would have different advantages.
> 
> Except, isn't the thread-id passed to a 'thread' condition a global
> thread-id?  i.e. "break foo thread 1" isn't thread 1 in every inferior,
> it's GDB's global thread 1, which is one thread in one inferior.
>
> So we could (if we implemented it) already limit into which inferiors a
> thread specific breakpoint is inserted by just figuring out which
> inferior that thread is in.

Yes, this would make sense.
 
> I think it makes sense, at least initially, to prevent use of 'thread'
> and 'inferior' together.  If we decide to relax this restriction later,
> then that's no problem.  It's much harder to add more restrictions
> later.

Ok, that's right.

...
> >> +	return 0
> >
> > As I wrote previously, the testsuite does not look consistent
> > about what to return here.  But "-1" makes more sense to me, because
> > not being able to run to main sounds like a major problem.
> 
> The difference in the other patch where you pointed this out is that the
> return as at the top level of the script (I fixed that other case as per
> your suggestion).
> 
> In this case the return is from a function.  We don't actually check the
> return value, but I'd rather leave this return consistent with all the
> other similar return statements, in all the other functions, in this
> test script.

I missed that this was returning from a function.  I see your point and it
makes sense.

> I've made all the other updates you suggested, these will be included in
> a V2 series shortly.

I'll go over the series once more as soon as I can.

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