[v5,0/2] gdb: setting BP with multiple locations only displays one location

  From: "Gerlicher, Klaus" <klaus.gerlicher@intel.com>

Hi Guinevere, all,

I also had another oversight in V4, Linaro tester was showing that
gdb.base/step-over-exit.exp is failing. On my ARM system I don't get two _exit
locations but the Linaro tester seems to have debug symbols installed for libc.

I had two oversights in V3 of this series, so I'm replacing it with V4. I don't
know if this is the right policy, so please let me know if this is typically
handled in another way.

Adding back most of V3 cover letter here which is still valid, for convenience:

Thanks for the V2 review. And sorry for the whitespace issues. I changed my
workflow to now git format-patch/am the patches which would show such issues,
hoping they are now not present.

This patch V2 had been reviewed by Guinevere in August and the new series
V3 here reflects the suggestions for patch V2. It changes the V2
(https://sourceware.org/pipermail/gdb-patches/2024-August/210906.html)
quite a lot, so not all comments from V2 are here. The most important aspect
is this:

> Since you are already changing these tests, we already have a proc
> called gdb_breakpoint(in lib/gdb.exp), which is supposed to more
> generically and thoroughly handle setting breakpoints. I think it would
> be best to change these generic gdb_test to gdb_breakpoint calls, and
> make gdb_breakpoint handle the new multiple breakpoint location. This is
> valid for many changes in this email,

> However, I won't block this patch on that, if it is too difficult or you
> end up needing to change all gdb_breakpoint calls for some reason (I
> wouldn't expect it, but I'm not great with expect or TCL, so I might be
> missing something).

There were some other minor nits that I have addressed or are not relevant
to V3 but I hope it is ok to not refer to them again.

I have decided to split this into two parts:

The 1st patch changes all the current locations where we use gdb_test "break"
and expect multiple locations, to using gdb_breakpoint. gdb_breakpoint is
changed so it will match multiple breakpoint locations properly.

The 2nd patch then adds the new multi-line output for the breakpoint command,
adds the set command for maximum locations printed, and modifies all tests that
are either impacted by the new set command (list more than 10 locations) or
multi-line output.

Please note that by changing to gdb_breakpoint we don't have pass prints in
the logs anymore. If these are required we could change gdb_breakpoint to
output them whenever a "-loc" is specified but I didn't think this was
necessary from a personal perspective.

Guinevere, you also thought that having a way to specify the actual regex to
match could be useful but I didn't find this is needed in any of our cases.

There's one MI specific test where the expected output is still handcoded. I
imagine the regex generated by gdb_multi_loc_regex could be converted to "~" MI
format but I didn't do this here. I'd love to do it in a follow up.

Thanks
Klaus

Gerlicher, Klaus (2):
  gdb: extend gdb_breakpoint for multiple locations
  gdb, breakpoint: output multiple bp locations

 gdb/NEWS                                      |   5 +
 gdb/breakpoint.c                              | 125 ++++++++++++++----
 gdb/doc/gdb.texinfo                           |  45 ++++++-
 gdb/testsuite/gdb.ada/bp_inlined_func.exp     |   4 +-
 gdb/testsuite/gdb.ada/homonym.exp             |   8 +-
 gdb/testsuite/gdb.ada/operator_bp.exp         |  18 +--
 .../gdb.base/condbreak-multi-context.exp      |  22 ++-
 gdb/testsuite/gdb.base/ctxobj.exp             |   4 +-
 gdb/testsuite/gdb.base/dtrace-probe.exp       |   4 +-
 gdb/testsuite/gdb.base/foll-fork.exp          |   7 +-
 gdb/testsuite/gdb.base/msym-bp-shl.exp        |   4 +-
 gdb/testsuite/gdb.base/msym-bp.exp            |   2 +-
 .../run-control-while-bg-execution.exp        |   2 +-
 gdb/testsuite/gdb.base/solib-symbol.exp       |   4 +-
 gdb/testsuite/gdb.base/stap-probe.exp         |   8 +-
 gdb/testsuite/gdb.base/step-over-exit.exp     |   2 +-
 gdb/testsuite/gdb.cp/breakpoint-locs.exp      |   2 +-
 gdb/testsuite/gdb.cp/ena-dis-br-range.exp     |   4 +-
 gdb/testsuite/gdb.cp/mb-ctor.exp              |   8 +-
 gdb/testsuite/gdb.cp/mb-inline.exp            |   8 +-
 gdb/testsuite/gdb.cp/mb-templates.exp         |  12 +-
 gdb/testsuite/gdb.cp/meth-typedefs.exp        |   2 +-
 gdb/testsuite/gdb.cp/ovldbreak.exp            |   8 +-
 gdb/testsuite/gdb.cp/paramless.exp            |   4 +-
 gdb/testsuite/gdb.cp/templates.exp            |  20 ++-
 gdb/testsuite/gdb.dwarf2/dw2-inline-break.exp |  20 ++-
 .../gdb.dwarf2/dw2-skip-prologue.exp          |   2 +-
 gdb/testsuite/gdb.linespec/break-asm-file.exp |   4 +-
 gdb/testsuite/gdb.linespec/cpcompletion.exp   |   2 +
 gdb/testsuite/gdb.linespec/linespec.exp       |  22 +--
 gdb/testsuite/gdb.linespec/multiple-locs.cc   |  41 ++++++
 gdb/testsuite/gdb.linespec/multiple-locs.exp  |  56 ++++++++
 .../mi-breakpoint-multiple-locations.exp      |   4 +-
 .../gdb.mi/user-selected-context-sync.exp     |  16 +--
 .../gdb.multi/bp-thread-specific.exp          |   6 +-
 .../gdb.multi/inferior-specific-bp.exp        |   3 +-
 .../gdb.multi/multi-target-continue.exp       |   3 +-
 .../gdb.multi/multi-target-ping-pong-next.exp |   6 +-
 gdb/testsuite/gdb.opt/inline-break.exp        |  23 ++--
 gdb/testsuite/gdb.python/py-bp-locations.exp  |   2 +-
 gdb/testsuite/gdb.python/py-breakpoint.exp    |   2 +-
 gdb/testsuite/lib/completion-support.exp      |   3 +
 gdb/testsuite/lib/gdb.exp                     | 121 +++++++++++++++--
 43 files changed, 446 insertions(+), 222 deletions(-)
 create mode 100644 gdb/testsuite/gdb.linespec/multiple-locs.cc
 create mode 100644 gdb/testsuite/gdb.linespec/multiple-locs.exp
  

On 9/16/24 5:19 AM, Klaus Gerlicher wrote:
> From: "Gerlicher, Klaus" <klaus.gerlicher@intel.com>
>
> Hi Guinevere, all,
>
> I also had another oversight in V4, Linaro tester was showing that
> gdb.base/step-over-exit.exp is failing. On my ARM system I don't get two _exit
> locations but the Linaro tester seems to have debug symbols installed for libc.
>
> I had two oversights in V3 of this series, so I'm replacing it with V4. I don't
> know if this is the right policy, so please let me know if this is typically
> handled in another way.
>
> Adding back most of V3 cover letter here which is still valid, for convenience:
>
> Thanks for the V2 review. And sorry for the whitespace issues. I changed my
> workflow to now git format-patch/am the patches which would show such issues,
> hoping they are now not present.
>
> This patch V2 had been reviewed by Guinevere in August and the new series
> V3 here reflects the suggestions for patch V2. It changes the V2
> (https://sourceware.org/pipermail/gdb-patches/2024-August/210906.html)
> quite a lot, so not all comments from V2 are here. The most important aspect
> is this:
>
>> Since you are already changing these tests, we already have a proc
>> called gdb_breakpoint(in lib/gdb.exp), which is supposed to more
>> generically and thoroughly handle setting breakpoints. I think it would
>> be best to change these generic gdb_test to gdb_breakpoint calls, and
>> make gdb_breakpoint handle the new multiple breakpoint location. This is
>> valid for many changes in this email,
>> However, I won't block this patch on that, if it is too difficult or you
>> end up needing to change all gdb_breakpoint calls for some reason (I
>> wouldn't expect it, but I'm not great with expect or TCL, so I might be
>> missing something).
> There were some other minor nits that I have addressed or are not relevant
> to V3 but I hope it is ok to not refer to them again.
>
> I have decided to split this into two parts:
>
> The 1st patch changes all the current locations where we use gdb_test "break"
> and expect multiple locations, to using gdb_breakpoint. gdb_breakpoint is
> changed so it will match multiple breakpoint locations properly.
>
> The 2nd patch then adds the new multi-line output for the breakpoint command,
> adds the set command for maximum locations printed, and modifies all tests that
> are either impacted by the new set command (list more than 10 locations) or
> multi-line output.
>
> Please note that by changing to gdb_breakpoint we don't have pass prints in
> the logs anymore. If these are required we could change gdb_breakpoint to
> output them whenever a "-loc" is specified but I didn't think this was
> necessary from a personal perspective.
>
> Guinevere, you also thought that having a way to specify the actual regex to
> match could be useful but I didn't find this is needed in any of our cases.
>
> There's one MI specific test where the expected output is still handcoded. I
> imagine the regex generated by gdb_multi_loc_regex could be converted to "~" MI
> format but I didn't do this here. I'd love to do it in a follow up.

Hi Klaus,

Sorry for the delaying in coming back to this series. I looked over both 
patches and with the exception of a typo (sent as reply to the relevant 
patch), these look ok to me. Reviewed-By: Guinevere Larsen 
<guinevere@redhat.com>

I hope they get approved soon, this is a great change!
  

Thanks Guinevere, I'll address the typo before I push (after I get approval).

Klaus

> -----Original Message-----
> From: Guinevere Larsen <guinevere@redhat.com>
> Sent: Friday, September 20, 2024 9:18 PM
> To: Gerlicher, Klaus <klaus.gerlicher@intel.com>; gdb-
> patches@sourceware.org; blarsen@redhat.com
> Subject: Re: [PATCH v5 0/2] gdb: setting BP with multiple locations only
> displays one location
> 
> On 9/16/24 5:19 AM, Klaus Gerlicher wrote:
> > From: "Gerlicher, Klaus" <klaus.gerlicher@intel.com>
> >
> > Hi Guinevere, all,
> >
> > I also had another oversight in V4, Linaro tester was showing that
> > gdb.base/step-over-exit.exp is failing. On my ARM system I don't get two
> _exit
> > locations but the Linaro tester seems to have debug symbols installed for
> libc.
> >
> > I had two oversights in V3 of this series, so I'm replacing it with V4. I don't
> > know if this is the right policy, so please let me know if this is typically
> > handled in another way.
> >
> > Adding back most of V3 cover letter here which is still valid, for convenience:
> >
> > Thanks for the V2 review. And sorry for the whitespace issues. I changed my
> > workflow to now git format-patch/am the patches which would show such
> issues,
> > hoping they are now not present.
> >
> > This patch V2 had been reviewed by Guinevere in August and the new series
> > V3 here reflects the suggestions for patch V2. It changes the V2
> > (https://sourceware.org/pipermail/gdb-patches/2024-
> August/210906.html)
> > quite a lot, so not all comments from V2 are here. The most important
> aspect
> > is this:
> >
> >> Since you are already changing these tests, we already have a proc
> >> called gdb_breakpoint(in lib/gdb.exp), which is supposed to more
> >> generically and thoroughly handle setting breakpoints. I think it would
> >> be best to change these generic gdb_test to gdb_breakpoint calls, and
> >> make gdb_breakpoint handle the new multiple breakpoint location. This is
> >> valid for many changes in this email,
> >> However, I won't block this patch on that, if it is too difficult or you
> >> end up needing to change all gdb_breakpoint calls for some reason (I
> >> wouldn't expect it, but I'm not great with expect or TCL, so I might be
> >> missing something).
> > There were some other minor nits that I have addressed or are not relevant
> > to V3 but I hope it is ok to not refer to them again.
> >
> > I have decided to split this into two parts:
> >
> > The 1st patch changes all the current locations where we use gdb_test
> "break"
> > and expect multiple locations, to using gdb_breakpoint. gdb_breakpoint is
> > changed so it will match multiple breakpoint locations properly.
> >
> > The 2nd patch then adds the new multi-line output for the breakpoint
> command,
> > adds the set command for maximum locations printed, and modifies all tests
> that
> > are either impacted by the new set command (list more than 10 locations) or
> > multi-line output.
> >
> > Please note that by changing to gdb_breakpoint we don't have pass prints in
> > the logs anymore. If these are required we could change gdb_breakpoint to
> > output them whenever a "-loc" is specified but I didn't think this was
> > necessary from a personal perspective.
> >
> > Guinevere, you also thought that having a way to specify the actual regex to
> > match could be useful but I didn't find this is needed in any of our cases.
> >
> > There's one MI specific test where the expected output is still handcoded. I
> > imagine the regex generated by gdb_multi_loc_regex could be converted to
> "~" MI
> > format but I didn't do this here. I'd love to do it in a follow up.
> 
> Hi Klaus,
> 
> Sorry for the delaying in coming back to this series. I looked over both
> patches and with the exception of a typo (sent as reply to the relevant
> patch), these look ok to me. Reviewed-By: Guinevere Larsen
> <guinevere@redhat.com>
> 
> I hope they get approved soon, this is a great change!
> 
> --
> Cheers,
> Guinevere Larsen
> She/Her/Hers
> 
> >
> > Thanks
> > Klaus
> >
> > Gerlicher, Klaus (2):
> >    gdb: extend gdb_breakpoint for multiple locations
> >    gdb, breakpoint: output multiple bp locations
> >
> >   gdb/NEWS                                      |   5 +
> >   gdb/breakpoint.c                              | 125 ++++++++++++++----
> >   gdb/doc/gdb.texinfo                           |  45 ++++++-
> >   gdb/testsuite/gdb.ada/bp_inlined_func.exp     |   4 +-
> >   gdb/testsuite/gdb.ada/homonym.exp             |   8 +-
> >   gdb/testsuite/gdb.ada/operator_bp.exp         |  18 +--
> >   .../gdb.base/condbreak-multi-context.exp      |  22 ++-
> >   gdb/testsuite/gdb.base/ctxobj.exp             |   4 +-
> >   gdb/testsuite/gdb.base/dtrace-probe.exp       |   4 +-
> >   gdb/testsuite/gdb.base/foll-fork.exp          |   7 +-
> >   gdb/testsuite/gdb.base/msym-bp-shl.exp        |   4 +-
> >   gdb/testsuite/gdb.base/msym-bp.exp            |   2 +-
> >   .../run-control-while-bg-execution.exp        |   2 +-
> >   gdb/testsuite/gdb.base/solib-symbol.exp       |   4 +-
> >   gdb/testsuite/gdb.base/stap-probe.exp         |   8 +-
> >   gdb/testsuite/gdb.base/step-over-exit.exp     |   2 +-
> >   gdb/testsuite/gdb.cp/breakpoint-locs.exp      |   2 +-
> >   gdb/testsuite/gdb.cp/ena-dis-br-range.exp     |   4 +-
> >   gdb/testsuite/gdb.cp/mb-ctor.exp              |   8 +-
> >   gdb/testsuite/gdb.cp/mb-inline.exp            |   8 +-
> >   gdb/testsuite/gdb.cp/mb-templates.exp         |  12 +-
> >   gdb/testsuite/gdb.cp/meth-typedefs.exp        |   2 +-
> >   gdb/testsuite/gdb.cp/ovldbreak.exp            |   8 +-
> >   gdb/testsuite/gdb.cp/paramless.exp            |   4 +-
> >   gdb/testsuite/gdb.cp/templates.exp            |  20 ++-
> >   gdb/testsuite/gdb.dwarf2/dw2-inline-break.exp |  20 ++-
> >   .../gdb.dwarf2/dw2-skip-prologue.exp          |   2 +-
> >   gdb/testsuite/gdb.linespec/break-asm-file.exp |   4 +-
> >   gdb/testsuite/gdb.linespec/cpcompletion.exp   |   2 +
> >   gdb/testsuite/gdb.linespec/linespec.exp       |  22 +--
> >   gdb/testsuite/gdb.linespec/multiple-locs.cc   |  41 ++++++
> >   gdb/testsuite/gdb.linespec/multiple-locs.exp  |  56 ++++++++
> >   .../mi-breakpoint-multiple-locations.exp      |   4 +-
> >   .../gdb.mi/user-selected-context-sync.exp     |  16 +--
> >   .../gdb.multi/bp-thread-specific.exp          |   6 +-
> >   .../gdb.multi/inferior-specific-bp.exp        |   3 +-
> >   .../gdb.multi/multi-target-continue.exp       |   3 +-
> >   .../gdb.multi/multi-target-ping-pong-next.exp |   6 +-
> >   gdb/testsuite/gdb.opt/inline-break.exp        |  23 ++--
> >   gdb/testsuite/gdb.python/py-bp-locations.exp  |   2 +-
> >   gdb/testsuite/gdb.python/py-breakpoint.exp    |   2 +-
> >   gdb/testsuite/lib/completion-support.exp      |   3 +
> >   gdb/testsuite/lib/gdb.exp                     | 121 +++++++++++++++--
> >   43 files changed, 446 insertions(+), 222 deletions(-)
> >   create mode 100644 gdb/testsuite/gdb.linespec/multiple-locs.cc
> >   create mode 100644 gdb/testsuite/gdb.linespec/multiple-locs.exp
> >

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928