Re: [PATCH v2 11/13] gdb: make user-created frames reinflatable

[Luis Machado <luis.machado@arm.com>]

On 1/25/23 03:45, Simon Marchi wrote:
> 
> 
> 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.)

It is somewhat historical, and it is something I'm planning to address eventually. Right now, though, we hardwire the PC to
obtaining LR.

>   - 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

 From your (very detailed) explanation, it does sound a bit iffy to abuse the frame id mechanism and use
the same id as the existing real frame.

> 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