Re: [PATCH] Guard against killing unrelated processes in amd64-disp-step.exp

[Andrew Burgess <aburgess@redhat.com>]

Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes:

> When testing current gdb trunk on Solaris/amd64, the whole session was
> reliably terminated by make check.  I could trace this to the following
> entry in gdb.arch/amd64-disp-step/gdb.log:
>
> FAIL: gdb.arch/amd64-disp-step.exp: add into rcx: send_signal=on: get inferior pid
> Executing on target: kill -ALRM -1    (timeout = 300)
> builtin_spawn -ignore SIGHUP kill -ALRM -1
>
> If $inferior_pid doesn't refer a single process for some reason, this
> kill would terminate either a process group or the whole session.
>
> This patch avoids this by ensuring that the pid arg is positive.
>
> Tested on amd64-pc-solaris2.11 and x86_64-pc-linux-gnu.
>
> Ok for trunk?
>
> 	Rainer
>
> -- 
> -----------------------------------------------------------------------------
> Rainer Orth, Center for Biotechnology, Bielefeld University
>
>
> diff --git a/gdb/testsuite/gdb.arch/amd64-disp-step.exp b/gdb/testsuite/gdb.arch/amd64-disp-step.exp
> --- a/gdb/testsuite/gdb.arch/amd64-disp-step.exp
> +++ b/gdb/testsuite/gdb.arch/amd64-disp-step.exp
> @@ -222,7 +222,10 @@ proc rip_test { reg test_start_label tes
>  	    # If we use 'signal' to send the signal GDB doesn't actually do
>  	    # the displaced step, but instead just delivers the signal.
>  	    set inferior_pid [get_inferior_pid]
> -	    remote_exec target "kill -ALRM $inferior_pid"
> +	    # Ensure that $inferior_pid refers to a single process.
> +	    if {$inferior_pid > 0} {
> +		remote_exec target "kill -ALRM $inferior_pid"
> +	    }

Does this not hide the fact that the test is no longer doing what it
expected?

I'm fine with the 'if {$inferior_pid > 0}' being added to ensure we
don't signal some random process(es), but I think we should probably add
something like:

  gdb_assert {[expr $inferior_pid > 0]} \
    "check for a sane inferior pid"
  if {$inferior_pid > 0} {
    remote_exec target "kill -ALRM $inferior_pid"
  }

This way you will still see a FAIL.

Thoughts?

Thanks,
Andrew