Re: [PATCH v2] Fix failure to detach if threads exit while detaching on linux

[Pedro Alves <palves@redhat.com>]

On 06/03/2016 03:49 PM, Antoine Tremblay wrote:

> @@ -1491,7 +1523,28 @@ linux_nat_detach (struct target_ops *ops, const char *args, int from_tty)
>        linux_fork_detach (args, from_tty);
>      }
>    else
> -    linux_ops->to_detach (ops, args, from_tty);
> +    {
> +      TRY
> +	{
> +	  linux_ops->to_detach (ops, args, from_tty);
> +	}
> +      CATCH (ex, RETURN_MASK_ERROR)
> +	{
> +	  if (!check_ptrace_stopped_lwp_gone (main_lwp))
> +	    {
> +	      delete_lwp (main_lwp->ptid);

Is deleting the right thing to do?  I think we'll likely
crash, the next time we try to do something (e.g., try
to detach again).  Seems like on the gdbserver side,
the lwp isn't deleted in this case.

> +	      throw_exception (ex);
> +	    }
> +	  /* Ignore the error since the thread is gone already.  */
> +	  else
> +	    {
> +	      inferior_ptid = null_ptid;
> +	      detach_inferior (pid);
> +	      inf_child_maybe_unpush_target (ops);

Please factor these three lines out of inf_ptrace_attach into
a separate function, exported from inf-ptrace.h and called
from both places.  E.g.:

  extern void inf_ptrace_attach_success (struct target_ops *ops);

> +	    }
> +	}
> +    }
> +  delete_lwp (main_lwp->ptid);
>  }
>  

> +clean_restart ${testfile}
> +
> +if ![runto_main] {
> +    fail "Can't run to main to check for trace support"

"to check for trace support" is stale.

> +    return -1
> +}
> +
> +gdb_breakpoint "_exit"
> +gdb_continue_to_breakpoint "_exit" ".*_exit.*"
> +
> +# Since the gdbserver-native board ends remote debugging on detach,
> +# we can't use gdb_test as this function intercepts the Ending remote
> +# debugging message and tries to restart gdb.

Eh, that looks like a bogus think for gdb_test_multiple to be doing.

Seems like it's been there since "forever":

commit 19fa4a0af34ff07f260fc75e903ab92d8ca03d33
Author:     Mike Werner <mtw@cygnus>
AuthorDate: Sun Feb 21 20:03:55 1993 +0000

        * gdb/testsuite: Initial creation of gdb/testsuite.
        Migrated dejagnu testcases and support files for testing nm to
        gdb/testsuite from deja-gnu.  These files were moved "as is"
        with no modifications.  This migration is part of a major overhaul
        of dejagnu.  The modifications to these testcases, etc., which
        will allow them to work with the new version of dejagnu will be
        made in a future update.

I'd support just removing that bit.

In any case, there's no need to avoid gdb_test/gdb_test_multiple.
Any user-specified pattern overrides gdb_test_multiple's internal
patterns.  So you just need to intercept the  "Ending remote debugging"
message yourself.

> However, it can't do that
> +# as gdb_exit on this board will call monitor exit which will fail as
> +# we're not remote debugging anymore after a detach in this case.
> +send_gdb "detach\n"
> +gdb_expect {

set test "detach"
gdb_test_multiple $test $test {
    -re "Detaching from .*, process $decimal\r\nEnding remote debugging\.\r\n$gdb_prompt $" {
        # This is what you get with "target remote".
        pass $test
    }
    -re "Detaching from .*, process $decimal\r\n$gdb_prompt $" {
        pass $test
    }
}

> +    -re "Detaching from .*, process $decimal\r\nEnding remote debugging\." {

Missing $gdb_prompt match.

> +	#Don't try to exit gdbserver see above comment.
> +	set gdbserver_reconnect_p 1

I don't understand this one.  It shouldn't be needed.

> +	pass "detach non-extended"

Please use the same test message for all branches:

  pass $test

> +    }
> +    -re "Detaching from .*, process $decimal\r\n$gdb_prompt $" {
> +	pass "detach"
> +    }
> +    -re ".*$gdb_prompt $" {
> +        fail "detach"
> +    }
> +    timeout {
> +	fail "detach timeout"

FYI, this would have been the standard:

> +	fail "$test (timeout)"

> +    }
> +}
> 

Thanks,
Pedro Alves