Re: [RFC] Fix variable lifetime related problem in gdb.base/store.exp

[Doug Evans <dje@google.com>]

On Wed, Jan 19, 2011 at 3:26 PM, Kevin Buettner <kevinb@redhat.com> wrote:
> I'm seeing some interesting failures in gdb.base/store.exp for
> mips64vrel-elf with --target_board set to vr4300-sim.  (The exact
> target_board setting isn't all that critical; vr4300-sim is but one
> example where things go awry.)
>
> Here is relevant log file output showing the problem:
>
> tbreak add_float
> Temporary breakpoint 15 at 0xa0100414: file /ironwood1/sourceware-clean/mips64vrel-elf/../src/gdb/testsuite/gdb.base/store.c, line 47.
> (gdb) PASS: gdb.base/store.exp: tbreak add_float
> continue
> Continuing.
>
> Temporary breakpoint 15, add_float (u=-1, v=-2) at /ironwood1/sourceware-clean/mips64vrel-elf/../src/gdb/testsuite/gdb.base/store.c:47
> 47        return u + v;
> (gdb) PASS: gdb.base/store.exp: continue to add_float
> up
> #1  0xa010072c in wack_float (u=-1, v=-2) at /ironwood1/sourceware-clean/mips64vrel-elf/../src/gdb/testsuite/gdb.base/store.c:110
> 110       l = add_float (l, r);
> (gdb) PASS: gdb.base/store.exp: upvar float l; up
> print l
> $48 = -1.21996474e-19
> (gdb) FAIL: gdb.base/store.exp: upvar float l; print old l, expecting -1
> print r
> $49 = -2
> (gdb) PASS: gdb.base/store.exp: upvar float l; print old r, expecting -2
> set variable l = 4
> (gdb) PASS: gdb.base/store.exp: upvar float l; set l to 4
> print l
> No symbol "l" in current context.
> (gdb) FAIL: gdb.base/store.exp: upvar float l; print new l, expecting 4
> tbreak add_double
> Temporary breakpoint 16 at 0xa0100454: file /ironwood1/sourceware-clean/mips64vrel-elf/../src/gdb/testsuite/gdb.base/store.c, line 53.
> (gdb) PASS: gdb.base/store.exp: tbreak add_double
> continue
> Continuing.
> mips-core: 4 byte read to unmapped address 0x40800000 at 0x40800000
>
> Program received signal SIGBUS, Bus error.
> 0x40800000 in ?? ()
> (gdb) FAIL: gdb.base/store.exp: continue to add_double
>
> These failures occur due to `l' in wack_float() being assigned (for
> this particular MIPS toolchain) to the `ra' (return address) register.
>
> The function wack_float() is defined as follows:
>
> float
> wack_float (register float u, register float v)
> {
>  register float l = u, r = v;
>  l = add_float (l, r);
>  return l + r;
> }
>
> The test case places a breakpoint in add_float(), runs to that
> breakpoint, and then goes up a frame to examine and modify certain
> variables.
>
> The lifetime of `l' with value obtained from `u' ends with invocation
> of add_float().  (More precisely, it ends as soon as l's value is
> copied to the argument register for the call to add_float().)  The
> result of the add_float() call is placed in `l' when the call is
> finished.
>
> Thus it's perfectly acceptable (though a bit perverse) to put l's value
> in the ra register.  (However, given that this is occurring at -O0, it is
> also very annoying that the user is unable to inspect l's pre-call
> value.)
>
> In any case, due to the fact that the lifetime of l-with-the-value-of-u
> ends once the call to add_float() is set up, the compiler is then free
> to use ra for other purposes which, of course, during a call, is used
> to temporarily hold the return address.
>
> This is why the test fails to print the old value of 'l'; the register
> to which it was assigned is now being used for another purpose, i.e,
> holding the return address.
>
> And, moreover, it explains the BUS error after the assignment:
> add_float() is a leaf function and, therefore, does not need to save
> ra on the stack.  So it simply uses it as is not expecting it to be
> changed upon return.
>
> That BUS error is reponsible for many cascade failures later on.
>
> My fix, below, is to assign (via GDB's "set variable" command) to `r'
> instead of `l' since the lifetime of r's value set at the beginning of
> the function extends beyond the call to add_float().  The patch also
> causes 'r', instead of 'l', to be examined immediately after the
> assignment.
>
> It won't fix the initial failure with `l' apparently having the wrong
> value.  I think this test ought to be preserved to remind us - should
> it fail - that although GCC's generated code might be correct, it is
> not as useful as it should be for debugging purposes.
>
> This fix does prevent the BUS error and the resulting cascade
> failures.  Stopping the cascade failures is important because we can
> then determine if there are any real failures amidst the cascade.
>
> Comments?
>
>        * gdb.base/store.exp (proc up_set): Set, and check, the variable
>        `r' rather than `l' due to the fact that the lifetime for the
>        pre-call value for `l' need not extend beyond the call of
>        add_<type>.
>
> Index: testsuite/gdb.base/store.exp
> ===================================================================
> RCS file: /cvs/src/src/gdb/testsuite/gdb.base/store.exp,v
> retrieving revision 1.17
> diff -u -p -r1.17 store.exp
> --- testsuite/gdb.base/store.exp        1 Jan 2011 15:33:43 -0000       1.17
> +++ testsuite/gdb.base/store.exp        19 Jan 2011 22:22:38 -0000
> @@ -95,10 +95,10 @@ proc up_set { t l r new } {
>        "${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}"
> +    gdb_test_no_output "set variable r = 4" \
> +       "${prefix}; set r to 4"
> +    gdb_test "print r" " = ${new}" \
> +       "${prefix}; print new r, expecting ${new}"
>  }
>
>  up_set "charest" "-1 .*" "-2 .*" "4 ..004."
>

Nasty.

The worry of course is that by changing the test to use r instead of l
are we breaking the intent of the test, at least on *some* target.
[since r has to survive the call gcc may be less inclined to put it in
a register]
OTOH, does up_set really provide any more coverage, as far as the
intent of the test (setting values that are in registers), than
check_set.  I don't know.

Since the high order bit here is, AIUI, to prevent the SIGBUS, an
alternative is to restore l afterwards.
I don't know if that's a better way to go.