call_function_by_hand doesn't restore target async?

call_function_by_hand doesn't restore target async?

[Doug Evans]

Hi.

Should there be a cleanup to restore target_async_mask?

call_function_by_hand has this:

  {
    struct cleanup *old_cleanups = make_cleanup (null_cleanup, 0);
    struct cleanup *old_cleanups2;
    int saved_async = 0;
    struct thread_info *tp = inferior_thread ();

    /* Save this thread's ptid, we need it later but the thread
       may have exited.  */
    this_thread_ptid = tp->ptid;

    /* If all error()s out of proceed ended up calling normal_stop
       (and perhaps they should; it already does in the special case
       of error out of resume()), then we wouldn't need this.  */
    make_cleanup (breakpoint_auto_delete_contents, NULL);

    disable_watchpoints_before_interactive_call_start ();
    tp->proceed_to_finish = 1; /* We want stop_registers, please... */

    if (target_can_async_p ())
      saved_async = target_async_mask (0);

    old_cleanups2 = make_cleanup_restore_integer (&suppress_resume_observer);
    suppress_resume_observer = 1;
    make_cleanup_restore_integer (&suppress_stop_observer);
    suppress_stop_observer = 1;
    proceed (real_pc, TARGET_SIGNAL_0, 0);
    do_cleanups (old_cleanups2);

    if (saved_async)
      target_async_mask (saved_async);

    enable_watchpoints_after_interactive_call_stop ();

    discard_cleanups (old_cleanups);
  }

target.h has this:

/* This is to be used ONLY within call_function_by_hand(). It provides
   a workaround, to have inferior function calls done in sychronous
   mode, even though the target is asynchronous. After
   target_async_mask(0) is called, calls to target_can_async_p() will
   return FALSE , so that target_resume() will not try to start the
   target asynchronously. After the inferior stops, we IMMEDIATELY
   restore the previous nature of the target, by calling
   target_async_mask(1). After that, target_can_async_p() will return
   TRUE. ANY OTHER USE OF THIS FEATURE IS DEPRECATED.

   FIXME ezannoni 1999-12-13: we won't need this once we move
   the turning async on and off to the single execution commands,
   from where it is done currently, in remote_resume().  */

#define target_async_mask(MASK)	\
  (current_target.to_async_mask (MASK))

I don't see any other calls to target_async_mask.  Given that it's
only to be used by call_function_by_hand that's understandable,
but then I don't understand how target_async_mask gets restored
if proceed errors out.

I'll add a fix to my dummy-frames patch if there's a bug here.
If there isn't a bug I'll at least add a comment. :-)
[and I'll need to start breaking it into a set of smaller patches ...]

Re: call_function_by_hand doesn't restore target async?

[Pedro Alves]

On Thursday 04 December 2008 20:18:37, Doug Evans wrote:

> Should there be a cleanup to restore target_async_mask?
> 

>     if (target_can_async_p ())
>       saved_async = target_async_mask (0);
> 
>     old_cleanups2 = make_cleanup_restore_integer (&suppress_resume_observer);
>     suppress_resume_observer = 1;
>     make_cleanup_restore_integer (&suppress_stop_observer);
>     suppress_stop_observer = 1;
>     proceed (real_pc, TARGET_SIGNAL_0, 0);
>     do_cleanups (old_cleanups2);
> 
>     if (saved_async)
>       target_async_mask (saved_async);

> 
> target.h has this:
> 
> /* This is to be used ONLY within call_function_by_hand(). It provides
>    a workaround, to have inferior function calls done in sychronous
>    mode, even though the target is asynchronous. After
>    target_async_mask(0) is called, calls to target_can_async_p() will
>    return FALSE , so that target_resume() will not try to start the
>    target asynchronously. After the inferior stops, we IMMEDIATELY
>    restore the previous nature of the target, by calling
>    target_async_mask(1). After that, target_can_async_p() will return
>    TRUE. ANY OTHER USE OF THIS FEATURE IS DEPRECATED.

That's the idealistic theory anyway...  It's also used, although not
through the target vector, in linux_nat_create_inferior.  In practice, and
especially with non-stop mode, making infcalls async is ranging somewhere
from hard to impossible.  An alternative path I've considered to remove
this masking, is to add an `options' parameter to target_wait so we'd pass
a 'TARGET_WNOHANG' flag to it when you want asyncness (in fetch_inferior_event),
and pass `0' in the call in wait_for_inferior (that's always blocking).
target_wait is modelled on `wait(pid)', so it sounds a good fit to me.
I've actually implemented it that way in gdbserver in the
multiprocess branch.

> I don't see any other calls to target_async_mask.  Given that it's
> only to be used by call_function_by_hand that's understandable,
> but then I don't understand how target_async_mask gets restored
> if proceed errors out.

Right, it doesn't.  inf-loop.c:inferior_event_handler has a
drastic attitude about exceptions --- it always pops the target, which
means that a cleanup will would most of the times set the async mask
in the wrong target, and thus say, e.g., remote_async_mask_value
will still be left dangling...  I think that adjusting the
target_wait interface like described above would be the best way
to fix this.

> I'll add a fix to my dummy-frames patch if there's a bug here.

> If there isn't a bug I'll at least add a comment. :-)
> [and I'll need to start breaking it into a set of smaller patches ...]
> 

-- 
Pedro Alves

Re: call_function_by_hand doesn't restore target async?

[Pedro Alves]

On Thursday 04 December 2008 21:23:16, Pedro Alves wrote:
> Right, it doesn't.  inf-loop.c:inferior_event_handler has a
> drastic attitude about exceptions --- it always pops the target, which
> means that a cleanup will would most of the times set the async mask
> in the wrong target, and thus say, e.g., remote_async_mask_value
> will still be left dangling...  I think that adjusting the
> target_wait interface like described above would be the best way
> to fix this.

Hmmm, sorry, that's a thinko.  When doing an infcall, of course you
don't go through fetch_inferior_event...  So a cleanup would indeed
fix most cases, although a few times, like when remote.c pops the
target due to losing the connection, a cleanup wil still set the
async mask in the wrong target.  (The parameter to target_wait
would solve this 100%).

Don't you love having two ways to do the same thing?

-- 
Pedro Alves

Re: call_function_by_hand doesn't restore target async?

[Doug Evans]

On Thu, Dec 4, 2008 at 1:23 PM, Pedro Alves <pedro@codesourcery.com> wrote:
> In practice, and
> especially with non-stop mode, making infcalls async is ranging somewhere
> from hard to impossible.

If that's the general consensus, at least for now, would there be any
objection to submitting a patch that has call_function_by_hand invoke
proceed() in a TRY_CATCH?  After cleaning things up it could rethrow.
It would greatly simplify the semantic load of what one has to deal
with there, and allow for easier fixing of some bugs in the process.

Re: call_function_by_hand doesn't restore target async?

[Pedro Alves]

On Thursday 04 December 2008 21:36:31, Doug Evans wrote:
> On Thu, Dec 4, 2008 at 1:23 PM, Pedro Alves <pedro@codesourcery.com> wrote:
> > In practice, and
> > especially with non-stop mode, making infcalls async is ranging somewhere
> > from hard to impossible.
> 
> If that's the general consensus, at least for now, would there be any

Dunno if that's the consensus; I'd really love it if someone proved me wrong.

> objection to submitting a patch that has call_function_by_hand invoke
> proceed() in a TRY_CATCH?  After cleaning things up it could rethrow.

I don't object in principle, but I'm no foreteller either.  :-)

-- 
Pedro Alves