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

[Doug Evans <dje at google dot com>]

On Wed, Jan 14, 2009 at 7:04 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> 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.

Ah.

>> +/* 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.

I've applied these suggestions to the appended patch.

>> +  volatile struct gdb_exception e;
>
> Does this need to be volatile in this routine?

Leftover turd.  Deleted.

>> +  const char *name;
>> +  /* For "at <hex-addr>".  */
>> +  char name_buf[50];
>
> GNU coding style frowns on "magic" array sizes like that ...

Fixed.

>> +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).

Fixed.

Ok to check in?

2009-01-18  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.
	All callers updated.
	(remove_dummy_frame,pop_dummy_frame,lookup_dummy_frame): 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,frame_info): Delete forward decls.
	(inferior_thread_state): New forward decl.
	(dummy_frame_push): Update prototype.
	* 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: (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.

Attachment: gdb-090118-infcall-6.patch.txt
Description: Text document