source annotation now prints source line

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

source annotation now prints source line

Bob Rossi
Hi,

When the source annotation is sent to the front end, the source line
is now also sent to the front end console. I believe this commit
introduced it,
  https://github.com/bminor/binutils-gdb/commit/ec8e2b6d3051f0b4b2a8eee9917898e95046c62f

Now CGDB displays,
    (gdb) n
    43          int i = 3;
    (gdb)

Instead of,
    (gdb) n
    (gdb)

CGDB is a front end, and so it has a source view to display the code to
the user. Why did GDB decide to also print the line of code to the front
end's console window as well? This is confusing.

Thanks,
Bob Rossi
Reply | Threaded
Open this post in threaded view
|

Re: source annotation now prints source line

Bob Rossi
On Sat, Apr 04, 2020 at 07:54:24PM -0400, Bob Rossi wrote:

> When the source annotation is sent to the front end, the source line
> is now also sent to the front end console. I believe this commit
> introduced it,
>   https://github.com/bminor/binutils-gdb/commit/ec8e2b6d3051f0b4b2a8eee9917898e95046c62f
>
> Now CGDB displays,
>     (gdb) n
>     43          int i = 3;
>     (gdb)
>
> Instead of,
>     (gdb) n
>     (gdb)
>
> CGDB is a front end, and so it has a source view to display the code to
> the user. Why did GDB decide to also print the line of code to the front
> end's console window as well? This is confusing.

The concept behind this commit seems incorrect.

The motivation for the patch isn't clearly explained in
the commit message. I believe I've given a reasonable explanation
on why this patch makes no sense for front ends.

Should I submit a patch reverting it?

Thanks,
Bob Rossi
Reply | Threaded
Open this post in threaded view
|

Re: source annotation now prints source line

Andrew Burgess
* Bob Rossi <[hidden email]> [2020-04-14 07:23:04 -0400]:

> On Sat, Apr 04, 2020 at 07:54:24PM -0400, Bob Rossi wrote:
> > When the source annotation is sent to the front end, the source line
> > is now also sent to the front end console. I believe this commit
> > introduced it,
> >   https://github.com/bminor/binutils-gdb/commit/ec8e2b6d3051f0b4b2a8eee9917898e95046c62f
> >
> > Now CGDB displays,
> >     (gdb) n
> >     43          int i = 3;
> >     (gdb)
> >
> > Instead of,
> >     (gdb) n
> >     (gdb)
> >
> > CGDB is a front end, and so it has a source view to display the code to
> > the user. Why did GDB decide to also print the line of code to the front
> > end's console window as well? This is confusing.
>
> The concept behind this commit seems incorrect.
>
> The motivation for the patch isn't clearly explained in
> the commit message. I believe I've given a reasonable explanation
> on why this patch makes no sense for front ends.
>
> Should I submit a patch reverting it?

The patch in context is discussed here:

  https://sourceware.org/pipermail/gdb-patches/2019-June/158310.html
  https://sourceware.org/pipermail/gdb-patches/2019-June/158350.html
  https://sourceware.org/pipermail/gdb-patches/2019-June/158351.html
  https://sourceware.org/pipermail/gdb-patches/2019-June/158352.html
  https://sourceware.org/pipermail/gdb-patches/2019-June/158353.html

I'm not sure you've convinced me yet that the idea behind the patch is
incorrect.  Annotations should be a (deprecated) way for F/Es to parse
GDB's output, but they shouldn't impact _what_ GDB prints.

In this particular case, printing the source line actually updates
some internal state, which impacts how later commands operate.  What
this means is that the users session will behave differently if they
have annotations on than when annotations are off.

I guess, what I don't understand is that if a F/E wants to hide a
particular piece of the output, why can't it just strip that from the
output stream?  The F/E must already be removing the annotation
markers, so all the output must be going through the F/E anyway.

Further, removing this particular piece of output makes sense for this
F/E, but is it always going to be true for all F/Es?

I haven't gone back and looked at the old behaviour, maybe I'll have
more thoughts once I've looked at that again.

Thanks,
Andrew
Reply | Threaded
Open this post in threaded view
|

Re: source annotation now prints source line

Andrew Burgess
* Andrew Burgess <[hidden email]> [2020-04-14 13:17:05 +0100]:

> * Bob Rossi <[hidden email]> [2020-04-14 07:23:04 -0400]:
>
> > On Sat, Apr 04, 2020 at 07:54:24PM -0400, Bob Rossi wrote:
> > > When the source annotation is sent to the front end, the source line
> > > is now also sent to the front end console. I believe this commit
> > > introduced it,
> > >   https://github.com/bminor/binutils-gdb/commit/ec8e2b6d3051f0b4b2a8eee9917898e95046c62f
> > >
> > > Now CGDB displays,
> > >     (gdb) n
> > >     43          int i = 3;
> > >     (gdb)
> > >
> > > Instead of,
> > >     (gdb) n
> > >     (gdb)
> > >
> > > CGDB is a front end, and so it has a source view to display the code to
> > > the user. Why did GDB decide to also print the line of code to the front
> > > end's console window as well? This is confusing.
> >
> > The concept behind this commit seems incorrect.
> >
> > The motivation for the patch isn't clearly explained in
> > the commit message. I believe I've given a reasonable explanation
> > on why this patch makes no sense for front ends.
> >
> > Should I submit a patch reverting it?
>
> The patch in context is discussed here:
>
>   https://sourceware.org/pipermail/gdb-patches/2019-June/158310.html
>   https://sourceware.org/pipermail/gdb-patches/2019-June/158350.html
>   https://sourceware.org/pipermail/gdb-patches/2019-June/158351.html
>   https://sourceware.org/pipermail/gdb-patches/2019-June/158352.html
>   https://sourceware.org/pipermail/gdb-patches/2019-June/158353.html
>
> I'm not sure you've convinced me yet that the idea behind the patch is
> incorrect.  Annotations should be a (deprecated) way for F/Es to parse
> GDB's output, but they shouldn't impact _what_ GDB prints.
>
> In this particular case, printing the source line actually updates
> some internal state, which impacts how later commands operate.  What
> this means is that the users session will behave differently if they
> have annotations on than when annotations are off.
>
> I guess, what I don't understand is that if a F/E wants to hide a
> particular piece of the output, why can't it just strip that from the
> output stream?  The F/E must already be removing the annotation
> markers, so all the output must be going through the F/E anyway.
>
> Further, removing this particular piece of output makes sense for this
> F/E, but is it always going to be true for all F/Es?
>
> I haven't gone back and looked at the old behaviour, maybe I'll have
> more thoughts once I've looked at that again.

For the record, here's GDB's output before the patch (8.3.1):

  ## START ##
  Temporary breakpoint 1, main () at hello.c:6
  6  printf ("Hello World\n");
  (gdb) set annotate 0xff

  ��pre-prompt
  (gdb)
  ��prompt
  n

  ��post-prompt

  ��starting
  Hello World

  ��source /home/andrew/tmp/hello.c:7:62:beg:0x401134

  ��stopped

  ��pre-prompt
  (gdb)
  ��prompt
  ## END ##

And here's the output after the patch (9.1):

  ## START ##
  Temporary breakpoint 1, main () at hello.c:6
  6  printf ("Hello World\n");
  (gdb) set annotate 0xff

  ��pre-prompt
  (gdb)
  ��prompt
  n

  ��post-prompt

  ��starting
  Hello World

  ��source /home/andrew/tmp/hello.c:7:62:beg:0x401134
  7  return 0;

  ��stopped

  ��pre-prompt
  (gdb)
  ��prompt
  ## END ##

I guess I'd still suggest that the "right" thing would be to strip the
output when processing GDB's output.  But, if you strongly disagree
then you could put forward a patch for discussion.  However, I think
you'd need to do more than revert the original patch.

If you look inside source.c:print_source_lines_base then you'll see
the current source line and symtab being updated.  This is done as
part of printing the '7  ..... return 0' line.  If you'd going to stop
this being printed, then you'd need to duplicate this behaviour
somewhere else, so that the current line/symtab are updated when
annotations are on.

Thanks,
Andrew
Reply | Threaded
Open this post in threaded view
|

Re: source annotation now prints source line

Bob Rossi
In reply to this post by Andrew Burgess
On Tue, Apr 14, 2020 at 01:17:05PM +0100, Andrew Burgess wrote:

> * Bob Rossi <[hidden email]> [2020-04-14 07:23:04 -0400]:
>
> > On Sat, Apr 04, 2020 at 07:54:24PM -0400, Bob Rossi wrote:
> > > When the source annotation is sent to the front end, the source line
> > > is now also sent to the front end console. I believe this commit
> > > introduced it,
> > >   https://github.com/bminor/binutils-gdb/commit/ec8e2b6d3051f0b4b2a8eee9917898e95046c62f
> > >
> > > Now CGDB displays,
> > >     (gdb) n
> > >     43          int i = 3;
> > >     (gdb)
> > >
> > > Instead of,
> > >     (gdb) n
> > >     (gdb)
> > >
> > > CGDB is a front end, and so it has a source view to display the code to
> > > the user. Why did GDB decide to also print the line of code to the front
> > > end's console window as well? This is confusing.
> >
> > The concept behind this commit seems incorrect.
> >
> > The motivation for the patch isn't clearly explained in
> > the commit message. I believe I've given a reasonable explanation
> > on why this patch makes no sense for front ends.
> >
> > Should I submit a patch reverting it?
>
> The patch in context is discussed here:
>
>   https://sourceware.org/pipermail/gdb-patches/2019-June/158310.html
>   https://sourceware.org/pipermail/gdb-patches/2019-June/158350.html
>   https://sourceware.org/pipermail/gdb-patches/2019-June/158351.html
>   https://sourceware.org/pipermail/gdb-patches/2019-June/158352.html
>   https://sourceware.org/pipermail/gdb-patches/2019-June/158353.html
>
> I'm not sure you've convinced me yet that the idea behind the patch is
> incorrect.  Annotations should be a (deprecated) way for F/Es to parse
> GDB's output, but they shouldn't impact _what_ GDB prints.

Annotation are still useful for the single purpose of allowing GDB
to tell you when the user is at the prompt. This allows front ends
to know when to issue a new command.

> In this particular case, printing the source line actually updates
> some internal state, which impacts how later commands operate.  What
> this means is that the users session will behave differently if they
> have annotations on than when annotations are off.

I see. Seems good to have fixed that. Although I wonder how relevant
that information was as front ends have been using annotations for
20+ years without having noticed. Was this found from a user bug report?

> I guess, what I don't understand is that if a F/E wants to hide a
> particular piece of the output, why can't it just strip that from the
> output stream?  The F/E must already be removing the annotation
> markers, so all the output must be going through the F/E anyway.

Front ends can not parse gdb's output. It's the golden rule.
It's unreliable and will ultimately fail.

> Further, removing this particular piece of output makes sense for this
> F/E, but is it always going to be true for all F/Es?

Yes, it does. It worked this way for 20+ years, and everyone was happy.
Was this added in because an actual user complained?

Even the tui does not show the source output in the console, even
after your patch.

Thanks,
Bob Rossi
Reply | Threaded
Open this post in threaded view
|

Re: source annotation now prints source line

Andrew Burgess
* Bob Rossi <[hidden email]> [2020-04-14 22:13:24 -0400]:

> On Tue, Apr 14, 2020 at 01:17:05PM +0100, Andrew Burgess wrote:
> > * Bob Rossi <[hidden email]> [2020-04-14 07:23:04 -0400]:
> >
> > > On Sat, Apr 04, 2020 at 07:54:24PM -0400, Bob Rossi wrote:
> > > > When the source annotation is sent to the front end, the source line
> > > > is now also sent to the front end console. I believe this commit
> > > > introduced it,
> > > >   https://github.com/bminor/binutils-gdb/commit/ec8e2b6d3051f0b4b2a8eee9917898e95046c62f
> > > >
> > > > Now CGDB displays,
> > > >     (gdb) n
> > > >     43          int i = 3;
> > > >     (gdb)
> > > >
> > > > Instead of,
> > > >     (gdb) n
> > > >     (gdb)
> > > >
> > > > CGDB is a front end, and so it has a source view to display the code to
> > > > the user. Why did GDB decide to also print the line of code to the front
> > > > end's console window as well? This is confusing.
> > >
> > > The concept behind this commit seems incorrect.
> > >
> > > The motivation for the patch isn't clearly explained in
> > > the commit message. I believe I've given a reasonable explanation
> > > on why this patch makes no sense for front ends.
> > >
> > > Should I submit a patch reverting it?
> >
> > The patch in context is discussed here:
> >
> >   https://sourceware.org/pipermail/gdb-patches/2019-June/158310.html
> >   https://sourceware.org/pipermail/gdb-patches/2019-June/158350.html
> >   https://sourceware.org/pipermail/gdb-patches/2019-June/158351.html
> >   https://sourceware.org/pipermail/gdb-patches/2019-June/158352.html
> >   https://sourceware.org/pipermail/gdb-patches/2019-June/158353.html
> >
> > I'm not sure you've convinced me yet that the idea behind the patch is
> > incorrect.  Annotations should be a (deprecated) way for F/Es to parse
> > GDB's output, but they shouldn't impact _what_ GDB prints.
>
> Annotation are still useful for the single purpose of allowing GDB
> to tell you when the user is at the prompt. This allows front ends
> to know when to issue a new command.
>
> > In this particular case, printing the source line actually updates
> > some internal state, which impacts how later commands operate.  What
> > this means is that the users session will behave differently if they
> > have annotations on than when annotations are off.
>
> I see. Seems good to have fixed that. Although I wonder how relevant
> that information was as front ends have been using annotations for
> 20+ years without having noticed. Was this found from a user bug report?
>
> > I guess, what I don't understand is that if a F/E wants to hide a
> > particular piece of the output, why can't it just strip that from the
> > output stream?  The F/E must already be removing the annotation
> > markers, so all the output must be going through the F/E anyway.
>
> Front ends can not parse gdb's output. It's the golden rule.
> It's unreliable and will ultimately fail.
>
> > Further, removing this particular piece of output makes sense for this
> > F/E, but is it always going to be true for all F/Es?
>
> Yes, it does. It worked this way for 20+ years, and everyone was happy.
> Was this added in because an actual user complained?
>
> Even the tui does not show the source output in the console, even
> after your patch.

To our great shame, the relevant code here is (from stack.c):

      if (deprecated_print_frame_info_listing_hook)
        deprecated_print_frame_info_listing_hook (sal.symtab, sal.line,
                                                  sal.line + 1, 0);
      else
        {
          // .... print the source lines ....
        }

And in tui/tui-hooks.c we have:

  deprecated_print_frame_info_listing_hook
    = tui_dummy_print_frame_info_listing_hook;

and:

  static void
  tui_dummy_print_frame_info_listing_hook (struct symtab *s,
                                           int line,
                                           int stopline,
                                           int noerror)
  {
  }

Why am I telling you this? It basically is a convoluted way of saying:

  if (!tui)
  {
     // ...... print the source lines ....
  }

This certainly backs up your point that GUI users don't want to see
the information in both the GDB terminal _and_ in their GUI.  Which
interestingly, would suggest that the TUI is going to have the same
set of bugs that the annotations would trigger.  But in terms of
implementation the TUI is special casing itself.

I'll take a look to see if there's a good way to give you the
functionality you're looking for and close the bugs off.

Thanks,
Andrew



>
> Thanks,
> Bob Rossi