From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12875 invoked by alias); 25 Jan 2012 20:08:20 -0000 Received: (qmail 12865 invoked by uid 22791); 25 Jan 2012 20:08:17 -0000 X-SWARE-Spam-Status: No, hits=-1.7 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 25 Jan 2012 20:07:58 +0000 Received: from svr-orw-fem-01.mgc.mentorg.com ([147.34.98.93]) by relay1.mentorg.com with esmtp id 1Rq98P-0000pt-4k from Luis_Gustavo@mentor.com for gdb-patches@sourceware.org; Wed, 25 Jan 2012 12:07:57 -0800 Received: from NA1-MAIL.mgc.mentorg.com ([147.34.98.181]) by svr-orw-fem-01.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Wed, 25 Jan 2012 12:07:57 -0800 Received: from [0.0.0.0] ([172.16.63.104]) by NA1-MAIL.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Wed, 25 Jan 2012 12:07:56 -0800 Message-ID: <4F20610B.5010403@mentor.com> Date: Wed, 25 Jan 2012 21:27:00 -0000 From: Luis Gustavo Reply-To: "Gustavo, Luis" User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.23) Gecko/20110922 Lightning/1.0b2 Thunderbird/3.1.15 MIME-Version: 1.0 To: gdb-patches@sourceware.org Subject: [PATCH] Fix breakpoint updates for multi-inferior Content-Type: multipart/mixed; boundary="------------040104020001050505060007" 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: 2012-01/txt/msg00876.txt.bz2 This is a multi-part message in MIME format. --------------040104020001050505060007 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-length: 1377 Hi, While working on the target-side condition evaluation patch, i stumbled upon the strange situation where GDB had more than a single "inserted" location in a list with multiple duplicate locations. Further investigation showed that the logic for finding the first inserted location at a specific address does not work when multiple inferiors are being debugged. This code is inside update_global_location_list (...). This is partly because we expect the list of locations to be sorted by breakpoint numbers and addresses. Suppose we're going through locations at address FOO, and we already defined the "first" for that set of locations. When a location does not match the "first" location of that address, we then assume we've gone past the locations at address FOO. This is correct for single inferiors. Now, consider a multi-inferior scenario and breakpoints with locations on multiple inferiors. The code will fail to match two locations due to the difference between the locations' program spaces, thus failing to mark duplicate locations correctly. This patch solves this by updating the locations one program space at a time, thus preventing multiple insertions of the same location. This bug shows up when doing multi-inferior debugging in GDBServer. You will notice GDB sending multiple insert/remove requests for the same address. OK? Luis --------------040104020001050505060007 Content-Type: text/x-patch; name="0001-fix_multiprocess_breaks.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0001-fix_multiprocess_breaks.diff" Content-length: 6984 2012-01-25 Luis Machado * breakpoint.c (update_global_location_list): Handle *point updates one program space at a time. * progspace.h (ALL_PROGRAM_SPACES): New helper macro. Index: gdb/gdb/breakpoint.c =================================================================== --- gdb.orig/gdb/breakpoint.c 2012-01-25 10:19:35.453821938 -0200 +++ gdb/gdb/breakpoint.c 2012-01-25 11:11:17.721821938 -0200 @@ -10379,6 +10379,7 @@ update_global_location_list (int should_ built bp_location from the current state of ALL_BREAKPOINTS. */ struct bp_location **old_location, **old_locp; unsigned old_location_count; + struct program_space *cur_pspace; old_location = bp_location; old_location_count = bp_location_count; @@ -10586,72 +10587,88 @@ update_global_location_list (int should_ } } - /* Rescan breakpoints at the same address and section, marking the - first one as "first" and any others as "duplicates". This is so - that the bpt instruction is only inserted once. If we have a - permanent breakpoint at the same place as BPT, make that one the - official one, and the rest as duplicates. Permanent breakpoints - are sorted first for the same address. - - Do the same for hardware watchpoints, but also considering the - watchpoint's type (regular/access/read) and length. */ - - bp_loc_first = NULL; - wp_loc_first = NULL; - awp_loc_first = NULL; - rwp_loc_first = NULL; - ALL_BP_LOCATIONS (loc, locp) + /* For each program space, go through the list of *points for that specific + program space. This is needed due to assumption (by the code below) that + two locations don't match only if they don't have the same address/type, + since the location list is sorted first by addresses/breakpoint numbers. + The locations also don't match when their program spaces are different + (multiprocess). */ + + ALL_PROGRAM_SPACES (cur_pspace) { - /* ALL_BP_LOCATIONS bp_location has LOC->OWNER always - non-NULL. */ - struct bp_location **loc_first_p; - b = loc->owner; - - if (!should_be_inserted (loc) - || !breakpoint_address_is_meaningful (b) - /* Don't detect duplicate for tracepoint locations because they are - never duplicated. See the comments in field `duplicate' of - `struct bp_location'. */ - || is_tracepoint (b)) - continue; - - /* Permanent breakpoint should always be inserted. */ - if (b->enable_state == bp_permanent && ! loc->inserted) - internal_error (__FILE__, __LINE__, - _("allegedly permanent breakpoint is not " - "actually inserted")); - - if (b->type == bp_hardware_watchpoint) - loc_first_p = &wp_loc_first; - else if (b->type == bp_read_watchpoint) - loc_first_p = &rwp_loc_first; - else if (b->type == bp_access_watchpoint) - loc_first_p = &awp_loc_first; - else - loc_first_p = &bp_loc_first; - - if (*loc_first_p == NULL - || (overlay_debugging && loc->section != (*loc_first_p)->section) - || !breakpoint_locations_match (loc, *loc_first_p)) + /* Rescan breakpoints at the same address and section, marking the + first one as "first" and any others as "duplicates". This is so + that the bpt instruction is only inserted once. If we have a + permanent breakpoint at the same place as BPT, make that one the + official one, and the rest as duplicates. Permanent breakpoints + are sorted first for the same address. + + Do the same for hardware watchpoints, but also considering the + watchpoint's type (regular/access/read) and length. */ + + bp_loc_first = NULL; + wp_loc_first = NULL; + awp_loc_first = NULL; + rwp_loc_first = NULL; + + ALL_BP_LOCATIONS (loc, locp) { - *loc_first_p = loc; - loc->duplicate = 0; - continue; - } + /* ALL_BP_LOCATIONS bp_location has LOC->OWNER always + non-NULL. */ + struct bp_location **loc_first_p; + b = loc->owner; + + /* Skip this location if it does not belong to the current + program space we are looking for. */ + if (loc->pspace != cur_pspace) + continue; + + if (!should_be_inserted (loc) + || !breakpoint_address_is_meaningful (b) + /* Don't detect duplicate for tracepoint locations because they + are never duplicated. See the comments in field `duplicate' of + ` struct bp_location'. */ + || is_tracepoint (b)) + continue; + + /* Permanent breakpoint should always be inserted. */ + if (b->enable_state == bp_permanent && ! loc->inserted) + internal_error (__FILE__, __LINE__, + _("allegedly permanent breakpoint is not " + "actually inserted")); + + if (b->type == bp_hardware_watchpoint) + loc_first_p = &wp_loc_first; + else if (b->type == bp_read_watchpoint) + loc_first_p = &rwp_loc_first; + else if (b->type == bp_access_watchpoint) + loc_first_p = &awp_loc_first; + else + loc_first_p = &bp_loc_first; + if (*loc_first_p == NULL + || (overlay_debugging && loc->section != (*loc_first_p)->section) + || !breakpoint_locations_match (loc, *loc_first_p)) + { + *loc_first_p = loc; + loc->duplicate = 0; + continue; + } - /* This and the above ensure the invariant that the first location - is not duplicated, and is the inserted one. - All following are marked as duplicated, and are not inserted. */ - if (loc->inserted) - swap_insertion (loc, *loc_first_p); - loc->duplicate = 1; - - if ((*loc_first_p)->owner->enable_state == bp_permanent && loc->inserted - && b->enable_state != bp_permanent) - internal_error (__FILE__, __LINE__, - _("another breakpoint was inserted on top of " - "a permanent breakpoint")); + /* This and the above ensure the invariant that the first location + is not duplicated, and is the inserted one. + All following are marked as duplicated, and are not inserted. */ + if (loc->inserted) + swap_insertion (loc, *loc_first_p); + loc->duplicate = 1; + + if ((*loc_first_p)->owner->enable_state == bp_permanent + && loc->inserted + && b->enable_state != bp_permanent) + internal_error (__FILE__, __LINE__, + _("another breakpoint was inserted on top of " + "a permanent breakpoint")); + } } if (breakpoints_always_inserted_mode () && should_insert Index: gdb/gdb/progspace.h =================================================================== --- gdb.orig/gdb/progspace.h 2012-01-25 10:21:24.321821938 -0200 +++ gdb/gdb/progspace.h 2012-01-25 10:41:55.701821938 -0200 @@ -193,6 +193,13 @@ struct program_space unsigned num_data; }; +/* Iterate over all program spaces. */ + +#define ALL_PROGRAM_SPACES(PSPACE) \ + for (PSPACE = program_spaces; \ + PSPACE; \ + PSPACE = PSPACE->next) + /* The object file that the main symbol table was loaded from (e.g. the argument to the "symbol-file" or "file" command). */ --------------040104020001050505060007--