From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16501 invoked by alias); 14 Jan 2009 15:07:19 -0000 Received: (qmail 16182 invoked by uid 22791); 14 Jan 2009 15:07:17 -0000 X-SWARE-Spam-Status: No, hits=-1.4 required=5.0 tests=AWL,BAYES_00,MSGID_FROM_MTA_HEADER,SPF_SOFTFAIL 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; Wed, 14 Jan 2009 15:06:31 +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 n0EF49pk032906 for ; Wed, 14 Jan 2009 15:04:09 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 n0EF45vI4124766 for ; Wed, 14 Jan 2009 16:04:06 +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 n0EF45ri010466 for ; Wed, 14 Jan 2009 16:04:05 +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 n0EF45Db010461; Wed, 14 Jan 2009 16:04:05 +0100 Message-Id: <200901141504.n0EF45Db010461@d12av02.megacenter.de.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Wed, 14 Jan 2009 16:04:05 +0100 Subject: Re: [RFA] dummy frame handling cleanup, plus inferior fun call signal handling improvement To: dje@google.com (Doug Evans) Date: Wed, 14 Jan 2009 15:07:00 -0000 From: "Ulrich Weigand" Cc: gdb-patches@sourceware.org, pedro@codesourcery.com In-Reply-To: <20090107065222.1E8B81C7A92@localhost> from "Doug Evans" at Jan 06, 2009 10:52:22 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: 2009-01/txt/msg00319.txt.bz2 Doug Evans wrote: > Ulrich suggested making the error messages more consistent. > I like it but after having gone through the exercise I have a question: > There are two MI testcases that check the precise wording, do we have to worry > about frontends that check the wording? > Maybe changes to the wording can be defered to a later patch? I don't think frontends are supposed to check the exact wording. (After all, the text may be translated ...) > 2009-01-06 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,dummy_frame_discard): New fns. > (dummy_frame_pop): Rewrite. Verify requested frame is in the > dummy frame stack. Restore program state. > (cleanup_dummy_frames): Rewrite. > (dummy_frame_sniffer): Update. Make static. > * dummy-frame.h (regcache): Delete forward decl. > (inferior_thread_state,dummy_frame): Add forward decls. > (dummy_frame_push): Update prototype. > (dummy_frame_discard): Declare. > * frame.c (frame_pop): dummy_frame_pop now does all the work for > DUMMY_FRAMEs. > * infcall.c (breakpoint_auto_delete_contents): Delete. > (get_function_name,run_inferior_call): New fns. > (call_function_by_hand): Simplify by moving some code to > get_function_name, run_inferior_call. Inferior function call wrapped > in TRY_CATCH so there's less need for cleanups and all exits from > proceed are handled similarily. Detect program exit. > Detect program stopping in a different thread. > Make error messages more consistent. > * inferior.h (inferior_thread_state): Declare (opaque type). > (save_inferior_thread_state,restore_inferior_thread_state, > make_cleanup_restore_inferior_thread_state, > discard_inferior_thread_state, get_inferior_thread_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. > (inferior_thread_state): New struct. > (save_inferior_thread_state,restore_inferior_thread_state, > do_restore_inferior_thread_state_cleanup, > make_cleanup_restore_inferior_thread_state, > discard_inferior_thread_state, > get_inferior_thread_state_regcache): New functions. > (inferior_status): Move stop_signal, stop_pc, registers to > inferior_thread_state. Remove restore_stack_info. > (save_inferior_status): Remove arg restore_stack_info. > All callers updated. Remove saving of state now saved by > save_inferior_thread_state. > (restore_inferior_status): Remove restoration of state now done by > restore_inferior_thread_state. > (discard_inferior_status): Remove freeing of registers, now done by > discard_inferior_thread_state. > > * gdb.base/break.exp: Update expected gdb output. > * gdb.base/sepdebug.exp: Ditto. > * gdb.mi/mi-syn-frame.exp: Ditto. > * gdb.mi/mi2-syn-frame.exp: Ditto. > > * gdb.base/call-signal-resume.exp: New file. > * gdb.base/call-signals.c: New file. > * gdb.base/unwindonsignal.exp: New file. > * gdb.base/unwindonsignal.c: New file. > * gdb.threads/interrupted-hand-call.exp: New file. > * gdb.threads/interrupted-hand-call.c: New file. > * gdb.threads/thread-unwindonsignal.exp: New file. This looks all very good to me, thanks! Just a couple of minor things I noticed: > +static void > +cleanup_dummy_frames (struct target_ops *target, int from_tty) > +{ > + while (dummy_frame_stack != NULL) > + { > + remove_dummy_frame (&dummy_frame_stack); > + } GNU coding style would be to omit the braces around a single statement. > +/* Discard DUMMY and remove it from the dummy frame stack. > + If the frame isn't found, flag an internal error. */ > + > +extern void dummy_frame_discard (struct dummy_frame *dummy); It would seem more in sync with the rest of the interfaces if this routine were to take a dummy_id instead of a struct dummy_frame pointer. That would allow us to keep that struct fully private to the implementation ... Also, it seems that you only ever call that function if the inferior exited while in an inferior call. Does it matter to discard the dummy frame in that case? After all, the program conceptually still would be in the inferior call, if it were running ... Once the program is restarted, the stale dummy frames will be garbage-collected anyway. > + volatile struct gdb_exception e; Does this need to be volatile in this routine? > + const char *name; > + /* For "at ". */ > + char name_buf[50]; GNU coding style frowns on "magic" array sizes like that ... > +struct inferior_thread_state * > +save_inferior_thread_state (void) > +{ > + struct inferior_thread_state *inf_state = XMALLOC (struct inferior_thread_state); > + struct thread_info *tp = inferior_thread (); > + struct inferior *inf = current_inferior (); It seems "inf" is never needed here (and some of the other new routines). Thanks, Ulrich -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE Ulrich.Weigand@de.ibm.com