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

Doug Evans
2008-12-01T20:52:07Z

Ping.

On Thu, Nov 20, 2008 at 12:07 AM, Doug Evans <dje@google.com> wrote:
> On Thu, Nov 20, 2008 at 12:03 AM, Doug Evans <dje@google.com> wrote:
>> 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.
>>
>
> Ummm, this time with the correct patch attached. Sorry!
>