[PATCH][gdb/testsuite] Allow some tests in gdb.base/store.exp to be unsupported
Andrew Burgess
andrew.burgess@embecosm.com
Mon Sep 2 18:20:00 GMT 2019
* Tom de Vries <tdevries@suse.de> [2019-09-02 13:00:21 +0200]:
> [ was: Re: [PATCH][gdb/testsuite] Rewrite gdb.base/store.exp into .s test ]
>
> On 29-08-19 20:07, Andrew Burgess wrote:
> > * Tom de Vries <tdevries@suse.de> [2019-08-29 19:47:12 +0200]:
> >
> >> Hi,
> >>
> >> The test-case gdb.base/store.exp fails with gcc 7.4.0:
> >> ...
> >> nr of unexpected failures 27
> >> ...
> >> and with gcc 4.8.5:
> >> ...
> >> nr of unexpected failures 9
> >> ...
> >>
> >> The test-case relies on "the compiler taking heed of requests for values to be
> >> stored in registers", which appearantly isn't the case anymore for
> >> modern gcc.
> >
> > Could you expand on this a little more. I took a quick look and it
> > appears more that variables just have missing location information.
>
> You're right, I jumped to a conclusion here, sorry for not being more
> careful.
>
> > Sure the test marks the variables with the 'register' keyword, but
> > surely GDB should still pass the test even if the variable is placed
> > on the stack?
> >
>
> Agreed (and indeed, not obeing the 'register' hint, emulated by "#define
> register" makes the test pass).
>
> >>
> >> Fix this by changing this into an assembly file test-case, and generating the
> >> assembly file using gcc 4.2.1.
> >>
> >> Tested on x86_64-linux.
> >>
> >> OK for trunk?
> >
> > No. What about architectures other than x86-64?
> >
> > I took a quick look, and maybe I missed something, but I don't think
> > that there are any architecture specific assembler tests in gdb.base/
> > and I don't think we should be adding any.
> >
> > Maybe we should add a version of this test into gdb.arch along with
> > the assembler file for x86-64.
> >
> > On a slightly different note, I've run into this test before, and I'm
> > pretty sure that the test is broken, it's been a while since I dug
> > into this but consider these snippets:
> >
> > ...
> >
> > float
> > add_float (register float u, register float v)
> > {
> > return u + v;
> > }
> >
> > ...
> >
> > wack_float (register float u, register float v)
> > {
> > register float l = u, r = v;
> > l = add_float (l, r);
> > return l + r;
> > }
> >
> > ...
> >
> > Part of the test involves breaking on 'add_float' then going 'up' to
> > 'wack_float' and printing 'l'. GDB expects an answer.
> >
> > My problem with this is that on many architectures, even at
> > optimisation level 0 'l' is dead, or at least non-recoverable at the
> > point of the call to add_float due to being placed in a caller saved
> > argument register.
> >
>
> I've investigated the FAILs related to the wack_float function, and the
> test-case expects to access and modify l, but it can't because there's
> no DW_AT_location for l, which AFAIU is valid behaviour of gcc for a
> register variable at -O0.
I don't understand why this should be the case. DWARF is perfectly
able to describe the location of something in a register, so why would
it be valid for GCC to skip emitting location information for
something just because it's in a register?
>
> So, ISTM the FAILs need to be fixed by marking the failing tests as
> unsupported, in case l shows up as <optimized out>.
I guess unsupported, but maybe KFAIL with an associated GCC bug would
be better?
Consider this GDB session to explain my thinking:
(gdb) break wack_float
Breakpoint 1 at 0x4005c8: file /path/to/gdb/src/gdb/testsuite/gdb.base/store.c, line 109.
(gdb) r
Starting program: /path/to/gdb/build/gdb/testsuite/outputs/gdb.base/store/store
Breakpoint 1, wack_float (u=-1, v=-2) at /path/to/gdb/src/gdb/testsuite/gdb.base/store.c:109
109 register float l = u, r = v;
(gdb) n
110 l = add_float (l, r);
(gdb) p u
$1 = -1
(gdb) p l
$2 = <optimized out>
(gdb)
We know where 'u' is, as I see it there's no reason why GCC couldn't
emit location information for 'l' that is identical to that for 'u'.
[ OK, if we're going to be supper picky then GCC could declare 'u'
dead after line 109 and then have only 'l' exist after that, but at
-O0 I'd probably hope that both 'u' and 'l' would remain alive for
the entire life of the function. ]
Thanks,
Andrew
>
> > Anyway, I agree with you that this test is in need of some clean up,
> > I'm just not convinced on this approach yet.
> >
>
> Better like this?
>
> Thanks,
> - Tom
> [gdb/testsuite] Allow some tests in gdb.base/store.exp to be unsupported
>
> The test-case gdb.base/store.exp fails with gcc 7.4.0:
> ...
> nr of unexpected failures 27
> ...
>
> The first FAIL:
> ...
> 110 l = add_float (l, r);
> (gdb) PASS: gdb.base/store.exp: continue to wack_float
> print l
> $21 = <optimized out>
> FAIL: gdb.base/store.exp: var float l; print old l, expecting -1
> ...
> relates to this bit in the test-case (compiled at -O0):
> ...
> 106 float
> 107 wack_float (register float u, register float v)
> 108 {
> 109 register float l = u, r = v;
> 110 l = add_float (l, r);
> 111 return l + r;
> 112 }
> ...
> and it expects to be able to read and modify variable l before executing line
> 110, but it already fails to read the value, because l has no DW_AT_location
> attribute in the debug info.
>
> Variable l is declared with the register keyword, and GCC implements the
> register keyword at -O0 like so:
> ...
> the compiler allocates distinct stack memory for all variables that do not
> have the register storage-class specifier; if register is specified, the
> variable may have a shorter lifespan than the code would indicate and may
> never be placed in memory.
> ...
>
> The fact that l has no DW_AT_location attribute, matches with the documented
> "variable may have a shorter lifespan that code would indicate", (though it
> is the most extreme case of it) so the gcc behaviour is valid. We can of
> course improve gcc to generate better debuginfo (filed gcc PR91611), but
> this not a wrong-debug problem.
>
> [ The test-case passes with gcc 4.2.1, but for the failing test discussed
> above, it passes simply because it doesn't store l in a register. ]
>
> With the debug info missing for l, reading and setting l is unsupported, so
> fix the FAIL by marking the test UNSUPPORTED instead.
>
> Tested on x86_64-linux.
>
> gdb/testsuite/ChangeLog:
>
> 2019-08-29 Tom de Vries <tdevries@suse.de>
>
> * gdb.base/store.exp: Allow register variables to be optimized out at
> -O0.
>
> ---
> gdb/testsuite/gdb.base/store.exp | 65 +++++++++++++++++++++++++++-------------
> 1 file changed, 45 insertions(+), 20 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.base/store.exp b/gdb/testsuite/gdb.base/store.exp
> index c5a7584101..9c19ce15a7 100644
> --- a/gdb/testsuite/gdb.base/store.exp
> +++ b/gdb/testsuite/gdb.base/store.exp
> @@ -55,18 +55,29 @@ proc check_set { t l r new add } {
> }
> }
>
> - gdb_test "print l" " = ${l}" \
> - "${prefix}; print old l, expecting ${l}"
> - gdb_test "print r" " = ${r}" \
> - "${prefix}; print old r, expecting ${r}"
> - gdb_test_no_output "set variable l = 4" \
> - "${prefix}; setting l to 4"
> - gdb_test "print l" " = ${new}" \
> - "${prefix}; print new l, expecting ${new}"
> - gdb_test "next" "return l \\+ r;" \
> - "${prefix}; next over add call"
> - gdb_test "print l" " = ${add}" \
> - "${prefix}; print incremented l, expecting ${add}"
> + set supported 1
> + set test "${prefix}; print old l, expecting ${l}"
> + gdb_test_multiple "print l" "$test" {
> + -re " = <optimized out>\r\n$gdb_prompt $" {
> + unsupported $test
> + set supported 0
> + }
> + -re " = ${l}\r\n$gdb_prompt $" {
> + pass $test
> + }
> + }
> + if { $supported } {
> + gdb_test "print r" " = ${r}" \
> + "${prefix}; print old r, expecting ${r}"
> + gdb_test_no_output "set variable l = 4" \
> + "${prefix}; setting l to 4"
> + gdb_test "print l" " = ${new}" \
> + "${prefix}; print new l, expecting ${new}"
> + gdb_test "next" "return l \\+ r;" \
> + "${prefix}; next over add call"
> + gdb_test "print l" " = ${add}" \
> + "${prefix}; print incremented l, expecting ${add}"
> + }
> }
>
> check_set "charest" "-1 .*" "-2 .*" "4 ..004." "2 ..002."
> @@ -81,20 +92,34 @@ check_set "doublest" "-1" "-2" "4" "2"
> #
>
> proc up_set { t l r new } {
> + global gdb_prompt
> +
> set prefix "upvar ${t} l"
> gdb_test "tbreak add_${t}"
> gdb_test "continue" "return u . v;" \
> "continue to add_${t}"
> gdb_test "up" "l = add_${t} .l, r.;" \
> "${prefix}; up"
> - gdb_test "print l" " = ${l}" \
> - "${prefix}; print old l, expecting ${l}"
> - gdb_test "print r" " = ${r}" \
> - "${prefix}; print old r, expecting ${r}"
> - gdb_test_no_output "set variable l = 4" \
> - "${prefix}; set l to 4"
> - gdb_test "print l" " = ${new}" \
> - "${prefix}; print new l, expecting ${new}"
> +
> + set supported 1
> + set test "${prefix}; print old l, expecting ${l}"
> + gdb_test_multiple "print l" "$test" {
> + -re " = <optimized out>\r\n$gdb_prompt $" {
> + unsupported $test
> + set supported 0
> + }
> + -re " = ${l}\r\n$gdb_prompt $" {
> + pass $test
> + }
> + }
> + if { $supported } {
> + gdb_test "print r" " = ${r}" \
> + "${prefix}; print old r, expecting ${r}"
> + gdb_test_no_output "set variable l = 4" \
> + "${prefix}; set l to 4"
> + gdb_test "print l" " = ${new}" \
> + "${prefix}; print new l, expecting ${new}"
> + }
> }
>
> up_set "charest" "-1 .*" "-2 .*" "4 ..004."
More information about the Gdb-patches
mailing list