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

Doug Evans
2008-11-20T08:04:46Z

[-- 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)