Re: [RFA] dummy frame handling cleanup, plus inferior fun call signal handling improvement

["Ulrich Weigand" <uweigand@de.ibm.com>]

Doug Evans wrote:

> Ulrich suggested making the error messages more consistent.
> I like it but after having gone through the exercise I have a question:
> There are two MI testcases that check the precise wording, do we have to worry
> about frontends that check the wording?
> Maybe changes to the wording can be defered to a later patch?

I don't think frontends are supposed to check the exact wording.
(After all, the text may be translated ...)

> 2009-01-06  Doug Evans  <dje@google.com>
> 
> 	* dummy-frame.c (dummy_frame): Replace regcache member with
> 	caller_state.
> 	(dummy_frame_push): Replace caller_regcache arg with caller_state.
> 	Return pointer to created dummy frame.  All callers updated.
> 	(remove_dummy_frame,do_dummy_frame_cleanup,pop_dummy_frame_from_ptr,
> 	lookup_dummy,lookup_dummy_id,dummy_frame_discard): New fns.
> 	(dummy_frame_pop): Rewrite.  Verify requested frame is in the
> 	dummy frame stack.  Restore program state.
> 	(cleanup_dummy_frames): Rewrite.
> 	(dummy_frame_sniffer): Update.  Make static.
> 	* dummy-frame.h (regcache): Delete forward decl.
> 	(inferior_thread_state,dummy_frame): Add forward decls.
> 	(dummy_frame_push): Update prototype.
> 	(dummy_frame_discard): Declare.
> 	* frame.c (frame_pop): dummy_frame_pop now does all the work for
> 	DUMMY_FRAMEs.
> 	* infcall.c (breakpoint_auto_delete_contents): Delete.
> 	(get_function_name,run_inferior_call): New fns.
> 	(call_function_by_hand): Simplify by moving some code to
> 	get_function_name, run_inferior_call.  Inferior function call wrapped
> 	in TRY_CATCH so there's less need for cleanups and all exits from
> 	proceed are handled similarily.  Detect program exit.
> 	Detect program stopping in a different thread.
> 	Make error messages more consistent.
> 	* inferior.h (inferior_thread_state): Declare (opaque type).
> 	(save_inferior_thread_state,restore_inferior_thread_state,
> 	make_cleanup_restore_inferior_thread_state,
> 	discard_inferior_thread_state, get_inferior_thread_state_regcache):
> 	Declare.
> 	(save_inferior_status): Update prototype.
> 	* infrun.c: #include "dummy-frame.h"
> 	(normal_stop): When stopped for the completion of an inferior function
> 	call, verify the expected stack frame kind.
> 	(inferior_thread_state): New struct.
> 	(save_inferior_thread_state,restore_inferior_thread_state,
> 	do_restore_inferior_thread_state_cleanup,
> 	make_cleanup_restore_inferior_thread_state,
> 	discard_inferior_thread_state,
> 	get_inferior_thread_state_regcache): New functions.
> 	(inferior_status): Move stop_signal, stop_pc, registers to
> 	inferior_thread_state.  Remove restore_stack_info.
> 	(save_inferior_status): Remove arg restore_stack_info.
> 	All callers updated.  Remove saving of state now saved by
> 	save_inferior_thread_state.
> 	(restore_inferior_status): Remove restoration of state now done by
> 	restore_inferior_thread_state.
> 	(discard_inferior_status): Remove freeing of registers, now done by
> 	discard_inferior_thread_state.
> 
> 	* gdb.base/break.exp: Update expected gdb output.
> 	* gdb.base/sepdebug.exp: Ditto.
> 	* gdb.mi/mi-syn-frame.exp: Ditto.
> 	* gdb.mi/mi2-syn-frame.exp: Ditto.
> 
> 	* gdb.base/call-signal-resume.exp: New file.
> 	* gdb.base/call-signals.c: New file.
> 	* gdb.base/unwindonsignal.exp: New file.
> 	* gdb.base/unwindonsignal.c: New file.
> 	* gdb.threads/interrupted-hand-call.exp: New file.
> 	* gdb.threads/interrupted-hand-call.c: New file.
> 	* gdb.threads/thread-unwindonsignal.exp: New file.

This looks all very good to me, thanks!

Just a couple of minor things I noticed:


> +static void
> +cleanup_dummy_frames (struct target_ops *target, int from_tty)
> +{
> +  while (dummy_frame_stack != NULL)
> +    {
> +      remove_dummy_frame (&dummy_frame_stack);
> +    }

GNU coding style would be to omit the braces around a single
statement.

> +/* Discard DUMMY and remove it from the dummy frame stack.
> +   If the frame isn't found, flag an internal error.  */
> +
> +extern void dummy_frame_discard (struct dummy_frame *dummy);

It would seem more in sync with the rest of the interfaces
if this routine were to take a dummy_id instead of a struct
dummy_frame pointer.  That would allow us to keep that struct
fully private to the implementation ...

Also, it seems that you only ever call that function if the
inferior exited while in an inferior call.  Does it matter
to discard the dummy frame in that case?  After all, the
program conceptually still would be in the inferior call,
if it were running ...   Once the program is restarted, the
stale dummy frames will be garbage-collected anyway.

> +  volatile struct gdb_exception e;

Does this need to be volatile in this routine?

> +  const char *name;
> +  /* For "at <hex-addr>".  */
> +  char name_buf[50];

GNU coding style frowns on "magic" array sizes like that ...

> +struct inferior_thread_state *
> +save_inferior_thread_state (void)
> +{
> +  struct inferior_thread_state *inf_state = XMALLOC (struct inferior_thread_state);
> +  struct thread_info *tp = inferior_thread ();
> +  struct inferior *inf = current_inferior ();

It seems "inf" is never needed here (and some of the
other new routines).


Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com