From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21735 invoked by alias); 7 Dec 2010 05:02:14 -0000 Received: (qmail 21724 invoked by uid 22791); 7 Dec 2010 05:02:11 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 07 Dec 2010 05:02:06 +0000 Received: (qmail 28523 invoked from network); 7 Dec 2010 05:02:04 -0000 Received: from unknown (HELO ?192.168.0.102?) (yao@127.0.0.2) by mail.codesourcery.com with ESMTPA; 7 Dec 2010 05:02:04 -0000 Message-ID: <4CFDBFC3.5000803@codesourcery.com> Date: Tue, 07 Dec 2010 05:02:00 -0000 From: Yao Qi User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.15) Gecko/20101027 Thunderbird/3.0.10 MIME-Version: 1.0 To: Jan Kratochvil CC: gdb-patches@sourceware.org Subject: Re: [patch 4/4] hw watchpoints made multi-inferior References: <20101206111443.GE27176@host0.dyn.jankratochvil.net> In-Reply-To: <20101206111443.GE27176@host0.dyn.jankratochvil.net> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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-12/txt/msg00067.txt.bz2 On 12/06/2010 07:14 PM, Jan Kratochvil wrote: Here are some my cents, although I can't approve or reject this patch. :) > gdb/ > 2010-12-06 Jan Kratochvil > > Fix watchpoints for multi-inferior. > * breakpoint.c > (update_watchpoint) pspace != current_program_space>: New. This changelog entry looks strange. You can describe what is this change here, like, * breakpoint.c (update_watchpoint): Return directly if breakpoint's pspace is not current_program_space. > * linux-nat.c (linux_nat_iterate_watchpoint_lwps): Fix > iterate_over_lwps FILTER. What is FILTER? Why capitalized? > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -1302,6 +1302,9 @@ update_watchpoint (struct breakpoint *b, int reparse) > if (!watchpoint_in_thread_scope (b)) > return; > > + if (b->pspace != current_program_space) > + return; > + > /* We don't free locations. They are stored in bp_location array and > update_global_locations will eventually delete them and remove > breakpoints if needed. */ > @@ -1879,6 +1882,7 @@ insert_breakpoint_locations (void) > int val = 0; > int disabled_breaks = 0; > int hw_breakpoint_error = 0; > + struct program_space *saved_current_program_space = current_program_space; > > struct ui_file *tmp_error_stream = mem_fileopen (); > struct cleanup *cleanups = make_cleanup_ui_file_delete (tmp_error_stream); > @@ -1908,7 +1912,8 @@ insert_breakpoint_locations (void) > if we aren't attached to any process yet, we should still > insert breakpoints. */ > if (!gdbarch_has_global_breakpoints (target_gdbarch) > - && ptid_equal (inferior_ptid, null_ptid)) > + && (ptid_equal (inferior_ptid, null_ptid) > + || b->pspace != saved_current_program_space)) > continue; Please add some comments here for this change. It is not that obvious to understand without multi-inferior context. > val = insert_bp_location (b, tmp_error_stream, > @@ -8265,6 +8270,7 @@ watch_command_1 (char *arg, int accessflag, int from_tty, > b = set_raw_breakpoint_without_location (NULL, bp_type); > set_breakpoint_number (internal, b); > b->thread = thread; > + b->pspace = current_program_space; > b->disposition = disp_donttouch; > b->exp = exp; > b->exp_valid_block = exp_valid_block; > @@ -9404,6 +9410,9 @@ update_global_location_list (int should_insert) > int keep_in_target = 0; > int removed = 0; > > + if (old_loc->pspace != current_program_space) > + continue; code indent problem? Put extra three spaces in front of "continue". > +static void > +i386_inferior_data_cleanup (struct inferior *inf, void *arg) > +{ > + struct i386_inferior_data *inf_data = arg; > + > + xfree (inf_data); > +} > + Can't we 'xfree (arg);' directly? > @@ -1287,7 +1287,7 @@ linux_nat_iterate_watchpoint_lwps > if (inf->pid == inferior_pid) > { > /* Standard mode. */ > - iterate_over_lwps (minus_one_ptid, > + iterate_over_lwps (pid_to_ptid (inferior_pid), > iterate_watchpoint_lwps_callback, &data); Please add some comments to explain this change "minus_one_ptid -> pid_to_ptid (inferior_pid)". This change is mentioned in changelog as "Fix iterate_over_lwps FILTER", but it is still no enough, IMO. > + > +gdb_test "add-inferior" "Added inferior 2" > +gdb_test "inferior 2" "witching to inferior 2 .*" > +gdb_load $binfile > + > +if ![runto_main] { Please add `perror "Couldn't run ${testfile}"' > + return > +} -- Yao (齐尧)