Re: [patch] Code cleanup: Remove parameter quitting [Re: Crash of GDB with gdbserver btrace enabled]

[Pedro Alves <palves at redhat dot com>]

On 03/25/2013 07:12 PM, Jan Kratochvil wrote:
> Hi Pedro,
> 
> I do not plan to do any changes mentioned below, it is just an affirmation
> future development has an agreed upon design.

Thanks.

> On Mon, 18 Mar 2013 21:25:12 +0100, Pedro Alves wrote:
>> Other Debug APIs may
>> require a "debug_api_foo_close ()" call that actually results in traffic,
>> to properly release the reference to the debug api library.  remote-mips.c
>> seems to be doing something like that, and it's the sort of code that
>> could hang/block for a while, so it seems to me like the original intent
>> of QUITTING would be to handle _that_ kind of blocking.  If "QUITTING",
>> GDB is quitting, so don't bother being nice to your debug API library,
>> if that takes long and hang a fast "quit" experience, as the whole GDB
>> process is going away anyway.  Otherwise, if not QUITTING, take care to
>> clean up properly, because the user may want to spawn a new debug session,
>> in the same GDB process.
> 
> Do you think that the mips_close packet communication should have been done
> only if !QUITTING?  

Nope.  The original intent of QUITTING appears to have been something like
that (or a short timeout or something), but all these years nobody thought
it really necessary in any port in the tree, including the mips port.
I'd have to see the trouble happening myself to access how I'd prefer to
see it addressed.  Otherwise, this is all a bunch of hand waving.  :-)

> This would mean the GDB "quit" command would leave the
> MIPS target in some inconsistent state, possibly not ready for a new
> connection later (assuming the MIPS target cannot accept new connection
> without previous mips_exit_debug packets).

Yeah.

> quit_command already does disconnect_tracing and then (target_detach or
> target_kill) before target_close.  I miss target_disconnect in quit_command.

Yeah.  I think the disconnect use case is a bit rare though.  I'm not
sure it's worth it to complicate the user interface.  I think we'd need small
use case study.  The user can always say no to "quit"'s query, and use
the "disconnect" command manually, then quit.

> This could do the packet communication part of mips_close.  mips_close then
> would not communicate anything so that mips_close is always safe in some
> broken communication cases.  

That does make sense.  Though, we'd be back to considering when do we need to
close without trying the full disconnect, and whether the distinction is
worth it.  But it does sound like a cleaner approach than QUIT_FLAG, if
we want to bring this back.

> And even the quit_command call of
> disconnect_tracing could be then removed and disconnect_tracing called only
> from remote_disconnect.

I think the questions disconnect_tracing does should be independent of target.
We don't do tracing in the native target, but that's just because nobody's
spent the effort.

> 
> That target_detach or target_kill could be kept only for compatibility with
> old targets (if there are any), normal targets should do
> target_detach/target_kill automatically from to_disconnect.

I don't think I agree with this part.  "quit"'s UI needs to know what
will happen, to present the query, and it just feels wrong to push
that decision down, rather than keeping the API more stateless / functional
style.  E.g., that seems to leave no good way for to_disconnect to know
to just disconnect, for the "disconnect" command.

-- 
Pedro Alves