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

[Doug Evans <dje@google.com>]

[-- Attachment #1: Type: text/plain, Size: 10757 bytes --]

On Tue, Nov 18, 2008 at 4:58 AM, Doug Evans <dje@google.com> wrote:
> Hi.  This patch was in progress when I wrote
> http://sourceware.org/ml/gdb-patches/2008-11/msg00391.html
> This patch subsumes that one.
>
> There are two things this patch does:
> 1) properly pop dummy frames more often
> 2) make the inferior resumable after an inferior function call gets a
>   signal without having to remember to do "signal 0"
>
> 1) properly pop dummy frames more often
>
> Ulrich asked:
>> I agree that it would be better if call_function_by_hand were to call
>> dummy_frame_pop in this case.  However, why exactly is this a bug?
>
> It's a bug because it's confusing to not pop the frame in the places
> where one is able to.  Plus it's, well, unclean.
> I recognize that the dummy frame stack can't be precisely managed because
> the user can do things like:
>
> set $orig_pc = $pc
> set $orig_sp = $sp
> break my_fun
> call my_fun()
> set $pc = $orig_pc
> set $sp = $orig_sp
> continue
>
> [This doesn't always work, but you get the idea.]
> At the "continue", the inferior function call no longer exists and
> gdb's mechanisms for removing the dummy frame from dummy_frame_stack
> will never trigger (until the program is resumed and cleanup_dummy_frames
> is called).  But we can still keep things clean
> and unconfusing for the common case.
>
> 2) make the inferior resumable after an inferior function call gets a
>   signal without having to remember to do "signal 0"
>
> If an inferior function call gets a signal (say SIGSEGV or SIGABRT),
> one should be able to easily resume program execution after popping the stack.
> It works today, but after manually popping the stack (e.g. with
> "frame <dummy> ; return" where <dummy> is the dummy stack frame's number)
> one has to remember to resume the program with "signal 0".
> This is because stop_signal doesn't get restored.
> Maybe there's a reason it shouldn't be, or maybe should be made an option,
> but the current behaviour seems user-unfriendly for the normal case.
>
> Restoring stop_signal also has the benefit that if an inferior function
> call is made after getting a signal, and the inferior function call
> doesn't complete (e.g. has a breakpoint and doesn't complete right away),
> the user can resume the program (after popping the inf fun call off the
> stack) with "continue" without having to remember what the signal was
> and remember to use "signal N".
>
> [BTW, IWBN if there was a command that did the equivalent of
> "frame <dummy> ; return", named say "abandon", so that one didn't have
> to go to the trouble of finding the dummy frame's stack number.
> "abandon" would have the additional benefit that if the stack
> was corrupted and one couldn't find the dummy frame, it would still
> work since everything it needs is in dummy_frame_stack - it doesn't need
> to examine the inferior's stack, except maybe for some sanity checking.
> Continuing the inferior may still not be possible, but it does give the
> user a more straightforward way to abandon an inferior function call
> than exists today.]
>
> The bulk of the patch goes into making (2) work in a clean way.
> Right now the inferior state that can be restored is a collection of
> inferior_status, regcache, and misc. things like stop_pc (see the
> assignment of stop_pc in normal_stop() when stop_stack_dummy).
> The patch organizes the state into two pieces: inferior program state
> (registers, stop_pc, stop_signal) and gdb session state
> (the rest of inferior_status).
> Program state is recorded on the dummy frame stack and is always
> restored, either when the inferior function call completes or
> when the stack frame is manually popped.
> inferior_status contains the things that only get restored
> if either the inferior function call successfully completes or
> it gets a signal and unwindonsignal is set.
>
> P.S. I've removed one copy of the regcache.  Hopefully I've structured
> things in a way that doesn't lose.
>
> NOTES:
> - this adds the unwindonsignal.exp testcase from before, plus a new
>  testcase that verifies one can resume the inferior after abandoning
>  an inferior function call that gets a signal
>
> It's a big patch so presumably there are issues with it.
> Comments?
>
> [tested on i386-linux and x86_64-linux]
>
> 2008-11-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.
>        Return pointer to created dummy frame.  All callers updated.
>        (remove_dummy_frame,do_dummy_frame_cleanup,pop_dummy_frame,
>        assert_dummy_present,pop_dummy_frame_below,lookup_dummy_id,
>        dummy_frame_discard,do_pop_dummy_frame_cleanup,
>        make_cleanup_pop_dummy_frame): New functions.
>        (dummy_frame_pop): Rewrite.  Verify requested frame is in the
>        dummy frame stack.  Restore program state.  Discard dummy frames below
>        the one being deleted.
>        (dummy_frame_sniffer): Update.
>        * dummy-frame.h (dummy_frame_push): Update prototype.
>        (dummy_frame_discard,make_cleanup_pop_dummy_frame): Declare.
>        * frame.c (frame_pop): dummy_frame_pop now does all the work for
>        DUMMY_FRAMEs.
>        * infcall.c (call_function_by_hand): Replace caller_regcache,
>        caller_regcache_cleanup with caller_state, caller_state_cleanup.
>        New locals dummy_frame, dummy_frame_cleanup.
>        Ensure dummy frame is popped/discarded for all possible exit paths.
>        * inferior.h (inferior_program_state): Declare (opaque type).
>        (save_inferior_program_state,restore_inferior_program_state,
>        make_cleanup_restore_inferior_program_state,
>        discard_inferior_program_state,
>        get_inferior_program_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 and use dummy_frame_pop.
>        (inferior_program_state): New struct.
>        (save_inferior_program_state,restore_inferior_program_state,
>        make_cleanup_restore_inferior_program_state,
>        discard_inferior_program_state,
>        get_inferior_program_state_regcache): New functions.
>        (save_inferior_status): Remove arg restore_stack_info.
>        All callers updated.  Remove saving of state now saved by
>        save_inferior_program_state.
>        (restore_inferior_status): Remove restoration of state now done by
>        restore_inferior_program_state.
>        (discard_inferior_status): Remove freeing of registers, now done by
>        discard_inferior_program_state.
>
>        * gdb.base/call-signal-resume.exp: New file.
>        * gdb.base/call-signals.c: New file.
>        * gdb.base/unwindonsignal.exp: New file.

ref: http://sourceware.org/ml/gdb-patches/2008-11/msg00454.html

Blech.  I went too far in trying to keep dummy_frame_stack accurate.
One can (theoretically) have multiple hand-function-calls outstanding
in multiple threads (and soon in multiple processes presumably).
Attached is a new version of the patch that goes back to having
dummy_frame_pop only popping the requested frame (and similarily for
dummy_frame_discard).

I wrote a testcase that calls functions in multiple threads (with
scheduler-locking on) by setting a breakpoint on the function being
called.  I think there's a bug in scheduler-locking support as when I
make the second call in the second thread, gdb makes the first thread
step over the breakpoint and then resume, and control returns to
call_function_by_hand with inferior_ptid changed to the first thread.
To protect call_function_by_hand from this it now flags an error if
the thread changes.
[I'll submit the testcase separately once I can get it to pass, unless
folks want it to see it.]

How's this?

2008-11-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.
        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, pop_dummy_frame,dummy_frame_discard,
        do_pop_dummy_frame_cleanup,make_cleanup_pop_dummy_frame): New
        functions.
        (dummy_frame_pop): Rewrite.  Verify requested frame is in the
        dummy frame stack.  Restore program state.
        (cleanup_dummy_frames): Rewrite.
        (dummy_frame_sniffer): Update.
        * dummy-frame.h (dummy_frame_push): Update prototype.
        (dummy_frame_discard,make_cleanup_pop_dummy_frame): Declare.
        * frame.c (frame_pop): dummy_frame_pop now does all the work for
        DUMMY_FRAMEs.
        * infcall.c (call_function_by_hand): Replace caller_regcache,
        caller_regcache_cleanup with caller_state, caller_state_cleanup.
        New locals dummy_frame, dummy_frame_cleanup, this_thread.
        Ensure dummy frame is popped/discarded for all possible exit paths.
        Detect program stopping in a different thread.
        * inferior.h (inferior_program_state): Declare (opaque type).
        (save_inferior_program_state,restore_inferior_program_state,
        make_cleanup_restore_inferior_program_state,
        discard_inferior_program_state,
        get_inferior_program_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 and use dummy_frame_pop.
        (inferior_program_state): New struct.
        (save_inferior_program_state,restore_inferior_program_state,
        do_restore_inferior_program_state_cleanup,
        make_cleanup_restore_inferior_program_state,
        discard_inferior_program_state,
        get_inferior_program_state_regcache): New functions.
        (inferior_status): Remove members stop_signal, stop_pc, registers,
        restore_stack_info.
        (save_inferior_status): Remove arg restore_stack_info.
        All callers updated.  Remove saving of state now saved by
        save_inferior_program_state.
        (restore_inferior_status): Remove restoration of state now done by
        restore_inferior_program_state.
        (discard_inferior_status): Remove freeing of registers, now done by
        discard_inferior_program_state.

        * gdb.base/call-signal-resume.exp: New file.
        * gdb.base/call-signals.c: New file.
        * gdb.base/unwindonsignal.exp: New file.

[-- Attachment #2: gdb-081119-sym-info-2.patch.txt --]
[-- Type: text/plain, Size: 3606 bytes --]

2008-11-19  Doug Evans  <dje@google.com>

	* printcmd.c (sym_info): Don't print the offset if it's zero.

Index: printcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/printcmd.c,v
retrieving revision 1.137
diff -u -p -r1.137 printcmd.c
--- printcmd.c	18 Nov 2008 21:31:26 -0000	1.137
+++ printcmd.c	20 Nov 2008 06:09:30 -0000
@@ -1013,6 +1013,8 @@ sym_info (char *arg, int from_tty)
 	&& (msymbol = lookup_minimal_symbol_by_pc_section (sect_addr, osect)))
       {
 	const char *obj_name, *mapped, *sec_name, *msym_name;
+	char *loc_string;
+	struct cleanup *old_chain;
 
 	matches = 1;
 	offset = sect_addr - SYMBOL_VALUE_ADDRESS (msymbol);
@@ -1020,43 +1022,55 @@ sym_info (char *arg, int from_tty)
 	sec_name = osect->the_bfd_section->name;
 	msym_name = SYMBOL_PRINT_NAME (msymbol);
 
+	/* Don't print the offset if it is zero.
+	   We assume there's no need to handle i18n of "sym + offset".  */
+	if (offset)
+	  xasprintf (&loc_string, "%s + %u", msym_name, offset);
+	else
+	  xasprintf (&loc_string, "%s", msym_name);
+
+	/* Use a cleanup to free loc_string in case the user quits
+	   a pagination request inside printf_filtered.  */
+	old_chain = make_cleanup (xfree, loc_string);
+
 	gdb_assert (osect->objfile && osect->objfile->name);
 	obj_name = osect->objfile->name;
 
 	if (MULTI_OBJFILE_P ())
 	  if (pc_in_unmapped_range (addr, osect))
 	    if (section_is_overlay (osect))
-	      printf_filtered (_("%s + %u in load address range of "
+	      printf_filtered (_("%s in load address range of "
 				 "%s overlay section %s of %s\n"),
-			       msym_name, offset,
-			       mapped, sec_name, obj_name);
+			       loc_string, mapped, sec_name, obj_name);
 	    else
-	      printf_filtered (_("%s + %u in load address range of "
+	      printf_filtered (_("%s in load address range of "
 				 "section %s of %s\n"),
-			       msym_name, offset, sec_name, obj_name);
+			       loc_string, sec_name, obj_name);
 	  else
 	    if (section_is_overlay (osect))
-	      printf_filtered (_("%s + %u in %s overlay section %s of %s\n"),
-			       msym_name, offset, mapped, sec_name, obj_name);
+	      printf_filtered (_("%s in %s overlay section %s of %s\n"),
+			       loc_string, mapped, sec_name, obj_name);
 	    else
-	      printf_filtered (_("%s + %u in section %s of %s\n"),
-			       msym_name, offset, sec_name, obj_name);
+	      printf_filtered (_("%s in section %s of %s\n"),
+			       loc_string, sec_name, obj_name);
 	else
 	  if (pc_in_unmapped_range (addr, osect))
 	    if (section_is_overlay (osect))
-	      printf_filtered (_("%s + %u in load address range of %s overlay "
+	      printf_filtered (_("%s in load address range of %s overlay "
 				 "section %s\n"),
-			       msym_name, offset, mapped, sec_name);
+			       loc_string, mapped, sec_name);
 	    else
-	      printf_filtered (_("%s + %u in load address range of section %s\n"),
-			       msym_name, offset, sec_name);
+	      printf_filtered (_("%s in load address range of section %s\n"),
+			       loc_string, sec_name);
 	  else
 	    if (section_is_overlay (osect))
-	      printf_filtered (_("%s + %u in %s overlay section %s\n"),
-			       msym_name, offset, mapped, sec_name);
+	      printf_filtered (_("%s in %s overlay section %s\n"),
+			       loc_string, mapped, sec_name);
 	    else
-	      printf_filtered (_("%s + %u in section %s\n"),
-			       msym_name, offset, sec_name);
+	      printf_filtered (_("%s in section %s\n"),
+			       loc_string, sec_name);
+
+	do_cleanups (old_chain);
       }
   }
   if (matches == 0)