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

[Antoine Tremblay <antoine dot tremblay at ericsson dot com>]

Pedro Alves writes:

> 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.
>

Indeed, OK.

>> +	      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.:
>

OK.

>   extern void inf_ptrace_attach_success (struct target_ops *ops);
>

OK. (detach_success)

>> +	    }
>> +	}
>> +    }
>> +  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.
>

Oops thanks.

>> +    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.
>

Right, I tested that before without sucess but had not the perfect regex
yet...

I'll use this now. thanks.

>> 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.
>

OK.

>> +	#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.
>

This one is needed since on the teardown of the .exp test gdb_exit is
called and calls monitor_exit which fails with a timeout this avoids
this and the original gdb_exit is called.

>> +	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)"
>
>> +    }
>> +}
>>
Noted, thx.