Re: [PATCH v2 1/2] Always show locations for breakpoints & show canonical location spec

[Eli Zaretskii <eliz@gnu.org>]

> Date: Wed, 25 May 2022 20:32:24 +0100
> Cc: gdb-patches@sourceware.org
> From: Pedro Alves <pedro@palves.net>
> 
> On 2022-05-24 14:06, Eli Zaretskii wrote:
> 
> > 
> >>>> +Enabled breakpoints and breakpoint locations are marked with @samp{y}.
> >>>
> >>> What does "enabled breakpoint location" mean?  One cannot enable a
> >>> location.  
> >>
> >> Sure we can.
> > 
> > We are mis-communicating.  "Location" is a frequently-used word that
> > has certain natural meaning in many contexts.  That natural meaning
> > doesn't support the notion of "enabling".  
> 
> Well, yeah, but above the text is very clearly talking about "breakpoint
> locations", i.e., the locations in the program where the breakpoint is armed.
> Enabling/disabling one of these locations is really about enabling/disabling
> the arming of a breakpoint at such a location.

Yes, and that's the point I tried to make: you can enable or disable a
breakpoint at some location, not enable/disable the location itself,
which is what the text was saying.

> > My suggestion is to use "location" (with or without additional
> > qualifiers) for only one of these two, if only because we tend to
> > shorthand that by using just "location".  We could use it for what you
> > now call "location specification", which would allow us to use just
> > "location" as it shorthand.  With your proposed text, "location" is
> > ambiguous, because it could allude to any of these two.  The other
> > meaning of "location" I propose to call "address in the code" or
> > "address" for short -- because that's what it conceptually is.  (If
> > you are bothered by the case of unloaded shared library and similar
> > situations, we could say "unresolved address" in those cases -- which
> > are rather marginal, so once explained, they can be ignored in the
> > rest of the text.  By contrast, these two "locations" are pervasive:
> > we need to refer to them all over the manual.  So it's much more
> > important IMO to make that part of our terminology clear and
> > unequivocal.)
> 
> I really think that this is the wrong direction, and working on
> the patch that I pointed at above really convinced me so even more.
> I believe that that patch will convince you as well.  Please take a look
> there.

I did, and it didn't.  It actually convinced me even more that we
cannot use "location" in both contexts without creating confusion.

> As another data point, after writing that other patch, i.e., just now,
> I went to look what other debuggers call similar things, and here's what I saw:

Other debuggers could mean other things, or they could be also wrong
in this aspect.  Our manual should make sense to us, even if other
debuggers decide to use different terminology.  Likewise when adopting
terminology used by others.

Moreover, half if not more of the evidence you provide is about things
I'm not objected to: I'm okay with calling "location spec" what was
formerly known as "linespec".  My problem is with the other use of
"location".

> > Can you tel concretely why you object to changing the "location"
> > terminology along these lines?  Even if you think the current
> > terminology is okay, what will we lose by switching the terminology I
> > propose above?
> 
> You lose the fact that it is the right name for it, and the fact that everyone
> understands what it means today, so you force everyone to learn something new
> for no good reason.

"Breakpoint address" is nothing new, either.  We use it all the time,
so I see no need for everyone to relearn.

> >> Even GDB's source code uses "location" for both -- struct bp_location
> >> for the breakpoint locations, the "1.1", "1.2" things, stored in each
> >> struct breakpoint in the breakpoint::loc field, as a linked list:
> > 
> > I know.  But I hope we can avoid terminology ambiguities even though
> > they are present in the code.  The reader of the GDB manual will not
> > necessarily read the GDB source at the same time, and is not required
> > to do so if all they want is to learn how to use GDB.
> 
> My point is that the ambiguity already exists, is old, but I plan to
> do something about, including fixing the code, and was am telling you
> what my plan for the source code was.

We agree about the general goal, but disagree about some minor point,
such as what to call "the place where we insert the breakpoint".

> > Preexisting text is not sacred.  It's okay to modify or even delete
> > parts of the old text where they are sub-optimal or aren't
> > contributing to the manual enough to justify their existence.
> 
> Certainly.  But it is also the case that you should expect that a contributor
> will assume that the terms used in current text will have already been vetted
> when the text was originally added, so reusing them or moving text around shouldn't
> require rewriting everything.

I think it is reasonable to improve text borrowed from previous
versions while we are doing significant changes in the area.  Such
changes are a good opportunity to improve the clarity of the manual.
Minor cleanups as part of more significant changes, both in code and
in the documentation, and are routinely done in many projects,
including in GDB.  So I don't think my requests to change what was
copied from the preexisting text are unreasonable.

> It is not correct to think of the locations as just "addresses",
> because they are more than that.  For example here are three breakpoints
> set at different functions, one of them inlined.  Note that both breakpoints'
> locations have the same address, though different function names and line numbers:
> 
> (gdb) b func_extern_caller
> Breakpoint 1 at 0x5555555552d3: file /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.opt/inline-break.c, line 208.
> (gdb) b func_inline_caller
> Note: breakpoint 1 also set at pc 0x5555555552d3.
> Breakpoint 2 at 0x5555555552d3: file /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.opt/inline-break.c, line 199.
> (gdb) info breakpoints 
> Num     Type           Disp Enb Address            What
> 1       breakpoint     keep y   0x00005555555552d3 in func_extern_caller at /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.opt/inline-break.c:208
> 2       breakpoint     keep y   0x00005555555552d3 in func_inline_caller at /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.opt/inline-break.c:199
>                                 ^^^^^^^^^^^^^^^^^^    ^^^^^^^^^^^^^^^^^^                                                                          ^^^
>                                     same!                  different...                                                                 different...
> 

I'm afraid I don't see the problem.  Sure, the addresses are
identical, but these are two different breakpoints: they have
different numbers.  I never said that if the addresses are identical,
the corresponding breakpoints are also equal: that is definitely not
true.  A breakpoint has many attributes besides the address where it
breaks: conditions, commands, etc.

> So they should be considered different locations.
> Also, the local variables you can access in each of the breakpoints's conditions are
> different, the breakpoints's commands will see a different context, etc., as the breakpoints
> are set in different functions, even though they have the same addresses.

Well, of course.  As I said, a breakpoint has many attributes, not
just the code address.

But now I'm beginning to wonder what is your definition of "location"
in this context?  What does it entail if two different "locations" can
be resolved to the same code address?  And where is that meaning of
"location" described?  The text you proposed uses "location(s) that
match(es) the location spec", which doesn't seem to imply any
attributes except the "place" where the breakpoint will be set, since
"location spec" is just source-level description of one or more such
"places".  How does "location" imply anything about local variables,
commands, etc.?

> So calling breakpoint locations "addresses" instead if just incorrect.

You seem to use "location" in some generalized sense, to mean many
things in addition to the place in the program, which I don't yet
fully understand.  But if so, why is it okay to use "location" in such
a generalized sense, but it is not okay to make a similar
generalization of "breakpoint address"?

I'm not wedded to "address", btw, I just don't want us to use
"location" for that.  If there's some alternative term, we could
consider it.  Although "address" is already being used for a very
similar, if not the same, thing, and thus is a natural candidate,
because people won't need to learn a new term.

> > When I say "not explained" I mean there's no text which says something
> > like "an ordinary breakpoint is ...", or some text that explains what
> > are the "not ordinary" breakpoints, and by doing that explains
> > implicitly what are the "ordinary" ones.  If the old text didn't have
> > that, it means that old text was also sub-optimal.  (And my reading of
> > these changes to the manual led me to believe that this distinction
> > will be more important once the behavior changes, because you modified
> > the original text even where the old one was not invalidated by the
> > changes in behavior.)  Thus the comment.
> > 
> 
> OK, but you have to understand that if a contributor just copies a term from
> the surrounding text, they'll think that it should be OK to use it
> again, otherwise how did the term get into the manual in the first place?
> Requiring that contributions that just add more instances of the same clean up
> the prior mess isn't fair, because adding one more use of the same term will not
> make any difference to the user's understanding, nor to the work necessary to clarify
> elsewhere in some intro text what the term means.  That can be done as an
> orthogonal change.

I don't see why we would need to wait for a separate patch.  What is
the advantage of that?  If the problem is that it's a lot of work for
you, you need just to ask, and I will write the explanation myself, so
you could use it in your patch.

> >> Thus "ordinary" here refers to actual code breakpoints, as opposed to the generic "breakpoint"
> >> term meaning all of code breakpoints, watchpoints, tracepoints, catchpoints.
> > 
> > How about saying that explicitly, before we start relying on this
> > terminology?
> 
> The text I'm changing _already_ relies on that terminology, so "before we start"
> doesn't apply.

I meant "before" in the reading order of the text, not in
chronological sense of when this text was originally written.