This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Prevent internal-error when computing $pc in ARM assembly code
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Martin Simmons <martin at lispworks dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Fri, 12 Jun 2015 09:21:13 -0400
- Subject: Re: [PATCH] Prevent internal-error when computing $pc in ARM assembly code
- Authentication-results: sourceware.org; auth=none
- References: <201505200949 dot t4K9nlqt015737 at heapu dot cam dot lispworks dot com> <20150609184408 dot GG2855 at adacore dot com> <201506111725 dot t5BHP5LR031293 at higson dot cam dot lispworks dot 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