From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31176 invoked by alias); 18 Jun 2010 10:41:58 -0000 Received: (qmail 31159 invoked by uid 22791); 18 Jun 2010 10:41:57 -0000 X-SWARE-Spam-Status: No, hits=-4.4 required=5.0 tests=AWL,BAYES_50,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_GJ,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 18 Jun 2010 10:41:50 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o5IAfWXn001441 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 18 Jun 2010 06:41:33 -0400 Received: from host0.dyn.jankratochvil.net (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o5IAfUuI020132 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 18 Jun 2010 06:41:32 -0400 Received: from host0.dyn.jankratochvil.net (localhost [127.0.0.1]) by host0.dyn.jankratochvil.net (8.14.4/8.14.4) with ESMTP id o5IAfU0V014563; Fri, 18 Jun 2010 12:41:30 +0200 Received: (from jkratoch@localhost) by host0.dyn.jankratochvil.net (8.14.4/8.14.4/Submit) id o5IAfTkl014562; Fri, 18 Jun 2010 12:41:29 +0200 Date: Fri, 18 Jun 2010 10:41:00 -0000 From: Jan Kratochvil To: Pedro Alves Cc: gdb-patches@sourceware.org, Stan Shebs Subject: Re: [patch 3/3] bpstat_what removal [rediff] Message-ID: <20100618104129.GA11850@host0.dyn.jankratochvil.net> References: <20100503200217.GD30386@host0.dyn.jankratochvil.net> <201006151608.20224.pedro@codesourcery.com> <20100615215402.GA29723@host0.dyn.jankratochvil.net> <201006162013.31064.pedro@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201006162013.31064.pedro@codesourcery.com> User-Agent: Mutt/1.5.20 (2009-12-10) 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: 2010-06/txt/msg00403.txt.bz2 On Wed, 16 Jun 2010 21:13:30 +0200, Pedro Alves wrote: > On Tuesday 15 June 2010 22:54:02, Jan Kratochvil wrote: > > I noted a wish of removing bpstat_what > > already on #gdb 2008-12-31T22:51:10Z I believe not aware of this PR that time. > > I believe the wish lasts longer than I have expressed it on #gdb. > > The comment mentioning that the table is not really needed is there > since 5.0. That's like 10 years ago. :-) My plan was to remove the whole needless bpstat_what interface but that has been rejected after my first post. Anyway it is offtopic now. > > Currently the interface was created with the goal of not introducing anything > > new. All its element have both their names and functionality exactly matching > > the already existing (and currently required to stay in place) GDB data > > structures. They had to copy them to comply with the separation requirements: > > Re: [patch 3/3] bpstat_what removal > > http://sourceware.org/ml/gdb-patches/2010-05/msg00186.html > > I don't see how that relates, sorry. I do not understand offhand what does exactly mean BPSTAT_WHAT_SINGLE. While in my proposed interface I find `this.stepping_over_breakpoint = 1;' is much easier to understand it will probably set `ecs->event_thread->stepping_over_breakpoint = 1;'. But I understand a code readability improvement can be postponed into different future patches. > > Contrary to it what exactly does BPSTAT_WHAT_STEP_RESUME and how to combine it > > with other events is unclear to me. > > /* Clear step resume breakpoint, and keep checking. */ > > Yes, there's an issue with step-resume's priority over other events. > step-resume takes priority over all other actions, even stops for other > breakpoints. It usually just works (breakpoint on top of step-resume > breakpoint, for instance), because when you hit the step-resume, the step > range _also_ ends, so we stop anyway. I can see that the (infrun.c) > optimization around: > > /* We are at the start of a different line. So stop. Note that > we don't stop if we step into the middle of a different line. > That is said to make things like for (;;) statements work > better. */ > > can make us miss a breakpoint in some rare cases. Acknowledging this > doesn't mean we can't tackle one issue at a time. ==> 1.c <== extern int f (void); int main (void) { int i; i = f (); return 0; } ==> 2.c <== int f (void) { return 1; } gcc -Wall -c -o 2.o 2.c; gcc -o 1 1.c 2.o -Wall -g # 2.o has no DWARF! gdb -nx -ex 'set breakpoint always-inserted on' -ex 'b *0x400481' -ex start -ex 'set debug infrun 1' -ex step -ex disass -ex 'p/x $pc' 1 8 i = f (); 0x000000000040047c <+8>: callq 0x40048c 0x0000000000400481 <+13>: mov %eax,-0x4(%rbp) 9 return 0; 0x0000000000400484 <+16>: mov $0x0,%eax infrun: prepare_to_wait infrun: target_wait (-1, status) = infrun: 1622 [process 1622], infrun: status->kind = stopped, signal = SIGTRAP infrun: infwait_normal_state infrun: TARGET_WAITKIND_STOPPED infrun: stop_pc = 0x400481 infrun: BPSTAT_WHAT_STEP_RESUME infrun: stepping inside range [0x40047c-0x400484] infrun: resume (step=1, signal=0), trap_expected=0 infrun: prepare_to_wait infrun: target_wait (-1, status) = infrun: 1622 [process 1622], infrun: status->kind = stopped, signal = SIGTRAP infrun: infwait_normal_state infrun: TARGET_WAITKIND_STOPPED infrun: stop_pc = 0x400481 infrun: BPSTAT_WHAT_STOP_NOISY infrun: stop_stepping That is it would be more correct to stop using BPSTAT_WHAT_STOP_NOISY immediately without the last resume() call. If we stop at 0x400481 and a breakpoint is placed there we can stop and print it. OTOH if the breakpoint is there left placed in the inferior GDB will stop again on it anyway so it has no user visible effect. My patch does: infrun: prepare_to_wait infrun: target_wait (-1, status) = infrun: 4037 [process 4037], infrun: status->kind = stopped, signal = SIGTRAP infrun: infwait_normal_state infrun: TARGET_WAITKIND_STOPPED infrun: stop_pc = 0x400481 infrun: bp_breakpoint: print_frame pf_yes infrun: bp_breakpoint: perform pe_stop infrun: bp_step_resume: perform pe_check_more infrun: bp_step_resume: bp_step_resume_on_stop yes infrun: summary: print_frame pf_yes infrun: summary: stop_step ss_default (ss_print_yes (stop_step 0)) infrun: summary: perform pe_stop infrun: summary: stepping_over_breakpoint no infrun: summary: bp_longjmp no infrun: summary: bp_longjmp_resume no infrun: summary: bp_step_resume_on_stop yes infrun: stop_stepping And therefore it does not invoke additional needless inferior resume there. I was trying yesterday to find a user-visible reproducer (not just a `set debug' output defect) but I have not found one soon enough and I have to move on other work so just posting this mail. I still believe in some cases it may be user visible or will be with the planned new gnu-ifunc, python and other breakpoint types. > > What if a new breakpoint type does not want to stop? > > BPSTAT_WHAT_SINGLE I do not find this easy to find out. > > And how they combine if they happend together with > > other events? > > The highest priority of the above wins. You describe your patch here. Based on it I made an ordered list: #define kc BPSTAT_WHAT_KEEP_CHECKING break; #define sgl BPSTAT_WHAT_SINGLE ecs->event_thread->stepping_over_breakpoint = 1; break; #define slr BPSTAT_WHAT_SET_LONGJMP_RESUME special ecs->event_thread->stepping_over_breakpoint = 1; keep_going (ecs); return; #define clr BPSTAT_WHAT_CLEAR_LONGJMP_RESUME special ecs->event_thread->stop_step = 1; stop_stepping (ecs); return; #define ss BPSTAT_WHAT_STOP_SILENT stop_print_frame = 0; stop_stepping (ecs); return; #define sn BPSTAT_WHAT_STOP_NOISY stop_print_frame = 1; stop_stepping (ecs); return; #define sr BPSTAT_WHAT_STEP_RESUME special if (special2) ecs->event_thread->stepping_over_breakpoint = 1; keep_going (ecs); return; else break; We do not want "highest priority of the above wins" but we want "higher priority is a superset of actions of any lower priority". This has been achieved in my patch according to the `set debug'-class reproducer above. > If you're going to stop, you'll never want to set longjmp or step resume > breakpoints, or start a step over. You're just going to stop, period. But former BPSTAT_WHAT_STEP_RESUME and BPSTAT_WHAT_SET_LONGJMP_RESUME (therefore those using `keep_going (ecs); return;' make a mess there as they cancel lower priorities wanting to stop. > (My feeling is that we should completely move the infrun-specific > breakpoints handling to infrun (longjmp, longjmp-resume, step-resume), > though it isn't clear _how_ is best. I hope that by eliminating > the table first, we can think of better interfaces afterwards. > We may even end up with something like yours, I'm not rejecting > it upfront, to be clear.) In general when I forget about my patch I find your solution definitely as an improvement over current FSF GDB HEAD state. I still would like to review it more, just hand waving it is correct in this mail. > > > - I feel that even getting rid of the table bpstat_what_main_action > > > table > > > > Yes, it has to be done. > > It doesn't _have_ to. There's nothing wrong with it, if you look at > it as a state machine (and once you remove the states that should never > have been put there in the first place). To make it correct any superset of actions of any element of the enum set would have to be represented by an element of that enum set. The enum set actions would have to be closed for the operation of unification. > I don't find it hard to read, but you do > have to think a bit about it to understand it the first time; Yes, I am mostly used to BPSTAT_WHAT_* now after spending many days with it. This only proves it must be removed one day to get on par with the contribution rate of Linux kernel. > Well, I agree with all that, but it just doesn't look simpler > to me at first look. :-) That's my main worry. I believe nullifying this layer will make it easier to simplify other parts of the code this code controls. That is to simplify code currently controlled by `ecs->event_thread->stop_step', `stop_print_frame', ecs->event_thread->stepping_over_breakpoint' (this one is the only one possibly clear enough) and others. But I have not even tried to simplify this part so I may have false predictions on the goals of my bpstat_what rework. Primarily this future simplification goal was heavily hit by the re-introduction of bpstat_what interface as requested after my first post. > > Probably it can although I find such patch more difficult than this one. > > I'll prove you wrong. :-) Your patch has a regression against my patch therefore I do not find it proven. I was going to try some BPSTAT_WHAT_* patch while keeping it regression free but when thinking about it now it probably would not be possible anyway. Your patch probably (I would have to verify this part first) does not have a regression against FSF GDB HEAD and therefore it is a good step. > You'll notice that this bpstat_what version is even a bit more simplified > than yours, because it centralizes BPSTAT_WHAT_SINGLE handling (it's really > a property of whether there's a breakpoint at PC when we keep going, This violates the goal of my patch to make its reviewing easier by not changing the behavior in any way for the cases only a single event happens. I would prefer to remove it from this your patch. > so it could even be eliminated if we're okay with adding a > breakpoint_inserted_here call somewhere else; if it stays, it's really > a property of the breakpoint location, not the breakpoint, it seems.), This seems as a nice simplifications but it is outside of (my intended) scope of this patch. Thanks, Jan