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


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!

Attachment: gdb-081119-infcall-2.patch.txt
Description: Text document


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