From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 55489 invoked by alias); 24 Sep 2019 02:09:57 -0000 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 Received: (qmail 55439 invoked by uid 89); 24 Sep 2019 02:09:57 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-18.8 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 24 Sep 2019 02:09:54 +0000 Received: from [10.0.0.11] (unknown [192.222.164.54]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id B37101E512; Mon, 23 Sep 2019 22:09:52 -0400 (EDT) Subject: Re: [PATCH 1/2] gdb: Remove a VEC from gdbsupport/btrace-common.h To: Andrew Burgess , gdb-patches References: <0b4986ad14493ae77db1dfd87d2a54d30c9ad65d.1569276387.git.andrew.burgess@embecosm.com> From: Simon Marchi Message-ID: Date: Tue, 24 Sep 2019 02:09:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.0 MIME-Version: 1.0 In-Reply-To: <0b4986ad14493ae77db1dfd87d2a54d30c9ad65d.1569276387.git.andrew.burgess@embecosm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-09/txt/msg00458.txt.bz2 On 2019-09-23 6:09 p.m., Andrew Burgess wrote: > Converts a VEC into a std::vector in gdbsupport/btrace-common.h. This > commit just performs a mechanical conversion and doesn't do any > refactoring. One consequence of this is that the std::vector must > actually be a pointer to std::vector as it is placed within a union. > It might be possible in future to refactor to a class hierarchy and > remove the need for a union, but I'd rather have that be a separate > change to make it easier to see the evolution of the code. Hi Andrew, I think this is a good approach for incremental progress. I noted a few things I would change, but nothing looked wrong to me in the patch. > @@ -1073,7 +1073,7 @@ btrace_compute_ftrace_bts (struct thread_info *tp, > > blk -= 1; > > - block = VEC_index (btrace_block_s, btrace->blocks, blk); > + block = &btrace->blocks->at (blk); Can you take the opportunity to make `block` a pointer to const? The `btrace` object it comes from is const, so if we had proper accessors to get the data out of it, the returned pointer would be const. > @@ -1581,11 +1580,10 @@ btrace_add_pc (struct thread_info *tp) > pc = regcache_read_pc (regcache); > > btrace.format = BTRACE_FORMAT_BTS; > - btrace.variant.bts.blocks = NULL; > + btrace.variant.bts.blocks = new std::vector ; > > - block = VEC_safe_push (btrace_block_s, btrace.variant.bts.blocks, NULL); > - block->begin = pc; > - block->end = pc; > + btrace_block block (pc, pc); > + btrace.variant.bts.blocks->push_back (block); The impact here would be minimal since the size of btrace_block is very small, but the idiomatic thing to do there would be to construct the object directly in the vector using emplace_back: > @@ -1724,9 +1722,9 @@ btrace_stitch_bts (struct btrace_data_bts *btrace, struct thread_info *tp) > In the second case, the delta trace vector should contain exactly one > entry for the partial block containing the current PC. Remove it. */ > if (first_new_block->end == last_insn.pc > - && VEC_length (btrace_block_s, btrace->blocks) == 1) > + && btrace->blocks->size () == 1) This fits on a single line. > @@ -2052,9 +2049,8 @@ parse_xml_btrace_block (struct gdb_xml_parser *parser, > begin = (ULONGEST *) xml_find_attribute (attributes, "begin")->value.get (); > end = (ULONGEST *) xml_find_attribute (attributes, "end")->value.get (); > > - block = VEC_safe_push (btrace_block_s, btrace->variant.bts.blocks, NULL); > - block->begin = *begin; > - block->end = *end; > + btrace_block block (*begin, *end); > + btrace->variant.bts.blocks->push_back (block); I suggest using emplace_back here too. > @@ -141,15 +141,12 @@ btrace_data_append (struct btrace_data *dst, > > /* We copy blocks in reverse order to have the oldest block at > index zero. */ > - blk = VEC_length (btrace_block_s, src->variant.bts.blocks); > + blk = src->variant.bts.blocks->size (); > while (blk != 0) > { > - btrace_block_s *block; > - > - block = VEC_index (btrace_block_s, src->variant.bts.blocks, > - --blk); > - > - VEC_safe_push (btrace_block_s, dst->variant.bts.blocks, block); > + btrace_block_s block > + = src->variant.bts.blocks->at (--blk); This fits on a single line. Also, I don't know if it would make a difference at all (or if it would even make things worse, since btrace_data is such a small structure), but I would instinctively have made `block` a const reference to avoid a copy. > @@ -43,11 +41,22 @@ struct btrace_block > > /* The address of the first byte of the last instruction in the block. */ > CORE_ADDR end; > + > + /* Simple constructor. */ > + btrace_block (CORE_ADDR begin, CORE_ADDR end) > + : begin (begin), > + end (end) > + { > + /* Nothing. */ > + } > + > +private: > + /* Don't create blocks without initialization. */ > + btrace_block () = delete; Is this explicit deletion needed? When you have a user-defined constructor, the compiler won't generate an implicit default contructor. It's the opposite, if we wanted it, we would need to do: btrace_block () = default; Also, if you need to delete some otherwise implicitly generated constructor or operator, you don't need to put it private, just the "= delete" is enough. > }; > > /* Define functions operating on a vector of branch trace blocks. */ > typedef struct btrace_block btrace_block_s; > -DEF_VEC_O (btrace_block_s); You can get rid of the typedef too, and just use `btrace_block` everywhere. > diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c > index 8625291cce9..e1da4a1645b 100644 > --- a/gdb/nat/linux-btrace.c > +++ b/gdb/nat/linux-btrace.c > @@ -271,11 +271,11 @@ perf_event_sample_ok (const struct perf_event_sample *sample) > In case the buffer overflows during sampling, one sample may have its lower > part at the end and its upper part at the beginning of the buffer. */ > > -static VEC (btrace_block_s) * > +static std::vector * > perf_event_read_bts (struct btrace_target_info* tinfo, const uint8_t *begin, > const uint8_t *end, const uint8_t *start, size_t size) > { > - VEC (btrace_block_s) *btrace = NULL; > + std::vector *btrace = new std::vector (); You can omit the parenthesis. I just mention this because you have omitted them elsewhere in the patch, so I thought, let's be consistent! > @@ -785,7 +785,7 @@ linux_read_bts (struct btrace_data_bts *btrace, > data_head = *pevent->data_head; > > /* Delete any leftover trace from the previous iteration. */ > - VEC_free (btrace_block_s, btrace->blocks); > + delete (btrace->blocks); You can remove the parenthesis here. > > if (type == BTRACE_READ_DELTA) > { > @@ -843,9 +843,9 @@ linux_read_bts (struct btrace_data_bts *btrace, > /* Prune the incomplete last block (i.e. the first one of inferior execution) > if we're not doing a delta read. There is no way of filling in its zeroed > BEGIN element. */ > - if (!VEC_empty (btrace_block_s, btrace->blocks) > + if (!btrace->blocks->empty () > && type != BTRACE_READ_DELTA) This could fit one a single line. Simon