[RFA] breakpoint.c, scanning epilogue if frame chain is invalid

[RFA] breakpoint.c, scanning epilogue if frame chain is invalid

[Corinna Vinschen]

Hi,

I'd like to propose the following patch to breakpoint.c:

	* breakpoint.c (watchpoint_check): Check for pc being in an
	epilogue if watchpoint frame couldn't be found.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.126
diff -u -p -r1.126 breakpoint.c
--- breakpoint.c	9 Aug 2003 14:57:30 -0000	1.126
+++ breakpoint.c	3 Sep 2003 09:14:02 -0000
@@ -2411,7 +2411,7 @@ watchpoint_check (void *p)
 	 Since we can't rely on the values of local variables after the
 	 stack has been destroyed, we are treating the watchpoint in that
 	 state as `not changed' without further checking. */
-      if (within_current_scope && fr == get_current_frame ()
+      if ((!within_current_scope || fr == get_current_frame ())
           && gdbarch_in_function_epilogue_p (current_gdbarch, read_pc ()))
 	return WP_VALUE_NOT_CHANGED;
       if (within_current_scope)

While tracking down a FAIL in testsuite/gdb.base/recurse.exp, I found
that the PC was in the epilogue of a function, at a point where the
FP has been messed up so that frame_find_by_id(b->watchpoint_frame)
was unable to find the watchpoint_frame (which would have been the
frame of the same function 4 recursions above).  As a result,
"within_current_scope" would have been set to FALSE and the above
code wouldn't call gdbarch_in_function_epilogue_p() even though it
would have recognized that the PC *is* in an epilogue currently and
would have *correctly* returned TRUE.

As a result of this misbehaviour, the to be checked watchpoint would
have been deleted because that's the consequence of not being in the
scope of the watchpoint.

The above patch is basically this:  If we couldn't find the watchpoint
frame, at least try to find out if PC is just in an epilogue.  If so,
it's probably the cause of failing to find the watchpoint frame so just
leave the watchpoint alone until we're on firmer ground again.  Not
calling gdbarch_in_function_epilogue_p() at this point is not exactly
what gdbarch_in_function_epilogue_p() was originally desigend for and
deleting the watchpoint instead is a bit of an overreaction.

Corinna

-- 
Corinna Vinschen
Cygwin Developer
Red Hat, Inc.

Re: [RFA] breakpoint.c, scanning epilogue if frame chain is invalid

[Eli Zaretskii]

> Date: Wed, 3 Sep 2003 11:38:15 +0200
> From: Corinna Vinschen <vinschen@redhat.com>
> 
> -      if (within_current_scope && fr == get_current_frame ()
> +      if ((!within_current_scope || fr == get_current_frame ())
>            && gdbarch_in_function_epilogue_p (current_gdbarch, read_pc ()))
>  	return WP_VALUE_NOT_CHANGED;
>        if (within_current_scope)
> [...]
> The above patch is basically this:  If we couldn't find the watchpoint
> frame, at least try to find out if PC is just in an epilogue.  If so,
> it's probably the cause of failing to find the watchpoint frame so just
> leave the watchpoint alone until we're on firmer ground again.

I'm not sure I understand why did you change

    within_current_scope && fr == get_current_frame ()

into

    (!within_current_scope || fr == get_current_frame ())

It doesn't seem to follow from the verbal description of the decision
you'd like the code to make.  Perhaps I'm missing something, so could
you please elaborate how the verbal description translates into the
code?

Re: [RFA] breakpoint.c, scanning epilogue if frame chain is invalid

[Corinna Vinschen]

On Wed, Sep 03, 2003 at 01:28:43PM +0200, Eli Zaretskii wrote:
> > Date: Wed, 3 Sep 2003 11:38:15 +0200
> > From: Corinna Vinschen <vinschen@redhat.com>
> > 
> > -      if (within_current_scope && fr == get_current_frame ()
> > +      if ((!within_current_scope || fr == get_current_frame ())
> >            && gdbarch_in_function_epilogue_p (current_gdbarch, read_pc ()))
> >  	return WP_VALUE_NOT_CHANGED;
> >        if (within_current_scope)
> > [...]
> > The above patch is basically this:  If we couldn't find the watchpoint
> > frame, at least try to find out if PC is just in an epilogue.  If so,
> > it's probably the cause of failing to find the watchpoint frame so just
> > leave the watchpoint alone until we're on firmer ground again.
> 
> I'm not sure I understand why did you change
> 
>     within_current_scope && fr == get_current_frame ()
> 
> into
> 
>     (!within_current_scope || fr == get_current_frame ())
> 
> It doesn't seem to follow from the verbal description of the decision
> you'd like the code to make.  Perhaps I'm missing something, so could
> you please elaborate how the verbal description translates into the
> code?

The lines before this code are

  fr = frame_find_by_id (b->watchpoint_frame);
  within_current_scope = (fr != NULL);

So "within_current_scope" actually means "Couldn't find watchpoint frame".
Being out of scope is one possible cause, being in an epilogue another.

  if (!within_current_scope ||     <==     fr == NULL
      fr == get_current_frame ())  <==  or fr != NULL and
                                           current frame == watchpoint frame
  then call gdbarch_in_function_epilogue_p(current pc).

So after the change gdbarch_in_function_epilogue_p() is also called if the
watchpoint frame couldn't be found because the frame chain is temporarily
broken.  Since this is very likely occuring in an epilogue, it's pretty
naturally to call gdbarch_in_function_epilogue_p().

Corinna

-- 
Corinna Vinschen
Cygwin Developer
Red Hat, Inc.

Re: [RFA] breakpoint.c, scanning epilogue if frame chain is invalid

[Eli Zaretskii]

> Date: Wed, 3 Sep 2003 13:18:28 +0200
> From: Corinna Vinschen <vinschen@redhat.com>
> 
>   if (!within_current_scope ||     <==     fr == NULL
>       fr == get_current_frame ())  <==  or fr != NULL and
>                                            current frame == watchpoint frame
>   then call gdbarch_in_function_epilogue_p(current pc).
> 
> So after the change gdbarch_in_function_epilogue_p() is also called if the
> watchpoint frame couldn't be found because the frame chain is temporarily
> broken.  Since this is very likely occuring in an epilogue, it's pretty
> naturally to call gdbarch_in_function_epilogue_p().

Thanks for the explanations, I think this patch is correct.

Re: [RFA] breakpoint.c, scanning epilogue if frame chain is invalid

[Andrew Cagney]

>> Date: Wed, 3 Sep 2003 13:18:28 +0200
>> From: Corinna Vinschen <vinschen@redhat.com>
>> 
>>   if (!within_current_scope ||     <==     fr == NULL
>>       fr == get_current_frame ())  <==  or fr != NULL and
>>                                            current frame == watchpoint frame
>>   then call gdbarch_in_function_epilogue_p(current pc).
>> 
>> So after the change gdbarch_in_function_epilogue_p() is also called if the
>> watchpoint frame couldn't be found because the frame chain is temporarily
>> broken.  Since this is very likely occuring in an epilogue, it's pretty
>> naturally to call gdbarch_in_function_epilogue_p().
> 
> 
> Thanks for the explanations, I think this patch is correct.

Corinna, can you add please add comments explaining this as part of the 
commit.

Andrew

Re: [RFA] breakpoint.c, scanning epilogue if frame chain is invalid

[Corinna Vinschen]

On Thu, Sep 04, 2003 at 07:53:40PM +0200, Eli Zaretskii wrote:
> > Date: Wed, 3 Sep 2003 13:18:28 +0200
> > From: Corinna Vinschen <vinschen@redhat.com>
> > 
> >   if (!within_current_scope ||     <==     fr == NULL
> >       fr == get_current_frame ())  <==  or fr != NULL and
> >                                            current frame == watchpoint frame
> >   then call gdbarch_in_function_epilogue_p(current pc).
> > 
> > So after the change gdbarch_in_function_epilogue_p() is also called if the
> > watchpoint frame couldn't be found because the frame chain is temporarily
> > broken.  Since this is very likely occuring in an epilogue, it's pretty
> > naturally to call gdbarch_in_function_epilogue_p().
> 
> Thanks for the explanations, I think this patch is correct.

Applied.

Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Developer
Red Hat, Inc.

Re: [RFA] breakpoint.c, scanning epilogue if frame chain is invalid

[Corinna Vinschen]

On Thu, Sep 04, 2003 at 01:21:33PM -0400, Andrew Cagney wrote:
> >>Date: Wed, 3 Sep 2003 13:18:28 +0200
> >>From: Corinna Vinschen <vinschen@redhat.com>
> >>
> >>  if (!within_current_scope ||     <==     fr == NULL
> >>      fr == get_current_frame ())  <==  or fr != NULL and
> >>                                           current frame == watchpoint 
> >>                                           frame
> >>  then call gdbarch_in_function_epilogue_p(current pc).
> >>
> >>So after the change gdbarch_in_function_epilogue_p() is also called if the
> >>watchpoint frame couldn't be found because the frame chain is temporarily
> >>broken.  Since this is very likely occuring in an epilogue, it's pretty
> >>naturally to call gdbarch_in_function_epilogue_p().
> >
> >
> >Thanks for the explanations, I think this patch is correct.
> 
> Corinna, can you add please add comments explaining this as part of the 
> commit.

Yes, I'll do that.  My commit was a bit too hasty anyway.

Corinna

-- 
Corinna Vinschen
Cygwin Developer
Red Hat, Inc.