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

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

> That doesn't follow though.  We have a ton of places that call
> target_terminal_our that don't need to put back the terminal in
> whatever state is was before.  The reason is that usually we'll
> be printing after the inferior stopped for some event.  If we end
> up re-resuming it, we'll call target_terminal_inferior then.

I am not sure I understand that part. The thread exit is noticed because
of a breakpoint being hit (a breakpoint in libthread-db). In all-stop, I
would expect that when the thread-exit breakpoint is hit, we stop all
threads, then process the event (display the message). After that, we
would need to resume the remaining threads, so the application continues
executing. If so, we would call target_terminal_inferior there.

But, I also remember you mentioning that these thread-db breakpoints are
not handled the same way as regular breakpoints. If I inspect the
target-stack when debugging a threaded program:

    The current target stack is:
      - multi-thread (multi-threaded child process.)
      - native (Native process)
      - exec (Local exec file)
      - None (None)

I would guess that the layer responsible for the all-stop behavior (stop
all the threads when one is stopped) is the native process one. The
thread-db breakpoint handling, however, is done in the multi-thread layer,
so the native target is not reached for handling these. So when the
message is printed, the other threads are actually still running.

Is this last explanation correct?

> But in the thread exit case, there's nothing to be re-resumed,
> so we need to care about it "manually".  If you take a backtrace
> at the point the thread exit event is printed, we should be
> still deep within the target backend code.
> 
> Please update the commit log to clarify this.

I would write this:

We need to manually restore the terminal setting in this particular observer.
In the case of the other observers that call target_terminal_ours, gdb will end
up resuming the inferior later in the execution and call
target_terminal_inferior. In the case of the thread exit event, we still need
to call target_terminal_ours to be able to print something, but there is nothing
that gdb will need to resume after that. We therefore need to call
target_terminal_inferior ourselves.

is this correct?

>>  extern int target_supports_terminal_ours (void);
>>  
>> +/* Make a cleanup that restores the state of the terminal to the current
>> +   value.  */
> 
> Should be "to the current state", I think.

Done.

Thanks, I will push if the clarification above is OK.

Simon