[RFA] frame.c (find_frame_sal): Check return value of get_frame_function.

[RFA] frame.c (find_frame_sal): Check return value of get_frame_function.

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 75 bytes --]

The return value for get_frame_function is usually checked for null.

OK?


[-- Attachment #2: null17.txt --]
[-- Type: text/plain, Size: 1098 bytes --]

2011-03-04  Michael Snyder  <msnyder@vmware.com>

	* frame.c (find_frame_sal): Check return value of get_frame_function.

Index: frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.291
diff -u -p -r1.291 frame.c
--- frame.c	7 Jan 2011 19:36:17 -0000	1.291
+++ frame.c	5 Mar 2011 01:27:39 -0000
@@ -1892,15 +1892,16 @@ find_frame_sal (struct frame_info *frame
 	sym = inline_skipped_symbol (inferior_ptid);
 
       init_sal (sal);
-      if (SYMBOL_LINE (sym) != 0)
+      if (sym && SYMBOL_LINE (sym) != 0)
 	{
 	  sal->symtab = SYMBOL_SYMTAB (sym);
 	  sal->line = SYMBOL_LINE (sym);
 	}
       else
-	/* If the symbol does not have a location, we don't know where
-	   the call site is.  Do not pretend to.  This is jarring, but
-	   we can't do much better.  */
+	/* If the symbol does not have a location (or we didn't find a
+	   symbol), we don't know where the call site is.  Do not
+	   pretend to.  This is jarring, but we can't do much
+	   better.  */
 	sal->pc = get_frame_pc (frame);
 
       return;

Re: [RFA] frame.c (find_frame_sal): Check return value of get_frame_function.

[Michael Snyder]

Michael Snyder wrote:
> The return value for get_frame_function is usually checked for null.
> 
> OK?
> 
Ping?

Re: [RFA] frame.c (find_frame_sal): Check return value of get_frame_function.

[Joel Brobecker]

> 2011-03-04  Michael Snyder  <msnyder@vmware.com>
> 
> 	* frame.c (find_frame_sal): Check return value of get_frame_function.

I think that the change is correct, but I'm not completely sure,
so a second pair of eyes would be nice.

My thinking: It is entirely plausible that get_next_frame (frame)
returns a frame with a PC for which there is no debugging info.
In that case, it's the same as not having line info.

> Index: frame.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/frame.c,v
> retrieving revision 1.291
> diff -u -p -r1.291 frame.c
> --- frame.c	7 Jan 2011 19:36:17 -0000	1.291
> +++ frame.c	5 Mar 2011 01:27:39 -0000
> @@ -1892,15 +1892,16 @@ find_frame_sal (struct frame_info *frame
>  	sym = inline_skipped_symbol (inferior_ptid);
>  
>        init_sal (sal);
> -      if (SYMBOL_LINE (sym) != 0)
> +      if (sym && SYMBOL_LINE (sym) != 0)
>  	{
>  	  sal->symtab = SYMBOL_SYMTAB (sym);
>  	  sal->line = SYMBOL_LINE (sym);
>  	}
>        else
> -	/* If the symbol does not have a location, we don't know where
> -	   the call site is.  Do not pretend to.  This is jarring, but
> -	   we can't do much better.  */
> +	/* If the symbol does not have a location (or we didn't find a
> +	   symbol), we don't know where the call site is.  Do not
> +	   pretend to.  This is jarring, but we can't do much
> +	   better.  */
>  	sal->pc = get_frame_pc (frame);
>  
>        return;


-- 
Joel

Re: [RFA] frame.c (find_frame_sal): Check return value of get_frame_function.

[Pedro Alves]

On Tuesday 15 March 2011 16:26:09, Joel Brobecker wrote:
> > 2011-03-04  Michael Snyder  <msnyder@vmware.com>
> > 
> >       * frame.c (find_frame_sal): Check return value of get_frame_function.
> 
> I think that the change is correct, but I'm not completely sure,
> so a second pair of eyes would be nice.
> 
> My thinking: It is entirely plausible that get_next_frame (frame)
> returns a frame with a PC for which there is no debugging info.
> In that case, it's the same as not having line info.

It would normally, but in this case, we've just found that
the next frame is an inlined function call.  Then it
certainly has debug info?  Otherwise, how would gdb know
it's an inlined function call?

-- 
Pedro Alves

Re: [RFA] frame.c (find_frame_sal): Check return value of get_frame_function.

[Joel Brobecker]

> It would normally, but in this case, we've just found that
> the next frame is an inlined function call.  Then it
> certainly has debug info?  Otherwise, how would gdb know
> it's an inlined function call?

Indeed, I think you're right. inline_skipped_symbol shouldn't
be returning a NULL symbol, as far as I can tell.  So should
we just add a gdb_assert with a comment explaining why we
expect sym to be non-NULL?

-- 
Joel

Re: [RFA] frame.c (find_frame_sal): Check return value of get_frame_function.

[Pedro Alves]

On Tuesday 15 March 2011 17:38:05, Joel Brobecker wrote:
> > It would normally, but in this case, we've just found that
> > the next frame is an inlined function call.  Then it
> > certainly has debug info?  Otherwise, how would gdb know
> > it's an inlined function call?
> 
> Indeed, I think you're right. inline_skipped_symbol shouldn't
> be returning a NULL symbol, as far as I can tell.  So should
> we just add a gdb_assert with a comment explaining why we
> expect sym to be non-NULL?

I think so.

-- 
Pedro Alves

Re: [RFA] frame.c (find_frame_sal): Check return value of get_frame_function.

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 549 bytes --]

Pedro Alves wrote:
> On Tuesday 15 March 2011 17:38:05, Joel Brobecker wrote:
>>> It would normally, but in this case, we've just found that
>>> the next frame is an inlined function call.  Then it
>>> certainly has debug info?  Otherwise, how would gdb know
>>> it's an inlined function call?
>> Indeed, I think you're right. inline_skipped_symbol shouldn't
>> be returning a NULL symbol, as far as I can tell.  So should
>> we just add a gdb_assert with a comment explaining why we
>> expect sym to be non-NULL?
> 
> I think so.
> 

How's this?



[-- Attachment #2: null17b.txt --]
[-- Type: text/plain, Size: 631 bytes --]

2011-03-15  Michael Snyder  <msnyder@vmware.com>

	* frame.c (find_frame_sal): Assert sym is not null.

Index: frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.292
diff -u -p -r1.292 frame.c
--- frame.c	9 Mar 2011 14:22:09 -0000	1.292
+++ frame.c	15 Mar 2011 19:11:48 -0000
@@ -1899,6 +1899,8 @@ find_frame_sal (struct frame_info *frame
       else
 	sym = inline_skipped_symbol (inferior_ptid);
 
+      /* If frame is inline, it certainly has symbols.  */
+      gdb_assert (sym);
       init_sal (sal);
       if (SYMBOL_LINE (sym) != 0)
 	{

Re: [RFA] frame.c (find_frame_sal): Check return value of get_frame_function.

[Pedro Alves]

On Tuesday 15 March 2011 19:12:46, Michael Snyder wrote:

> How's this?

Looks right to me.

-- 
Pedro Alves

Re: [RFA] frame.c (find_frame_sal): Check return value of get_frame_function.

[Michael Snyder]

Pedro Alves wrote:
> On Tuesday 15 March 2011 19:12:46, Michael Snyder wrote:
> 
>> How's this?
> 
> Looks right to me.
> 

Thanks, committed.