Re: [PATCH v2] gdb/stack.c: avoid stale pointers when printing frame arguments

[Bruno Larsen <blarsen@redhat.com>]

On 6/15/22 12:43, Tom Tromey wrote:
>>>>>> "Bruno" == Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Bruno> @@ -738,8 +738,17 @@ call_function_by_hand (struct value *function,
> Bruno>  		       type *default_return_type,
> Bruno>  		       gdb::array_view<value *> args)
> Bruno>  {
> Bruno> -  return call_function_by_hand_dummy (function, default_return_type,
> Bruno> -				      args, NULL, NULL);
> Bruno> +  scoped_restore_selected_frame restore_frame;
> Bruno> +  struct value *ret = call_function_by_hand_dummy (function,
> Bruno> +						   default_return_type,
> Bruno> +						   args, NULL, NULL);
> Bruno> +  /* Ensure that the frame cache does not contain any frames that were
> Bruno> +     specific to the handmade function call.  If something is leftover
> Bruno> +     while GDB has kept a reference to a frame (for instance, when a
> Bruno> +     stacktrace is being printed and a pretty printed called an inferior
> Bruno> +     function), GDB could crash.  */
> Bruno> +  reinit_frame_cache ();
> 
> I don't understand the need for this call.
> It seems like it must be redundant, because the frame cache should be
> reset by causing the inferior to run during the inferior call.

Oh yes, it must be from an earlier iteration (or me just not understanding how frames work).

All the changes to the function will be reverted locally, as the scoped_restore was here
because of the re-initialization of the frame cache.

> 
> Bruno> +      struct frame_id id = get_frame_id (get_selected_frame (NULL));
> Bruno>        if (execution_direction == EXEC_REVERSE)
> Bruno>  	gdb_printf (_("Run back to call of "));
> Bruno>        else
> Bruno> @@ -1866,6 +1867,10 @@ finish_command (const char *arg, int from_tty)
> Bruno>  	}
>   
> Bruno>        print_stack_frame (get_selected_frame (NULL), 1, LOCATION, 0);
> Bruno> +      frame = frame_find_by_id (id);
> Bruno> +      if (frame == nullptr)
> Bruno> +	  error (_("frames disappeared mid-printing."));
> Bruno> +      frame = get_prev_frame (frame);
> 
> This code probably needs a comment since it's non-obvious why 'finish'
> would need to do the frame-id thing.

I'll add a comment too, but as an explanation to you: Everywhere that prints a frame
will need this because if a frame is printed, it may call a function by hand, which
makes this pointer stale.

> 
> I think error text normally does not end in ".".
> I see a lot of these though, I wonder if we ought to remove them all.

A very rough 'grep error.*\.\" ' showed 32 lines with this.

Since I'm already making a new version, I can change this on my patch, no problem

> 
> Bruno> +	    error (_("frames disappeared mid-printing."));
> 
> Here too.
> 
> Bruno> +  /* print_frame can invalidate frame, so cache the frame_id to rebuild
> Bruno> +     the whole stack later, if needed.  */
> Bruno> +  struct frame_id frame_id = get_frame_id (frame);
> 
> This repeated code makes me wonder if perhaps the print_frame_info API
> ought to be changed instead.

print_frame_info could return an updated pointer to the frame being printed, or receive a
frame_info** and update it in-place.  However, We'll still need to check for a null pointer
after calling, since we won't want print_frame_info to error out if a frame no longer
exists.  We might not care when printing.

Does any of these options sound like a better option? Or would we always want to error out
if the frame disappears from under us?


Cheers!
Bruno Larsen
> 
> Tom
>