From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from CAN01-YT3-obe.outbound.protection.outlook.com (mail-yt3can01on2049.outbound.protection.outlook.com [40.107.115.49]) by sourceware.org (Postfix) with ESMTPS id ABE6D3858CDB for ; Thu, 18 May 2023 18:40:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org ABE6D3858CDB Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=efficios.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=efficios.com ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=OCLQtlMjZC17+j5WtT14AGbdNrPx3poWE5NgJvHfcyafrk0U35/riSGqYQQNGC+8kp8WhjoPFlQ53t3hQ0JyTB2jTU+4Bquki7ADjj1KLiI9jjwI8+3rp1AC49oIY97kTfrTE9BRlE3e9Aj69JswnIVBs2JPWTic2ZQzxokV9qUg/6FyGVdr/rDKI4qlOKM2llyXybLLv3LJPOAr4iJAIitunYwZT3U2ASXc85BAqpki2ofZvKiPDUg+H2fRQXfC4hl2eukexrFQ+Q+VsY/XhCaku9gZBP65GhZ+Q2H0U1tiWAlDUg04JgjMnQ98jAjT0UXcEiOG904mqmqYNDmRyQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=Zgyrh1bQFfqSbFd37W/QXjxtSaWnW1BIjSpw6kgdGb0=; b=g8ZDHy5jrDTGSFAlziZwgRmr1q5jiNto2+npVMbRrCGvrN5ZSCrwR9iGnpcVib+4nSzt9J0g4q1waRX8lxMYVS006enY9VYROGs+d/GGe+fWdd8YtWeYPK32LslePclNP1WC7H0YpnIWRnnNkJbHEdtIpFWsIur9T8ZstSXT7/54WylrF3Z+w1y7bqTK7G6NxQoLqXEnDn6RtIDGRDs4nQzleKyCF4GarRq2Tnlnh4JFNhiHurFcsGMcbpXjlEKMVIsf+ObtlqXek1GIvxt3lw41yw3bpaCehm8fRtmTIsKKvqfwiYhSPgtnQxXfZFQORtBksePjplLVHndPQdY4QQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=efficios.com; dmarc=pass action=none header.from=efficios.com; dkim=pass header.d=efficios.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=efficios.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=Zgyrh1bQFfqSbFd37W/QXjxtSaWnW1BIjSpw6kgdGb0=; b=SzhQAskuFA+rmE0ahijUZpjxHovvyUJjyfyVsjO0CCNKFJ2rNE9zmGRI2l4m/Wc2rAyRkfUt50JTald1koGi//P9smjLRWzMS6iVCYmPSwQEjyBaLihScpvlsFV9I7dam2IoCuUYfxD01SoVIxGfu6H4+HidbufZTjYVFks7No2PoK/6Fldg5I6SIlqkzeuEUkxMLDaIpQ6Lps2pdBX3SKR37i7LHu5Yo+1pozjmf+dwanStkq/qvUbYfqcCxOE02v2BUtoHKeUERR2EPTu/fcPekE+7arlREbnmXGmS07xCwA2oX2lGMprrR788SOs9sHS0IW8Cp53H0JnYS4xmKw== Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=efficios.com; Received: from YT1PR01MB2828.CANPRD01.PROD.OUTLOOK.COM (2603:10b6:b01:a::23) by YT1PR01MB8780.CANPRD01.PROD.OUTLOOK.COM (2603:10b6:b01:cb::8) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6411.17; Thu, 18 May 2023 18:40:42 +0000 Received: from YT1PR01MB2828.CANPRD01.PROD.OUTLOOK.COM ([fe80::b7c2:2912:524e:8e80]) by YT1PR01MB2828.CANPRD01.PROD.OUTLOOK.COM ([fe80::b7c2:2912:524e:8e80%6]) with mapi id 15.20.6411.019; Thu, 18 May 2023 18:40:42 +0000 Message-ID: <5503a76c-10b6-08d9-4b4b-d8f2f673bced@efficios.com> Date: Thu, 18 May 2023 14:40:40 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [PATCH 07/12] gdb: link breakpoint locations with intrusive_list Content-Language: fr To: Andrew Burgess , Simon Marchi via Gdb-patches References: <20230511144832.17974-1-simon.marchi@efficios.com> <20230511144832.17974-8-simon.marchi@efficios.com> <87a5y1aed8.fsf@redhat.com> From: Simon Marchi In-Reply-To: <87a5y1aed8.fsf@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-ClientProxiedBy: YQBPR0101CA0207.CANPRD01.PROD.OUTLOOK.COM (2603:10b6:c01:67::30) To YT1PR01MB2828.CANPRD01.PROD.OUTLOOK.COM (2603:10b6:b01:a::23) MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: YT1PR01MB2828:EE_|YT1PR01MB8780:EE_ X-MS-Office365-Filtering-Correlation-Id: 6517b1f3-5711-434b-0bec-08db57cf621d X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: 4EzqJ4kRvgdzSR5tiRBJp1VCIIo6N/M+3QnOeK3EZarTny4F+nulDptLiLGLD1l3JogiCqobCbTZCsYpKPqlQUDUoJ7Xo9aZzq20mPOdnk2wv1cc83s1SKGxAz9HsAIZzTs9Ruk83va6AozTtZ6qIDHZM7IDcdh+MgE6FVDjYFjGbB7n5tZgRW+QJz6l5fnKshaGdIsfHgHUOecP/PkWAGkXVymgywVP2v0i5/6QQs+dEdxl8fWBqb4b6s6sBOb7AVqxNUNi7ERd5YnQiF6cvncH83xHVVqjxfG89K3SIs0SEQCwUheS+hR28Qhzv8i+PMpYU6Ai5ty1mlybRwJzbU9u2iJJa9EHKBhdVs4rsJJRFDvSWtpvcLwD2RMLx5kX1vRg+c/z+SutnmVDF4+ZROSjH+jTP3rrEVwJSV76izKKJAC9fGtCbv7mejY0dHpS1mte10DU7bUF26BaqVA1CnC5u6S6gLRX/DSdsJ2Q2zHIARoU7L8RQE7ZvvN4i0sU05HuhLaXBIaGlv5RzBdJpyvAaG1ccCmRbcC8GX6Ikc33kFgsQawKK9DyN4VK+Ip4M9+gi3WYE01JLvbGgNaPCrZQRpfDMiZ/K1xKZ4gaSl2gpR5gpaEcZbWhtU6QwEZBSxfRknqNiOxwXHx+Dh0KFw== X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:YT1PR01MB2828.CANPRD01.PROD.OUTLOOK.COM;PTR:;CAT:NONE;SFS:(13230028)(39830400003)(346002)(396003)(366004)(376002)(136003)(451199021)(5660300002)(36756003)(41300700001)(83380400001)(2906002)(2616005)(31696002)(38100700002)(8676002)(26005)(186003)(44832011)(6512007)(8936002)(6506007)(45080400002)(66556008)(66476007)(66946007)(31686004)(478600001)(6486002)(316002)(110136005)(43740500002)(45980500001);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?a0VNR0ZpQ3ZjWEErQVlQM2NnMW1CQ2FNR2UvbnR5bHBNa3RxdktvWnRjWDUz?= =?utf-8?B?ZllnaFFNUVoza1I1MzVtSmJvZWtpVmVkc0JDWDlkY0ZTZUxvNFgwNlFsWjI5?= =?utf-8?B?MlpKYlNtZDNrdUxXV2FCRVRycVZqL1JxUXZ6cE9TZkROOUJUWTZxa3MxMWdv?= =?utf-8?B?b2hCeUtwQWZ1dkFrZ2VHeU9aNDFhaUR1OUt1aWd0aTJSbXliMlZkV2xSTHNL?= =?utf-8?B?WUlSbWRkaldXMm00SjN0K1JyTWt5MytCS1RRSm9FaXdJUHE2bjFIVGwzYi8z?= =?utf-8?B?T0J6QTQvcU1FQnNmY2toUTVZaElSNm9ldERjNldNcld2NWMxZnNCUm12ZTF5?= =?utf-8?B?d3hMUVF2R2wyYkhKckZOVERWc0hYUU9ySGxNTjhmQUZzKzVTUXV2alNUYTgv?= =?utf-8?B?S2toUzBpcWljM2dhOFJuRm5xNTdzeFRJclZvQUErV09tMU5vMDNYV1M0T0Uy?= =?utf-8?B?ZmpaTml0MFNxeXQxV0VVUlMwQXMwWms0M0VOSDFIRjBNbThlR2hHT0EwT3gx?= =?utf-8?B?aWNOVjFuanVOTlhzS2VlRVNjK0NqV1pwVHdWOHZ6WTU4ZVZ6S09kNk9lMXpD?= =?utf-8?B?ZlhsaUJGNEFnS3JnL1prVmM5cWJCM1ZNby9IL2tDRG1iWVd4djA4c25iV2Fv?= =?utf-8?B?U3pqL0hxRjkrYnRvTnZxNkVXVStGKzJqUWdyQ2R5a0I3WlpEODc5R2hMcVMx?= =?utf-8?B?R2dvZWpBbUttSHFPb3hWdDlKb1l5VHVGaHc0cEh1STc2ZU9adkZCNGFTcGR0?= =?utf-8?B?dDhaeWsxdXZTV2wzb1dmekhmMlNnSFM4RTlCZkMrVHVZSzMwejBPT2FYY1Yy?= =?utf-8?B?L3JVdDJPZi9UR3RBTmwyVHl2dTU4L3JNK0dTcHFrbXRBZ25QVUJOc2xJcE5V?= =?utf-8?B?S0FyZmFoMGVnTmkyKzNReW1GdFZsS2EyeWFYU2tFcXB0MVdFTDNsaGdHczRP?= =?utf-8?B?M2FGdXZJYldMU0VqZnBXMlhCaGdaM1E5MjU4SGhiSmVZTlZWVm1MWVowZmRK?= =?utf-8?B?SnlCRStIcWxYcStsaEZKUW9tS01ONERva1B0OXljUnA2OUdDMWVrb01CSEJm?= =?utf-8?B?OXF2RmtaNkJZaTZzYjNucFk0N1RkL0IvRWgxRmd4SDRJdXRaK1VOOVJ3aWR5?= =?utf-8?B?WHJXRDY5WER3K1NNb05BUmlvaXZCNHFCWVJpV3Z5a0x0cUR6cE5WSnN1L2dU?= =?utf-8?B?OCtuSURHaWxLSnJ6SjJYazJKUnZKWG5hcDVZVzFEV2N5cyt5alpnZjB5WGxP?= =?utf-8?B?WHJFWkQyOVdTckxPc0hNa2lnK015eEtFc1k4NG5qTjRuVUY1NldObHNDTlhl?= =?utf-8?B?bWFuSTdIRFpUcGMwVnRuZWt3T01VcFJJdllIcnFZV0I3eXRubHRoZlpvMGlT?= =?utf-8?B?RjlkM1FUdGJvOVNlRklRQmRGNE16dmdRNUdPaXVIYmg1V2ExRVlKQmZMTmI3?= =?utf-8?B?RHViL2x6WXdmY0N1a1pWczdROHE0MzBlaU1iRC8yNUlEZE1rRGd5ZmNSS0tH?= =?utf-8?B?V0taUFhLM0cvK0EwK1ZIc05NQmxZRm1rU1dhaVBCd3pQREJvcVlFa3BKZFoy?= =?utf-8?B?R0tpNG5QWkdvQ2EyN1VLbEUvS0RKekc4SGRZT3oyUUNUWWtZQ1Zha3NoMkFy?= =?utf-8?B?c3dqb01qdnBjdkhvSkRqdndOOW01T0FERzUzbFU4dE4zcEVtbnB3K09FL3Zw?= =?utf-8?B?WGh6MGJNelYvTDFsNW1mQ2gxcGZqZ3NSb041SmFFd1F6dG5WN2VzMXdqWGw5?= =?utf-8?B?TTFBMS92cHdhWU92bzdvNlhPODJua3orM1A1Q1E4Ny9RejJac2RaczBNbEo5?= =?utf-8?B?V3MrUzI3WTlOSGtJcm5nSHBFdjYwaGZZUlJ3YlVVZGNZSVNPakFHZFkrcml2?= =?utf-8?B?UUo3dG0yd3BXcGxPNUUwaGlPT1YybENOWEo2aUVSWnB0OFcwSjRqMTFuVHZJ?= =?utf-8?B?cUhHMCs5dGJDNU5WWmJpVnVrYWQyZGI1OGNJeWxnV2ZlTEg2Y3RLejUzTENo?= =?utf-8?B?cUdnM0JGUDFWQVI2VnpuOFd1R2V4WTZrbmZRdlkrQ29FejAwU3h2VFBoVysy?= =?utf-8?B?d2V2R2N0WVBtYUc0aWhvWENXRUl1QTZrSmRkTXkyL3gzeVU1RURoNVhjemNr?= =?utf-8?Q?wwTuJXhq+a6IzdRvG4HCFLd4U?= X-OriginatorOrg: efficios.com X-MS-Exchange-CrossTenant-Network-Message-Id: 6517b1f3-5711-434b-0bec-08db57cf621d X-MS-Exchange-CrossTenant-AuthSource: YT1PR01MB2828.CANPRD01.PROD.OUTLOOK.COM X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 May 2023 18:40:42.2663 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 4f278736-4ab6-415c-957e-1f55336bd31e X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: JNyfH5xBCMu/P/ZpkHG81IwFHyDGZG5o8VmZ87ge9AGbGKqBO3T/GJ5LhCGaoGhhMEXbsf3TqhdU8axfmhdi5g== X-MS-Exchange-Transport-CrossTenantHeadersStamped: YT1PR01MB8780 X-Spam-Status: No, score=-3040.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: >> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c >> index e9b6574bba77..6ec0f6a11e88 100644 >> --- a/gdb/breakpoint.c >> +++ b/gdb/breakpoint.c >> @@ -1049,13 +1049,18 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp, >> the error and the condition string will be rejected. >> This two-pass approach is taken to avoid setting the >> state of locations in case of a reject. */ >> - for (bp_location *loc : b->locations ()) >> + auto bp_loc_range = b->locations (); >> + for (auto bp_loc_it = bp_loc_range.begin (); >> + bp_loc_it != bp_loc_range.end (); >> + ++bp_loc_it) >> { >> + bp_location &loc = **bp_loc_it; >> + > > You could switch this to: > > for (const bp_location *loc : b->locations ()) > > if we could also do .... (continued below) > >> try >> { >> const char *arg = exp; >> - parse_exp_1 (&arg, loc->address, >> - block_for_pc (loc->address), 0); >> + parse_exp_1 (&arg, loc.address, >> + block_for_pc (loc.address), 0); >> if (*arg != 0) >> error (_("Junk at end of expression")); >> break; >> @@ -1065,7 +1070,7 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp, >> /* Condition string is invalid. If this happens to >> be the last loc, abandon (if not forced) or continue >> (if forced). */ >> - if (loc->next == nullptr && !force) >> + if (std::next (bp_loc_it) == bp_loc_range.end () && !force) > > ... this: > > if (loc == b->last_loc ()) > throw > > This requires adding 'breakpoint::last_loc ()' but we already have > 'breakpoint::first_loc ()' so that doesn't seem crazy, and I think the > resulting code would be easier to read maybe? Yeah, that's a good idea. It makes the code read just like the comment does (if the loc is the last loc), which is a plus for readability. Note that I used &b->last_loc (), as I made last_loc return a reference (like first_loc). >> @@ -4518,28 +4510,23 @@ bpstat_locno (const bpstat *bs) >> const struct breakpoint *b = bs->breakpoint_at; >> const struct bp_location *bl = bs->bp_location_at.get (); >> >> - int locno = 0; >> - >> if (b != nullptr && b->has_multiple_locations ()) >> { >> - const bp_location *bl_i; >> - >> - for (bl_i = b->loc; >> - bl_i != bl && bl_i->next != nullptr; >> - bl_i = bl_i->next) >> - locno++; >> + int locno = 1; >> >> - if (bl_i == bl) >> - locno++; >> - else >> + for (bp_location *loc : b->locations ()) >> { >> - warning (_("location number not found for breakpoint %d address %s."), >> - b->number, paddress (bl->gdbarch, bl->address)); >> - locno = 0; >> + if (bl == loc) >> + return locno; >> + >> + ++locno; >> } >> + >> + warning (_("location number not found for breakpoint %d address %s."), >> + b->number, paddress (bl->gdbarch, bl->address)); >> } > > I might be missing something here, but could this code be simplified by > using std::distance in some way? We already use std::distance later in > this patch to count the total number of locations, so it feels like this > should work. > > I know that the current code allows for the possibility that the > location (from the bp_stat) might not be in the location list of the > breakpoint (from bp_stat), but, surely that can't happen, right? Surely > the location was originally found by looking through the locations of > the breakpoint... so it should be certain that we find the location. > > I guess maybe we could remove the breakpoint_at from bp_stat, and just > use the owner of the bp_location_at variable, that way we could be > certain about all this... > > Either way, there's nothing wrong with your code above, just wondering > if we can make it tighter... I'm not sure about whether we are sure that the location is in its parent location list, there is perhaps some corner cases about handling a breakpoint hit event on a location that has been removed since. However, I see that before putting a location in the moribund_locations vector, we set its owner to NULL. So if the owner field is non-NULL, it should be safe to assume the location is in the list. I don't think it's easy to use std::distance right now though, because with the reference_to_pointer_iterator wrapper, we can't go from item to iterator in O(1). We can if we had access to the intrusive_list directly, since it has an "iterator_to" method. In any case, I will not make such a change in this patch to avoid mixing things together. I tried to make this patch a straightforward translation without any unnecessary algorithmic changes. I'll check if it becomes possible at the end of the series though. >> @@ -8312,12 +8299,8 @@ code_breakpoint::add_location (const symtab_and_line &sal) >> sal.pspace); >> >> /* Sort the locations by their ADDRESS. */ >> - new_loc = allocate_location (); >> - for (tmp = &(loc); *tmp != NULL && (*tmp)->address <= adjusted_address; >> - tmp = &((*tmp)->next)) >> - ; >> - new_loc->next = *tmp; >> - *tmp = new_loc; >> + bp_location *new_loc = this->allocate_location (); >> + breakpoint::add_location (*new_loc, adjusted_address); > > I notice here the code basically does: > > 1. Create bp_location object, > 2. Add bp_location object to list, > 3. Initialise bp_location object. > > We have to pass adjusted_address through because the address of new_loc > has not yet been initialised. > > Could we reorder things so we: > > 1. Create bp_location object, > 2. Initialise bp_location object, > 3. Add bp_location object to list. > > The benefit I think would be that we can remove these two functions: > > void breakpoint::add_location (bp_location &loc); > void breakpoint::add_location (bp_location &loc, CORE_ADDR address); > > And we replace them with one function: > > void breakpoint::add_location (bp_location &loc, bool sorted = false); > > The implementation would be free to use 'loc.address' for the sorting. Good idea, I will do that. That seems like a minor / low-risk change, so I'll do it in this patch. For consistency, I moved the "add_location" call after setting loc->address in update_watchpoint as well, even though it wouldn't matter in that case. >> @@ -633,30 +630,71 @@ struct breakpoint >> /* Allocate a location for this breakpoint. */ >> virtual struct bp_location *allocate_location (); >> >> + /* Return a range of this breakpoint's locations. */ >> + bp_location_range locations () const; >> + >> + /* Add LOC at the end of the location list of this breakpoint. >> + >> + LOC must have this breakpoint as its owner. LOC must not already be linked >> + in a location list. */ >> + void add_location (bp_location &loc); >> + >> + /* Add LOC in the location list of this breakpoint, sorted by address. >> + >> + LOC must have this breakpoint as its owner. LOC must not already be linked >> + in a location list. */ >> + void add_location (bp_location &loc, CORE_ADDR address); > > It would be nice for the comment to mention what ADDRESS means here. > However, my earlier comments for where this function is used might mean > this is no longer needed in this form. Yes, this gets removed. Thanks, Simon