[PATCH] stack: fix gdb.dwarf2/dw2-undefined-ret-addr.exp regression

[PATCH] stack: fix gdb.dwarf2/dw2-undefined-ret-addr.exp regression

[Markus Metzger]

Commit a038fa3e14a4 stack: check frame_unwind_caller_id adds a frame_id check to
frame_info and treats a missing frame_id as NOT_AVAILABLE_ERROR.  This causes a
regression in gdb.dwarf2/dw2-undefined-ret-addr.exp.

Treat a missing frame_id as OPTIMIZED_OUT_ERROR instead.

See also https://sourceware.org/ml/gdb-patches/2016-07/msg00273.html.

2016-07-22  Markus Metzger  <markus.t.metzger@intel.com>

gdb/
	* stack.c (frame_info): Call val_print_not_saved instead of
	val_print_unavailable if frame_id check fails.
---
 gdb/stack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/stack.c b/gdb/stack.c
index b9e74df..9782457 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -1508,7 +1508,7 @@ frame_info (char *addr_exp, int from_tty)
   printf_filtered ("saved %s = ", pc_regname);
 
   if (!frame_id_p (frame_unwind_caller_id (fi)))
-    val_print_unavailable (gdb_stdout);
+    val_print_not_saved (gdb_stdout);
   else
     {
       TRY
-- 
1.8.3.1

Re: [PATCH] stack: fix gdb.dwarf2/dw2-undefined-ret-addr.exp regression

[Yao Qi]

Markus Metzger <markus.t.metzger@intel.com> writes:

> Commit a038fa3e14a4 stack: check frame_unwind_caller_id adds a frame_id check to
> frame_info and treats a missing frame_id as NOT_AVAILABLE_ERROR.  This causes a
> regression in gdb.dwarf2/dw2-undefined-ret-addr.exp.
>
> Treat a missing frame_id as OPTIMIZED_OUT_ERROR instead.
>
> See also https://sourceware.org/ml/gdb-patches/2016-07/msg00273.html.
>
> 2016-07-22  Markus Metzger  <markus.t.metzger@intel.com>
>
> gdb/
> 	* stack.c (frame_info): Call val_print_not_saved instead of
> 	val_print_unavailable if frame_id check fails.

It is good to me.  I read this patch several times since July, because
of something else in a038fa3e14a4.  Do we need to move
frame_unwind_caller_id into try block? like this,

  TRY
   {
      if (!frame_id_p (frame_unwind_caller_id (fi)))
         val_print_unavailable (gdb_stdout);
      else
       {
          caller_pc = frame_unwind_caller_pc (fi);
          caller_pc_p = 1;
       }
   }
  CATCH (ex, ....)

frame_unwind_caller_id calls get_prev_frame_always which only catches
MEMORY_ERROR.  IIUC, get_prev_frame_always and it callees may throw
NOT_AVAILABLE_ERROR.

-- 
Yao (齐尧)

RE: [PATCH] stack: fix gdb.dwarf2/dw2-undefined-ret-addr.exp regression

[Metzger, Markus T]

> -----Original Message-----
> From: Yao Qi [mailto:qiyaoltc@gmail.com]
> Sent: Thursday, October 6, 2016 11:32 AM
> To: Metzger, Markus T <markus.t.metzger@intel.com>
> Cc: gdb-patches@sourceware.org; qiyaoltc@gmail.com; palves@redhat.com
> Subject: Re: [PATCH] stack: fix gdb.dwarf2/dw2-undefined-ret-addr.exp
> regression

Hi Yao,

Thanks for your review.


> It is good to me.  I read this patch several times since July, because
> of something else in a038fa3e14a4.  Do we need to move
> frame_unwind_caller_id into try block? like this,
> 
>   TRY
>    {
>       if (!frame_id_p (frame_unwind_caller_id (fi)))
>          val_print_unavailable (gdb_stdout);
>       else
>        {
>           caller_pc = frame_unwind_caller_pc (fi);
>           caller_pc_p = 1;
>        }
>    }
>   CATCH (ex, ....)
> 
> frame_unwind_caller_id calls get_prev_frame_always which only catches
> MEMORY_ERROR.  IIUC, get_prev_frame_always and it callees may throw
> NOT_AVAILABLE_ERROR.

I didn't say that get_prev_frame_always may throw NOT_AVAILABLE_ERROR.
I said that I would treat the case where the trace ends with tailcall frames without
an actual caller frame like NOT_AVAILABLE_ERROR (and this patch is changing this
now to OPTIMIZED_OUT_ERROR).

The problem was that get_prev_frame_always may return NULL, which
skip_artificial_frames did not handle, causing GDB to crash.  We didn't really have
such a case before, but with record-btrace, you could end up with a bunch of
tailcall frames where the actual caller is not contained in the trace.

Pedro suggested that:  "
All the frame_unwind_caller_XXX methods should retrieve information
about the same frame that frame_unwind_caller_id returns.  They should
all really be guarded by a frame_unwind_caller_id check, like:

      if (frame_id_p (frame_unwind_caller_id (frame)))
"

As to whether the callers of frame_unwind_caller_id need to expect an exception,
I can't say for sure but I don't think it likely.  There are a few such checks in GDB
already and none of them seems to expect an exception.

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH] stack: fix gdb.dwarf2/dw2-undefined-ret-addr.exp regression

[Yao Qi]

On Thu, Oct 6, 2016 at 12:03 PM, Metzger, Markus T
<markus.t.metzger@intel.com> wrote:
> As to whether the callers of frame_unwind_caller_id need to expect an exception,
> I can't say for sure but I don't think it likely.  There are a few such checks in GDB
> already and none of them seems to expect an exception.
>

I suspect it may happen when we collect part of the stack in tracepoint,
switch to read data from traceframes, and do unwinding.  Something on
stack may be unavailable for building frame_id.

To be clear, your patch is OK to go in.  Please push it into master and
7.12.  Thanks for your patience on keeping pinging this patch, and
sorry the delayed review.

-- 
Yao (齐尧)