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

[Simon Marchi <simon.marchi@ericsson.com>]

Hi Philipp,

On 17-03-03 12:24 PM, Philipp Rudo wrote:
> 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.

I agree.  I think a good way of stating the direction to take is to reduce
the usage of the inferior_ptid global variable.  As I mentioned in the patch
message, it currently has two functions:

 1. User/front-end selected thread
 2. Internally selected thread

This patch tries to separate those two concerns.  #1 doesn't really have the
choice to be global at the moment, because we have a single globally shared user
selection.  If we decide to have a selection per UI, it would change that.

#2 is where we can reduce the usage of the global right now.  inferior_ptid is
in many ways used to indirectly pass information to called functions (on which
thread to operate), whereas this information should be passed down the callstack,
as you say.

Whether we can use the same debug_context objects for storing the user selection
and the debug contexts passed down the stack, I don't know.  Whoever implements
this will discover the answer.

So I think that my patch, even though it doesn't address the problem of the
globals directly, is in line with the long-term direction you are proposing.

> 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.

It would be very interesting indeed, to be able to choose the level of
abstraction at which to look at the system.

> 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.

Indeed, I will do that.

Thanks a lot for the comments.

Simon