From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2881 invoked by alias); 30 May 2009 21:08:37 -0000 Received: (qmail 2872 invoked by uid 22791); 30 May 2009 21:08:36 -0000 X-SWARE-Spam-Status: No, hits=-1.1 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,SARE_MSGID_LONG40,SPF_PASS 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.43rc1) with ESMTP; Sat, 30 May 2009 21:08:27 +0000 Received: from spaceape8.eur.corp.google.com (spaceape8.eur.corp.google.com [172.28.16.142]) by smtp-out.google.com with ESMTP id n4UL8O7R029834 for ; Sat, 30 May 2009 14:08:25 -0700 Received: from yx-out-2324.google.com (yxb8.prod.google.com [10.190.1.72]) by spaceape8.eur.corp.google.com with ESMTP id n4UL8Mj9024811 for ; Sat, 30 May 2009 14:08:23 -0700 Received: by yx-out-2324.google.com with SMTP id 8so4029018yxb.47 for ; Sat, 30 May 2009 14:08:21 -0700 (PDT) MIME-Version: 1.0 Received: by 10.90.70.15 with SMTP id s15mr3499154aga.1.1243717701797; Sat, 30 May 2009 14:08:21 -0700 (PDT) In-Reply-To: References: Date: Sat, 30 May 2009 21:08:00 -0000 Message-ID: Subject: Re: RFC: next/finish/etc -vs- exceptions From: Doug Evans To: Tom Tromey Cc: gdb-patches@sourceware.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-System-Of-Record: true 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: 2009-05/txt/msg00654.txt.bz2 Hi. My limited knowledge didn't find anything seriously broken 1/2 :-), but I do have a few comments. On Fri, May 29, 2009 at 3:32 PM, Tom Tromey wrote: > With the current gdb, a "next" over a call that throws an exception > can cause gdb to act as though the user typed "continue". =A0This can be > very frustrating when debugging C++ (or Java) code. =A0This also happens > with other commands, particularly "finish", "until", and "advance". > > This patch fixes this problem. =A0It relies on a new debugging hook in > GCC's unwinder, so it will only work for GCC 4.5 and later. I wouldn't block on this, but one aspect of good u/i is setting expectation= s. If something is known to not work, telling the user up front is usually better than staying silent and leaving it to the user to figure out. To that end, should gdb print a warning if _Unwind_DebugHook isn't found? Your patch isn't alone in this, and I'm not sure when and how to print such warnings. It just seems to me to be a good idea to have such warnings. [To minimize the verbosity, gdb could print a general warning when it detects such conditions, with instructions on how to get further details. E.g. "GDB has detected conditions that will hinder some debugging efforts. Use `mumble' for further information." or some such. That way the user might just get one warning at startup, and that's it. And we could expand on the conditions to detect without affecting the basic session flow.] The user might get used to the message, start ignoring it, etc. etc. Maybe it could be off by default. The high order bit though is that there would be standard things the user can do to figure out why (e.g. in this case) "next" isn't working the way s/he is expecting it to (for typically frequent cases that are reasonable to handle - stripped libpthread anyone?). Just a thought. > @@ -6356,6 +6403,7 @@ struct until_break_command_continuation_args > =A0{ > =A0 struct breakpoint *breakpoint; > =A0 struct breakpoint *breakpoint2; > + =A0int thread_num; > =A0}; > > =A0/* This function is called by fetch_inferior_event via the Having to record the thread here seems orthogonal to handling next-ing over exceptions (e.g. why isn't it also required for longjmp?). I wouldn't necessarily put it in a separate patch though (regardless of whether it is or isn't orthogonal). Maybe add further comments explaining what's going on here? > @@ -3851,6 +3876,112 @@ insert_longjmp_resume_breakpoint (CORE_ADDR pc) > =A0 =A0 set_momentary_breakpoint_at_pc (pc, bp_longjmp_resume); > =A0} > > +/* Insert an exception resume breakpoint. =A0TP is the thread throwing > + =A0 the exception. =A0The block B is the block of the unwinder debug ho= ok > + =A0 function. =A0FRAME is the frame corresponding to the call to this > + =A0 function. =A0SYM is the symbol of the function argument holding the > + =A0 target PC of the exception. =A0*/ > + > +static void > +insert_exception_resume_breakpoint (struct thread_info *tp, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 str= uct block *b, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 str= uct frame_info *frame, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 str= uct symbol *sym) > +{ > + =A0struct gdb_exception e; > + > + =A0/* We want to ignore errors here. =A0*/ > + =A0TRY_CATCH (e, RETURN_MASK_ALL) > + =A0 =A0{ > + =A0 =A0 =A0struct symbol *vsym; > + =A0 =A0 =A0struct value *value; > + =A0 =A0 =A0CORE_ADDR handler; > + =A0 =A0 =A0struct breakpoint *bp; > + > + =A0 =A0 =A0vsym =3D lookup_symbol (SYMBOL_LINKAGE_NAME (sym), b, VAR_DO= MAIN, NULL); > + =A0 =A0 =A0value =3D read_var_value (vsym, frame); > + =A0 =A0 =A0handler =3D value_as_address (value); > + > + =A0 =A0 =A0/* We're going to replace the current step-resume breakpoint > + =A0 =A0 =A0 =A0with a longjmp-resume breakpoint. =A0*/ s/longjmp/exception/ ? > + =A0 =A0 =A0delete_step_resume_breakpoint (tp); > + > + =A0 =A0 =A0if (debug_infrun) > + =A0 =A0 =A0 fprintf_unfiltered (gdb_stdlog, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 "infrun: exception = resume at %lx\n", > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (unsigned long) han= dler); > + > + =A0 =A0 =A0bp =3D set_momentary_breakpoint_at_pc (handler, bp_exception= _resume); > + =A0 =A0 =A0inferior_thread ()->step_resume_breakpoint =3D bp; > + =A0 =A0} > +} > + > +/* This is called when an exception has been intercepted. =A0Check to > + =A0 see whether the exception's destination is of interest, and if so, > + =A0 set an exception resume breakpoint there. =A0*/ > + > +static void > +check_exception_resume (struct execution_control_state *ecs, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct frame_info *frame, s= truct symbol *func) > +{ > + =A0struct gdb_exception e; > + > + =A0TRY_CATCH (e, RETURN_MASK_ALL) > + =A0 =A0{ > + =A0 =A0 =A0struct block *b; > + =A0 =A0 =A0struct dict_iterator iter; > + =A0 =A0 =A0struct symbol *sym; > + =A0 =A0 =A0int argno =3D 0; > + > + =A0 =A0 =A0b =3D SYMBOL_BLOCK_VALUE (func); > + =A0 =A0 =A0ALL_BLOCK_SYMBOLS (b, iter, sym) > + =A0 =A0 =A0 { > + =A0 =A0 =A0 =A0 if (!SYMBOL_IS_ARGUMENT (sym)) > + =A0 =A0 =A0 =A0 =A0 continue; It's a bit odd to check function arguments for something like this. But maybe I just don't understand the implementation enough. There's an implementation aspect here that's not clear to me. Can you add comments elaborating on the argno =3D=3D 0 vs argno > 0 cases? Can we assume that function arguments will be seen in program order here? [or am I misinterpreting argno =3D=3D 0 vs !=3D 0 ?] > + > + =A0 =A0 =A0 =A0 if (argno =3D=3D 0) > + =A0 =A0 =A0 =A0 =A0 { > + =A0 =A0 =A0 =A0 =A0 =A0 struct symbol *vsym; > + =A0 =A0 =A0 =A0 =A0 =A0 struct value *value; > + =A0 =A0 =A0 =A0 =A0 =A0 CORE_ADDR cfa, nexting_cfa; > + =A0 =A0 =A0 =A0 =A0 =A0 struct gdbarch *arch; > + > + =A0 =A0 =A0 =A0 =A0 =A0 vsym =3D lookup_symbol (SYMBOL_LINKAGE_NAME (sy= m), > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 b, = VAR_DOMAIN, NULL); > + =A0 =A0 =A0 =A0 =A0 =A0 value =3D read_var_value (vsym, frame); > + =A0 =A0 =A0 =A0 =A0 =A0 cfa =3D value_as_address (value); > + > + =A0 =A0 =A0 =A0 =A0 =A0 arch =3D get_frame_arch (frame); > + =A0 =A0 =A0 =A0 =A0 =A0 nexting_cfa =3D ecs->event_thread->exception_fr= ame; > + =A0 =A0 =A0 =A0 =A0 =A0 if (debug_infrun) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 fprintf_unfiltered (gdb_stdlog, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 "in= frun: comparing exception target %lx with next-frame %lx: %d\n", > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (un= signed long) cfa, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (un= signed long) nexting_cfa, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 !ne= xting_cfa || gdbarch_inner_than (arch, cfa, nexting_cfa)); > + > + =A0 =A0 =A0 =A0 =A0 =A0 if (!nexting_cfa > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 || gdbarch_inner_than (arch, cfa, nexti= ng_cfa)) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 { > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Not an interesting exception. =A0*/ > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 break; > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 } > + =A0 =A0 =A0 =A0 =A0 =A0 ++argno; > + =A0 =A0 =A0 =A0 =A0 } > + =A0 =A0 =A0 =A0 else > + =A0 =A0 =A0 =A0 =A0 { > + =A0 =A0 =A0 =A0 =A0 =A0 /* If we got here, then we want to set the exce= ption > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0resume breakpoint at the address held in= the second > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0argument to the function. =A0*/ > + > + =A0 =A0 =A0 =A0 =A0 =A0 insert_exception_resume_breakpoint (ecs->event_= thread, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0 =A0 =A0 =A0 =A0 b, frame, sym); > + =A0 =A0 =A0 =A0 =A0 =A0 break; > + =A0 =A0 =A0 =A0 =A0 } > + =A0 =A0 =A0 } > + =A0 =A0} > +} > + > =A0static void > =A0stop_stepping (struct execution_control_state *ecs) > =A0{ > [...] > diff --git a/gdb/testsuite/gdb.cp/Makefile.in b/gdb/testsuite/gdb.cp/Make= file.in > index f4a989c..1a2b908 100644 > --- a/gdb/testsuite/gdb.cp/Makefile.in > +++ b/gdb/testsuite/gdb.cp/Makefile.in > @@ -4,7 +4,7 @@ srcdir =3D @srcdir@ > =A0EXECUTABLES =3D ambiguous annota2 anon-union cplusfuncs cttiadd \ > =A0 =A0 =A0 =A0derivation inherit local member-ptr method misc \ > =A0 =A0 =A0 =A0 overload ovldbreak ref-typ ref-typ2 templates userdef vir= tfunc namespace \ > - =A0 =A0 =A0 ref-types ref-params method2 > + =A0 =A0 =A0 ref-types ref-params method2 gdb9593 > > =A0all info install-info dvi install uninstall installcheck check: > =A0 =A0 =A0 =A0@echo "Nothing to be done for $@..." Blech. I haven't been paying attention to this myself. It's really nice to be able to add new testcases *without* having to edit any files like Makefile.in.