Re: [patch 2/2] Do not bpstat_clear_actions on throw_exception #3

[Pedro Alves <pedro at codesourcery dot com>]

On Tuesday 23 August 2011 21:32:58, Jan Kratochvil wrote:
> On Mon, 22 Aug 2011 19:06:15 +0200, Pedro Alves wrote:
> > Not all cases.  In async mode, handle_inferior_event is called
> > _outside_ of execute_command, directly by the event loop (well, almost
> > directly).  Thus any exception thrown between bpstat_stop_status is called,
> > and the bpstat_do_actions call in inf-loop.c, will leave the thread
> > with a dangling bpstat too.  Might be good enough to wrap
> > handle_inferior_event with a similar cleanup?
> 
> Done, thanks.  On top of the same patch 1/2 as before.
> 
> Testing is more difficult, going to post now patch 3/2 but in fact the real
> continuation is not tested there as the testcase gets caught by
> execute_command.  I haven't done a proper testcase for the async mode.

I think a hook-stop that errors would be sufficient to leave
breakpoint commands dangling (normal_stop is called after
handle_inferior_event, from fetch_inferior_event)

> No regressions on {x86_64,x86_64-m32,i686}-fedora16pre-linux-gnu.

Almost there, but not yet, sorry. :-(  The cleanup is installed before
fetch_inferior_event (non-stop) switches inferior_ptid to the event thread
with a cleanup that restores back to the previous selected thread, which
means, this happens:

inferior_event_handler
  -> install cleanup to clear the bpstat of the current thread
  -> fetch_inferior_event
    -> install cleanup to restore the selected thread 
    -> switch to the event thread
    -> handle_inferior_event
       -> bpstat_stop_status on the now current event thread
       -> some error is thrown, e.g., while unwinding for step/next.
            -> the cleanup to restore the previous thread is ran
            -> the cleanup to clear the bpstat of the current
               thread is ran, but the current thread is not the
               event thread.  The bpstat of the event thread is left
               handling.

Installing the cleanup in fetch_inferior_event _after_
make_cleanup_restore_current_thread would fix this.

All-stop doesn't switch to the event thread until within
handle_inferior_event, so that still leaves a window where an
exception would run the cleanup with the old thread selected, but I'm
not seeing a scenario where that'd be a problem, and plus, the
current code (clearing bpstat actions from throw_exception) gets
that wrong too.

So the patch looks okay to me with that change.  Thanks.

-- 
Pedro Alves