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

[Tom Tromey <tom@tromey.com>]

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

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 think error text normally does not end in ".".
I see a lot of these though, I wonder if we ought to remove them all.

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.

Tom