From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7205 invoked by alias); 15 Jun 2010 21:54:28 -0000 Received: (qmail 7192 invoked by uid 22791); 15 Jun 2010 21:54:26 -0000 X-SWARE-Spam-Status: No, hits=-4.8 required=5.0 tests=AWL,BAYES_40,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; Tue, 15 Jun 2010 21:54:17 +0000 Received: from int-mx04.intmail.prod.int.phx2.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.17]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o5FLs6Ax015745 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 15 Jun 2010 17:54:06 -0400 Received: from host0.dyn.jankratochvil.net (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx04.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o5FLs3EJ016883 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 15 Jun 2010 17:54:05 -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 o5FLs32l031977; Tue, 15 Jun 2010 23:54:03 +0200 Received: (from jkratoch@localhost) by host0.dyn.jankratochvil.net (8.14.4/8.14.4/Submit) id o5FLs2pM031976; Tue, 15 Jun 2010 23:54:02 +0200 Date: Tue, 15 Jun 2010 21:54: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: <20100615215402.GA29723@host0.dyn.jankratochvil.net> References: <20100503200217.GD30386@host0.dyn.jankratochvil.net> <20100517214507.GC25843@host0.dyn.jankratochvil.net> <20100612170137.GA2636@host0.dyn.jankratochvil.net> <201006151608.20224.pedro@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201006151608.20224.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/msg00357.txt.bz2 On Tue, 15 Jun 2010 17:08:19 +0200, Pedro Alves wrote: > Do you think it would be hard to split this into smaller pieces? Yes, I will do - just later. IMO we should probably more talk about its goal than about specifics of the implementation. > I've tried to review this a couple > of times already, but it looks tricky and easy to miss something. I wrote some notes for it before, attached them for illustration. I believe one should not follow them when reviewing the patch - for obvious reasons. > - It should be possible to fix PR 9436 without We can pretend PR 9436 does not exist. 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. > In fact, it's not clear to me yet why is the new interface better. The interface was better (=completely removed) in the original post: [patch 3/3] bpstat_what removal http://sourceware.org/ml/gdb-patches/2010-05/msg00050.html 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 "names and functionality exactly matching" - in the case of elements like `ss_print_no' one can single-key jump on its definition to see `ecs->event_thread->stop_step value 1.', naming the element ecs_event_thread_stop_step_value_1 would be probably worse but I can change it to the longer form if anyone would prefer it that way. 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. */ Moreover I find this patch only as a first step for further simplifications. For instance at least ecs->event_thread->stop_step and stop_print_frame should definitely be changed and also simplified. This is not a part of this patch (and I admit I currently do not a plan to do such next part soon). Nullifying the layer makes it easier for futher simplifications downwards. > To recap, IMO, the current problem with bpstat_main_action is that a few of > its values aren't really independent and mutually exclusive > with the others -- BPSTAT_WHAT_CHECK_SHLIBS and BPSTAT_WHAT_CHECK_JIT. What if a new breakpoint type wants to stop? What if a new breakpoint type does not want to stop? And how they combine if they happend together with other events? While there exists answer for it I do not know offhand. I know offhand with my implementation. I should post a patch introducing new breakpoint types in both variants of the bpstat_what implementation. > How about fixing the PR that way, and adding the new testcase without all > the revamping? That'd be surely a step in the right direction. The revamping was the goal. I only found the PR to have some advocacy for its acceptance but to be honest I would prefer to forget about the PR now at all. > - I feel that even getting rid of the table bpstat_what_main_action > table Yes, it has to be done. > could be done without changing the interface between breakpoint.c > and infrun.c (removing bpstat_main_action), and other way around too. Changing the interface was also the goal - the goal of simplifying GDB. So far I thought the patches should have the goal of making the patched code the best we can (in this case to make it more simple). If we track the goal of a minimal patchset to reach some functionality we can completely drop this patch as its primary goal is no new functionality but just a code simplification (plus simplification of future code extensions). > That is, it feels like we could tackle these changes independently. > Not sure though. Probably it can although I find such patch more difficult than this one. I just mapped the effect of the current code and wrote a new code behaving (hopefully) the same. > Let me know what you think, and if you disagree, I'll try harder at > reviewing this. I believe we should try to find the shared goal of such patch first. Thanks for reply! Jan ------------------------------------------------------------------------------ bs->breakpoint_at == NULL no_effect kc BPSTAT_WHAT_KEEP_CHECKING bp_none no_effect kc BPSTAT_WHAT_KEEP_CHECKING bp_watchpoint && !stop no_effect kc BPSTAT_WHAT_KEEP_CHECKING bp_hardware_watchpoint && !stop no_effect kc BPSTAT_WHAT_KEEP_CHECKING bp_read_watchpoint && !stop no_effect kc BPSTAT_WHAT_KEEP_CHECKING bp_access_watchpoint && !stop no_effect kc BPSTAT_WHAT_KEEP_CHECKING bp_catchpoint && !stop no_effect kc BPSTAT_WHAT_KEEP_CHECKING bp_watchpoint && stop && !print wp_silent ss BPSTAT_WHAT_STOP_SILENT bp_hardware_watchpoint && stop && !print wp_silent ss BPSTAT_WHAT_STOP_SILENT bp_read_watchpoint && stop && !print wp_silent ss BPSTAT_WHAT_STOP_SILENT bp_access_watchpoint && stop && !print wp_silent ss BPSTAT_WHAT_STOP_SILENT bp_breakpoint && stop && !print bp_silent ss BPSTAT_WHAT_STOP_SILENT bp_hardware_breakpoint && stop && !print bp_silent ss BPSTAT_WHAT_STOP_SILENT bp_until && stop && !print bp_silent ss BPSTAT_WHAT_STOP_SILENT bp_finish && stop && !print bp_silent ss BPSTAT_WHAT_STOP_SILENT bp_catchpoint && stop && !print bp_silent ss BPSTAT_WHAT_STOP_SILENT bp_call_dummy bp_silent ss BPSTAT_WHAT_STOP_SILENT STOP_STACK_DUMMY bp_std_terminate bp_silent ss BPSTAT_WHAT_STOP_SILENT STOP_STD_TERMINATE bp_watchpoint && stop && print wp_noisy sn BPSTAT_WHAT_STOP_NOISY bp_hardware_watchpoint && stop && print wp_noisy sn BPSTAT_WHAT_STOP_NOISY bp_read_watchpoint && stop && print wp_noisy sn BPSTAT_WHAT_STOP_NOISY bp_access_watchpoint && stop && print wp_noisy sn BPSTAT_WHAT_STOP_NOISY bp_breakpoint && stop && print bp_noisy sn BPSTAT_WHAT_STOP_NOISY bp_hardware_breakpoint && stop && print bp_noisy sn BPSTAT_WHAT_STOP_NOISY bp_until && stop && print bp_noisy sn BPSTAT_WHAT_STOP_NOISY bp_finish && stop && print bp_noisy sn BPSTAT_WHAT_STOP_NOISY bp_catchpoint && stop && print bp_noisy sn BPSTAT_WHAT_STOP_NOISY bs->breakpoint_at->owner == NULL bp_nostop sgl BPSTAT_WHAT_SINGLE bp_breakpoint && !stop bp_nostop sgl BPSTAT_WHAT_SINGLE bp_hardware_breakpoint && !stop bp_nostop sgl BPSTAT_WHAT_SINGLE bp_until && !stop bp_nostop sgl BPSTAT_WHAT_SINGLE bp_finish && !stop bp_nostop sgl BPSTAT_WHAT_SINGLE bp_step_resume && !stop bp_nostop sgl BPSTAT_WHAT_SINGLE bp_watchpoint_scope bp_nostop sgl BPSTAT_WHAT_SINGLE bp_thread_event bp_nostop sgl BPSTAT_WHAT_SINGLE bp_overlay_event bp_nostop sgl BPSTAT_WHAT_SINGLE bp_longjmp_master bp_nostop sgl BPSTAT_WHAT_SINGLE bp_std_terminate_master bp_nostop sgl BPSTAT_WHAT_SINGLE bp_longjmp long_jump slr BPSTAT_WHAT_SET_LONGJMP_RESUME bp_longjmp_resume long_resume clr BPSTAT_WHAT_CLEAR_LONGJMP_RESUME bp_step_resume && stop step_resume sr BPSTAT_WHAT_STEP_RESUME bp_shlib_event shlib shl BPSTAT_WHAT_CHECK_SHLIBS bp_jit_event jit_event jit BPSTAT_WHAT_CHECK_JIT bp_tracepoint internal_error ("bpstat_what: tracepoint encountered") bp_fast_tracepoint internal_error ("bpstat_what: tracepoint encountered") err: long_jump && clr long_resume && sgl long_resume && slr long_resume && clr #define kc BPSTAT_WHAT_KEEP_CHECKING break; #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 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 sr BPSTAT_WHAT_STEP_RESUME special if (special2) ecs->event_thread->stepping_over_breakpoint = 1; keep_going (ecs); return; else break; #define shl BPSTAT_WHAT_CHECK_SHLIBS special if (stop_on_solib_events || is (STOP_STACK_DUMMY) || is (STOP_STD_TERMINATE)) stop_stepping (ecs); return; else ecs->event_thread->stepping_over_breakpoint = 1; break; #define jit BPSTAT_WHAT_CHECK_JIT special ecs->event_thread->stepping_over_breakpoint = 1; break; BPSTAT_WHAT_LAST #define err BPSTAT_WHAT_STOP_NOISY default = force keep-going | check reasons why to stop? | always stop default = stepping_over_breakpoint no | yes ecs->event_thread->stop_step default=0 | 1 | 0 stop_print_frame default == 1 | 0 | 1 < BPSTAT_WHAT_KEEP_CHECKING: Check reasons why to stop otherwise keep-going. < BPSTAT_WHAT_SINGLE: Check reasons why to stop, otherwise force stepping over breakpoint. < STEPPING_KEEP_GOING: Force stepping over breakpoint, suppressing checking reasons why to stop. < STOP_STEP: Always stop (not printing the source line). < STOP_STEPPING: Stronger than STOP_STEP by printing the source line (stop_step == 0). < BPSTAT_WHAT_STOP_SILENT: Always stop (stop_print_frame == 0). < BPSTAT_WHAT_STOP_NOISY: Stronger than BPSTAT_WHAT_STOP_SILENT by stop_print_frame == 1. ------------------------------------------------------------------------------ STOP_STEPPING stop_stepping (ecs); return; STOP_STEP ecs->event_thread->stop_step = 1; stop_stepping (ecs); return; STEPPING_KEEP_GOING ecs->event_thread->stepping_over_breakpoint = 1; keep_going (ecs); return; #define kc BPSTAT_WHAT_KEEP_CHECKING break; #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 sgl BPSTAT_WHAT_SINGLE ecs->event_thread->stepping_over_breakpoint = 1; break; ------------------------------------------------------------------------------ #define slr BPSTAT_WHAT_SET_LONGJMP_RESUME special STEPPING_KEEP_GOING #define clr BPSTAT_WHAT_CLEAR_LONGJMP_RESUME special STOP_STEP #define sr BPSTAT_WHAT_STEP_RESUME special if (special2) STEPPING_KEEP_GOING else BPSTAT_WHAT_KEEP_CHECKING #define shl BPSTAT_WHAT_CHECK_SHLIBS special if (stop_on_solib_events || is (STOP_STACK_DUMMY) || is (STOP_STD_TERMINATE)) STOP_STEPPING else BPSTAT_WHAT_SINGLE #define jit BPSTAT_WHAT_CHECK_JIT special BPSTAT_WHAT_SINGLE KEEP_CHECKING STOP_SILENT STOP_NOISY SINGLE SET_LONGJMP_RESUME CLEAR_LONGJMP_RESUME STEP_RESUME CHECK_SHLIBS CHECK_JIT