[PR,tdep/17379] Fix internal-error when stack pointer is invalid

  Thanks Sergio for your feedback. I'm resending the patch with an additional
testcase as you suggested.

--
Edjunior

gdb/ChangeLog
2014-09-11  Edjunior Barbosa Machado  <emachado@linux.vnet.ibm.com>
	    Ulrich Weigand  <uweigand@de.ibm.com>

	PR tdep/17379
	* rs6000-tdep.c (rs6000_frame_cache): Use safe_read_memory_integer
	instead of read_memory_unsigned_integer.

gdb/testcase/ChangeLog
2014-09-11  Edjunior Barbosa Machado  <emachado@linux.vnet.ibm.com>

	PR tdep/17379
	* gdb.arch/powerpc-stackless.S: New file.
	* gdb.arch/powerpc-stackless.exp: New file.

---
 gdb/rs6000-tdep.c                            |   11 ++++--
 gdb/testsuite/gdb.arch/powerpc-stackless.S   |   24 +++++++++++++
 gdb/testsuite/gdb.arch/powerpc-stackless.exp |   48 ++++++++++++++++++++++++++
 3 files changed, 80 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.arch/powerpc-stackless.S
 create mode 100644 gdb/testsuite/gdb.arch/powerpc-stackless.exp
  

On Thursday, September 11 2014, Edjunior Barbosa Machado wrote:

> Thanks Sergio for your feedback. I'm resending the patch with an additional
> testcase as you suggested.

Nice, thanks :-).

> diff --git a/gdb/testsuite/gdb.arch/powerpc-stackless.exp b/gdb/testsuite/gdb.arch/powerpc-stackless.exp
> new file mode 100644
> index 0000000..f4b2a90
[...]
> +standard_testfile powerpc-stackless.S

Just another really minor nit, no need to resubmit the patch because of
this.  You can write:

  standard_testfile .S

Other than that, it is perfect.

Thanks a lot!
  

On 09/12/2014 03:46 AM, Edjunior Barbosa Machado wrote:
> +# Run until SIGSEGV.
> +gdb_run_cmd
> +
> +gdb_expect {
> +    -re "Program received signal SIGSEGV.*$gdb_prompt $" {
> +	pass "run until SIGSEGV"
> +    }
> +    -re ".*$gdb_prompt $" {
> +	fail "run until SIGSEGV"
> +    }
> +    timeout {
> +	fail "run until SIGSEGV (timeout)"
> +    }
> +}

gdb_expect should only be used when gdb_test or gdb_test_multiple
really can't be used.  Please write instead:

gdb_run_cmd

set test "run until SIGSEGV"
gdb_test_multiple "" $test {
    -re "Program received signal SIGSEGV.*$gdb_prompt $" {
	pass $test
    }
}

gdb_test_multiple will already issue a FAIL if it sees the prompt
or gets a timeout, and in addition will catch other problems,
like internal errors.

Thanks,
Pedro Alves
  

> set test "run until SIGSEGV"
> gdb_test_multiple "" $test {
>     -re "Program received signal SIGSEGV.*$gdb_prompt $" {
> 	pass $test
>     }
> }

Taking this one step further, wouldn't a simpler gdb_test also work
in this case?
  

On 09/12/2014 02:00 PM, Joel Brobecker wrote:
>> set test "run until SIGSEGV"
>> gdb_test_multiple "" $test {
>>     -re "Program received signal SIGSEGV.*$gdb_prompt $" {
>> 	pass $test
>>     }
>> }
> 
> Taking this one step further, wouldn't a simpler gdb_test also work
> in this case?

Yeah, good point.

We still need to use gdb_run_cmd to cover remote testing,
so that'd be:

 gdb_test "" "Program received signal SIGSEGV.*" "run until SIGSEGV"

ISTR that gdb_test doesn't allow empty command, but I may well
be mistaken.  And if it doesn't, maybe it should.

Thanks,
Pedro Alves
  

> We still need to use gdb_run_cmd to cover remote testing,
> so that'd be:
> 
>  gdb_test "" "Program received signal SIGSEGV.*" "run until SIGSEGV"
> 
> ISTR that gdb_test doesn't allow empty command, but I may well
> be mistaken.  And if it doesn't, maybe it should.

This is me pretending that I had noticed that the command was empty
and knowing that this was still OK :-). But once you mentioned it,
I knew I had already done something like that. See gdb.ada/bp_reset.exp:

    gdb_run_cmd
    gdb_test "" "Breakpoint $decimal, foo\\.nested_sub \\(\\).*"

Doing a quick grep, we have a number of occurences where we use
an empty command when calling gdb_test.  And looking at gdb_test's
implementation, it just passes the first argument to gdb_test_multiple,
so it should indeed be equivalent.  (phew, that was close! ;-)).
  

On 09/12/2014 02:50 PM, Joel Brobecker wrote:
>> We still need to use gdb_run_cmd to cover remote testing,
>> so that'd be:
>>
>>  gdb_test "" "Program received signal SIGSEGV.*" "run until SIGSEGV"
>>
>> ISTR that gdb_test doesn't allow empty command, but I may well
>> be mistaken.  And if it doesn't, maybe it should.
> 
> This is me pretending that I had noticed that the command was empty
> and knowing that this was still OK :-). But once you mentioned it,
> I knew I had already done something like that. See gdb.ada/bp_reset.exp:
> 
>     gdb_run_cmd
>     gdb_test "" "Breakpoint $decimal, foo\\.nested_sub \\(\\).*"
> 
> Doing a quick grep, we have a number of occurences where we use
> an empty command when calling gdb_test.  And looking at gdb_test's
> implementation, it just passes the first argument to gdb_test_multiple,
> so it should indeed be equivalent.  (phew, that was close! ;-)).

:-)

I'm writing a test that converts all gdb_run_cmd -> gdb_expect
cases to avoid this from spreading further.

Thanks,
Pedro Alves