This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: Regression: Re: [PATCH] Fix some i386 unwinder inconcistencies
> Date: Mon, 13 Jun 2011 18:11:14 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
>
> On Mon, 13 Jun 2011 17:37:01 +0200, Mark Kettenis wrote:
> > > -PASS: gdb.base/watchpoint-cond-gone.exp: Catch the no longer valid watchpoint
> > > +FAIL: gdb.base/watchpoint-cond-gone.exp: Catch the no longer valid watchpoint
> >
> > Odd, that tests still passes for me on i386-unknown-openbsd4.9.
>
> > PASS: gdb.base/watchpoint-cond-gone.exp: Catch the no longer valid watchpoint
> >
> > Something did change though. Before my change:
>
> In both cases on Fedora it stops at the same place:
> 0x080483e0 in func () at ./gdb.base/watchpoint-cond-gone.c:28^M
Are you sure? If that's the case, there must be debug info that tells
GDB that the watchpoint goes out of scope. Smells like there is a
flaw in the watchpoint code where it notices that the watchpoint goes
out of scope, but still tries to evaluate the watchpoint condition.
> Which seems correct to me as it is -O0 -g code, therefore without variable
> tracking for instruction-precise DW_AT_location, therefore variables become
> invalid by the `leave' instruction:
>
> 80483df: c9 leave
> 80483e0: c3 ret
>
>
> > Something did change though. Before my change:
> > So the watchpoint went out of scope before the function returned.
>
> Yes, because the frame got destroyed, the variable is no longer valid.
True.
> > Whereas after my change:
> > the watchpoint went out of scope when the function returned, as I believe it should.
>
> I do not agree, when you stop at the `ret' instruction above the watchpoint
> should be already deleted there because its watched variable is no longer
> valid.
Frame IDs are nothing but a unique identifier for a particular
invocation of a function. We're still inside that function when we're
at the 'ret' instruction, not in a different function, so the frame ID
should be the same. This is how the frame unwinding code was
designed. There is other code in GDB that depends on this.
> > > -XFAIL: gdb.mi/mi-watch.exp: sw: watchpoint trigger (stopped at wrong place)
> > > +XFAIL: gdb.mi/mi-watch.exp: sw: watchpoint trigger (unknown output after running)
> > > -XFAIL: gdb.mi/mi2-watch.exp: sw: watchpoint trigger (stopped at wrong place)
> > > +XFAIL: gdb.mi/mi2-watch.exp: sw: watchpoint trigger (unknown output after running)
> >
> > Ok, I'm seeing these as well. Didn't classify these as a regression
> > since they went from XFAIL to XFAIL. They seem to be related to the
> > fact that I changed get_frame_pc into get_frame_func. That change is
> > correct though.
>
> If you believe it is correct then you should fix the testcase.
> I find "(stopped at wrong place)"->"(unknown output after running)" to be
> a regression.
Probably. The MI tests are mostly pure magic to me though :(.
> > I think this can be avoided by implementing the
> > in_function_epilogue_p() gdbarch method for i386/amd64. In fact, that
> > method already seems to be implemented. It just isn't registered.
>
> After `leave' the local variables are no longer valid, they are already
> located under $sp and can be overwritten by any interrupt that time, no GDB
> unwinder can fix it.
Right. But it is not the job of the unwinder to fix this.
There should be debug info to tell us exactly when a certain variable
goes out of scope, and the breakpoint/watchpoint code should use it.
In absence of that debug info, assuming that the watchpoint goes out
of scope when the function returns, combined with the
in_function_epilogue_p() check will have to do the job.
> Also for the epilogue unwinder you would need to somehow fix:
> 1441 /* This restriction could be lifted if other unwinders are known to
> 1442 compute the frame base in a way compatible with the DWARF
> 1443 unwinder. */
> 1444 if (! frame_unwinder_is (this_frame, &dwarf2_frame_unwind))
> 1445 error (_("can't compute CFA for this frame"));
All unwinders are supposed to return a frame base that is "compatible"
amongst unwinders, including the DWARF one. Now that may be tricky if
compilers don't agree on what the frame base (CFA) is. But we should
get this right for GCC, and that's all I care about. If you'd ask me,
that check should be removed.