diff mbox

S390: Add use of unavailable-stack frame ID

Message ID m31t7m37zm.fsf@oc1027705133.ibm.com
State New
Headers show

Commit Message

Andreas Arnez March 7, 2016, 10:19 a.m. UTC
When determining the frame ID of an inline frame, GDB currently asserts
that a valid ID of the underlying real frame is found, and that it does
not match outer_frame_id.  From inline_frame_this_id():

  /* For now, require we don't match outer_frame_id either (see
     comment above).  */
  gdb_assert (!frame_id_eq (*this_id, outer_frame_id));

However, this assertion may fail when the real frame's unwinder can not
determine the frame ID.  This happened on an s390x target with a binary
that lacked call frame information and also confused the prologue
analyzer, because then s390_frame_this_id() left the frame ID at its
default.

To fix this, this change enhances s390_frame_this_id such that an
unavailable-stack frame ID is built if no frame base can be determined
but the function address is available.

gdb/ChangeLog:

	* s390-linux-tdep.c (s390_prologue_frame_unwind_cache): Store
	frame func's PC in info->func before any other failure can occur.
	(s390_frame_this_id): Use frame_id_build_unavailable_stack if
	info->func has been filled out.
---
 gdb/s390-linux-tdep.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Comments

Andreas Arnez March 9, 2016, 4:16 p.m. UTC | #1
Pushed.

On Mon, Mar 07 2016, Andreas Arnez wrote:

> When determining the frame ID of an inline frame, GDB currently asserts
> that a valid ID of the underlying real frame is found, and that it does
> not match outer_frame_id.  From inline_frame_this_id():
>
>   /* For now, require we don't match outer_frame_id either (see
>      comment above).  */
>   gdb_assert (!frame_id_eq (*this_id, outer_frame_id));
>
> However, this assertion may fail when the real frame's unwinder can not
> determine the frame ID.  This happened on an s390x target with a binary
> that lacked call frame information and also confused the prologue
> analyzer, because then s390_frame_this_id() left the frame ID at its
> default.
>
> To fix this, this change enhances s390_frame_this_id such that an
> unavailable-stack frame ID is built if no frame base can be determined
> but the function address is available.
>
> gdb/ChangeLog:
>
> 	* s390-linux-tdep.c (s390_prologue_frame_unwind_cache): Store
> 	frame func's PC in info->func before any other failure can occur.
> 	(s390_frame_this_id): Use frame_id_build_unavailable_stack if
> 	info->func has been filled out.
Pedro Alves March 10, 2016, 8:26 p.m. UTC | #2
On 03/07/2016 10:19 AM, Andreas Arnez wrote:
> When determining the frame ID of an inline frame, GDB currently asserts
> that a valid ID of the underlying real frame is found, and that it does
> not match outer_frame_id.  From inline_frame_this_id():
>
>    /* For now, require we don't match outer_frame_id either (see
>       comment above).  */
>    gdb_assert (!frame_id_eq (*this_id, outer_frame_id));
>
> However, this assertion may fail when the real frame's unwinder can not
> determine the frame ID.  This happened on an s390x target with a binary
> that lacked call frame information and also confused the prologue
> analyzer, because then s390_frame_this_id() left the frame ID at its
> default.
>
> To fix this, this change enhances s390_frame_this_id such that an
> unavailable-stack frame ID is built if no frame base can be determined
> but the function address is available.
>

Seeing all these makes me want this:

  https://sourceware.org/ml/gdb-patches/2016-02/msg00778.html

even more.  :-)

Thanks,
Pedro Alves
Andreas Arnez March 11, 2016, 9:23 a.m. UTC | #3
On Thu, Mar 10 2016, Pedro Alves wrote:

> On 03/07/2016 10:19 AM, Andreas Arnez wrote:
>> When determining the frame ID of an inline frame, GDB currently asserts
>> that a valid ID of the underlying real frame is found, and that it does
>> not match outer_frame_id.  From inline_frame_this_id():
>>
>>    /* For now, require we don't match outer_frame_id either (see
>>       comment above).  */
>>    gdb_assert (!frame_id_eq (*this_id, outer_frame_id));
>>
>> However, this assertion may fail when the real frame's unwinder can not
>> determine the frame ID.  This happened on an s390x target with a binary
>> that lacked call frame information and also confused the prologue
>> analyzer, because then s390_frame_this_id() left the frame ID at its
>> default.
>>
>> To fix this, this change enhances s390_frame_this_id such that an
>> unavailable-stack frame ID is built if no frame base can be determined
>> but the function address is available.
>>
>
> Seeing all these makes me want this:
>
>  https://sourceware.org/ml/gdb-patches/2016-02/msg00778.html
>
> even more.  :-)

Agreed.  It's on Yao's todo list, right?  ;-) --

  https://sourceware.org/ml/gdb-patches/2016-02/msg00858.html

--
Andreas
Yao Qi March 15, 2016, 8:11 a.m. UTC | #4
Andreas Arnez <arnez@linux.vnet.ibm.com> writes:

>> Seeing all these makes me want this:
>>
>>  https://sourceware.org/ml/gdb-patches/2016-02/msg00778.html
>>
>> even more.  :-)
>
> Agreed.  It's on Yao's todo list, right?  ;-) --
>
>   https://sourceware.org/ml/gdb-patches/2016-02/msg00858.html

Yes, but it is in the bottom.  Your patch is a good evidence that we
need to fix all "unavailable stack" stuff in target independent part.
diff mbox

Patch

diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c
index 7f860b6..155bc66 100644
--- a/gdb/s390-linux-tdep.c
+++ b/gdb/s390-linux-tdep.c
@@ -2011,9 +2011,12 @@  s390_prologue_frame_unwind_cache (struct frame_info *this_frame,
      bother searching for it -- with modern compilers this would be mostly
      pointless anyway.  Trust that we'll either have valid DWARF-2 CFI data
      or else a valid backchain ...  */
-  func = get_frame_func (this_frame);
-  if (!func)
-    return 0;
+  if (!get_frame_func_if_available (this_frame, &info->func))
+    {
+      info->func = -1;
+      return 0;
+    }
+  func = info->func;
 
   /* Try to analyze the prologue.  */
   result = s390_analyze_prologue (gdbarch, func,
@@ -2167,7 +2170,6 @@  s390_prologue_frame_unwind_cache (struct frame_info *this_frame,
       info->local_base = prev_sp - size;
     }
 
-  info->func = func;
   return 1;
 }
 
@@ -2267,7 +2269,11 @@  s390_frame_this_id (struct frame_info *this_frame,
     = s390_frame_unwind_cache (this_frame, this_prologue_cache);
 
   if (info->frame_base == -1)
-    return;
+    {
+      if (info->func != -1)
+	*this_id = frame_id_build_unavailable_stack (info->func);
+      return;
+    }
 
   *this_id = frame_id_build (info->frame_base, info->func);
 }