From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id A35333858CDB for ; Wed, 24 May 2023 13:04:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A35333858CDB Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1684933497; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=yHoi6PJoSf6tcGItv5XgInr7ASHIILboih7cZCpCjjc=; b=AYTAQ6FlX7X/Q4PEs2dC2l1bfnOIl732HXeA3jyrWAv3np3/6k5RtHTYiYg/rfgRaAb/yI 2Gfr+AB5NtBp2ZwqNgyla+c0GfIj3m2P07Sx8GMkwb3Wy4ZXMBbyMxDi4tASn+hmY9QuyL QrGNjrmTJ4fQQyVJMcNuBqcKvUrd7cI= Received: from mail-qv1-f69.google.com (mail-qv1-f69.google.com [209.85.219.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-374-80NwD0yGPJa7Za9n84CmXw-1; Wed, 24 May 2023 09:04:55 -0400 X-MC-Unique: 80NwD0yGPJa7Za9n84CmXw-1 Received: by mail-qv1-f69.google.com with SMTP id 6a1803df08f44-62387e4de2bso12770196d6.2 for ; Wed, 24 May 2023 06:04:55 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684933495; x=1687525495; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=yHoi6PJoSf6tcGItv5XgInr7ASHIILboih7cZCpCjjc=; b=bebDyZBtcX3CSBDg2Mi3dOHkZLDr4QPiuKKcdTqIhhUcpCtBF3Aasdt21NX8bR+qoh blWAm3YA464WMMB4m+7l1eqXMTFEySu4wr2F0+iNnscW/wChM9WwgaSi5EHEKCIw/3Yw Z5njCVNc/8k71D2dl/fgQKo3RvX/MOmUMoYshSMVtKZGYWkL7ZvVZfk1YkYPMvBMe80R oJPiXRDtIqIwm5+ufpsyHj15vDbPVlsGGrotQgSON/Pes/Wd8isYoiDAmKy6RLTcUlKM Igme3LggyFtPiLN4imf168199jKpRgFOeNJe8kj1uyy2onGIZbknHpRrmrhMPOLak9hl Tilw== X-Gm-Message-State: AC+VfDzsa+6BDlvEAERDQIX7dM1EKdjq38mX86husiONXugPbBdWl6fA TEYmXe/dlfhMl4ZTe1K8OyoJ3u7MjdJ9B1R13eWydGsM4Y364Ad8R/X2qaEihIiUbEoBLA2tAaO ZfMJ3uuoGQIDZ1kkiEXiTwp/m1kbyaA== X-Received: by 2002:a05:6214:d02:b0:61b:5afc:d4be with SMTP id 2-20020a0562140d0200b0061b5afcd4bemr29653286qvh.7.1684933495096; Wed, 24 May 2023 06:04:55 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ6N1cnCUsT9ScNMFhS8AbABgsNqIDlS9oBC+A2ELQoFJZBh2kS7VV7cKNxmHyEQTALqY+nyyQ== X-Received: by 2002:a05:6214:d02:b0:61b:5afc:d4be with SMTP id 2-20020a0562140d0200b0061b5afcd4bemr29653244qvh.7.1684933494564; Wed, 24 May 2023 06:04:54 -0700 (PDT) Received: from localhost (11.72.115.87.dyn.plus.net. [87.115.72.11]) by smtp.gmail.com with ESMTPSA id x19-20020ae9f813000000b00757882b0224sm3234646qkh.49.2023.05.24.06.04.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 24 May 2023 06:04:54 -0700 (PDT) From: Andrew Burgess To: Simon Marchi , Simon Marchi via Gdb-patches Subject: Re: [PATCH 07/12] gdb: link breakpoint locations with intrusive_list In-Reply-To: <5503a76c-10b6-08d9-4b4b-d8f2f673bced@efficios.com> References: <20230511144832.17974-1-simon.marchi@efficios.com> <20230511144832.17974-8-simon.marchi@efficios.com> <87a5y1aed8.fsf@redhat.com> <5503a76c-10b6-08d9-4b4b-d8f2f673bced@efficios.com> Date: Wed, 24 May 2023 14:04:46 +0100 Message-ID: <87bki97udt.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,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: Simon Marchi writes: >>> 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. Not a problem, I did wonder if there was going to be some reason why this wasn't a helpful suggestion :) Thanks for looking into it though. Andrew > >>> @@ -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