public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Simon Marchi <simon.marchi@polymtl.ca>
To: Luis Machado <luis.machado@arm.com>,
	Tom de Vries <tdevries@suse.de>,
	gdb-patches@sourceware.org
Subject: Re: [PATCH v2 11/13] gdb: make user-created frames reinflatable
Date: Tue, 24 Jan 2023 22:45:00 -0500	[thread overview]
Message-ID: <f1bfbc78-bccc-f0e3-2e13-06857785223d@polymtl.ca> (raw)
In-Reply-To: <0fdfcde0-f51f-6f59-82c3-1089d2d016f0@arm.com>



On 1/24/23 03:22, Luis Machado wrote:
> On 1/24/23 03:55, Simon Marchi wrote:
>>
>>
>> On 1/23/23 09:34, Luis Machado wrote:
>>> On 1/23/23 12:57, Tom de Vries wrote:
>>>> On 12/14/22 04:34, Simon Marchi via Gdb-patches wrote:
>>>>>    gdb/testsuite/gdb.base/frame-view.exp    | 47 ++++++++++++--
>>>>
>>>> Hi,
>>>>
>>>> on aarch64-linux I get:
>>>> ...
>>>> FAIL: gdb.base/frame-view.exp: with_pretty_printer=true: frame (GDB internal error)
>>>> FAIL: gdb.base/frame-view.exp: with_pretty_printer=true: frame again (GDB internal error)
>>>
>>>
>>> I can reproduce this as well. It also happens on arm-linux Ubuntu 22.04/20.04, with a similar kind of backtrace.
>>
>> I can reproduce too, I have a potential fix.  I'll try to send it
>> tomorrow.
> 
> Let me know if I can help with testing it on one of the targets.

Ok, so I have a patch that fixes the test, but I'm not sure it's really
good.

First, here's my analysis of what I think is happening.

 - When we create the user frame (the "select-frame view" command), we
   create a sentinel frame just for our user-created frame, in
   create_new_frame.  This sentinel frame has the same id as the regular
   sentinel frame.
 - When printing the frame, after doing the "select-frame view"
   command, the argument's pretty printer is invoked, which does an
   inferior function call (this is the point of the test).  This clears
   the frame cache, including the "real" sentinel frame, which sets the
   sentinel_frame global to nullptr.
 - Later in the frame-printing process (when printing the second
   argument), the auto-reinflation mechanism (which would have been the
   manual reinflate call before, doesn't matter here) re-creates the
   user frame by calling create_new_frame again, creating its own
   special sentinel frame again.  However, note that the "real" sentinel
   frame, the sentinel_frame global, is still nullptr.  If the selected
   frame had been a regular frame, we would have called
   get_current_frame at some point during the reinflation, which would
   have re-created the "real" sentinel frame.  But it's not the case
   here.
 - Deep down the stack, something wants to fill in the unwind stop
   reason for frame 0, which requires trying to unwind frame 1.  This
   leads us to trying to unwind the PC of frame 1:

     #0  gdbarch_unwind_pc (gdbarch=0xffff8d010080, next_frame=...) at /home/simark/src/binutils-gdb/gdb/gdbarch.c:2955
     #1  0x000000000134569c in dwarf2_tailcall_sniffer_first (this_frame=..., tailcall_cachep=0xffff773fcae0, entry_cfa_sp_offsetp=0xfffff7f7d450)
         at /home/simark/src/binutils-gdb/gdb/dwarf2/frame-tailcall.c:390
     #2  0x0000000001355d84 in dwarf2_frame_cache (this_frame=..., this_cache=0xffff773fc928) at /home/simark/src/binutils-gdb/gdb/dwarf2/frame.c:1089
     #3  0x00000000013562b0 in dwarf2_frame_unwind_stop_reason (this_frame=..., this_cache=0xffff773fc928) at /home/simark/src/binutils-gdb/gdb/dwarf2/frame.c:1101
     #4  0x0000000001990f64 in get_prev_frame_always_1 (this_frame=...) at /home/simark/src/binutils-gdb/gdb/frame.c:2281
     #5  0x0000000001993034 in get_prev_frame_always (this_frame=...) at /home/simark/src/binutils-gdb/gdb/frame.c:2376
     #6  0x000000000199b814 in get_frame_unwind_stop_reason (frame=...) at /home/simark/src/binutils-gdb/gdb/frame.c:3051
     #7  0x0000000001359cd8 in dwarf2_frame_cfa (this_frame=...) at /home/simark/src/binutils-gdb/gdb/dwarf2/frame.c:1356
     #8  0x000000000132122c in dwarf_expr_context::execute_stack_op (this=0xfffff7f80170, op_ptr=0xffff8d8883ee "\217\002", op_end=0xffff8d8883ee "\217\002")
         at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:2110
     #9  0x0000000001317b30 in dwarf_expr_context::eval (this=0xfffff7f80170, addr=0xffff8d8883ed "\234\217\002", len=1) at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1239
     #10 0x000000000131d68c in dwarf_expr_context::execute_stack_op (this=0xfffff7f80170, op_ptr=0xffff8d88840e "", op_end=0xffff8d88840e "") at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1811
     #11 0x0000000001317b30 in dwarf_expr_context::eval (this=0xfffff7f80170, addr=0xffff8d88840c "\221p", len=2) at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1239
     #12 0x0000000001314c3c in dwarf_expr_context::evaluate (this=0xfffff7f80170, addr=0xffff8d88840c "\221p", len=2, as_lval=true, per_cu=0xffff90b03700, frame=..., addr_info=0x0, 
         type=0xffff8f6c8400, subobj_type=0xffff8f6c8400, subobj_offset=0) at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1078
     #13 0x000000000149f9e0 in dwarf2_evaluate_loc_desc_full (type=0xffff8f6c8400, frame=..., data=0xffff8d88840c "\221p", size=2, per_cu=0xffff90b03700, per_objfile=0xffff9070b980, 
         subobj_type=0xffff8f6c8400, subobj_byte_offset=0, as_lval=true) at /home/simark/src/binutils-gdb/gdb/dwarf2/loc.c:1513
     #14 0x00000000014a0100 in dwarf2_evaluate_loc_desc (type=0xffff8f6c8400, frame=..., data=0xffff8d88840c "\221p", size=2, per_cu=0xffff90b03700, per_objfile=0xffff9070b980, as_lval=true)
         at /home/simark/src/binutils-gdb/gdb/dwarf2/loc.c:1557
     #15 0x00000000014aa584 in locexpr_read_variable (symbol=0xffff8f6cd770, frame=...) at /home/simark/src/binutils-gdb/gdb/dwarf2/loc.c:3052
 - AArch64 defines a special "prev register" function,
   aarch64_dwarf2_prev_register, to handle unwinding the PC.  This
   function does

     frame_unwind_register_unsigned (this_frame, AARCH64_LR_REGNUM);

   (I must admit that I don't really understand this part, it seems to
    me like you would want to unwind LR's value from this_frame->next...
    but anyway.)
 - frame_unwind_register_unsigned ultimately creates a lazy register
   value, saving the frame id of this_frame->next.  this_frame is the
   user-created frame, to this_frame->next is the special sentinel frame
   we created for it.
 - When time comes to un-lazify the value, value_fetch_lazy_register
   calls frame_find_by_id, to find the frame with the id we saved.
 - frame_find_by_id sees it's the sentinel frame id, so returns the
   sentinel_frame global, which is, if you remember, nullptr.
 - We hit the `gdb_assert (next_frame != NULL)` assertion.

I have a hard time pinpointing a single thing that is wrong.  The fact
that the user-created frames have their own sentinel frame, which share
the same id as the real sentinel frame, that sounds like it can't work
with frame_find_by_id.  In the case without the pretty printer, where
things appear to work, it means that frame_find_by_id returns the "real"
sentinel frame, part of the "real" stack frame chain, and not the
sentinel that is attached to the user-created frame.  Things still kinda
work in the end because those two sentinel frames behave the same, as
far as "prev register" is concerned.

Ultimately, I don't really understand how this "frame view" feature is
useful.  Sure, we pretend that the frame's sp and pc is what the user
gave, but what about the other registers?  It seems to me like you are
very likely to get bogus values.

The patch I had in mind was to give user-created frames a special
unwinder, rather than letting other unwinders (the arch-specific ones,
the dwarf2 one, etc) kick in.  That unwinder would always return the
unwind stop reason UNWIND_OUTERMOST, which effectively makes the
user-created frame an outer frame, and we would never try to unwind any
register value from it.  It makes the test pass, because it breaks the
chain of event explained above.  But it's probably just papering over
the actual problem.

Simon

  reply	other threads:[~2023-01-25  3:45 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-14  3:34 [PATCH v2 00/13] Make frame_info_ptr automatic Simon Marchi
2022-12-14  3:34 ` [PATCH v2 01/13] gdb: move type_map_instance to compile/compile.c Simon Marchi
2022-12-14  3:34 ` [PATCH v2 02/13] gdb: move compile_instance to compile/compile.h Simon Marchi
2022-12-14  3:34 ` [PATCH v2 03/13] gdb: remove language.h include from frame.h Simon Marchi
2022-12-14  3:34 ` [PATCH v2 04/13] gdb: move sect_offset and cu_offset to dwarf2/types.h Simon Marchi
2022-12-14  3:34 ` [PATCH v2 05/13] gdb: move call site types to call-site.h Simon Marchi
2022-12-14  3:34 ` [PATCH v2 06/13] gdb: move frame_info_ptr to frame.{c,h} Simon Marchi
2022-12-20 17:01   ` Bruno Larsen
2023-01-03 18:59     ` Simon Marchi
2022-12-14  3:34 ` [PATCH v2 07/13] gdb: add frame_id::user_created_p Simon Marchi
2022-12-14  3:34 ` [PATCH v2 08/13] gdb: add user-created frames to stash Simon Marchi
2022-12-14  3:34 ` [PATCH v2 09/13] gdb: add create_new_frame(frame_id) overload Simon Marchi
2022-12-14  3:34 ` [PATCH v2 10/13] gdb: make it possible to restore selected user-created frames Simon Marchi
2022-12-14  3:34 ` [PATCH v2 11/13] gdb: make user-created frames reinflatable Simon Marchi
2023-01-23 12:57   ` Tom de Vries
2023-01-23 14:34     ` Luis Machado
2023-01-24  3:55       ` Simon Marchi
2023-01-24  8:22         ` Luis Machado
2023-01-25  3:45           ` Simon Marchi [this message]
2023-01-30  8:49             ` Luis Machado
2023-01-30 16:20               ` Simon Marchi
2022-12-14  3:34 ` [PATCH v2 12/13] gdb: make frame_info_ptr grab frame level and id on construction Simon Marchi
2022-12-14  3:34 ` [PATCH v2 13/13] gdb: make frame_info_ptr auto-reinflatable Simon Marchi
2022-12-20 16:57 ` [PATCH v2 00/13] Make frame_info_ptr automatic Bruno Larsen
2023-01-03 19:00   ` Simon Marchi
2023-01-03 19:09 ` Simon Marchi
2023-01-18 18:10 ` Tom Tromey
2023-01-19  3:40   ` Simon Marchi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f1bfbc78-bccc-f0e3-2e13-06857785223d@polymtl.ca \
    --to=simon.marchi@polymtl.ca \
    --cc=gdb-patches@sourceware.org \
    --cc=luis.machado@arm.com \
    --cc=tdevries@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).