Re: [PATCHv3 0/3] Restore thread and frame patches

[Andrew Burgess <andrew.burgess@embecosm.com>]

Ping!

Pedro - I do feel I've addressed your initial feedback, I'd love to
hear what your position is on this new version, even if it's just to
confirm you are still opposed to this patch.

Many thanks,
Andrew

* Andrew Burgess <andrew.burgess@embecosm.com> [2020-09-25 23:24:06 +0100]:

> This is a third attempt to get some, or maybe all of this work merged.
> Or to get some idea on what might be seen as an acceptable direction
> to take this work.
> 
> I originally posted this here (v2):
> 
>   https://sourceware.org/pipermail/gdb-patches/2020-April/167984.html
> 
> and (v1):
> 
>   https://sourceware.org/pipermail/gdb-patches/2020-February/166202.html
>   https://sourceware.org/pipermail/gdb-patches/2020-April/167215.html
> 
> Changes since V2:
> 
>  - Rebase to current master.
> 
>  - Fixed minor coding style issues, and improved a comment as pointed
>    out by Baris.
> 
>  - Reordered the patches so #2 is now #1.  I think that current #1
>    which is really a restructure might be worth merging even if #2 and
>    #3 never get merged, hence placing it first.
> 
>  - A few minor tweaks to take acount of general code changes since v2.
> 
> There was some initial positive feedback on some of the v1 patches,
> but Pedro was not convinced:
> 
>   https://sourceware.org/pipermail/gdb-patches/2020-April/167223.html
> 
> I'll quote his feedback here, and reply to it inline:
> 
> > Frankly, I'm not really sure I like this.  It seems like a can of worms to
> > me...  It's going to cause us to have to decide whether it's a good idea to
> > save/restore all kinds of state in the objects hierarchy.  E.g., if this is
> > reasonable, then it would also be reasonable to restore the selected
> > Ada task.  And frames.  And maybe the selected source file and line for
> > list.  Once we gain support for fibers, coroutines, etc. we'll
> > then need to apply the same logic.  Etc.  And then maybe we'll need
> > some way to query the selected thread of a given inferior.  Etc.
> 
> This feedback was on v1 of the patch where I changed the default
> behaviour to be restore thread/frame, since v2 the default is to
> maintain GDB's current behaviour and have the restore be a feature a
> user must turn on.
> 
> I think this addresses the concern Pedro raised as we no longer have
> an inconsistent position, some things are restored while others are
> not, instead we have a switch to allow somethings to be restored while
> we lack a switch to allow other things to be restored.
> 
> While I agree with Pedro's original feedback that a user might be
> confused, "why is XXX restored, but not YYY", now they will simply be
> left wondering, "Why is there no switch to restore YYY?".  Though they
> might wonder why the switch doesn't exist, and might be disapointed
> even, I don't think it will leave them confused with the actual
> behaviour of GDB.
> 
> Further, though "restoring stuff" as a broad category clearly covers
> all the things Pedro mentions, restoring of each thing will require
> its own piece of work.  I don't think we should prevent merging this
> just because there might be some other (similar, but unrelated) part
> of GDB that could also be saved and restored.
> 
> > 
> > The current rule is quite simple.  If you select a object then its container
> > is implicitly selected.  So if you select a thread, you implicitly select
> > its inferior, and implicitly select its target.
> 
> OK, but that's looking upward, a frame is in a thread, a thread is in
> an inferior, an inferior is in a target.  These changes are about lookig down.
> 
> >                                                   And the first/initial container
> > object is selected.
> 
> I'm confused!  Above you talk about containers as looking upward
> thread -> inferior -> target, but I think you're now talking about
> things as looking down, in which case talking about containers seems
> like poor terminology.
> 
> When we switch to an inferior then its containing target is
> automatically selected, but there's only one of those to select.
> 
> >                        It seems very natural to me that "inferior 2" ends up
> > selecting the initial thread of inferior 2.  I.e., normally, thread 2.1.
> 
> I'd certainly never want to suggest you're wrong for preferring a
> particular behaviour.  For me though the choice of the first thread
> seems pretty arbitrary, instead the idea of restoring the selected
> object within a container seems more consistent.
> 
> However, I feel I've addressed this concern by making both of the
> save/restore features being off by default.
> 
> Thoughts, or feedback on any or all of these patches is always
> welcome.
> 
> Thanks,
> Andrew
> 
> ---
> 
> Andrew Burgess (3):
>   gdb: unify two copies of restore_selected_frame
>   gdb: Restore previously selected thread when switching inferior
>   gdb: Track the current frame for each thread
> 
>  gdb/ChangeLog                                 |  53 +++
>  gdb/NEWS                                      |  21 ++
>  gdb/doc/ChangeLog                             |  14 +
>  gdb/doc/gdb.texinfo                           |  42 ++-
>  gdb/frame.c                                   |  84 +++++
>  gdb/frame.h                                   |  85 +++++
>  gdb/gdbthread.h                               |  15 +-
>  gdb/inferior.c                                |  58 ++-
>  gdb/inferior.h                                |  10 +
>  gdb/infrun.c                                  |  25 +-
>  gdb/testsuite/ChangeLog                       |  10 +
>  .../gdb.threads/restore-selected-frame.c      |  85 +++++
>  .../gdb.threads/restore-selected-frame.exp    | 336 ++++++++++++++++++
>  gdb/testsuite/gdb.threads/restore-thread.c    | 248 +++++++++++++
>  gdb/testsuite/gdb.threads/restore-thread.exp  | 219 ++++++++++++
>  gdb/thread.c                                  | 128 +++----
>  16 files changed, 1331 insertions(+), 102 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.threads/restore-selected-frame.c
>  create mode 100644 gdb/testsuite/gdb.threads/restore-selected-frame.exp
>  create mode 100644 gdb/testsuite/gdb.threads/restore-thread.c
>  create mode 100644 gdb/testsuite/gdb.threads/restore-thread.exp
> 
> -- 
> 2.25.4
>