[PATCH] Remove zero PC check from blockframe.c:inside_main_func()

[PATCH] Remove zero PC check from blockframe.c:inside_main_func()

[Mark Kettenis]

It really makes no sense to check for a zero PC here.  This function
is only colled from frame.c:get_prev_frame(), and there we already
deal with PC being zero.

The whole concept of using a zero PC as a marker for the end of the
frame chain is somewhat flawed.  It prevents us from providing a
meaningful backtrace when the program has called a null function
pointer; see backtrace/1476.  At the very least we will have to treat
a zero PC in the innermost differently.  Classifying the a zero PC as
being inside the "main" function doesn't help.  Therefore this patch
removes the first obstackle in fixing that PR.

Objections.  Otherwise I'll commit this within a few days.

Mark


Index: ChangeLog
from  Mark Kettenis  <kettenis@gnu.org>
 
	* blockframe.c (inside_main_func): Don't treat a zero PC specially.
	Needed to fix PR backtrace/1476.

Index: blockframe.c
===================================================================
RCS file: /cvs/src/src/gdb/blockframe.c,v
retrieving revision 1.83
diff -u -p -r1.83 blockframe.c
--- blockframe.c 13 Dec 2003 13:16:52 -0000 1.83
+++ blockframe.c 13 Dec 2003 14:50:01 -0000
@@ -73,17 +73,12 @@ deprecated_inside_entry_file (CORE_ADDR 
 }
 
 /* Test whether PC is in the range of addresses that corresponds to
-   the "main" function.
-
-   A PC of zero is always considered to be the bottom of the stack.  */
+   the "main" function.  */
 
 int
 inside_main_func (CORE_ADDR pc)
 {
   struct minimal_symbol *msymbol;
-
-  if (pc == 0)
-    return 1;
 
   if (symfile_objfile == 0)
     return 0;

Re: [PATCH] Remove zero PC check from blockframe.c:inside_main_func()

[Andrew Cagney]

> It really makes no sense to check for a zero PC here.  This function
> is only colled from frame.c:get_prev_frame(), and there we already
> deal with PC being zero.
> 
> The whole concept of using a zero PC as a marker for the end of the
> frame chain is somewhat flawed.  It prevents us from providing a
> meaningful backtrace when the program has called a null function
> pointer; see backtrace/1476.  At the very least we will have to treat
> a zero PC in the innermost differently.  Classifying the a zero PC as
> being inside the "main" function doesn't help.  Therefore this patch
> removes the first obstackle in fixing that PR.
> 
> Objections.  Otherwise I'll commit this within a few days.

FYI, this was made active with:

         * blockframe.c: Include "gdbcmd.h" and "command.h".
         (backtrace_below_main): New variable.
         (file_frame_chain_valid, func_frame_chain_valid)
         (nonnull_frame_chain_valid, generic_file_frame_chain_valid)
         (generic_func_frame_chain_valid): Remove functions.
         (frame_chain_valid, do_flush_frames_sfunc): New functions.
         (_initialize_blockframe): New function.
         * Makefile.in (blockframe.o): Update dependencies.
         * frame.c (frame_saved_regs_id_unwind, get_prev_frame): Remove 
FIXME
         comment.  Call frame_chain_valid ().
         * frame.h: Remove old prototypes.  Add prototype for
         frame_chain_valid and update comments to match.
         * gdbarch.sh: Change FRAME_CHAIN_VALID into a predicated function.
         Remove old comment.
         * gdbarch.h: Regenerated.
         * gdbarch.c: Regenerated.

rather than the new frame code.

I looked at the new frame code and apart from the wild-card logic, there 
weren't any obvious PC==0 tests.

Andrew

Re: [PATCH] Remove zero PC check from blockframe.c:inside_main_func()

[Mark Kettenis]

   Date: Sat, 13 Dec 2003 14:02:10 -0500
   From: Andrew Cagney <cagney@gnu.org>

   > It really makes no sense to check for a zero PC here.  This function
   > is only colled from frame.c:get_prev_frame(), and there we already
   > deal with PC being zero.
   > 
   > The whole concept of using a zero PC as a marker for the end of the
   > frame chain is somewhat flawed.  It prevents us from providing a
   > meaningful backtrace when the program has called a null function
   > pointer; see backtrace/1476.  At the very least we will have to treat
   > a zero PC in the innermost differently.  Classifying the a zero PC as
   > being inside the "main" function doesn't help.  Therefore this patch
   > removes the first obstackle in fixing that PR.
   > 
   > Objections.  Otherwise I'll commit this within a few days.

   FYI, this was made active with:

	    * blockframe.c: Include "gdbcmd.h" and "command.h".
	    (backtrace_below_main): New variable.
	    (file_frame_chain_valid, func_frame_chain_valid)
	    (nonnull_frame_chain_valid, generic_file_frame_chain_valid)
	    (generic_func_frame_chain_valid): Remove functions.
	    (frame_chain_valid, do_flush_frames_sfunc): New functions.
	    (_initialize_blockframe): New function.
	    * Makefile.in (blockframe.o): Update dependencies.
	    * frame.c (frame_saved_regs_id_unwind, get_prev_frame): Remove 
   FIXME
	    comment.  Call frame_chain_valid ().
	    * frame.h: Remove old prototypes.  Add prototype for
	    frame_chain_valid and update comments to match.
	    * gdbarch.sh: Change FRAME_CHAIN_VALID into a predicated function.
	    Remove old comment.
	    * gdbarch.h: Regenerated.
	    * gdbarch.c: Regenerated.

   rather than the new frame code.

Well yes, but ...

   I looked at the new frame code and apart from the wild-card logic, there 
   weren't any obvious PC==0 tests.

... there is a slightly non-obvious PC == 0 test in get_prev_frame():

   ... 
   if (frame_pc_unwind (this_frame) == 0)
     {
       ...
       return NULL;
     }
   ...

We should probably trust the sentinel frame here, and allow it to
unwind.  That is consistent with what we do earlier on.  What about
the attached patch?  Together with the blockframe patch (you're not
actually objecting to that one are you?), this fixes backtrace/1476.

Mark


Index: frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.153
diff -u -p -r1.153 frame.c
--- frame.c 10 Dec 2003 17:40:42 -0000 1.153
+++ frame.c 13 Dec 2003 21:47:43 -0000
@@ -1732,6 +1732,7 @@ struct frame_info *
 get_prev_frame (struct frame_info *this_frame)
 {
   struct frame_info *prev_frame;
+  CORE_ADDR pc;
 
   if (frame_debug)
     {
@@ -1961,7 +1962,8 @@ get_prev_frame (struct frame_info *this_
      because (well ignoring the PPC) a dummy frame can be located
      using THIS_FRAME's frame ID.  */
 
-  if (frame_pc_unwind (this_frame) == 0)
+  pc = frame_pc_unwind (this_frame);
+  if (this_frame->level >= 0 && pc == 0)
     {
       /* The allocated PREV_FRAME will be reclaimed when the frame
 	 obstack is next purged.  */

Re: [PATCH] Remove zero PC check from blockframe.c:inside_main_func()

[Andrew Cagney]

> RCS file: /cvs/src/src/gdb/frame.c,v
> retrieving revision 1.153
> diff -u -p -r1.153 frame.c
> --- frame.c 10 Dec 2003 17:40:42 -0000 1.153
> +++ frame.c 13 Dec 2003 21:47:43 -0000
> @@ -1732,6 +1732,7 @@ struct frame_info *
>  get_prev_frame (struct frame_info *this_frame)
>  {
>    struct frame_info *prev_frame;
> +  CORE_ADDR pc;
>  
>    if (frame_debug)
>      {
> @@ -1961,7 +1962,8 @@ get_prev_frame (struct frame_info *this_
>       because (well ignoring the PPC) a dummy frame can be located
>       using THIS_FRAME's frame ID.  */
>  
> -  if (frame_pc_unwind (this_frame) == 0)
> +  pc = frame_pc_unwind (this_frame);
> +  if (this_frame->level >= 0 && pc == 0)
>      {
>        /* The allocated PREV_FRAME will be reclaimed when the frame
>  	 obstack is next purged.

Can it be deleted?

This would likely affect the initial call sequence made to the unwinder 
- frame_pc_unwind may not be called first (?).  But I also think that 
the reason for insisting on an explicit pc unwind may have also been 
removed - the new code is written more robustly anyway.

If not, I'd prefer the PC delcaration to be made local to that code.

Andrew

Re: [PATCH] Remove zero PC check from blockframe.c:inside_main_func()

[Mark Kettenis]

   Date: Sat, 13 Dec 2003 19:22:47 -0500
   From: Andrew Cagney <cagney@gnu.org>

   > RCS file: /cvs/src/src/gdb/frame.c,v
   > retrieving revision 1.153
   > diff -u -p -r1.153 frame.c
   > --- frame.c 10 Dec 2003 17:40:42 -0000 1.153
   > +++ frame.c 13 Dec 2003 21:47:43 -0000
   > @@ -1732,6 +1732,7 @@ struct frame_info *
   >  get_prev_frame (struct frame_info *this_frame)
   >  {
   >    struct frame_info *prev_frame;
   > +  CORE_ADDR pc;
   >  
   >    if (frame_debug)
   >      {
   > @@ -1961,7 +1962,8 @@ get_prev_frame (struct frame_info *this_
   >       because (well ignoring the PPC) a dummy frame can be located
   >       using THIS_FRAME's frame ID.  */
   >  
   > -  if (frame_pc_unwind (this_frame) == 0)
   > +  pc = frame_pc_unwind (this_frame);
   > +  if (this_frame->level >= 0 && pc == 0)
   >      {
   >        /* The allocated PREV_FRAME will be reclaimed when the frame
   >  	 obstack is next purged.

   Can it be deleted?

I think so.  I tested i386-unknown-freebsd4.7, i386-pc-solaris2.9,
x86_64-unknown-freebsd5.2 and alpha-unknown-freenbsd5.2, and things
didn't change.

   This would likely affect the initial call sequence made to the unwinder 
   - frame_pc_unwind may not be called first (?).  But I also think that 
   the reason for insisting on an explicit pc unwind may have also been 
   removed - the new code is written more robustly anyway.

I think I agree.  So shall I remove the code?

Mark

Re: [PATCH] Remove zero PC check from blockframe.c:inside_main_func()

[Mark Kettenis]

   Date: Sat, 13 Dec 2003 16:09:29 +0100 (CET)
   From: Mark Kettenis <kettenis@chello.nl>

   It really makes no sense to check for a zero PC here.  This function
   is only colled from frame.c:get_prev_frame(), and there we already
   deal with PC being zero.

   [...]

   Objections.  Otherwise I'll commit this within a few days.

   Mark


   Index: ChangeLog
   from  Mark Kettenis  <kettenis@gnu.org>

	   * blockframe.c (inside_main_func): Don't treat a zero PC specially.
	   Needed to fix PR backtrace/1476.

I've checked this in now.

Re: [PATCH] Remove zero PC check from blockframe.c:inside_main_func()

[Andrew Cagney]

>    Date: Sat, 13 Dec 2003 19:22:47 -0500
>    From: Andrew Cagney <cagney@gnu.org>
> 
>    > RCS file: /cvs/src/src/gdb/frame.c,v
>    > retrieving revision 1.153
>    > diff -u -p -r1.153 frame.c
>    > --- frame.c 10 Dec 2003 17:40:42 -0000 1.153
>    > +++ frame.c 13 Dec 2003 21:47:43 -0000
>    > @@ -1732,6 +1732,7 @@ struct frame_info *
>    >  get_prev_frame (struct frame_info *this_frame)
>    >  {
>    >    struct frame_info *prev_frame;
>    > +  CORE_ADDR pc;
>    >  
>    >    if (frame_debug)
>    >      {
>    > @@ -1961,7 +1962,8 @@ get_prev_frame (struct frame_info *this_
>    >       because (well ignoring the PPC) a dummy frame can be located
>    >       using THIS_FRAME's frame ID.  */
>    >  
>    > -  if (frame_pc_unwind (this_frame) == 0)
>    > +  pc = frame_pc_unwind (this_frame);
>    > +  if (this_frame->level >= 0 && pc == 0)
>    >      {
>    >        /* The allocated PREV_FRAME will be reclaimed when the frame
>    >  	 obstack is next purged.
> 
>    Can it be deleted?
> 
> I think so.  I tested i386-unknown-freebsd4.7, i386-pc-solaris2.9,
> x86_64-unknown-freebsd5.2 and alpha-unknown-freenbsd5.2, and things
> didn't change.
> 
>    This would likely affect the initial call sequence made to the unwinder 
>    - frame_pc_unwind may not be called first (?).  But I also think that 
>    the reason for insisting on an explicit pc unwind may have also been 
>    removed - the new code is written more robustly anyway.
> 
> I think I agree.  So shall I remove the code?

Yes, just watch for comments claiming that the pc is unwound first 
though though.  I believe it's now really entirely determined by the 
per-architecture frame sniffers.

Andrew