Re: [PATCH v2] Restore terminal state in mi_thread_exit (PR gdb/17627)

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

On 2014-12-02 07:08 PM, Patrick Palka wrote:
> On Tue, Dec 2, 2014 at 5:10 PM, Simon Marchi <simon.marchi@ericsson.com> wrote:
>> When a thread exits, the terminal is left in mode "terminal_is_ours"
>> while the target executes. This patch fixes that.
>>
>> From my understanding, a function calling target_terminal_ours expects
>> that the terminal could be in any state at the moment it is called.
>> Therefore, it should be its reponsibility to put back the terminal in
>> whatever state it was before being called.
> 
> It seems to me that the other observer callbacks defined in
> mi-interp.c are also affected by this issue, that is, the issue of
> having to restore the original terminal state after altering it.

Probably, although this one is the only one for which I saw the problem happening.
For the other handlers that call target_terminal_ours, my guess is that it happens
that the terminal gets reset somewhere else in the program, for unrelated reasons.

Like I said in the patch log, if the function explicitly calls target_terminal_ours,
it's because it expects that the terminal could not be ours at the beginning of its
execution. Thus, it makes no sense to call target_terminal_ours without resetting it.

For correctness, indeed, I think that the same should be applied to the few observers
in that file that call target_terminal_ours.

> So I
> wonder if it would make sense to shift this responsibility to the
> observer module itself (i.e. generic_observer_notify()), so that all
> observers implicitly restore the original terminal state when they
> return.  That way this kind of pattern wouldn't have to be duplicated
> for each individual observer.

I wouldn't put that responsibility in the observer module itself. It's a pretty
generic piece of code (not tied to GDB business logic) and should stay that way
I think.

Also, I think that for clarity it's better to leave that responsibility of changing
the terminal mode to the functions that know that something is going to be printed
(which are not necessarily the functions that actually print the things). Moving that
responsibility to some code that has nothing to do with printing (e.g. observer, or
the caller of observer_notify_*) would make things more confusing. Basically, separation
of concerns.