[RFA/sparc] pb doing next over struct-return function

[RFA/sparc] pb doing next over struct-return function

[Joel Brobecker]

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

Hello,

This is on sparc - solaris 2.8:

Consider the following code:

        with Ada.Text_IO; use Ada.Text_IO;
        
        procedure Small is
           I : Integer := 123;
        begin
           Put_Line (Integer'Image(I));  -- line 6
           I := I + 1;
        end Small;

Compile it with:

        % gnatmake -g small

And then try to to a next over line 6:

        % gdb small
        (gdb) b small.adb:6
        (gdb) run
        Starting program: /[...]/small 
        
        Breakpoint 1, small () at small.adb:6
        6          Put_Line (Integer'Image(I));
        (gdb) n
         123
        
        Program exited normally.

Ooops, the debugger doesn't stop at line 7! Here is why:

First of all, Integer'Image is a call to a function which transforms
the Integer variable I into a string. The actually name of the function
is: system__img_int__image_integer.

When GDB does the stepping, it eventually lands inside that function.
Finds out that it has stepped inside it by comparing the frame ID of
the previous frame against the step_frame_id, and therefore puts a
breakpoint at the return address and resumes the execution.

Unfortunately, the breakpoint breakpoint location chosen is not
correct, it's one instruction too early, and that causes the breakpoint
to never be hit. Hence the "Program exited normally".

The reason for the incorrect breakpoint location is that GDB
does not see that system__img_int__image_integer is a struct-return
function. So the address pointed by "saved_pc + 8" is a "unimp"
instruction, which of course is never executed unless something
is badly wrong.

Digging deeper, I found that this address is incorrectly computed
because cache->struct_return_p in sparc32_frame_cache() is not
set. And the reason for it not being set is that there is no
debugging information available for system__img_int__image_integer,
because it is part of the GNAT runtime, which is compiled without
debugging information.

So I made a small change to sparc32_frame_cache() to fallback to
a small heuristic that should help determine whether the function
is a struct-return or not based on the instruction found at
"saved_pc + 8". If it is a "unimp", then for chances are the
function won't return there, but one instruction later. And hence,
we must have a struct-return function.

This fixes the problem above, and does not introduce any regression
in the testsuite.

2004-11-22  Joel Brobecker  <brobecker@gnat.com>

        * sparc-tdep.c (is_unimp_insn): New function.
        (sparc32_frame_cache): For functions where there is no debugging
        information to help us determine whether it's a struct-return
        function or not, fallback on checking whether the instruction
        at the return address is a "unimp" instruction or not.

Tested on sparc-solaris. Ok to commit?

Thanks,
-- 
Joel

[-- Attachment #2: sparc-tdep.c.diff --]
[-- Type: text/plain, Size: 1460 bytes --]

Index: sparc-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/sparc-tdep.c,v
retrieving revision 1.156
diff -u -p -r1.156 sparc-tdep.c
--- sparc-tdep.c	21 Nov 2004 20:11:09 -0000	1.156
+++ sparc-tdep.c	23 Nov 2004 05:01:15 -0000
@@ -640,6 +640,17 @@ sparc_frame_cache (struct frame_info *ne
   return cache;
 }
 
+/* Return True if the instruction corresponding to PC is a "unimp"
+   instruction.  */
+
+static int
+is_unimp_insn (CORE_ADDR pc)
+{
+  const unsigned long insn = sparc_fetch_instruction (pc);
+  
+  return ((insn & 0xc1c00000) == 0);
+}
+
 struct sparc_frame_cache *
 sparc32_frame_cache (struct frame_info *next_frame, void **this_cache)
 {
@@ -665,6 +676,21 @@ sparc32_frame_cache (struct frame_info *
 	    cache->struct_return_p = 1;
 	}
     }
+  else
+    {
+      /* There is no debugging information for this function to
+         help us determine whether this function returns a struct
+         or not.  So we rely on another heuristic which is to check
+         the instruction at the return address and see if this is
+         a "unimp" instruction.  If it is, then it is struct-return
+         function.  */
+      CORE_ADDR pc;
+      int regnum = cache->frameless_p ? SPARC_O7_REGNUM : SPARC_I7_REGNUM;
+
+      pc = frame_unwind_register_unsigned (next_frame, regnum) + 8;
+      if (is_unimp_insn (pc))
+        cache->struct_return_p = 1;
+    }
 
   return cache;
 }

Re: [RFA/sparc] pb doing next over struct-return function

[Mark Kettenis]

   Date: Mon, 22 Nov 2004 21:35:44 -0800
   From: Joel Brobecker <brobecker@adacore.com>

   Digging deeper, I found that this address is incorrectly computed
   because cache->struct_return_p in sparc32_frame_cache() is not
   set. And the reason for it not being set is that there is no
   debugging information available for system__img_int__image_integer,
   because it is part of the GNAT runtime, which is compiled without
   debugging information.

Just an aside, is there anything against shipping/compiling the GNAT
runtime with debug information?

   So I made a small change to sparc32_frame_cache() to fallback to
   a small heuristic that should help determine whether the function
   is a struct-return or not based on the instruction found at
   "saved_pc + 8". If it is a "unimp", then for chances are the
   function won't return there, but one instruction later. And hence,
   we must have a struct-return function.

   This fixes the problem above, and does not introduce any regression
   in the testsuite.

   2004-11-22  Joel Brobecker  <brobecker@gnat.com>

	   * sparc-tdep.c (is_unimp_insn): New function.
	   (sparc32_frame_cache): For functions where there is no debugging
	   information to help us determine whether it's a struct-return
	   function or not, fallback on checking whether the instruction
	   at the return address is a "unimp" instruction or not.

Makes sense to me.  Could you do me a favour and rename the function
to sparc_unimp_insn_p?  If you feel like it you may move it to just
after sparc_fetch_instruction, which seems a somwhat more logical
place to me (but only slightly so).  Please also fix a few
spelling-mistakes in comments:

   +/* Return True if the instruction corresponding to PC is a "unimp"
   +   instruction.  */

Return non-zero if the instruction at PC is an "unimp" instruction.
       ^^^^^^^^                              ^

   +  else
   +    {
   +      /* There is no debugging information for this function to
   +         help us determine whether this function returns a struct
   +         or not.  So we rely on another heuristic which is to check
   +         the instruction at the return address and see if this is
   +         a "unimp" instruction.  If it is, then it is struct-return
   +         function.  */

an "unimp" instruction.  If it is, then it s a struct-return
 ^                                           ^

Thanks,

Mark

Re: [RFA/sparc] pb doing next over struct-return function

[Eli Zaretskii]

> Date: Tue, 23 Nov 2004 09:33:06 +0100 (CET)
> From: Mark Kettenis <kettenis@gnu.org>
> CC: gdb-patches@sources.redhat.com
> 
> Return non-zero if the instruction at PC is an "unimp" instruction.
>        ^^^^^^^^                              ^
> 
>    +  else
>    +    {
>    +      /* There is no debugging information for this function to
>    +         help us determine whether this function returns a struct
>    +         or not.  So we rely on another heuristic which is to check
>    +         the instruction at the return address and see if this is
>    +         a "unimp" instruction.  If it is, then it is struct-return
>    +         function.  */
> 
> an "unimp" instruction.
>  ^                                           ^

Really?  I'm not a native English speaker, but I think "a unimp" is
correct.  It's like "a university", isn't it?

Perhaps "the unimp instruction" would be better, though, since it's a
name of a specific instruction.

Re: [RFA/sparc] pb doing next over struct-return function

[Richard Earnshaw]

On Tue, 2004-11-23 at 11:29, Eli Zaretskii wrote:
> > Date: Tue, 23 Nov 2004 09:33:06 +0100 (CET)
> > From: Mark Kettenis <kettenis@gnu.org>
> > CC: gdb-patches@sources.redhat.com
> > 
> > Return non-zero if the instruction at PC is an "unimp" instruction.
> >        ^^^^^^^^                              ^
> > 
> >    +  else
> >    +    {
> >    +      /* There is no debugging information for this function to
> >    +         help us determine whether this function returns a struct
> >    +         or not.  So we rely on another heuristic which is to check
> >    +         the instruction at the return address and see if this is
> >    +         a "unimp" instruction.  If it is, then it is struct-return
> >    +         function.  */
> > 
> > an "unimp" instruction.
> >  ^                                           ^
> 
> Really?  I'm not a native English speaker, but I think "a unimp" is
> correct.  It's like "a university", isn't it?
> 
> Perhaps "the unimp instruction" would be better, though, since it's a
> name of a specific instruction.

Generally 'an' is used if the sound from the 'u' is as in 'um', but 'a'
is used if it is as in 'you'.  So, 'an umbrella', but 'a uniform'.

The example above is difficult to rule on, because the mnemonic isn't a
real word.  Is the pronunciation as a single word, or as a set of
letters (u-n-i-m-p)?  If the former it's probably 'an'; for the latter
it's probably 'a'.  Since that's a matter of choice, then it's hard to
ever be 100% accurate in this case.

Confused?  You should be...  This is English we are talking about...

R.

Re: [RFA/sparc] pb doing next over struct-return function

[Joel Brobecker]

>    Digging deeper, I found that this address is incorrectly computed
>    because cache->struct_return_p in sparc32_frame_cache() is not
>    set. And the reason for it not being set is that there is no
>    debugging information available for system__img_int__image_integer,
>    because it is part of the GNAT runtime, which is compiled without
>    debugging information.
> 
> Just an aside, is there anything against shipping/compiling the GNAT
> runtime with debug information?

Yes. One of the reasons is size of debugging information. With Ada,
it's pretty large.

>    2004-11-22  Joel Brobecker  <brobecker@gnat.com>
> 
> 	   * sparc-tdep.c (is_unimp_insn): New function.
> 	   (sparc32_frame_cache): For functions where there is no debugging
> 	   information to help us determine whether it's a struct-return
> 	   function or not, fallback on checking whether the instruction
> 	   at the return address is a "unimp" instruction or not.
> 
> Makes sense to me.  Could you do me a favour and rename the function
> to sparc_unimp_insn_p?  If you feel like it you may move it to just
> after sparc_fetch_instruction, which seems a somwhat more logical
> place to me (but only slightly so).  Please also fix a few
> spelling-mistakes in comments:

Thanks. Checked in with the changes you requested (renaming the new
function, moving it up, and making the spelling corrections).

Although ``a "unimp" instruction" seem more natural to me, I did
follow your recommendation. It's easy to change it back if it turns
out one day that the use of "a" in this case is more common than
the use of "an".

Thanks,
-- 
Joel

Re: [RFA/sparc] pb doing next over struct-return function

[Paul Hilfinger]

 > > an "unimp" instruction.
 > >  ^                                           ^
 > 
 > Really?  I'm not a native English speaker, but I think "a unimp" is
 > correct.  It's like "a university", isn't it?

Well, not really.  "University" starts with the semi-consonant sound
"y" (I'm sure there is more formal term than semi-consonant, but I am
no linguist), whereas "unimp" starts with a vowel sound, "uh" as in
"onion".  

 > Perhaps "the unimp instruction" would be better, though, since it's a
 > name of a specific instruction.

Would be a conservative choice, but alas the context refers to an
instruction INSTANCE, not an instruction category.  Thus, the
implication of "the" would be either that there is one and only one
unimp instruction in the universe, and here it is, or that I had
previously identified a small section of my assembly listing, and I am
now referring to the one unimp instruction in that section.

Paul Hilfinger