Re: [PATCH 2/7] gdb: add prepare_reinflate/reinflate around print_frame_args in info_frame_command_core

[Simon Marchi <simon.marchi@polymtl.ca>]

On 11/8/22 04:32, Bruno Larsen wrote:
> On 07/11/2022 16:53, Simon Marchi via Gdb-patches wrote:
>> I noticed this crash:
>>
>>      $ ./gdb --data-directory=data-directory -nx -q \
>>            testsuite/outputs/gdb.python/pretty-print-call-by-hand/pretty-print-call-by-hand \
>>       -x testsuite/outputs/gdb.python/pretty-print-call-by-hand/pretty-print-call-by-hand.py \
>>       -ex "b g" -ex r
>>      (gdb) info frame
>>      Stack level 0, frame at 0x7fffffffdd80:
>>       rip = 0x555555555160 in g
>>          (/home/simark/src/binutils-gdb/gdb/testsuite/gdb.python/pretty-print-call-by-hand.c:41); saved rip = 0x5555555551a3
>>       called by frame at 0x7fffffffdda0
>>       source language c.
>>       Arglist at 0x7fffffffdd70, args: mt=mytype is 0x555555556004 "hello world",
>>          depth=10
>>
>>      Fatal signal: Segmentation fault
>>
>> This is another case of frame_info being invalidated under a function's
>> feet.  The stack trace when the frame_info get invalidated looks like:
>>
>>      ... many frames to pretty print the arg, that eventually invalidate the frame_infos ...
>>      #35 0x00005568d0a8ab24 in print_frame_arg (fp_opts=..., arg=0x7ffc3216bcb0) at /home/simark/src/binutils-gdb/gdb/stack.c:489
>>      #36 0x00005568d0a8cc75 in print_frame_args (fp_opts=..., func=0x621000233210, frame=..., num=-1, stream=0x60b000000300)
>>          at /home/simark/src/binutils-gdb/gdb/stack.c:898
>>      #37 0x00005568d0a9536d in info_frame_command_core (fi=..., selected_frame_p=true) at /home/simark/src/binutils-gdb/gdb/stack.c:1682
>>
>> print_frame_args knows that print_frame_arg can invalidate frame_info
>> objects, and therefore calls prepare_reinflate/reinflate.  However,
>> info_frame_command_core has a separate frame_info_ptr instance (it is
>> passed by value / copy).  So info_frame_command_core needs to know that
>> print_frame_args can invalidate frame_info objects, and therefore needs
>> to prepare_reinflate/reinflate as well.  Add those calls, and enhance
>> the gdb.python/pretty-print-call-by-hand.exp test to test that command.
> 
> Can confirm that the crash is reproducible and that the patch fixes the problem. Sorry for missing it the first time. Makes me wonder if I also missed this in print_frame... Either way:

I checked print_frame, it could use prepare_reinflate/reinflate.  But
the only way for the frame variable to be re-used after the
print_frame_args call is if:

   (funname == NULL || sal.symtab == NULL)

If that expression is true, then it's likely that we don't have debug
info for the function, so it's not likely that some pretty printer was
used.

But this shows how the current frame_info_ptr is error-prone: you have
two know which functions can, deep down their call tree, reinit the
frame cache.  And all their callers that have a frame_info_ptr object,
recursively, must explicitly do prepare_reinflate / reinflate to protect
themselves against their frame_info object being invalidated.  It's very
easy to forget some spots.

I'm currently working on making frame_info_ptr work automatically,
meaning it would grab the wrapped frame id automatically on
construction, and reinflate the frame automatically if needed in
operator-> or operator*.  The thing that currently throws a wrench in
all that is the "frame view" command, which allows you to create and
select frames out of thin air.  And these frames are currently not
reinflatable.  And that prevents using my version of frame_info_ptr in
code paths that can process that kind of frame, like print_frame_args.
It defeats the purpose of frame_info_ptr, because those are the places
that would benefit from that the most.  So I'm currently checking if I
can improve "frame view" and the support for those user-created frames
just enough to make frame_info_ptr support them.

> Reviewed-By: Bruno Larsen <blarsen@redhat.com>

Thanks,

Simon