[ob(ish)/committed] Fix SEGV in hppa_frame_cache

[ob(ish)/committed] Fix SEGV in hppa_frame_cache

[Joel Brobecker]

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

I was happily testing what I hoped would be the latest version of
the next/step patch replacing a complex condition by a frame ID
comparison, when I discovered that it caused a few problems on
HP/UX... But of course, HP/UX just got frame-ified!

Here is a description of the first problem I looked into:

        (in gdb.base)
        % gdb
        (gdb) file coremaker
        (gdb) core-file corefile
        (gdb) up
        *** SEGV ***

Ooops!

What happens is that we hit the following code in hppa_frame_cache():

        /* Yow! */
        u = find_unwind_entry (frame_func_unwind (next_frame));
        if (!u)
          return;

Unfortunately, that return causes the return value to be undefined.
And we later crash while trying to dereference this undefined value
in hppa_frame_this_id().

So I fixed it with the attached patch. This fixed 8 tests.
I didn't commit it to the 6.1 branch yet, as I wanted to wait for
Andrew's comments first. Don't want to disturb the branch too much.

There is also something that bothers me. If I understand this code well,
it looks like we are going to abort the unwinding as soon as we hit a
frame for which we can't find an associated function. Is that correct?
That would be very unfortunate, especially after we manage to install
the next/step patch I was testing; Once this patch is installed, the
chances us GDB trying to unwind from an unknown location will be more
important, no? If we don't know how to find our way out of there, then
the next/step machinery will be weakened. Andrew, if you confirm my
understanding is correct, I'll try to see if we can do better.

2004-03-04  J. Brobecker  <brobecker@gnat.com>

        * hppa-tdep.c (hppa_frame_cache): Avoid undefined return value.

Comments? Ok for the branch? (already committed in HEAD as, err,
obvious - well, at least we don't crash anymore :-)

Thanks,
-- 
Joel

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

Index: hppa-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/hppa-tdep.c,v
retrieving revision 1.129
diff -u -p -r1.129 hppa-tdep.c
--- hppa-tdep.c	27 Feb 2004 21:47:53 -0000	1.129
+++ hppa-tdep.c	5 Mar 2004 04:46:32 -0000
@@ -4617,7 +4617,7 @@ hppa_frame_cache (struct frame_info *nex
   /* Yow! */
   u = find_unwind_entry (frame_func_unwind (next_frame));
   if (!u)
-    return;
+    return (*this_cache);
 
   /* Turn the Entry_GR field into a bitmask.  */
   saved_gr_mask = 0;

Re: [ob(ish)/committed] Fix SEGV in hppa_frame_cache

[Andrew Cagney]

> I was happily testing what I hoped would be the latest version of
> the next/step patch replacing a complex condition by a frame ID
> comparison, when I discovered that it caused a few problems on
> HP/UX... But of course, HP/UX just got frame-ified!
> 
> Here is a description of the first problem I looked into:
> 
>         (in gdb.base)
>         % gdb
>         (gdb) file coremaker
>         (gdb) core-file corefile
>         (gdb) up
>         *** SEGV ***
> 
> Ooops!
> 
> What happens is that we hit the following code in hppa_frame_cache():
> 
>         /* Yow! */
>         u = find_unwind_entry (frame_func_unwind (next_frame));
>         if (!u)
>           return;

That 'll learn me for using the HP compiler :-/

> Unfortunately, that return causes the return value to be undefined.
> And we later crash while trying to dereference this undefined value
> in hppa_frame_this_id().
> 
> So I fixed it with the attached patch. This fixed 8 tests.
> I didn't commit it to the 6.1 branch yet, as I wanted to wait for
> Andrew's comments first. Don't want to disturb the branch too much.

For HP/UX, I think we'll be doing frequent pull-ups - this will be just 
the first of many bugs :-(  Definitly go ahead.

> There is also something that bothers me. If I understand this code well,
> it looks like we are going to abort the unwinding as soon as we hit a
> frame for which we can't find an associated function. Is that correct?

Yes, its taken straight from hppa_frame_find_saved_regs.

 > That would be very unfortunate, especially after we manage to install
 > the next/step patch I was testing; Once this patch is installed, the
 > chances us GDB trying to unwind from an unknown location will be more
 > important, no? If we don't know how to find our way out of there, then
 > the next/step machinery will be weakened. Andrew, if you confirm my
 > understanding is correct, I'll try to see if we can do better.

The code just needs to be sufficient to unwind one level of PC/FP - so 
that the caller can be found.  Having some of the frame saved pc code 
before the abort would improve things.  As for the SP:

You must be looking for a challenge, from frame_chain:

       if (!u)
         {
           /* We could find this information by examining prologues.  I 
don't
              think anyone has actually written any tools (not even "strip")
              which leave them out of an executable, so maybe this is a moot
              point.  */
           /* ??rehrauer: Actually, it's quite possible to stepi your 
way into
              code that doesn't have unwind entries.  For example, 
stepping into
              the dynamic linker will give you a PC that has none. 
Thus, I've
              disabled this warning. */
#if 0
           warning ("Unable to find unwind for PC 0x%x -- Help!", 
get_frame_pc (t
mp_frame));
#endif
           return (CORE_ADDR) 0;
         }

Andrew

Re: [ob(ish)/committed] Fix SEGV in hppa_frame_cache

[Joel Brobecker]

> >Unfortunately, that return causes the return value to be undefined.
> >And we later crash while trying to dereference this undefined value
> >in hppa_frame_this_id().
> >
> >So I fixed it with the attached patch. This fixed 8 tests.
> >I didn't commit it to the 6.1 branch yet, as I wanted to wait for
> >Andrew's comments first. Don't want to disturb the branch too much.
> 
> For HP/UX, I think we'll be doing frequent pull-ups - this will be just 
> the first of many bugs :-(  Definitly go ahead.

Sorry for the delay. I just committed this change on the branch.

2004-03-10  J. Brobecker  <brobecker@gnat.com>

        * hppa-tdep.c (hppa_frame_cache): Avoid undefined return value.

Thanks,
-- 
Joel

PS: I am unfortunately a bit busy at the moment, but I'd like to reread
your comments again, and send mine if needed. So I'm keeping your message
in my inbox...

Re: [ob(ish)/committed] Fix SEGV in hppa_frame_cache

[Andrew Cagney]

> I was happily testing what I hoped would be the latest version of
> the next/step patch replacing a complex condition by a frame ID
> comparison, when I discovered that it caused a few problems on
> HP/UX... But of course, HP/UX just got frame-ified!
> 
> Here is a description of the first problem I looked into:
> 
>         (in gdb.base)
>         % gdb
>         (gdb) file coremaker
>         (gdb) core-file corefile
>         (gdb) up
>         *** SEGV ***
> 
> Ooops!
> 
> What happens is that we hit the following code in hppa_frame_cache():
> 
>         /* Yow! */
>         u = find_unwind_entry (frame_func_unwind (next_frame));
>         if (!u)
>           return;

That 'll learn me for using the HP compiler :-/

> Unfortunately, that return causes the return value to be undefined.
> And we later crash while trying to dereference this undefined value
> in hppa_frame_this_id().
> 
> So I fixed it with the attached patch. This fixed 8 tests.
> I didn't commit it to the 6.1 branch yet, as I wanted to wait for
> Andrew's comments first. Don't want to disturb the branch too much.

For HP/UX, I think we'll be doing frequent pull-ups - this will be just 
the first of many bugs :-(  Definitly go ahead.

> There is also something that bothers me. If I understand this code well,
> it looks like we are going to abort the unwinding as soon as we hit a
> frame for which we can't find an associated function. Is that correct?

Yes, its taken straight from hppa_frame_find_saved_regs.

 > That would be very unfortunate, especially after we manage to install
 > the next/step patch I was testing; Once this patch is installed, the
 > chances us GDB trying to unwind from an unknown location will be more
 > important, no? If we don't know how to find our way out of there, then
 > the next/step machinery will be weakened. Andrew, if you confirm my
 > understanding is correct, I'll try to see if we can do better.

The code just needs to be sufficient to unwind one level of PC/FP - so 
that the caller can be found.  Having some of the frame saved pc code 
before the abort would improve things.  As for the SP:

You must be looking for a challenge, from frame_chain:

       if (!u)
         {
           /* We could find this information by examining prologues.  I 
don't
              think anyone has actually written any tools (not even "strip")
              which leave them out of an executable, so maybe this is a moot
              point.  */
           /* ??rehrauer: Actually, it's quite possible to stepi your 
way into
              code that doesn't have unwind entries.  For example, 
stepping into
              the dynamic linker will give you a PC that has none. 
Thus, I've
              disabled this warning. */
#if 0
           warning ("Unable to find unwind for PC 0x%x -- Help!", 
get_frame_pc (t
mp_frame));
#endif
           return (CORE_ADDR) 0;
         }

Andrew

[ob(ish)/committed] Fix SEGV in hppa_frame_cache

[Joel Brobecker]

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

I was happily testing what I hoped would be the latest version of
the next/step patch replacing a complex condition by a frame ID
comparison, when I discovered that it caused a few problems on
HP/UX... But of course, HP/UX just got frame-ified!

Here is a description of the first problem I looked into:

        (in gdb.base)
        % gdb
        (gdb) file coremaker
        (gdb) core-file corefile
        (gdb) up
        *** SEGV ***

Ooops!

What happens is that we hit the following code in hppa_frame_cache():

        /* Yow! */
        u = find_unwind_entry (frame_func_unwind (next_frame));
        if (!u)
          return;

Unfortunately, that return causes the return value to be undefined.
And we later crash while trying to dereference this undefined value
in hppa_frame_this_id().

So I fixed it with the attached patch. This fixed 8 tests.
I didn't commit it to the 6.1 branch yet, as I wanted to wait for
Andrew's comments first. Don't want to disturb the branch too much.

There is also something that bothers me. If I understand this code well,
it looks like we are going to abort the unwinding as soon as we hit a
frame for which we can't find an associated function. Is that correct?
That would be very unfortunate, especially after we manage to install
the next/step patch I was testing; Once this patch is installed, the
chances us GDB trying to unwind from an unknown location will be more
important, no? If we don't know how to find our way out of there, then
the next/step machinery will be weakened. Andrew, if you confirm my
understanding is correct, I'll try to see if we can do better.

2004-03-04  J. Brobecker  <brobecker@gnat.com>

        * hppa-tdep.c (hppa_frame_cache): Avoid undefined return value.

Comments? Ok for the branch? (already committed in HEAD as, err,
obvious - well, at least we don't crash anymore :-)

Thanks,
-- 
Joel

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

Index: hppa-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/hppa-tdep.c,v
retrieving revision 1.129
diff -u -p -r1.129 hppa-tdep.c
--- hppa-tdep.c	27 Feb 2004 21:47:53 -0000	1.129
+++ hppa-tdep.c	5 Mar 2004 04:46:32 -0000
@@ -4617,7 +4617,7 @@ hppa_frame_cache (struct frame_info *nex
   /* Yow! */
   u = find_unwind_entry (frame_func_unwind (next_frame));
   if (!u)
-    return;
+    return (*this_cache);
 
   /* Turn the Entry_GR field into a bitmask.  */
   saved_gr_mask = 0;

Re: [ob(ish)/committed] Fix SEGV in hppa_frame_cache

[Joel Brobecker]

> >Unfortunately, that return causes the return value to be undefined.
> >And we later crash while trying to dereference this undefined value
> >in hppa_frame_this_id().
> >
> >So I fixed it with the attached patch. This fixed 8 tests.
> >I didn't commit it to the 6.1 branch yet, as I wanted to wait for
> >Andrew's comments first. Don't want to disturb the branch too much.
> 
> For HP/UX, I think we'll be doing frequent pull-ups - this will be just 
> the first of many bugs :-(  Definitly go ahead.

Sorry for the delay. I just committed this change on the branch.

2004-03-10  J. Brobecker  <brobecker@gnat.com>

        * hppa-tdep.c (hppa_frame_cache): Avoid undefined return value.

Thanks,
-- 
Joel

PS: I am unfortunately a bit busy at the moment, but I'd like to reread
your comments again, and send mine if needed. So I'm keeping your message
in my inbox...