Answearing to both Daniel and Nick at the same time.

A Monday 17 March 2008 22:05:15, Daniel Jacobowitz wrote:
> >
> > So, I split linux_nat_detach in two (linux_nat_detach_inferior, and
> > linux_nat_detach proper which calls linux_nat_detach_inferior), and
> > added a re_check_for_thread_db, to be used after following the child,
> > so thread_db knows about the new current inferior.
>
> I don't like this :-( There's more that thread_db_attach does.  If we
> don't disable thread event reporting, then it's likely to stop and
> wait for the debugger the next time it creates a thread.  Yes, this
> was already broken when following a vfork.  And it may be broken if
> the child creates threads when we follow the parent of a fork.  But
> if we can avoid breaking it further, that would be nice.
>

Ok.  Although we could do the rest from a forget_about_thread_db function,
after tinkering about several other possibilities (including
passing magic args to target_detach ((char*) -1, -1) :-), ...

> Do we really have to disable async when detaching?  Maybe we could do
> so only at attach and create, if it doesn't matter how async is set
> the rest of the time.  Remove the normal_mask / blocked_mask bits
> from linux_nat_disable_async and call it in else blocks after the
> two calls to linux_nat_enable_async.  What do you think?
>

... I've settled for this.  In doing that, I added a way to
drain the local event queue of all the pending events we
already waited on of lwp's we're detaching from, so they
don't linger around always waiking up the event loop.  linux_nat_kill
already drains the events, in the non-multifork case.  The multifork
case doesn't kill all lwp's explicitly, only the main processes,
so I drain the events in that case.  When we get to mourn by the
normal exit path, by definition there should be no
pending events left.

The other change I made, was in linux_nat_terminal_{inferior|ours}, 
and linux_nat_wait, regarding the passing SIGINT to the inferior.
I noticed that interrupting the inferior with ^C wasn't working
in the attach case in the previous patches.  To fix it, I call
set_siging_trap in linux_nat_terminal_inferior, and clear it in
linux_nat_terminal_ours, but only in the sync_execution case.
In linux_nat_wait, only do the SIGINT dance if in sync mode.
This is similar to what remote.c does, although remote.c uses
an async_signal_handler for that.  I had made a patch that
moves that code from remote.c into event-loop.c for targets other
than remote to share the functionality/implementation, but ended
up not using it, because native unix targets don't need
the complexity.  Windows will, though.

----


A Tuesday 18 March 2008 02:46:58, Nick Roberts wrote:
> I've really gone through your patch to try to understand it, not review it
> but you might as well have my thoughts.
>
>  > 	* linux-nat.h (re_check_for_thread_db): Declare.
>
> recheck_for_thread_db (assuming it means "check again").
>
> The word "re" on it's own is an abbreviation for "regarding" and
> in GDB it's generally used for "regular expression".
>

I see.  Well, Daniel was against that change, so that problem
is gone.

>  >...
>  > +static void
>  > +clear_waitpid_queue (void)
>  > +{
>  > +  struct waitpid_result *event = waitpid_queue;
>  > +  while (event)
>  > +    {
>  > +      struct waitpid_result *next = event->next;
>  > +      xfree (event);
>  > +      event = next;
>  > +    }
>  > +  waitpid_queue = NULL;
>  > +}
>
>    struct waitpid_result *next, *event = waitpid_queue;
>    while (event)
>      {
>        next = event->next;
>        xfree (event);
>        event = next;
>
> I guess the compiler will optimise it anyway but shouldn't
> the declaration be outside the loop?
>

I don't understand why you find that an issue.  That variable
is only used inside the while scope.  Anyway, this function
can now replaced by a call to the new drain_queued_events.

>  >...
>  > +/* Disable async mode.  */
>  > +
>  > +static void
>  > +linux_nat_disable_async (void)
>
> and
>
>  > +/* Enable async mode.  */
>  > +
>  > +static void
>  > +linux_nat_enable_async (void)
>
>  Previously you've added linux_nat_async_events (int enable).  So should
>  this be:
>
>  linux_nat_toggle_async (int enable)
>
>  for consistency?  (or linux_nat_async_events ->
> linux_nat_async_disable_events linux_nat_async_enable_events)
>

Indeed.  See new patch.  The form that takes an int is better suited
for the three cases introduced, so I kept that form.  Thanks for
the suggestion, it looks nicer now.

>  >...
>  > +  add_setshow_zinteger_cmd ("lin-lwp-async", no_class,
>  > +			    &debug_linux_nat_async, _("\
>  > +Set debugging of GNU/Linux async lwp module."), _("\
>  > +Show debugging of GNU/Linux async lwp module."), _("\
>  > +Enables printf debugging output."),
>  > +			    NULL,
>  > +			    show_debug_linux_nat_async,
>  > +			    &setdebuglist, &showdebuglist);
>
> add_setshow_boolean_cmd ("lin-lwp-async", class_maintenance,
>
> I guess you copied from "lin-lwp" but I think this should be
> boolean too (only a few currently are: timestamp and xml)
>
> The other "set debug" commands are also in class_maintenance, although
> I'm not sure what effect that has here.

I've moved them in the class_maintenance.  I suspect that
the debug commands are integers not booleans so we can have
several debug levels.  I count approximatelly the same amount of
boolean vs integer debug mode vars.

New patch attached.  Again no regressions on x86_64-unknown-linux-gnu,
if async mode is off (default), and only regressions on define.exp (*)
if async is on, provided this patch is in: 
http://sourceware.org/ml/gdb-patches/2008-03/msg00233.html
(fixes commands.exp)

(*) - again, not related to this patch, it's a general async problem.

-- 
Pedro Alves