This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH RFC] Decouple user selection from internal selection


Hi Simon

I have mixed feelings for this patch.  On the one hand I think its good
you try to untangle the different uses of the global variables.  On the
other hand your approach is just an other workaround for the main
problem, using global variables.

For me the long-term goal should be to get rid of the global
variables.  This would require to define a "debug_context" which can be
a user selected as well as a GDB internal context.  This
"debug_context" (or parts of it) can then be passed down the stack.  So
every function gets the information it needs from its caller.  This not
only gives you flexibility, as you can have different contexts, but
also prevents bugs where the global variables are not properly
set/restored, like you have with MI.

Those different "debug_contexts" will also be needed for the
multi-target idea Andreas and I have.  With this feature we want to
allow the user to take a look at a given problem from the point of view
of different targets. For example consider the potential target stack
(from [1]):

  - goroutine (Goroutine)
  - multi-thread (multi-threaded child process.)  <-- linux-thread-db
  - lk-user (User-space task) <-- focuses on one user-space process
  - lk-thread (Linux kernel threads) <-- uses the kernel's task list
  - core (Local core dump file) <-- "threads" are actually CPUs
  - exec (Local exec file)
  - None (None)

In this scenario there are five different views on threads (from
hardware threads to goroutines).  The user might be interested in all
of them and wants to choose a target and get its perspective on the
problem.  But switching between different targets, with different views
on threads/inferiors, using global variables would be a pain and error
prone.  That's why we prefer the approach described above.

Of course getting rid of the global variables would be a huge job.
Here your approach could be an intermediate solution (I understand your
"user_selection" to be a "debug_context").  It allows us to update GDB
over time by passing it down the stack one after one and apply the
context to the global variables for the parts which where not updated,
yet.  

Independent to what I said before you should split up this patch into a
series to make review easier.  For example the introduction of
thread_info::put/get in gdbthread.h or scoped_restore_suppress_output
in ui-out.c are independent of the user-selection and should go in
separate patches.

Philipp

[1] https://sourceware.org/ml/gdb-patches/2017-02/msg00106.html


On Thu, 23 Feb 2017 17:28:50 -0500
Simon Marchi <simon.marchi@ericsson.com> wrote:

> I am sending this as an RFC because it's far from complete and
> definitive, but I'd like to gather some comments and opinions before
> going further in this direction.
> 
> The goal of this patch is to decouple the notion of the user-selected
> inferior/thread/frame from GDB's internally selected
> inferior/thread/frame.
> 
> Currently, for example, the inferior_ptid variable has two jobs:
> 
>  - it's the user-selected thread: it's changed by the "thread"
> command. Other commands (continue, backtrace, etc) apply to this
> thread.
>  - it's the internally-selected thread: it defines the thread GDB is
>    currently "working" on.  For example, implementations of
>    to_xfer_partial will refer to it to know from which thread to
>    read/write memory.
> 
> Because of this dual usage, if we want to do some operations on a
> thread other than the currently selected one, we have to save the
> current inferior/thread/frame and restore them when we're done.
> Failing to do so would result in an unexpected selection switch for
> the user.
> 
> To improve this, Pedro suggested in [1] to decouple the two
> concepts.  This is essentially what this patch is trying to do.
> 
> A new "user_selection" object is introduced, which contains the
> selected inferior/thread/frame from the point of view of the user.
> Before every command, we "apply" this selection to the core of GDB to
> make sure the internal selection matches the user selection.
> 
> There is a single user selection for the whole GDB (named "global
> user-selection"), but as was mentioned in the linked thread, it opens
> the door to having different selections for different UIs.  This means
> that each UI would have its own user-selection object, which would be
> applied to the core prior to executing commands from this UI.
> 
> The global user-selection object only gets modified when we really
> intend to change it.  It can be because of the thread /
> -thread-select / up / down / frame / inferior commands, a breakpoint
> hit in all-stop, an inferior exit, etc.
> 
> The problem that initially prompted this effort is that the "--thread"
> flag of MI commands changes the user-selected thread under the user's
> feet.  My initial attempt to fix it was to restore the selection after
> the MI command execution.  However, some cases are hard to get right.
> For example:
> 
>   (thread 1 is currently selected)
>   -interpreter-exec --thread 2 console "thread 3"
> 
> Restoring the selected thread to thread 1 after the MI command
> execution wrongfully cancels the switch to thread 3.  So it's hard to
> determine when we should or shouldn't restore.   With the current
> patch, it works naturally: the --thread flag doesn't touch the
> user-selected thread, only the internal one.  The "thread 3" command
> updates the user selection.
> 
> Another difficulty is to send the right notifications to MI when the
> user selection changes.  That means to not miss any, but not send too
> many either.  Getting it somewhat right lead to ugly hacks (see the
> command_notifies_uscc_observer function) and even then it's not
> perfect (see the kfails in user-selected-context-sync.exp test).
> With the proposed method, it's easy to know when the user-selection
> changes and send notifications.
> 
> With this patch, there are probably a few usage of
> make_cleanup_restore_current_thread that are not needed anymore, if
> they are only used to restore the user selection.  I kept removing
> them for a later time though.
> 
> In the current state, there are a few minor regressions in the
> testsuite (especially some follow-fork stuff I'm not sure how to
> handle), but the vast majority of the previously passing tests still
> pass.
> 
> I've pushed the patch to users/simark/user-selection-rfc on
> sourceware. Comments are welcome!
> 
> Thanks,
> 
> Simon
> 
> [1] https://sourceware.org/ml/gdb-patches/2016-08/msg00031.html


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]