From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2336 invoked by alias); 20 Nov 2008 08:04:46 -0000 Received: (qmail 1894 invoked by uid 22791); 20 Nov 2008 08:04:21 -0000 X-Spam-Check-By: sourceware.org Received: from smtp-out.google.com (HELO smtp-out.google.com) (216.239.45.13) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 20 Nov 2008 08:03:25 +0000 Received: from wpaz9.hot.corp.google.com (wpaz9.hot.corp.google.com [172.24.198.73]) by smtp-out.google.com with ESMTP id mAK83NIh024913 for ; Thu, 20 Nov 2008 00:03:23 -0800 Received: from rv-out-0506.google.com (rvbg37.prod.google.com [10.140.83.37]) by wpaz9.hot.corp.google.com with ESMTP id mAK830mn014425 for ; Thu, 20 Nov 2008 00:03:21 -0800 Received: by rv-out-0506.google.com with SMTP id g37so339221rvb.25 for ; Thu, 20 Nov 2008 00:03:21 -0800 (PST) MIME-Version: 1.0 Received: by 10.140.157.1 with SMTP id f1mr1091185rve.196.1227168201253; Thu, 20 Nov 2008 00:03:21 -0800 (PST) In-Reply-To: <20081118125838.0613C412301@localhost> References: <20081118125838.0613C412301@localhost> Date: Thu, 20 Nov 2008 15:02:00 -0000 Message-ID: Subject: Re: [RFA] dummy frame handling cleanup, plus inferior fun call signal handling improvement From: Doug Evans To: gdb-patches@sourceware.org Content-Type: multipart/mixed; boundary=000e0cd29c70b725a3045c1a5c2a X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-11/txt/msg00545.txt.bz2 --000e0cd29c70b725a3045c1a5c2a Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-length: 10757 On Tue, Nov 18, 2008 at 4:58 AM, Doug Evans 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 ; return" where 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 ; 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 > > * 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 * 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. --000e0cd29c70b725a3045c1a5c2a Content-Type: text/plain; charset=US-ASCII; name="gdb-081119-sym-info-2.patch.txt" Content-Disposition: attachment; filename="gdb-081119-sym-info-2.patch.txt" Content-Transfer-Encoding: base64 X-Attachment-Id: f_fnr46qfv0 Content-length: 4889 MjAwOC0xMS0xOSAgRG91ZyBFdmFucyAgPGRqZUBnb29nbGUuY29tPgoKCSog cHJpbnRjbWQuYyAoc3ltX2luZm8pOiBEb24ndCBwcmludCB0aGUgb2Zmc2V0 IGlmIGl0J3MgemVyby4KCkluZGV4OiBwcmludGNtZC5jCj09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT0KUkNTIGZpbGU6IC9jdnMvc3JjL3NyYy9nZGIvcHJpbnRj bWQuYyx2CnJldHJpZXZpbmcgcmV2aXNpb24gMS4xMzcKZGlmZiAtdSAtcCAt cjEuMTM3IHByaW50Y21kLmMKLS0tIHByaW50Y21kLmMJMTggTm92IDIwMDgg MjE6MzE6MjYgLTAwMDAJMS4xMzcKKysrIHByaW50Y21kLmMJMjAgTm92IDIw MDggMDY6MDk6MzAgLTAwMDAKQEAgLTEwMTMsNiArMTAxMyw4IEBAIHN5bV9p bmZvIChjaGFyICphcmcsIGludCBmcm9tX3R0eSkKIAkmJiAobXN5bWJvbCA9 IGxvb2t1cF9taW5pbWFsX3N5bWJvbF9ieV9wY19zZWN0aW9uIChzZWN0X2Fk ZHIsIG9zZWN0KSkpCiAgICAgICB7CiAJY29uc3QgY2hhciAqb2JqX25hbWUs ICptYXBwZWQsICpzZWNfbmFtZSwgKm1zeW1fbmFtZTsKKwljaGFyICpsb2Nf c3RyaW5nOworCXN0cnVjdCBjbGVhbnVwICpvbGRfY2hhaW47CiAKIAltYXRj aGVzID0gMTsKIAlvZmZzZXQgPSBzZWN0X2FkZHIgLSBTWU1CT0xfVkFMVUVf QUREUkVTUyAobXN5bWJvbCk7CkBAIC0xMDIwLDQzICsxMDIyLDU1IEBAIHN5 bV9pbmZvIChjaGFyICphcmcsIGludCBmcm9tX3R0eSkKIAlzZWNfbmFtZSA9 IG9zZWN0LT50aGVfYmZkX3NlY3Rpb24tPm5hbWU7CiAJbXN5bV9uYW1lID0g U1lNQk9MX1BSSU5UX05BTUUgKG1zeW1ib2wpOwogCisJLyogRG9uJ3QgcHJp bnQgdGhlIG9mZnNldCBpZiBpdCBpcyB6ZXJvLgorCSAgIFdlIGFzc3VtZSB0 aGVyZSdzIG5vIG5lZWQgdG8gaGFuZGxlIGkxOG4gb2YgInN5bSArIG9mZnNl dCIuICAqLworCWlmIChvZmZzZXQpCisJICB4YXNwcmludGYgKCZsb2Nfc3Ry aW5nLCAiJXMgKyAldSIsIG1zeW1fbmFtZSwgb2Zmc2V0KTsKKwllbHNlCisJ ICB4YXNwcmludGYgKCZsb2Nfc3RyaW5nLCAiJXMiLCBtc3ltX25hbWUpOwor CisJLyogVXNlIGEgY2xlYW51cCB0byBmcmVlIGxvY19zdHJpbmcgaW4gY2Fz ZSB0aGUgdXNlciBxdWl0cworCSAgIGEgcGFnaW5hdGlvbiByZXF1ZXN0IGlu c2lkZSBwcmludGZfZmlsdGVyZWQuICAqLworCW9sZF9jaGFpbiA9IG1ha2Vf Y2xlYW51cCAoeGZyZWUsIGxvY19zdHJpbmcpOworCiAJZ2RiX2Fzc2VydCAo b3NlY3QtPm9iamZpbGUgJiYgb3NlY3QtPm9iamZpbGUtPm5hbWUpOwogCW9i al9uYW1lID0gb3NlY3QtPm9iamZpbGUtPm5hbWU7CiAKIAlpZiAoTVVMVElf T0JKRklMRV9QICgpKQogCSAgaWYgKHBjX2luX3VubWFwcGVkX3JhbmdlIChh ZGRyLCBvc2VjdCkpCiAJICAgIGlmIChzZWN0aW9uX2lzX292ZXJsYXkgKG9z ZWN0KSkKLQkgICAgICBwcmludGZfZmlsdGVyZWQgKF8oIiVzICsgJXUgaW4g bG9hZCBhZGRyZXNzIHJhbmdlIG9mICIKKwkgICAgICBwcmludGZfZmlsdGVy ZWQgKF8oIiVzIGluIGxvYWQgYWRkcmVzcyByYW5nZSBvZiAiCiAJCQkJICIl cyBvdmVybGF5IHNlY3Rpb24gJXMgb2YgJXNcbiIpLAotCQkJICAgICAgIG1z eW1fbmFtZSwgb2Zmc2V0LAotCQkJICAgICAgIG1hcHBlZCwgc2VjX25hbWUs IG9ial9uYW1lKTsKKwkJCSAgICAgICBsb2Nfc3RyaW5nLCBtYXBwZWQsIHNl Y19uYW1lLCBvYmpfbmFtZSk7CiAJICAgIGVsc2UKLQkgICAgICBwcmludGZf ZmlsdGVyZWQgKF8oIiVzICsgJXUgaW4gbG9hZCBhZGRyZXNzIHJhbmdlIG9m ICIKKwkgICAgICBwcmludGZfZmlsdGVyZWQgKF8oIiVzIGluIGxvYWQgYWRk cmVzcyByYW5nZSBvZiAiCiAJCQkJICJzZWN0aW9uICVzIG9mICVzXG4iKSwK LQkJCSAgICAgICBtc3ltX25hbWUsIG9mZnNldCwgc2VjX25hbWUsIG9ial9u YW1lKTsKKwkJCSAgICAgICBsb2Nfc3RyaW5nLCBzZWNfbmFtZSwgb2JqX25h bWUpOwogCSAgZWxzZQogCSAgICBpZiAoc2VjdGlvbl9pc19vdmVybGF5IChv c2VjdCkpCi0JICAgICAgcHJpbnRmX2ZpbHRlcmVkIChfKCIlcyArICV1IGlu ICVzIG92ZXJsYXkgc2VjdGlvbiAlcyBvZiAlc1xuIiksCi0JCQkgICAgICAg bXN5bV9uYW1lLCBvZmZzZXQsIG1hcHBlZCwgc2VjX25hbWUsIG9ial9uYW1l KTsKKwkgICAgICBwcmludGZfZmlsdGVyZWQgKF8oIiVzIGluICVzIG92ZXJs YXkgc2VjdGlvbiAlcyBvZiAlc1xuIiksCisJCQkgICAgICAgbG9jX3N0cmlu ZywgbWFwcGVkLCBzZWNfbmFtZSwgb2JqX25hbWUpOwogCSAgICBlbHNlCi0J ICAgICAgcHJpbnRmX2ZpbHRlcmVkIChfKCIlcyArICV1IGluIHNlY3Rpb24g JXMgb2YgJXNcbiIpLAotCQkJICAgICAgIG1zeW1fbmFtZSwgb2Zmc2V0LCBz ZWNfbmFtZSwgb2JqX25hbWUpOworCSAgICAgIHByaW50Zl9maWx0ZXJlZCAo XygiJXMgaW4gc2VjdGlvbiAlcyBvZiAlc1xuIiksCisJCQkgICAgICAgbG9j X3N0cmluZywgc2VjX25hbWUsIG9ial9uYW1lKTsKIAllbHNlCiAJICBpZiAo cGNfaW5fdW5tYXBwZWRfcmFuZ2UgKGFkZHIsIG9zZWN0KSkKIAkgICAgaWYg KHNlY3Rpb25faXNfb3ZlcmxheSAob3NlY3QpKQotCSAgICAgIHByaW50Zl9m aWx0ZXJlZCAoXygiJXMgKyAldSBpbiBsb2FkIGFkZHJlc3MgcmFuZ2Ugb2Yg JXMgb3ZlcmxheSAiCisJICAgICAgcHJpbnRmX2ZpbHRlcmVkIChfKCIlcyBp biBsb2FkIGFkZHJlc3MgcmFuZ2Ugb2YgJXMgb3ZlcmxheSAiCiAJCQkJICJz ZWN0aW9uICVzXG4iKSwKLQkJCSAgICAgICBtc3ltX25hbWUsIG9mZnNldCwg bWFwcGVkLCBzZWNfbmFtZSk7CisJCQkgICAgICAgbG9jX3N0cmluZywgbWFw cGVkLCBzZWNfbmFtZSk7CiAJICAgIGVsc2UKLQkgICAgICBwcmludGZfZmls dGVyZWQgKF8oIiVzICsgJXUgaW4gbG9hZCBhZGRyZXNzIHJhbmdlIG9mIHNl Y3Rpb24gJXNcbiIpLAotCQkJICAgICAgIG1zeW1fbmFtZSwgb2Zmc2V0LCBz ZWNfbmFtZSk7CisJICAgICAgcHJpbnRmX2ZpbHRlcmVkIChfKCIlcyBpbiBs b2FkIGFkZHJlc3MgcmFuZ2Ugb2Ygc2VjdGlvbiAlc1xuIiksCisJCQkgICAg ICAgbG9jX3N0cmluZywgc2VjX25hbWUpOwogCSAgZWxzZQogCSAgICBpZiAo c2VjdGlvbl9pc19vdmVybGF5IChvc2VjdCkpCi0JICAgICAgcHJpbnRmX2Zp bHRlcmVkIChfKCIlcyArICV1IGluICVzIG92ZXJsYXkgc2VjdGlvbiAlc1xu IiksCi0JCQkgICAgICAgbXN5bV9uYW1lLCBvZmZzZXQsIG1hcHBlZCwgc2Vj X25hbWUpOworCSAgICAgIHByaW50Zl9maWx0ZXJlZCAoXygiJXMgaW4gJXMg b3ZlcmxheSBzZWN0aW9uICVzXG4iKSwKKwkJCSAgICAgICBsb2Nfc3RyaW5n LCBtYXBwZWQsIHNlY19uYW1lKTsKIAkgICAgZWxzZQotCSAgICAgIHByaW50 Zl9maWx0ZXJlZCAoXygiJXMgKyAldSBpbiBzZWN0aW9uICVzXG4iKSwKLQkJ CSAgICAgICBtc3ltX25hbWUsIG9mZnNldCwgc2VjX25hbWUpOworCSAgICAg IHByaW50Zl9maWx0ZXJlZCAoXygiJXMgaW4gc2VjdGlvbiAlc1xuIiksCisJ CQkgICAgICAgbG9jX3N0cmluZywgc2VjX25hbWUpOworCisJZG9fY2xlYW51 cHMgKG9sZF9jaGFpbik7CiAgICAgICB9CiAgIH0KICAgaWYgKG1hdGNoZXMg PT0gMCkK --000e0cd29c70b725a3045c1a5c2a--