[PATCH 2/6] Introduce `pre_expanded sals'

[PATCH 2/6] Introduce `pre_expanded sals'

[Sergio Durigan Junior]

Hi,

This patch introduces the `pre_expanded sals'.  Tom, please, if you have
additional comments to make regarding this modification, feel free to
reply to this message.

This was regtested on the compile farm.

Thanks,

Sergio.

---
 gdb/ChangeLog    |    8 ++++++++
 gdb/breakpoint.c |   52 ++++++++++++++++++++++++++++++++++++++++++++--------
 gdb/linespec.h   |    4 ++++
 3 files changed, 56 insertions(+), 8 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index a5d2aa1..f20653b 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2011-04-04  Tom Tromey  <tromey@redhat.com>
+
+	* breakpoint.c (create_breakpoints_sal): Added code to handle
+	pre-expanded sals.
+	(create_breakpoint): Likewise.
+	(breakpoint_re_set_one): Likewise.
+	* linespec.h (struct linespec_result) <pre_expanded>: New field.
+
 2011-04-03  Joel Brobecker  <brobecker@adacore.com>
 
 	GDB 7.3 branch created (branch timestamp: 2011-04-01 01:00 UTC)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 2a25c8d..3927171 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -7629,6 +7629,16 @@ create_breakpoints_sal (struct gdbarch *gdbarch,
 {
   int i;
 
+  if (canonical->pre_expanded)
+    {
+      create_breakpoint_sal (gdbarch, sals, canonical->canonical[0],
+			     cond_string, type, disposition,
+			     thread, task, ignore_count, ops,
+			     from_tty, enabled, internal,
+			     canonical->special_display);
+      return;
+    }
+
   for (i = 0; i < sals.nelts; ++i)
     {
       struct symtabs_and_lines expanded = 
@@ -8154,7 +8164,7 @@ create_breakpoint (struct gdbarch *gdbarch,
         mention (b);
     }
   
-  if (sals.nelts > 1)
+  if (sals.nelts > 1 && !canonical.pre_expanded)
     {
       warning (_("Multiple breakpoints were set.\nUse the "
 		 "\"delete\" command to delete unwanted breakpoints."));
@@ -10952,12 +10962,14 @@ update_breakpoint_locations (struct breakpoint *b,
    On return, FOUND will be 1 if any SaL was found, zero otherwise.  */
 
 static struct symtabs_and_lines
-addr_string_to_sals (struct breakpoint *b, char *addr_string, int *found)
+addr_string_to_sals (struct breakpoint *b, char *addr_string, int *found,
+		     int *pre_expanded)
 {
   char *s;
   int marker_spec, not_found;
   struct symtabs_and_lines sals = {0};
   struct gdb_exception e;
+  int my_pre_expanded = 0;
 
   s = addr_string;
   marker_spec = b->type == bp_static_tracepoint && is_marker_spec (s);
@@ -10976,8 +10988,27 @@ addr_string_to_sals (struct breakpoint *b, char *addr_string, int *found)
 	    error (_("marker %s not found"), b->static_trace_marker_id);
 	}
       else
-	sals = decode_line_1 (&s, 1, (struct symtab *) NULL, 0,
-			      NULL, &not_found);
+	{
+	  struct linespec_result canonical;
+
+	  init_linespec_result (&canonical);
+	  sals = decode_line_1 (&s, 1, (struct symtab *) NULL, 0,
+				&canonical, &not_found);
+
+	  /* We don't need the contents.  */
+	  if (canonical.canonical)
+	    {
+	      int i;
+
+	      for (i = 0; i < sals.nelts; ++i)
+		xfree (canonical.canonical[i]);
+	      xfree (canonical.canonical);
+	    }
+
+	  my_pre_expanded = canonical.pre_expanded;
+	  if (pre_expanded)
+	    *pre_expanded = my_pre_expanded;
+	}
     }
   if (e.reason < 0)
     {
@@ -11010,7 +11041,7 @@ addr_string_to_sals (struct breakpoint *b, char *addr_string, int *found)
 
   if (!not_found)
     {
-      gdb_assert (sals.nelts == 1);
+      gdb_assert (my_pre_expanded || sals.nelts == 1);
 
       resolve_sal_pc (&sals.sals[0]);
       if (b->condition_not_parsed && s && s[0])
@@ -11049,22 +11080,27 @@ re_set_breakpoint (struct breakpoint *b)
   struct symtabs_and_lines expanded = {0};
   struct symtabs_and_lines expanded_end = {0};
   struct cleanup *cleanups = make_cleanup (null_cleanup, NULL);
+  int pre_expanded = 0;
 
   input_radix = b->input_radix;
   save_current_space_and_thread ();
   switch_to_program_space_and_thread (b->pspace);
   set_language (b->language);
 
-  sals = addr_string_to_sals (b, b->addr_string, &found);
+  sals = addr_string_to_sals (b, b->addr_string, &found, &pre_expanded);
   if (found)
     {
       make_cleanup (xfree, sals.sals);
-      expanded = expand_line_sal_maybe (sals.sals[0]);
+      if (pre_expanded)
+	expanded = sals;
+      else
+	expanded = expand_line_sal_maybe (sals.sals[0]);
     }
 
   if (b->addr_string_range_end)
     {
-      sals_end = addr_string_to_sals (b, b->addr_string_range_end, &found);
+      sals_end = addr_string_to_sals (b, b->addr_string_range_end, &found,
+				      NULL);
       if (found)
 	{
 	  make_cleanup (xfree, sals_end.sals);
diff --git a/gdb/linespec.h b/gdb/linespec.h
index d8d2ec9..458235c 100644
--- a/gdb/linespec.h
+++ b/gdb/linespec.h
@@ -30,6 +30,10 @@ struct linespec_result
      display mechanism would do the wrong thing.  */
   int special_display;
 
+  /* If non-zero, the linespec result should be considered to be a
+     "pre-expanded" multi-location linespec.  */
+  int pre_expanded;
+
   /* If non-NULL, an array of canonical names for returned
      symtab_and_line objects.  The array has as many elements as the
      `nelts' field in the symtabs_and_line returned by decode_line_1.

Re: [PATCH 2/6] Introduce `pre_expanded sals'

[Tom Tromey]

>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

Sergio> This patch introduces the `pre_expanded sals'.  Tom, please, if
Sergio> you have additional comments to make regarding this
Sergio> modification, feel free to reply to this message.

This is just a way for decode_line_1 to tell the breakpoint code that
the returned sals has multiple locations but should still create just a
single breakpoint.  We needed this because a given SystemTap probe name
may have multiple locations.

This patch by itself doesn't introduce any behavior changes.

Tom

Re: [PATCH 2/6] Introduce `pre_expanded sals'

[Pedro Alves]

On Wednesday 06 April 2011 21:13:36, Tom Tromey wrote:
> >>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
> 
> Sergio> This patch introduces the `pre_expanded sals'.  Tom, please, if
> Sergio> you have additional comments to make regarding this
> Sergio> modification, feel free to reply to this message.
> 
> This is just a way for decode_line_1 to tell the breakpoint code that
> the returned sals has multiple locations but should still create just a
> single breakpoint.  We needed this because a given SystemTap probe name
> may have multiple locations.

Hmm, doesn't sound right.  Conceptually, breakpoint locations are
multiple expansions of the same source location.  Different source locations
are different breakpoints.  E.g, bp_location doesn't have line
number or source file fields.  From the user's perpective, there's
only a single "point" in the source code for all the multiple locations
for a single breakpoint.

-- 
Pedro Alves

Re: [PATCH 2/6] Introduce `pre_expanded sals'

[Jan Kratochvil]

On Tue, 12 Apr 2011 13:18:08 +0200, Pedro Alves wrote:
> Hmm, doesn't sound right.  Conceptually, breakpoint locations are
> multiple expansions of the same source location.  Different source locations
> are different breakpoints.  E.g, bp_location doesn't have line
> number or source file fields.  From the user's perpective, there's
> only a single "point" in the source code for all the multiple locations
> for a single breakpoint.

What about generalizing this so that breakpoint locations is a set of PCs
matching the breakpoint expression?  Currently I do not understand how to fix:
	KFAIL: gdb.cp/re-set-overloaded.exp: start (GDB internal error) (PRMS: breakpoints/11657)

as currently breakpoint_re_set would need to create/delete user-visible
breakpoint numbers.

With the "new" (for some years) syntax `enable X.Y' ane `disable X.Y' one can
control which breakpoints are active easily.

(gdb) break C::C
Breakpoint 1 at 0x7d1: file ./gdb.cp/re-set-overloaded.cc, line 22.
Breakpoint 2 at 0x7ba: file ./gdb.cp/re-set-overloaded.cc, line 21. (2 locations)
warning: Multiple breakpoints were set.
Use the "delete" command to delete unwanted breakpoints.
(gdb) disable 2.2
(gdb) info breakpoints 
Num     Type           Disp Enb Address            What
1       breakpoint     keep y   0x00000000000007d1 in C::C(int) at ./gdb.cp/re-set-overloaded.cc:22
2       breakpoint     keep y   <MULTIPLE>         
2.1                         y     0x00000000000007ba in C::C() at ./gdb.cp/re-set-overloaded.cc:21
2.2                         n     0x00000000000007c4 in C::C() at ./gdb.cp/re-set-overloaded.cc:21

So that `set multiple-symbols ask' could be removed (probably completely incl.
the `cancel' option).

I find the current two kinds of multiple breakpoints confusing to users (at
least to myself).


Thanks,
Jan

Re: [PATCH 2/6] Introduce `pre_expanded sals'

[Pedro Alves]

On Tuesday 12 April 2011 12:53:08, Jan Kratochvil wrote:
> On Tue, 12 Apr 2011 13:18:08 +0200, Pedro Alves wrote:
> > Hmm, doesn't sound right.  Conceptually, breakpoint locations are
> > multiple expansions of the same source location.  Different source locations
> > are different breakpoints.  E.g, bp_location doesn't have line
> > number or source file fields.  From the user's perpective, there's
> > only a single "point" in the source code for all the multiple locations
> > for a single breakpoint.
> 
> What about generalizing this so that breakpoint locations is a set of PCs
> matching the breakpoint expression?  

I disagree with that generalization.  E.g., next, you'll want that
a break on "C::C" (with C being a C++ class) sets a location on each
contructor overload).  And then breakpoints set in in-charge and
not-in-charge versions of each of those constructors ends up at the
same hierarchycal level under that super breakpoint.  If the user wants
to disable one of the (source level) overloads, he now needs to know
about the multiple locations of that specific overload.  I think you'd
want a "breakpoint group", with 3 levels of hierarchy.  Another argument,
is that frontends and users using them aren't expecting that a single
breakpoint is represented by more than one visual "point", circle next to
the sources, or something like that.  Hitting F8 to toggle a breakpoint's
enablement changing some other location source "point" enablement
in the sources not currently visible seems to break some abstration
to me.  I think such design change needs to consider all these
issues (and be experimented with some frontend).

I strongly suggest not relying on changing this as prerequisite
for stap support.

> Currently I do not understand how to fix:
> 	KFAIL: gdb.cp/re-set-overloaded.exp: start (GDB internal error) (PRMS: breakpoints/11657)
> 
> as currently breakpoint_re_set would need to create/delete user-visible
> breakpoint numbers.

Dunno.  Create new breakpoints or the extra overloads.  Or pick C::C() if it
exists, or any other contructor (or overload),  the first that matches, and
stick to it.  Or do consider generalizing like described above.

> 
> With the "new" (for some years) syntax `enable X.Y' ane `disable X.Y' one can
> control which breakpoints are active easily.
> 
> (gdb) break C::C
> Breakpoint 1 at 0x7d1: file ./gdb.cp/re-set-overloaded.cc, line 22.
> Breakpoint 2 at 0x7ba: file ./gdb.cp/re-set-overloaded.cc, line 21. (2 locations)
> warning: Multiple breakpoints were set.
> Use the "delete" command to delete unwanted breakpoints.
> (gdb) disable 2.2
> (gdb) info breakpoints 
> Num     Type           Disp Enb Address            What
> 1       breakpoint     keep y   0x00000000000007d1 in C::C(int) at ./gdb.cp/re-set-overloaded.cc:22
> 2       breakpoint     keep y   <MULTIPLE>         
> 2.1                         y     0x00000000000007ba in C::C() at ./gdb.cp/re-set-overloaded.cc:21
> 2.2                         n     0x00000000000007c4 in C::C() at ./gdb.cp/re-set-overloaded.cc:21
> 
> So that `set multiple-symbols ask' could be removed (probably completely incl.
> the `cancel' option).
> 
> I find the current two kinds of multiple breakpoints confusing to users (at
> least to myself).
> 
> 
> Thanks,
> Jan
> 

-- 
Pedro Alves

Re: [PATCH 2/6] Introduce `pre_expanded sals'

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> I disagree with that generalization.  E.g., next, you'll want that
Pedro> a break on "C::C" (with C being a C++ class) sets a location on each
Pedro> contructor overload).

Yes.

Pedro> And then breakpoints set in in-charge and
Pedro> not-in-charge versions of each of those constructors ends up at the
Pedro> same hierarchycal level under that super breakpoint.  If the user wants
Pedro> to disable one of the (source level) overloads, he now needs to know
Pedro> about the multiple locations of that specific overload.

Sure, because `break C::C' is ambiguous.

In some particular subset of such cases, gdb recognizes the ambiguity
and presents a menu to the user.  In other cases gdb either does
something random (it picks a location according to a hidden algorithm)
or sets a breakpoint on all locations (the `break file.c:73' inline case
-- but even that seems to involve some mystery, since from what I see it
seems to check for matching enclosing function names).

I don't think this is a very good state of affairs.

Pedro> Another argument, is that frontends and users using them aren't
Pedro> expecting that a single breakpoint is represented by more than
Pedro> one visual "point", circle next to the sources, or something like
Pedro> that.  Hitting F8 to toggle a breakpoint's enablement changing
Pedro> some other location source "point" enablement in the sources not
Pedro> currently visible seems to break some abstration to me.  I think
Pedro> such design change needs to consider all these issues (and be
Pedro> experimented with some frontend).

I think we should not worry excessively about CLI users adapting.

I see what you mean about MI, but in practice I think MI clients
generally set `file:line' breakpoints anyway.

In any case historical arguments don't apply to the SystemTap case,
which is new.  No existing front end will request probe points.

Tom

Re: [PATCH 2/6] Introduce `pre_expanded sals'

[Matt Rice]

On Tue, Apr 12, 2011 at 6:30 AM, Pedro Alves <pedro@codesourcery.com> wrote:
> Another argument,
> is that frontends and users using them aren't expecting that a single
> breakpoint is represented by more than one visual "point", circle next to
> the sources, or something like that.  Hitting F8 to toggle a breakpoint's
> enablement changing some other location source "point" enablement
> in the sources not currently visible seems to break some abstration
> to me.  I think such design change needs to consider all these
> issues (and be experimented with some frontend).

I think this "visual point" metaphor to some extent is already broken,
I more often than one might expect end up setting multiple breakpoints
on the same function
with different conditions and or commands.

could be as simple as a number inside the "visual point", and adding a
right-click 'breakpoint editor', thing to bring up another window.

to me, this seems like something which should be allowed, but not
something to surprise a user with.
thus, this leads me to ask, does this happen because it is the users
intent, or something else.

Re: [PATCH 2/6] Introduce `pre_expanded sals'

[Eli Zaretskii]

> Date: Tue, 12 Apr 2011 15:22:01 -0700
> From: Matt Rice <ratmice@gmail.com>
> Cc: Jan Kratochvil <jan.kratochvil@redhat.com>, gdb-patches@sourceware.org, 	Tom Tromey <tromey@redhat.com>, Sergio Durigan Junior <sergiodj@redhat.com>
> 
> On Tue, Apr 12, 2011 at 6:30 AM, Pedro Alves <pedro@codesourcery.com> wrote:
> > Another argument,
> > is that frontends and users using them aren't expecting that a single
> > breakpoint is represented by more than one visual "point", circle next to
> > the sources, or something like that.  Hitting F8 to toggle a breakpoint's
> > enablement changing some other location source "point" enablement
> > in the sources not currently visible seems to break some abstration
> > to me.  I think such design change needs to consider all these
> > issues (and be experimented with some frontend).
> 
> I think this "visual point" metaphor to some extent is already broken,
> I more often than one might expect end up setting multiple breakpoints
> on the same function
> with different conditions and or commands.

That's different: you are talking about several breakpoints at the
same location, whereas the issue at hand is whether a single
breakpoint could cover multiple source locations.

Re: [PATCH 2/6] Introduce `pre_expanded sals'

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

[ multi-location breakpoint stuff ]
Pedro> I strongly suggest not relying on changing this as prerequisite
Pedro> for stap support.

Yesterday I started wondering if this patch series could go in if
re-expressed as catchpoints.

That is, instead of:

   break probe:arg

we would use:

   catch probe arg

The drawback here is that the linespec approach works automatically with
tracepoints.  We could fix this via a new argument to 'strace', say '-p'
(for "probe").

This approach would circumvent the need for the pre_expanded sals patch.

Tom

Re: [PATCH 2/6] Introduce `pre_expanded sals'

[Sergio Durigan Junior]

Tom Tromey <tromey@redhat.com> writes:

>>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
>
> [ multi-location breakpoint stuff ]
> Pedro> I strongly suggest not relying on changing this as prerequisite
> Pedro> for stap support.
>
> Yesterday I started wondering if this patch series could go in if
> re-expressed as catchpoints.
>
> That is, instead of:
>
>    break probe:arg
>
> we would use:
>
>    catch probe arg

IMHO this is OK.  I would prefer to see this command as a breakpoint
because I have always seen catchpoints as "event-oriented breakpoints",
such as the calling/returning of a syscall, or a fork, or exec.  But
this is my understanding, so...

However, I think that the stap integration is an important feature and
shouldn't be blocked anymore.

> The drawback here is that the linespec approach works automatically with
> tracepoints.  We could fix this via a new argument to 'strace', say '-p'
> (for "probe").

Sounds good.

Regards,

Sergio.

Re: [PATCH 2/6] Introduce `pre_expanded sals'

[Tom Tromey]

>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

Tom> Yesterday I started wondering if this patch series could go in if
Tom> re-expressed as catchpoints.

Sergio> IMHO this is OK.  I would prefer to see this command as a breakpoint
Sergio> because I have always seen catchpoints as "event-oriented breakpoints",
Sergio> such as the calling/returning of a syscall, or a fork, or exec.

Yeah, I think this distinction generally makes sense.

However, I thought of one other reason we might prefer a catchpoint: if
we add "objfile:"-style linespecs ("break libc.so:malloc"), then we are
going to run into trouble if anybody tries to debug a program named
"probe" -- because "break probe:spec" is handled pretty early in
linespec.

Let me know what you think.  In the absence of comments I am going to
implement this.

Tom

Re: [PATCH 2/6] Introduce `pre_expanded sals'

[Sergio Durigan Junior]

Tom Tromey <tromey@redhat.com> writes:

>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
> Tom> Yesterday I started wondering if this patch series could go in if
> Tom> re-expressed as catchpoints.
>
> Sergio> IMHO this is OK.  I would prefer to see this command as a breakpoint
> Sergio> because I have always seen catchpoints as "event-oriented breakpoints",
> Sergio> such as the calling/returning of a syscall, or a fork, or exec.
>
> However, I thought of one other reason we might prefer a catchpoint: if
> we add "objfile:"-style linespecs ("break libc.so:malloc"), then we are
> going to run into trouble if anybody tries to debug a program named
> "probe" -- because "break probe:spec" is handled pretty early in
> linespec.

All right, I see what you mean.  Personally, I think that if this
behavior happens, then it means we should probably fix linespec in order
to evaluate the `probe:' part earlier.

Anyway, as I said in my earlier reply, I think that holding the stap
patch because of this change in particular is something we may not want
(I certainly don't), so for me this is the strongest argument for why we
could change it from breakpoint to catchpoint.

> Let me know what you think.  In the absence of comments I am going to
> implement this.

As I said in the beginning, I'm OK with that change.  But obviously I'm
not a maintainer, and I'm also an interested part in this being accepted
:-).

Re: [PATCH 2/6] Introduce `pre_expanded sals'

[Tom Tromey]

Sergio> All right, I see what you mean.  Personally, I think that if this
Sergio> behavior happens, then it means we should probably fix linespec in order
Sergio> to evaluate the `probe:' part earlier.

I hadn't thought of that.  It seems insufficient to me, though.  Suppose
that "break probe:something" matches both a probe named "something" and
a function in the executable "probe".  In this case, the breakpoint will
have to match both locations (due to the spec I'm implementing), but in
a way the locations would have very different meanings.

Sergio> As I said in the beginning, I'm OK with that change.  But obviously I'm
Sergio> not a maintainer, and I'm also an interested part in this being accepted
Sergio> :-).

I'm going to work on it then.

thanks,
Tom

Re: [PATCH 2/6] Introduce `pre_expanded sals'

[Sergio Durigan Junior]

Tom Tromey <tromey@redhat.com> writes:

> Sergio> All right, I see what you mean.  Personally, I think that if this
> Sergio> behavior happens, then it means we should probably fix linespec in order
> Sergio> to evaluate the `probe:' part earlier.
>
> I hadn't thought of that.  It seems insufficient to me, though.  Suppose
> that "break probe:something" matches both a probe named "something" and
> a function in the executable "probe".  In this case, the breakpoint will
> have to match both locations (due to the spec I'm implementing), but in
> a way the locations would have very different meanings.

Hm, ok.  I was thinking superficially about the problem, and assuming
that we would only accept the `probe:' for stap probes.  Anyway, you're
right, it would be much more difficult to handle this case.

> Sergio> As I said in the beginning, I'm OK with that change.  But obviously I'm
> Sergio> not a maintainer, and I'm also an interested part in this being accepted
> Sergio> :-).
>
> I'm going to work on it then.

Thanks a lot!

Re: [PATCH 2/6] Introduce `pre_expanded sals'

[Tom Tromey]

Tom> Yesterday I started wondering if this patch series could go in if
Tom> re-expressed as catchpoints.

I looked into this quite a bit and I think it won't work out.

It works out fine for plain catchpoints, but it still falls down if we
want to set tracepoints at SystemTap probes.  Doing this means we get
into the same issues with multiple locations that we were trying to
avoid by moving to catchpoints.

Now I am considering a third approach, namely rebasing the SystemTap
work on the ambiguous linespec proposal that I'm implementing (this will
solve the multiple location problem), and then using option-like syntax
for probes, like "break -p probe".

Tom

Re: [PATCH 2/6] Introduce `pre_expanded sals'

[Jerome Guitton]

Jan Kratochvil (jan.kratochvil@redhat.com):

> With the "new" (for some years) syntax `enable X.Y' ane `disable X.Y' one can
> control which breakpoints are active easily.
> 
> (gdb) break C::C
> Breakpoint 1 at 0x7d1: file ./gdb.cp/re-set-overloaded.cc, line 22.
> Breakpoint 2 at 0x7ba: file ./gdb.cp/re-set-overloaded.cc, line 21. (2 locations)
> warning: Multiple breakpoints were set.
> Use the "delete" command to delete unwanted breakpoints.
> (gdb) disable 2.2
> (gdb) info breakpoints 
> Num     Type           Disp Enb Address            What
> 1       breakpoint     keep y   0x00000000000007d1 in C::C(int) at ./gdb.cp/re-set-overloaded.cc:22
> 2       breakpoint     keep y   <MULTIPLE>         
> 2.1                         y     0x00000000000007ba in C::C() at ./gdb.cp/re-set-overloaded.cc:21
> 2.2                         n     0x00000000000007c4 in C::C() at ./gdb.cp/re-set-overloaded.cc:21
> 
> So that `set multiple-symbols ask' could be removed (probably
> completely incl.  the `cancel' option).


On the contrary, I was wondering if a proper use of the 'ask/all' option
was not precisely the solution here. 'all' would mean: break on any
location that matches the linespec given by the user. If a dll is loaded
that have a location that matches this linespec, then add a new location
for this breakpoint.

On 'ask', you would be asked only for instances that have a different
canonical location, e.g.

(break) C::C
[0] cancel
[1] all
[2] C::C() at ./gdb.cp/re-set-overloaded.cc:21
[3] C::C(int) at ./gdb.cp/re-set-overloaded.cc:22
> 

Choosing 'all' would trigger a multi-location breakpoint and behave
like above.  Selecting some would generate different single-location
breakpoints on the canonical location.

We may even have a command to edit an "all" breakpoint and convert it
into a list of selected single-location breakpoints. It may be easier
to manage than 3-tiers.

(break) breakpoint-edit 1
[0] cancel
[1] all
[2] C::C() at ./gdb.cp/re-set-overloaded.cc:21
[3] C::C(int) at ./gdb.cp/re-set-overloaded.cc:22
> 

Now Tom's question remains: what happens if a unambiguous breakpoint
location becomes ambiguous at dll load? I would suppose that the
appropriate behavior would depend on the 'all/ask' setting: 'all'
means all, so new locations are added, the breakpoints, becomes a
multi-location breakpoint. 'ask' is the main problem. the appropriate
behavior would certainly be to ask the user if the breakpoint should
become 'all' or should be canonicalized. The second option could be a
problem, I suppose. A compromise would be to convert inconditionally to 'all'
with a warning, letting the user edit the breakpoint latter to convert it
into selected single-location breakpoints if he/she wants.


> I find the current two kinds of multiple breakpoints confusing to users (at
> least to myself).

I can only agree. From a user point of view, I feel that homonyms, generics
and inlining should be handled the same way; the distinction that I
need is the 'all/ask' one: either I break on all locations of a
breakpoint (homonyms, generic, inlining, whatever), and add any new
location that a dll load would bring; or I want to select some of them
(presumably one) and have breakpoints on these ones only, and never
add new breakpoints if new occurences are brought by a dll.

Re: [PATCH 2/6] Introduce `pre_expanded sals'

[Joel Brobecker]

I also wanted to say that, as it turns out, Jerome is here in Vancouver
for the week with the goal of adding handling for homonyms in GDB.
So the timing for this discussion could hardly be any better. The
downside, if we want to be able to take better advantage of his trip
is that a quick decision would be helpful so we can get started on
sending out implementation proposals.

I also wanted to give a summary of our discussions on this topic
that occured yesterday, after we read all the threads. We went
back and forth between Pedro's proposal and Tom's. We were trying
to think from the user's perspective, and with a focus on C++ and
Ada (we don't understand stap all that well, but hopefully it will
also apply there).

Right now, there are several situations that can lead to multiple
breakpoint addresses: Generics/templates, homonyms (functions with
the same name), and inlining. In Ada, we've traditionally treated
generics & homonyms differently (multiple breakpoints created) from
inlining (single breakpoint, multiple locations). But does it have
to be that way?

For homonyms, we feel that the typical case is that the user is
usually interested in debugging on specific function. If there
is a bunch of functions called "increment" but only one shows
symptoms of a bug, that's the one that he really wants to break
in.

On the other hand, are generics and templates all that different
from inlining? Thinking about typical debugging scenarios, though,
we think it's common for someone to debug a specific instantiation of
a generic, because it knows that this instantiation shows a bug, while
the others are not. So it's important to be able to specify that
a breakpoint applies to one specific instance, which means that
we're leaning towards handling generics/templates the same as
we'd handle homonyms.

Pedro's idea of federating these SLOC-based breakpoints inside
a 3rd-tier linespec-based breakpoint would probably work. But,
as Jerome and I kept discussing, we felt that maybe it wasn't
necessary. In terms of Ada itself, we've been able to work without
them. Perhaps C++ usage is sufficiently different that the concept
would be more interesting there?

But do we need the 3rd-tier consolidator that Pedro suggests?
We felt that the federation might come from the fact that we wanted GDB
to be smart enough to create new breakpoint locations when re-evaluating
breakpoints, for instance if a shared library got mapped and caused new
locations to match. That's where Jerome's suggestion of treating "all"
breakpoints as dynamic (vs treating specific-instance breakpoints as
static) comes into play.  If the user uses an umbiguous linespec, and
then selects "all" (either automatically via multiple-symbol, or
manually via the multiple-choice menu), then create a dynamic version of
the breakpoint, which will constantly re-evaluate its locations.
Otherwise, create a series of static breakpoints, whose source location
are well enough defined to be unique.

There is a form of canonical linespec that we have been using at
AdaCore, which we think could be used universally:

    FILE:FUNCTION:LINE

The FUNCTION name is the fully qualified name. For Ada, it means
for instance:

        foo.adb:pck.gen.func:8

In C++ (bare in mind that we don't know much about C++ debugging),
it could be something like this:

        foo.cc:GetMax<int>(int):8

But now, Jerome just whispered in my ears that using a multiple-location
breakpoint for homonyms would not work well in the following case:
Imagine ther user is trying to put a condition on a breakpoint
that has locations in units of different languages. What language
would we use? Right now, he says that, for lang auto, we do the nice
thing of using the unit's language to evaluate the condition.

So, in the end, it seems difficult to imagine that one single
breakpoint could have multiple slocs. And it also seems that
the suggestion of treating "all" breakpoints as multiple-location
breakpoints would suffer from the same problem.

So, perhaps the 3rd-tier linespec-based breakpoint might make sense,
after all... If we were to go that route, I'm just worried about
the potential for confusing the user, with 3 levels of breakpoints,
and I'm wondering if this could be helped... Perhaps we could hide
the intermediate level, and just show the top and bottom levels to
the user (I'm thinking of breakpoint notifications as well as the
output of "info break").

In terms of implementation, if we were to decide on a 3rd-tier
linespec-based breakpoint, I'd rather have it all the time, even
for non-ambiguous breakpoints (which might become ambiguous later,
as Tom points out). Even if a bit wasteful, I think it's easier to
deal with, rather than having a polymorphic list of breakpoints...

I still maintain, however, that it's important to be able to break
on a selected subset of locations via the multiple-choice menu. We've
had some users who immediately asked about this as soon as we switched
the default to breaking on all matching locations.

-- 
Joel

Re: [PATCH 2/6] Introduce `pre_expanded sals'

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Tom> This is just a way for decode_line_1 to tell the breakpoint code that
Tom> the returned sals has multiple locations but should still create just a
Tom> single breakpoint.  We needed this because a given SystemTap probe name
Tom> may have multiple locations.

Pedro> Hmm, doesn't sound right.  Conceptually, breakpoint locations are
Pedro> multiple expansions of the same source location.  Different
Pedro> source locations are different breakpoints.  E.g, bp_location
Pedro> doesn't have line number or source file fields.  From the user's
Pedro> perpective, there's only a single "point" in the source code for
Pedro> all the multiple locations for a single breakpoint.

Could you explain why this is important?  I agree this is how it is, but
I think it is actually somewhat confusing at times.


The problem I see with respect to the SystemTap change is that a given
probe location does not have a canonical name.

E.g., suppose you have a probe `program:name' that is invoked at 2
different spots in the source.  Suppose further that `break
probe:program:name' sets 2 breakpoints.  Now consider 2 scenarios:
First, the developer deletes a probe point.  Second, the developer adds
a probe point.

With the single-breakpoint approach, both of these do (what I consider
to be) the right thing -- the user ends up with what he asked for.  I
don't see how they can work with the multiple-breakpoint approach.


Furthermore, I think the `probe:' prefix should let us lift this
restriction anyhow.  It is a way of saying "this is not an ordinary
source location, but something else".


I think the deeper confusion in gdb is that an ambiguous name sets a
single breakpoint, with a single location, but the meaning of this name
is decided arbitrarily (say, by psymtab expansion order).

That is, `break file.c:73' sets a breakpoint in the first `file.c' we
happen to trip across.  Or, `break function' sets a breakpoint in the
first `function'.

I'd rather change gdb to set a breakpoint at all matching locations, and
let the user disambiguate if that is really what he wanted.

I hit this while debugging gdb itself from time to time -- try `break
parse_number' and guess where it gets set.

Tom

Re: [PATCH 2/6] Introduce `pre_expanded sals'

[Eli Zaretskii]

> From: Tom Tromey <tromey@redhat.com>
> Cc: gdb-patches@sourceware.org, Sergio Durigan Junior <sergiodj@redhat.com>
> Date: Tue, 12 Apr 2011 14:26:32 -0600
> 
> Furthermore, I think the `probe:' prefix should let us lift this
> restriction anyhow.  It is a way of saying "this is not an ordinary
> source location, but something else".

If so, perhaps we shouldn't consider them locations at all, but rather
symbolic names of a set of locations.

> I'd rather change gdb to set a breakpoint at all matching locations, and
> let the user disambiguate if that is really what he wanted.

How would she disambiguate that?

Anyway, these issues were discussed at length in the past (with your
suggestion being on the table, as well as others), so perhaps we all
should re-read that before reiterating the same arguments again.

Re: [PATCH 2/6] Introduce `pre_expanded sals'

[Tom Tromey]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

>> I'd rather change gdb to set a breakpoint at all matching locations, and
>> let the user disambiguate if that is really what he wanted.

Eli> How would she disambiguate that?

Use a full path.  Or we can add more ways.  There are some bugs open
about this.

Eli> Anyway, these issues were discussed at length in the past (with your
Eli> suggestion being on the table, as well as others), so perhaps we all
Eli> should re-read that before reiterating the same arguments again.

Do you have a link?

Tom

Re: [PATCH 2/6] Introduce `pre_expanded sals'

[Eli Zaretskii]

> From: Tom Tromey <tromey@redhat.com>
> Cc: pedro@codesourcery.com, gdb-patches@sourceware.org, sergiodj@redhat.com
> Date: Wed, 13 Apr 2011 13:20:34 -0600
> 
> Eli> Anyway, these issues were discussed at length in the past (with your
> Eli> suggestion being on the table, as well as others), so perhaps we all
> Eli> should re-read that before reiterating the same arguments again.
> 
> Do you have a link?

Sorry for the delay, I needed to look them up.  Here's what I found:

  http://sourceware.org/ml/gdb-patches/2003-10/msg00213.html
  http://sourceware.org/ml/gdb/2004-07/msg00161.html
  http://sourceware.org/ml/gdb/2004-09/msg00101.html
  http://sourceware.org/ml/gdb/2006-01/msg00102.html
  http://sourceware.org/ml/gdb/2006-01/msg00117.html

Since Daniel was an active participant, perhaps he could point to
other discussions if I missed them.

Re: [PATCH 2/6] Introduce `pre_expanded sals'

[Pedro Alves]

On Tuesday 12 April 2011 21:26:32, Tom Tromey wrote:
> >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
> 
> Tom> This is just a way for decode_line_1 to tell the breakpoint code that
> Tom> the returned sals has multiple locations but should still create just a
> Tom> single breakpoint.  We needed this because a given SystemTap probe name
> Tom> may have multiple locations.
> 
> Pedro> Hmm, doesn't sound right.  Conceptually, breakpoint locations are
> Pedro> multiple expansions of the same source location.  Different
> Pedro> source locations are different breakpoints.  E.g, bp_location
> Pedro> doesn't have line number or source file fields.  From the user's
> Pedro> perpective, there's only a single "point" in the source code for
> Pedro> all the multiple locations for a single breakpoint.
> 
> Could you explain why this is important?  I agree this is how it is, but
> I think it is actually somewhat confusing at times.

The main problem (ignoring that frontends aren't prepared to have a 
breakpoint map to more than one source file:line) is that if you allowed
multiple locations (addresses) at different source lines under the same
breakpoint, with the same hierarchy we have today, 

Consider the in-charge, and not-in-charge versions of a constructor
(, or multiple expansions of an inline function, or instantiations
of a template function (about the same thing)).

A::A()
{
}

A::A(int overload)
{
}


Now, do "break A::A".  The user only really cares about source
locations, so he expects two locations, one for each overload.
But, if you allow locations with different line numbers under
the same breakpoint, you'd get something like:

 (gdb) info breakpoints 
 Num     Type           Disp Enb Address            What
 1       breakpoint     keep y   <MULTIPLE>         
 1.1                         y     0x00000000004005d2 in A::A() at foo.cc:33
 1.2                         y     0x00000000004005ec in A::A() at foo.cc:33
 1.3                         y     0x00000000004005d2 in A::A(int) at foo.cc:38
 1.4                         y     0x00000000004005ec in A::A(int) at foo.cc:38


Now, say I didn't want to A::A(int) overload to break afterall.  I now
need to disable two separate locations.  If you think this isn't bad,
consider inline functions, and overloads.

void
inline_func()
{
}

void
inline_func(int overload)
{
}

void
inline_func(bar overload)
{
}

You do "break inline_func".  There are two overloads, so the
user (well, I would) naturaly want to see one entry per
overload under the breakpoint hierarchy, but instead,
since the functions were inlined 200 times each, he gets:

 (gdb) info breakpoints 
 Num     Type           Disp Enb Address            What
 1       breakpoint     keep y   <MULTIPLE>         
 1.1                         y     0x00000000004005d2 in inline_func() at blah
 1.2                         y     0x00000000004005ec in inline_func(int) at blah
 1.1                         y     0x00000000004005d2 in inline_func(bar) at blah
 1.2                         y     0x00000000004005ec in inline_func(int) at blah
 1.3                         y     0x00000000004005ec in inline_func(int) at blah
 1.4                         y     0x00000000004005ec in inline_func(int) at blah
 1.5                         y     0x00000000004005ec in inline_func(int) at blah
 ... 
 1.100                         y     0x00000000004005ec in inline_func(int) at blah
 1.101                         y     0x00000000004005ec in inline_func(int) at blah
 1.102                         y     0x00000000004005ec in inline_func(int) at blah
 1.103                         y     0x00000000004005ec in inline_func(int) at blah
 ...

you get the point.  If a shared library is loaded meanwhile that used
the inline function, you'll end up with even more locations.  But,
good luck if you now decide that you want to the disable only the
inline_func(bar) locations.

So you see, "locations" represent the expansions of a given source
line in GDB.  If you go follow Eli's links, you will see that there
were a lot of different names proposed for the "locations" concept,
and they all resolved around things like "physical addresses".
GDB as is could even hide the multiple locations from users, and
most wouldn't care.  The multiple locations show through more
of the machine/binary/compiled code than most users care about.

This is why I proposed in the response to Jan, that if we want
to group different "_source_ locations" under the same breakpoint,
we implement it as a 3rd hierarchy, so you'd get:

 (gdb) info breakpoints 
 Num     Type           Disp Enb Address            What
 1       breakpoint     keep y   <MULTIPLE>         
 1.1                         y     <MULTIPLE>
 1.1.1                         y     0x00000000004005d2 in inline_func() at blah
 1.1.2                         y     0x0000000000XXXXXX in inline_func() at blah
 1.1.N                         y     0x0000000000YYYYYY in inline_func() at blah

 1.2                         y     <MULTIPLE>
 1.2.1                         y     0x00000000004005ec in inline_func(int) at blah
 1.2.2                         y     0x0000000000XXXXXX in inline_func(int) at blah
 1.2.N                         y     0x0000000000YYYYYY in inline_func(int) at blah

 1.3                         y     <MULTIPLE>
 1.3.1                         y     0x0000000000400781 in inline_func(bar) at blah
 1.3.2                         y     0x0000000000XXXXXX in inline_func(bar) at blah
 1.3.N                         y     0x0000000000YYYYY in inline_func(bar) at blah

etc.  (could probably be made prettier.)

This way, if the user wants to disable inline_func(bar), he can do disable "1.3".

> The problem I see with respect to the SystemTap change is that a given
> probe location does not have a canonical name.
> 
> E.g., suppose you have a probe `program:name' that is invoked at 2
> different spots in the source.  Suppose further that `break
> probe:program:name' sets 2 breakpoints.  Now consider 2 scenarios:
> First, the developer deletes a probe point.  Second, the developer adds
> a probe point.

(3rd, developer notices a typo, and renames the probe at a given
line in the program.)

> With the single-breakpoint approach, both of these do (what I consider
> to be) the right thing -- the user ends up with what he asked for.  I
> don't see how they can work with the multiple-breakpoint approach.

You still have manifestations of the same problem.  Location numbers are
supposed to be stable.  For example, users can enable/disable locations
individually.  Say, you have:

 (gdb) info breakpoints 
 Num     Type           Disp Enb Address            What
 1       breakpoint     keep y   <MULTIPLE>         
 1.1                         y     "program:name" in foo_func() at blah.c:12
 1.2                         n     "program:name" in bar_func() at blah.c:20
 1.3                         y     "program:name" in qux_func() at blah.c:45

If the program changes, say the 1.2 location disappears, you've now
confused GDB --- how will it know that 1.3 is supposed to be enabled,
given that it is now the second location?  (multiple variations of
this issue).

LTTng/UST also has the same issue (marker names are not required to
be unique, though it's advised they are), and we try to heuristically
cope with it as best as possible.  This is
decode_static_tracepoint_spec returning a list of sals
matching the marker spec, the static_trace_marker_id_idx
field in the breakpoint structure.

	      /* Given that its possible to have multiple markers with
		 the same string id, if the user is creating a static
		 tracepoint by marker id ("strace -m MARKER_ID"), then
		 store the sals index, so that breakpoint_re_set can
		 try to match up which of the newly found markers
		 corresponds to this one  */
	      tp->static_trace_marker_id_idx = i;

and see also all the resynching update_static_tracepoint tries
to do.

> I think the deeper confusion in gdb is that an ambiguous name sets a
> single breakpoint, with a single location, but the meaning of this name
> is decided arbitrarily (say, by psymtab expansion order).
> 
> That is, `break file.c:73' sets a breakpoint in the first `file.c' we
> happen to trip across.  Or, `break function' sets a breakpoint in the
> first `function'.
> 
> I'd rather change gdb to set a breakpoint at all matching locations, and
> let the user disambiguate if that is really what he wanted.

See, "bp_location" concerns with address locations while the user
the user is concerned about "source locations".  I think that
if "bp_location" had been named "bp_compiled_address", we wouldn't
be having this discussion.  :-)

> 
> I hit this while debugging gdb itself from time to time -- try `break
> parse_number' and guess where it gets set.

It's not necessarily a bad thing (and not necessarily a good thing
either).   If you have a big project, with a bunch of static
"foo" functions, it's not unreasonable to have "b foo"
break at the "closest" function with that name in your context.

What would "list parse_number" do if "break parse_number" breaks
in all the instances of that static function?

-- 
Pedro Alves

Re: [PATCH 2/6] Introduce `pre_expanded sals'

[Jan Kratochvil]

On Mon, 18 Apr 2011 20:38:28 +0200, Pedro Alves wrote:
> (ignoring that frontends aren't prepared to have a 
> breakpoint map to more than one source file:line)

I do not address this part in this mail, I do not know much about it.


> Now, do "break A::A".
[...]
>  (gdb) info breakpoints 
>  Num     Type           Disp Enb Address            What
>  1       breakpoint     keep y   <MULTIPLE>         
>  1.1                         y     0x00000000004005d2 in A::A() at foo.cc:33
>  1.2                         y     0x00000000004005ec in A::A() at foo.cc:33
>  1.3                         y     0x00000000004005d2 in A::A(int) at foo.cc:38
>  1.4                         y     0x00000000004005ec in A::A(int) at foo.cc:38
> 
> Now, say I didn't want to A::A(int) overload to break afterall.

If I want to deal with `A::A()' and `A::A(int)' separately I will never type
`break A::A' in the first place but create `break A::A()' and `break A::A(int)'
as two separate breakpoints (or just one of them).


> This is why I proposed in the response to Jan, that if we want
> to group different "_source_ locations" under the same breakpoint,
> we implement it as a 3rd hierarchy, so you'd get:

I agree 3-level hierarchy is the best but I do not think it is practically
worth it, it again creates a feature no normal user is going to use.


> You do "break inline_func".  There are two overloads, so the

While offtopic in general breakpoint on inlined functions do not yet work:
	[Bug breakpoints/10738] Cannot set breakpoint on inlined function


>  (gdb) info breakpoints 
>  Num     Type           Disp Enb Address            What
>  1       breakpoint     keep y   <MULTIPLE>         
>  1.1                         y     "program:name" in foo_func() at blah.c:12
>  1.2                         n     "program:name" in bar_func() at blah.c:20
>  1.3                         y     "program:name" in qux_func() at blah.c:45
> 
> If the program changes, say the 1.2 location disappears, you've now
> confused GDB --- how will it know that 1.3 is supposed to be enabled,
> given that it is now the second location?  (multiple variations of
> this issue).

Besides unspecifically broken reread_symbols during program change normally
update_breakpoint_locations on any nontrivial C++ code finds
HAVE_AMBIGUOUS_NAMES true, uses breakpoint_locations_match and as if the
program really has changed any way the addresses will not match and specific
bp_location enabled-flag will get reset back to enabled anyway.

It also happens with any PIE program but that is more my fault (as its fix in
Fedora is too ugly to be even submitted).


> > I hit this while debugging gdb itself from time to time -- try `break
> > parse_number' and guess where it gets set.
> 
> It's not necessarily a bad thing (and not necessarily a good thing
> either).   If you have a big project, with a bunch of static
> "foo" functions, it's not unreasonable to have "b foo"
> break at the "closest" function with that name in your context.

The problem with GDB is IMO that "your context" is very random, user may not
be much aware there is any context, with the debuggee stopping here and there.


> What would "list parse_number" do if "break parse_number" breaks
> in all the instances of that static function?

A good point, it should error, it needs more specific input.


Thanks,
Jan

Re: [PATCH 2/6] Introduce `pre_expanded sals'

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

[...]
Pedro> This is why I proposed in the response to Jan, that if we want
Pedro> to group different "_source_ locations" under the same breakpoint,
Pedro> we implement it as a 3rd hierarchy, so you'd get:

Ok, I think I mostly understand this proposal.

I would like to work through this and come to some agreement, since I am
planning to implement the result -- not just for SystemTap probes but
also because I recently starting looking at an ambiguous name feature
request we've had open in RH bugzilla for far too long.

My current understanding is that an ambiguous linespec should cause
`break' to make a new "3rd-tier" breakpoint with sub-breakpoints for
each separate source location.  These sub-breakpoints would have
canonicalized linespecs for better stable naming.  Furthermore they
would be created and destroyed automatically in response to inferior
changes.

FWIW, I tend to think the 3-tier solution is too complicated.  I think
having two ways that a breakpoint can have multiple locations will
confuse users.

Also, I came up with some cases I don't understand; I feel certain there
are more lurking.

Suppose the user does `break function' where `function' is *not*
ambiguous.  This makes an ordinary breakpoint.  Now suppose the inferior
calls dlopen and the new library has a definition of `function'.  What
happens?

1. Breakpoint changes from ordinary to 3-tier.
2. We add logic to canonicalize the existing breakpoint's linespec
   and do not set a new breakpoint.
3. We didn't make an ordinary breakpoint after all, all breakpoints are
   3-tier.

Choice 1 seems potentially weird, though maybe it would work ok?

Choice 2 seems bad in some circumstances.  `function' might be inlined
in some places, in which case you would actively want new breakpoint
locations to be set.

Choice 3 introduces complexity for all uses even though most breakpoints
aren't in fact ambiguous.


Here is an oddity: consider `break probe:something'.  Now suppose this
probe was used in multiple locations -- one having debuginfo and two not
having debuginfo.

I think this implies that a 3-tier breakpoint must have one special
sub-breakpoint that collects locations from debuginfo-less matches.  Or
perhaps each could be given its own sub-breakpoint -- but there is no
sensible way to provide canonical linespecs here, so matching won't
really work.


Suppose the inferior uses dlmopen and loads the same library twice.  Now
there is really no way to canonicalize a linespec -- it can't even be
objfile-relative (postulate for a moment that there is a syntax for
this).



I think most of these cases are readily resolveable with the "break at
all matching locations" approach:

* Unambiguous -> ambiguous transition is no problem.
  Breakpoint condition may no longer parse -- not super, but workable.

  (As an aside: seeing those "can't parse" messages has bugged me in the
  past, it seems like it would be good to have a way to tell GDB "if you
  can't parse it, please stop right away so I can fix the darn thing".)

* The no-debuginfo SystemTap probe case: no problem in itself, and we
  could simply mandate that such a breakpoint can only have certain
  expressions attached (also mentioned below).

* dlmopen.  Also no problem, the user asked for it :)


One advantage of 3-tier is that it does let you put different conditions
on different sub-breakpoints.  (But the general idea of the simpler
approach is that you should disambiguate if you want tight control.
Which gives me the idea that we could make a new break command that does
just does automatically.)


I think either case will need some expansion of the matching heuristic
for breakpoints, and maybe some linespec additions to make it possible
to disambiguate more cases.


I am not very concerned about the MI problem.  We can emit a new field
holding a vector of file:line locations, or something like that.  Front
ends will adapt.

[...]
Pedro> You still have manifestations of the same problem.  Location numbers are
Pedro> supposed to be stable.  For example, users can enable/disable locations
Pedro> individually.

This problem is fundamental, just because there is no stable name for
source locations in the face of changes to the source.  We are always
going to have heuristics in GDB, and some losing cases.

Pedro> See, "bp_location" concerns with address locations while the user
Pedro> the user is concerned about "source locations".  I think that
Pedro> if "bp_location" had been named "bp_compiled_address", we wouldn't
Pedro> be having this discussion.  :-)

For the SystemTap probe case in particular, I am not so sure.

I mean, I can see how it would be useful to an MI consumer if GDB
emitted file:line locations for all the locations hit by a "probe:"
breakpoint.  This is not fundamental to one approach or the other, it
can be made to work.

I think it would be a reasonable choice on our part to simply mandate
that a SystemTap probe breakpoint have a single condition (for all
locations) which could only refer to globals and probe arguments.

Pedro> It's not necessarily a bad thing (and not necessarily a good thing
Pedro> either).   If you have a big project, with a bunch of static
Pedro> "foo" functions, it's not unreasonable to have "b foo"
Pedro> break at the "closest" function with that name in your context.

Sure, but that isn't what actually happens.  And, this would require
separate thinking.  Like, suppose the "closest" metric yields an
ambiguous result, what should happen?

Pedro> What would "list parse_number" do if "break parse_number" breaks
Pedro> in all the instances of that static function?

Error or give a menu.

Tom

Re: [PATCH 2/6] Introduce `pre_expanded sals'

[Jan Kratochvil]

On Mon, 04 Apr 2011 05:08:23 +0200, Sergio Durigan Junior wrote:
>  static struct symtabs_and_lines
> -addr_string_to_sals (struct breakpoint *b, char *addr_string, int *found)
> +addr_string_to_sals (struct breakpoint *b, char *addr_string, int *found,
> +		     int *pre_expanded)
>  {
>    char *s;
>    int marker_spec, not_found;
>    struct symtabs_and_lines sals = {0};
>    struct gdb_exception e;
> +  int my_pre_expanded = 0;

Such as proposal:
  if (pre_expanded == NULL)
    pre_expanded = &my_pre_expanded;

and use only `pre_expanded' in the code so that one does not forget to update
both.  (+Probably some *pre_expanded preinitialization.)


[...]
> +	  my_pre_expanded = canonical.pre_expanded;
> +	  if (pre_expanded)
> +	    *pre_expanded = my_pre_expanded;
> +	}
>      }
>    if (e.reason < 0)
>      {
> @@ -11010,7 +11041,7 @@ addr_string_to_sals (struct breakpoint *b, char *addr_string, int *found)
>  
>    if (!not_found)
>      {
> -      gdb_assert (sals.nelts == 1);
> +      gdb_assert (my_pre_expanded || sals.nelts == 1);
>  
>        resolve_sal_pc (&sals.sals[0]);
>        if (b->condition_not_parsed && s && s[0])
> @@ -11049,22 +11080,27 @@ re_set_breakpoint (struct breakpoint *b)
>    struct symtabs_and_lines expanded = {0};
>    struct symtabs_and_lines expanded_end = {0};
>    struct cleanup *cleanups = make_cleanup (null_cleanup, NULL);
> +  int pre_expanded = 0;
>  
>    input_radix = b->input_radix;
>    save_current_space_and_thread ();
>    switch_to_program_space_and_thread (b->pspace);
>    set_language (b->language);
>  
> -  sals = addr_string_to_sals (b, b->addr_string, &found);
> +  sals = addr_string_to_sals (b, b->addr_string, &found, &pre_expanded);
>    if (found)
>      {
>        make_cleanup (xfree, sals.sals);
> -      expanded = expand_line_sal_maybe (sals.sals[0]);
> +      if (pre_expanded)
> +	expanded = sals;
> +      else
> +	expanded = expand_line_sal_maybe (sals.sals[0]);
>      }
>  
>    if (b->addr_string_range_end)
>      {
> -      sals_end = addr_string_to_sals (b, b->addr_string_range_end, &found);
> +      sals_end = addr_string_to_sals (b, b->addr_string_range_end, &found,
> +				      NULL);
>        if (found)
>  	{
>  	  make_cleanup (xfree, sals_end.sals);
> diff --git a/gdb/linespec.h b/gdb/linespec.h
> index d8d2ec9..458235c 100644
> --- a/gdb/linespec.h
> +++ b/gdb/linespec.h
> @@ -30,6 +30,10 @@ struct linespec_result
>       display mechanism would do the wrong thing.  */
>    int special_display;
>  
> +  /* If non-zero, the linespec result should be considered to be a
> +     "pre-expanded" multi-location linespec.  */
> +  int pre_expanded;

Here could be the comment by Tom from the follow-up mail as this comment does
not explain much.


> +
>    /* If non-NULL, an array of canonical names for returned
>       symtab_and_line objects.  The array has as many elements as the
>       `nelts' field in the symtabs_and_line returned by decode_line_1.


Thanks,
Jan