From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7791 invoked by alias); 4 Dec 2008 15:32:03 -0000 Received: (qmail 7776 invoked by uid 22791); 4 Dec 2008 15:32:02 -0000 X-Spam-Check-By: sourceware.org Received: from mtagate5.de.ibm.com (HELO mtagate5.de.ibm.com) (195.212.29.154) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 04 Dec 2008 15:31:19 +0000 Received: from d12nrmr1607.megacenter.de.ibm.com (d12nrmr1607.megacenter.de.ibm.com [9.149.167.49]) by mtagate5.de.ibm.com (8.13.8/8.13.8) with ESMTP id mB4FVF4g506492 for ; Thu, 4 Dec 2008 15:31:15 GMT Received: from d12av02.megacenter.de.ibm.com (d12av02.megacenter.de.ibm.com [9.149.165.228]) by d12nrmr1607.megacenter.de.ibm.com (8.13.8/8.13.8/NCO v9.1) with ESMTP id mB4FVFuN4218936 for ; Thu, 4 Dec 2008 16:31:15 +0100 Received: from d12av02.megacenter.de.ibm.com (loopback [127.0.0.1]) by d12av02.megacenter.de.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id mB4FVEAX030238 for ; Thu, 4 Dec 2008 16:31:15 +0100 Received: from tuxmaker.boeblingen.de.ibm.com (tuxmaker.boeblingen.de.ibm.com [9.152.85.9]) by d12av02.megacenter.de.ibm.com (8.12.11.20060308/8.12.11) with SMTP id mB4FVEKt030233; Thu, 4 Dec 2008 16:31:14 +0100 Message-Id: <200812041531.mB4FVEKt030233@d12av02.megacenter.de.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Thu, 4 Dec 2008 16:31:14 +0100 Subject: Re: [RFA] dummy frame handling cleanup, plus inferior fun call signal handling improvement To: dje@google.com (Doug Evans) Date: Thu, 04 Dec 2008 15:32:00 -0000 From: "Ulrich Weigand" Cc: pedro@codesourcery.com (Pedro Alves), gdb-patches@sourceware.org In-Reply-To: from "Doug Evans" at Dec 02, 2008 10:03:10 PM MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit 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-12/txt/msg00067.txt.bz2 Doug Evans wrote: > /* Everything's ready, push all the info needed to restore the > caller (and identify the dummy-frame) onto the dummy-frame > stack. */ >- dummy_frame_push (caller_regcache, &dummy_id); >- discard_cleanups (caller_regcache_cleanup); >+ dummy_frame = dummy_frame_push (caller_state, &dummy_id); >+ /* Do this before calling make_cleanup_pop_dummy_frame. */ >+ discard_cleanups (caller_state_cleanup); >+ dummy_frame_cleanup = make_cleanup_pop_dummy_frame (dummy_frame); This will cause any random error during proceed to pop the dummy frame -- but the frame could still be on the call chain, couldn't it? I think we should only (explicitly) pop the dummy frame either after successful completion of the call, or when we do an explicit unwind. >+ if (! ptid_equal (this_thread_ptid, inferior_thread ()->ptid)) >+ { >+ /* We've switched threads. This can happen if another thread gets a >+ signal or breakpoint while our thread was running. >+ There's no point in restoring the inferior status, >+ we're in a different thread. */ >+ discard_cleanups (inf_status_cleanup); >+ discard_inferior_status (inf_status); >+ /* Keep the dummy frame record, if the user switches back to the >+ thread with the hand-call, we'll need it. */ >+ error (_("\ >+The current thread has changed while making a function call from GDB.\n\ >+The state of the function call has been lost.\n\ >+It may be recoverable by changing back to the original thread\n\ >+and examining the state.")); >+ } The various error messages in this function all use different wording to express the same fact: - Evaluation of the expression containing the function (%s) will be abandoned. [signal case] - When the function (%s) is done executing, GDB will silently stop (instead of continuing to evaluate the expression containing the function call). [breakpoint case] - The state of the function call has been lost. It may be recoverable by changing back to the original thread and examining the state. [your new case] It would be preferable to use the same wording. Maybe Eli has some thoughts here ... Also, to provide additional information, it might be nice to distinguish between the cases: The program being debugged was signaled in another thread ... and The program being debugged stopped in another thread ... >+/* Two structures are used to record inferior state. > >+ inferior_program_state contains state about the program itself like its >+ registers and any signal it received when it last stopped. >+ This state must be restored regardless of how the inferior function call >+ ends (either successfully, or after it hits a breakpoint or signal) >+ if the program is to properly continue where it left off. >+ >+ inferior_status contains state regarding gdb's control of the inferior >+ itself like stepping control. It also contains session state like the >+ user's currently selected frame. >+ This state is only restored upon successful completion of the >+ inferior function call. Hmmm, that's not quite what what the code actually does. The state is also restored if some error is thrown during the proceed call, for example. I'm not sure whether this is really the right thing to do ... > if (stop_stack_dummy) > { >- /* Pop the empty frame that contains the stack dummy. POP_FRAME >- ends with a setting of the current frame, so we can use that >- next. */ >- frame_pop (get_current_frame ()); >- /* Set stop_pc to what it was before we called the function. >- Can't rely on restore_inferior_status because that only gets >- called if we don't stop in the called function. */ >- stop_pc = read_pc (); >- select_frame (get_current_frame ()); >+ /* Pop the empty frame that contains the stack dummy. >+ This also restores all inferior state prior to the call. >+ If the current frame is not a stack dummy, do nothing and warn >+ the user. */ >+ struct frame_info *frame = get_current_frame (); >+ if (get_frame_type (frame) == DUMMY_FRAME) >+ { >+ dummy_frame_pop (get_frame_id (frame)); >+ } >+ else >+ { >+ /* We avoid calling the frame a dummy frame as it has little >+ meaning to the user. */ >+ printf_filtered (_("\ >+Stopped after an inferior function call, but not in the expected stack frame.\n\ >+Proceed with caution.\n")); >+ } I don't quite see how this can ever happen; stop_stack_dummy should be true only for a bp_call_dummy breakpoint, which is only recognized if the current frame ID matches the dummy frame ID. So for the above check to trigger would require that we're in a frame with dummy frame ID but not a dummy frame ... >+struct inferior_program_state > { > enum target_signal stop_signal; > CORE_ADDR stop_pc; >+ struct regcache *registers; >+}; Isn't stop_pc redundant? We should be able just set stop_pc to regcache_read_pc (registers) after restoring the registers, just like the code above did ... > struct inferior_status * >-save_inferior_status (int restore_stack_info) >+save_inferior_status () (void), please. Thanks, Ulrich -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE Ulrich.Weigand@de.ibm.com