From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 125931 invoked by alias); 26 Sep 2019 11:32:25 -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 125922 invoked by uid 89); 26 Sep 2019 11:32:25 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-14.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_1,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=stays, conclusion X-HELO: mail-wr1-f46.google.com Received: from mail-wr1-f46.google.com (HELO mail-wr1-f46.google.com) (209.85.221.46) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 26 Sep 2019 11:32:23 +0000 Received: by mail-wr1-f46.google.com with SMTP id b9so2353440wrs.0 for ; Thu, 26 Sep 2019 04:32:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=lRzDEQz9njDCytGYqJFmyF6zxp3xZsH8Y5g6WGfERNM=; b=OYgsRcD6YU06p3l+A6sH0guEB1VHVsdzolMOBqaalNJg5O98SbPjQG2pm8l5rsC1R9 DgjsUn9hQOwMESgG1BfDJI7mYt9EPjDUgQOeCR5XV2PO+zvUZFGf9a96CfFu8PsSk8hf h2buZCN/fZ4pP3Fm5+wHl1uFHc8naW+xplE+Jq7CIYNCTx9ZZwgbkQYj1pfiHpS6SahO uZj3GwlHF9Z2zv2v+QJU32B/UYHj3xtfTcKQgtMCnCZrKBtAwZxOQsD5JQ8rNIaYBmfY TXCqYPpeSbjOSYzIOPl5JM8qdBO0MYHv/2qbNW7u32I8Eo4BhDO8xhMj5NozRcBHmo3l jIUw== Return-Path: Received: from localhost (host86-128-12-122.range86-128.btcentralplus.com. [86.128.12.122]) by smtp.gmail.com with ESMTPSA id e6sm1672318wrp.91.2019.09.26.04.32.20 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 26 Sep 2019 04:32:20 -0700 (PDT) Date: Thu, 26 Sep 2019 11:32:00 -0000 From: Andrew Burgess To: "Metzger, Markus T" Cc: gdb-patches , Simon Marchi , Tom Tromey Subject: Re: [PATCHv3 1/3] gdb: Remove a VEC from gdbsupport/btrace-common.h Message-ID: <20190926113219.GS4962@embecosm.com> References: <87lfucf2qs.fsf@tromey.com> <2edcf1984fc1dbe09ceaac8d79f80ca2051e1062.1569455609.git.andrew.burgess@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Fortune: Linux: Because rebooting is for adding new hardware X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2019-09/txt/msg00510.txt.bz2 * Metzger, Markus T [2019-09-26 08:47:23 +0000]: > Hello Andrew, > > I can help you run the gdb.btrace suite. Do you have a user branch > with these patches? You can find this branch here: https://github.com/T-J-Teru/binutils-gdb/tree/gdb-remove-vec Any additional testing is always welcome, though from Simon's feedback I believe that I should be testing this OK - I'm running on a post-Broadwell Intel CPU, and I am linking GDB against libipt, and see no regressions over the entire testsuite. > > > > @@ -1068,13 +1069,12 @@ btrace_compute_ftrace_bts (struct thread_info *tp, > > > > while (blk != 0) > > { > > - btrace_block_s *block; > > CORE_ADDR pc; > > > > blk -= 1; > > > > - block = VEC_index (btrace_block_s, btrace->blocks, blk); > > - pc = block->begin; > > + const btrace_block &block = btrace->blocks->at (blk); > > + pc = block.begin; > > We could also turn BTRACE->BLOCKS into a const & outside the loop. I was worried about the case where we get here and btrace->blocks == nullptr. I don't know if such a path is possible, I had a little through the code and my conclusion was that the answer is not obvious. So ideally I'd leave the code as close to the original as possible to avoid introducing bugs. > > > > @@ -1581,11 +1580,9 @@ 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 ; > > A stack-allocated vector should do. Looks like we leaked the one-entry > vector before. Nope the btrace object is stack local, and its destructor deletes the vector, so its easier if this just stays being new'd for now. > > > > @@ -2052,9 +2048,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; > > + gdb_assert (btrace->variant.bts.blocks != nullptr); > > We don't check most pointers in GDB. Besides, we just allocated the vector. > The invariant is that if BTRACE->FORMAT == BTRACE_FORMAT_BTS, the vector > is non-null. You are correct. Thanks. I've removed this assertion. > > > > @@ -3095,7 +3090,8 @@ btrace_maint_update_packets (struct btrace_thread_info > > *btinfo, > > case BTRACE_FORMAT_BTS: > > /* Nothing to do - we operate directly on BTINFO->DATA. */ > > *begin = 0; > > - *end = VEC_length (btrace_block_s, btinfo->data.variant.bts.blocks); > > + gdb_assert (btinfo->data.variant.bts.blocks != nullptr); > > See above. More below. I think you've convinced me that having all of these assertions around isn't great. So I've taken a slightly different approach. I'll post another iteration soon. Thanks, Andrew