This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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!
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]