Re: [PATCH] Prevent internal-error when computing $pc in ARM assembly code

[Joel Brobecker <brobecker@adacore.com>]

> Thanks for looking at it.
> 
> The code being debugged uses r11 (ARM_FP_REGNUM) as a global pointer
> rather than a frame register, so lines 1968...1984 of arm-tdep.c ("We
> have no symbol information.") computes bogus values for prologue_start
> and prologue_end.

OK, so indeed the problem occurs because arm_analyze_prologue
was given a bogus PC range. Great!

> > Also, for our piece of mind, we normally ask that the change be
> > validated by running the testsuite. Did you do that? If yes,
> > on which platform?
> 
> Yes, I ran make check-gdb on Raspbian GNU/Linux 7, but it produces
> around 1200 failures with or without my change, including some
> non-deterministic ones.

That's perfect. What we ask is that the failures before and after
are the same.

> Ah, sorry.  Here is the new version.  I've also included a new test, which
> passes for me on Raspbian GNU/Linux 7.

Awesome, we love tests! Thank you.

> gdb/ChangeLog:
> 
> 	* arm-tdep.c (arm_analyze_prologue): Read memory without throwing an
> 	exception, to allow debugging of assembly code.
> 
> gdb/testsuite/ChangeLog:
> 	* gdb.arch/arm-r11-non-pointer.S: New file.
> 	* gdb.arch/arm-r11-non-pointer.exp: New file.

Looks good. A few little things in your new testcase...

> +++ b/gdb/testsuite/gdb.arch/arm-r11-non-pointer.S
> @@ -0,0 +1,45 @@
> +/* Copyright 2010-2015 Free Software Foundation, Inc.

Did you really mean for the date range to be 2010-2015?
If yes, then great. If not, can you adjust it?

> diff --git a/gdb/testsuite/gdb.arch/arm-r11-non-pointer.exp b/gdb/testsuite/gdb.arch/arm-r11-non-pointer.exp
> new file mode 100644
> index 0000000..1def957
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/arm-r11-non-pointer.exp
> @@ -0,0 +1,69 @@
> +# Copyright 2010-2015 Free Software Foundation, Inc.

Same here...

> +set testfile "arm-r11-non-pointer"
> +set srcfile ${testfile}.S
> +set binfile ${objdir}/${subdir}/${testfile}

Can you use "standard_testfile .S" instead of the 3 commands above?

> +set additional_flags "-Wa,-g"
> +
> +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug $additional_flags]] != "" } {
> +    untested arm-r11-non-pointer.exp
> +    return -1
> +}
> +
> +# Get things started.
> +
> +clean_restart ${testfile}

Can you use prepare_for_testing instead of gdb_compile and
clean_restart?

> +gdb_test "x/i \$pc" \
> +    ".*bl.*0x.*" \
> +    "x/i pc"
> +
> +gdb_test "stepi" \
> +    "\\\?\\\? \\\(\\\) at .*$srcfile:.*" \
> +    "stepi"
> +
> +gdb_test "x/i \$pc" \
> +    ".*ldr.*r11,.*" \
> +    "x/i pc"
> +
> +gdb_test "stepi" \
> +    "\\\?\\\? \\\(\\\) at .*$srcfile:.*" \
> +    "stepi"
> +
> +gdb_test "x/i \$pc" \
> +    ".*mov.*r11,.*r11.*" \
> +    "x/i pc"
> +

We require that all test "labels" be unique. Would you mind ammending
them to make them so. Eg: replace the first "x/i pc" by "x/i pc at
such and such instruction", etc. Same for the "stepi" ones.

Thank you,
-- 
Joel