From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f41.google.com (mail-wm1-f41.google.com [209.85.128.41]) by sourceware.org (Postfix) with ESMTPS id 1AA66382CCAA for ; Fri, 20 May 2022 19:20:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1AA66382CCAA Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wm1-f41.google.com with SMTP id p5-20020a1c2905000000b003970dd5404dso4860423wmp.0 for ; Fri, 20 May 2022 12:20:35 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=Wivuf5NhruTX5CCqFy/ha1akAkcaaiKiN12q1iS7J7k=; b=egc74jTMuN9flhr0NE9eZ6taXqaHb9tb8BSJhsQpbXd7149uw6D2ryJWraRiZVKEjG yGN0mQWIfjPbrDbDtkDhpz4eEo7wKHpFtbDi/SqfXcxuLavVy8KBxkOKHIKV/SL+UrXW E9Qv4pKXXvwkrfOVa6p+C0nG5tHWUkvOqUTaF0tt/MNOHSeRuoeq/UlFF8gzdLWgZeXU Uu4E0c7SuRcYKTTdKWiDCeu2oZyZaIYGuIh0ZagmUCI0bpeIF7q5GShdgGW2uzNOMYBA 8boGIVay25UIz7u+9Q+xqDTOqyheFQF+q7NrZLb/UG0YzKQSRmznBPwqyIct3K+eC977 trtA== X-Gm-Message-State: AOAM533RXx/nV9Kt0RJj8HxuS0FvfReuVYaSHOyP05dQSsKLJyoQQK7X HnpJXW85207SaCfKZEjcjAq5fND/8rA= X-Google-Smtp-Source: ABdhPJwB2cgZWKBpz+MVvSQpVz/QFWUR5w30aZcSXyQC2Xinpy9ME06IIZBs9yLNbayNhNj9uHaDyw== X-Received: by 2002:a05:600c:1f0b:b0:395:c014:8cf with SMTP id bd11-20020a05600c1f0b00b00395c01408cfmr9485418wmb.32.1653074433955; Fri, 20 May 2022 12:20:33 -0700 (PDT) Received: from ?IPV6:2001:8a0:f924:2600:209d:85e2:409e:8726? ([2001:8a0:f924:2600:209d:85e2:409e:8726]) by smtp.gmail.com with ESMTPSA id o20-20020adfca14000000b0020cf33b4e53sm3135639wrh.116.2022.05.20.12.20.32 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 20 May 2022 12:20:32 -0700 (PDT) Message-ID: <00f64ccb-92b1-5400-b95a-2dd9e6e35310@palves.net> Date: Fri, 20 May 2022 20:20:31 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 Subject: Re: [PATCH 09/23] init_breakpoint_sal -> base_breakpoint::base_breakpoint Content-Language: en-US To: Lancelot SIX Cc: gdb-patches@sourceware.org References: <20220516184030.665489-1-pedro@palves.net> <20220516184030.665489-10-pedro@palves.net> <20220520133941.lymcglw2sovyjycn@ubuntu.lan> From: Pedro Alves In-Reply-To: <20220520133941.lymcglw2sovyjycn@ubuntu.lan> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, 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 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 20 May 2022 19:20:37 -0000 On 2022-05-20 14:40, Lancelot SIX wrote: > Hi, > > Some styling remarks below. > > On Mon, May 16, 2022 at 07:40:16PM +0100, Pedro Alves wrote: >> This converts init_breakpoint_sal to a base_breakpoint constructor. >> >> It removes a use of init_raw_breakpoint. >> >> To avoid manually adding a bunch of parameters to >> new_breakpoint_from_type, and manually passing them down to the >> constructors of a number of different base_breakpoint subclasses, make >> new_breakpoint_from_type a variable template function. >> >> Change-Id: I4cc24133ac4c292f547289ec782fc78e5bbe2510 >> --- >> gdb/breakpoint.c | 239 +++++++++++++++++++++++------------------------ >> gdb/breakpoint.h | 16 ++++ >> 2 files changed, 133 insertions(+), 122 deletions(-) >> @@ -8326,81 +8331,68 @@ init_breakpoint_sal (base_breakpoint *b, struct gdbarch *gdbarch, >> >> gdb_assert (!sals.empty ()); >> >> - for (const auto &sal : sals) >> - { >> - struct bp_location *loc; >> + thread = thread_; >> + task = task_; >> >> - if (from_tty) >> - { >> - struct gdbarch *loc_gdbarch = get_sal_arch (sal); >> - if (!loc_gdbarch) >> - loc_gdbarch = gdbarch; >> + cond_string = std::move (cond_string_); >> + extra_string = std::move (extra_string_); >> + ignore_count = ignore_count_; >> + enable_state = enabled_ ? bp_enabled : bp_disabled; >> + disposition = disposition_; >> >> - describe_other_breakpoints (loc_gdbarch, >> - sal.pspace, sal.pc, sal.section, thread); >> - } >> + if (type == bp_static_tracepoint >> + || type == bp_static_marker_tracepoint) >> + { >> + auto *t = static_cast (this); > > I think the space before "<" should be removed (I think I saw this in > one of the previous patches too). Thanks, I fixed it in the patch that introduced it (patch #6). I then did: $ git diff upstream/master... | grep static_cast + auto *t = static_cast (this); + = static_cast (tp->control.single_step_breakpoints); with the whole series applied, so it looks like this was the only case. > >> + struct static_tracepoint_marker marker; >> >> - if (&sal == &sals[0]) >> + if (strace_marker_p (this)) >> { >> - init_raw_breakpoint (b, sal, type); >> - b->thread = thread; >> - b->task = task; >> + /* We already know the marker exists, otherwise, we >> + wouldn't see a sal for it. */ >> + const char *p >> + = &event_location_to_string (location_.get ())[3]; > > Looks like that the statement can fit on one line now. Done. > >> + const char *endp; >> >> - b->cond_string = std::move (cond_string); >> - b->extra_string = std::move (extra_string); >> - b->ignore_count = ignore_count; >> - b->enable_state = enabled ? bp_enabled : bp_disabled; >> - b->disposition = disposition; >> + p = skip_spaces (p); >> >> - if ((flags & CREATE_BREAKPOINT_FLAGS_INSERTED) != 0) >> - b->loc->inserted = 1; >> + endp = skip_to_space (p); >> >> - if (type == bp_static_tracepoint >> - || type == bp_static_marker_tracepoint) >> - { >> - auto *t = static_cast (b); >> - struct static_tracepoint_marker marker; >> - >> - if (strace_marker_p (b)) >> - { >> - /* We already know the marker exists, otherwise, we >> - wouldn't see a sal for it. */ >> - const char *p >> - = &event_location_to_string (b->location.get ())[3]; >> - const char *endp; >> - >> - p = skip_spaces (p); >> - >> - endp = skip_to_space (p); >> - >> - t->static_trace_marker_id.assign (p, endp - p); >> - >> - gdb_printf (_("Probed static tracepoint " >> - "marker \"%s\"\n"), >> - t->static_trace_marker_id.c_str ()); >> - } >> - else if (target_static_tracepoint_marker_at (sal.pc, &marker)) >> - { >> - t->static_trace_marker_id = std::move (marker.str_id); >> + t->static_trace_marker_id.assign (p, endp - p); >> >> - gdb_printf (_("Probed static tracepoint " >> - "marker \"%s\"\n"), >> - t->static_trace_marker_id.c_str ()); >> - } >> - else >> - warning (_("Couldn't determine the static " >> - "tracepoint marker to probe")); >> - } >> + gdb_printf (_("Probed static tracepoint " >> + "marker \"%s\"\n"), > > This literal string can also fit into one line, no need to split it. Done. > >> + t->static_trace_marker_id.c_str ()); >> + } >> + else if (target_static_tracepoint_marker_at (sals[0].pc, &marker)) >> + { >> + t->static_trace_marker_id = std::move (marker.str_id); >> >> - loc = b->loc; >> + gdb_printf (_("Probed static tracepoint " >> + "marker \"%s\"\n"), > > Same here. Done. > >> + t->static_trace_marker_id.c_str ()); >> } >> else >> + warning (_("Couldn't determine the static " >> + "tracepoint marker to probe")); > > And probably here too. Done. > >> + } >> + >> + for (const auto &sal : sals) >> + { >> + if (from_tty) >> { >> - loc = b->add_location (sal); >> - if ((flags & CREATE_BREAKPOINT_FLAGS_INSERTED) != 0) >> - loc->inserted = 1; >> + struct gdbarch *loc_gdbarch = get_sal_arch (sal); >> + if (!loc_gdbarch) > > Should be "loc_gdbarch == nullptr" > >> + loc_gdbarch = gdbarch; Done. >> + >> + describe_other_breakpoints (loc_gdbarch, >> + sal.pspace, sal.pc, sal.section, thread); >> } >> >> + bp_location *new_loc = add_location (sal); >> + if ((flags & CREATE_BREAKPOINT_FLAGS_INSERTED) != 0) >> + new_loc->inserted = 1; >> + >> /* Do not set breakpoint locations conditions yet. As locations >> are inserted, they get sorted based on their addresses. Let >> the list stabilize to have reliable location numbers. */ > > Also, down the line, there are some leftover NULL which could become > nullptr. This is from code which just god re-indented, but while at > updating it, it could be nice to fix this too. All done locally. Thanks!