From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 102404 invoked by alias); 13 Dec 2019 16:02:13 -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 102394 invoked by uid 89); 13 Dec 2019 16:02:13 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-18.6 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: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 13 Dec 2019 16:02:10 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id xBDG22iI008860 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 13 Dec 2019 11:02:07 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca xBDG22iI008860 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=polymtl.ca; s=default; t=1576252927; bh=BKDpDs2CghzZB/SA1Z6jVlDwAQnoPImuidVlQsFoNQk=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=GWuTuAJkDaxyHuK++ehqkxi/p8PFGVmDtliXopgsYbq+WuBBLZpGi4NJ9otYTT4Je 82O9CyjTcF66kmBmZ/kILCO7xBPsV5MVQSiTp2OuL8BDWEmkMOjPRsWaj3dk17hZpH ji779NqgvdNVOzdZsEn8c49Em+qYjsuKxrVrcRr8= Received: from [172.16.0.95] (192-222-181-218.qc.cable.ebox.net [192.222.181.218]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id E8EC61E573; Fri, 13 Dec 2019 11:02:01 -0500 (EST) Subject: Re: [PATCH 4/7] jit: make gdb_symtab::blocks a vector To: Christian Biesinger Cc: gdb-patches References: <20191213060323.1799590-1-simon.marchi@polymtl.ca> <20191213060323.1799590-5-simon.marchi@polymtl.ca> From: Simon Marchi Message-ID: <4eb506cf-b407-82b8-8f5d-e2c0481c431f@polymtl.ca> Date: Fri, 13 Dec 2019 16:02:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2019-12/txt/msg00599.txt.bz2 On 2019-12-13 10:16 a.m., Christian Biesinger via gdb-patches wrote: > On Fri, Dec 13, 2019, 01:18 Simon Marchi wrote: > >> This patch changes the gdb_symtab::blocks linked list to be an >> std::vector, simplifying memory management. >> >> Currently, the list is sorted as blocks are created. It is easier (and >> probably a bit more efficient) to sort them once at the end, so this is >> what I did. >> >> A note about the comment on the "next" field: >> >> /* gdb_blocks are linked into a tree structure. Next points to the >> next node at the same depth as this block and parent to the >> parent gdb_block. */ >> >> I don't think it's true that "next" points to the next node at the same >> depth. It might happen to be true for some nodes, but it can't be true >> in general, as this is a simple linked list containing all the created >> blocks. >> >> gdb/ChangeLog: >> >> * jit.c (struct gdb_block) : Remove field. >> (struct gdb_symtab) <~gdb_symtab>: Adjust to std::vector. >> : Change type to std::vector. >> : Remove. >> (compare_block): Remove. >> (jit_block_open_impl): Adjust to std::vector. Place the new >> block at the end, don't mind about sorting. >> (finalize_symtab): Adjust to std::vector, sort the blocks vector >> before using it. >> --- >> gdb/jit.c | 111 +++++++++++++++--------------------------------------- >> 1 file changed, 31 insertions(+), 80 deletions(-) >> >> diff --git a/gdb/jit.c b/gdb/jit.c >> index eace83e583d3..bb855e09f59b 100644 >> --- a/gdb/jit.c >> +++ b/gdb/jit.c >> @@ -428,10 +428,8 @@ jit_read_code_entry (struct gdbarch *gdbarch, >> >> struct gdb_block >> { >> - /* gdb_blocks are linked into a tree structure. Next points to the >> - next node at the same depth as this block and parent to the >> - parent gdb_block. */ >> - struct gdb_block *next, *parent; >> + /* The parent of this block. */ >> + struct gdb_block *parent; >> >> /* Points to the "real" block that is being built out of this >> instance. This block will be added to a blockvector, which will >> @@ -456,14 +454,8 @@ struct gdb_symtab >> >> ~gdb_symtab () >> { >> - gdb_block *gdb_block_iter, *gdb_block_iter_tmp; >> - >> - for ((gdb_block_iter = this->blocks, >> - gdb_block_iter_tmp = gdb_block_iter->next); >> - gdb_block_iter; >> - gdb_block_iter = gdb_block_iter_tmp) >> + for (gdb_block *gdb_block_iter : this->blocks) >> { >> - gdb_block_iter_tmp = gdb_block_iter->next; >> xfree ((void *) gdb_block_iter->name); >> xfree (gdb_block_iter); >> } >> @@ -471,10 +463,7 @@ struct gdb_symtab >> >> /* The list of blocks in this symtab. These will eventually be >> converted to real blocks. */ >> - struct gdb_block *blocks = nullptr; >> - >> - /* The number of blocks inserted. */ >> - int nblocks = 0; >> + std::vector blocks; >> >> /* A mapping between line numbers to PC. */ >> gdb::unique_xmalloc_ptr linetable; >> @@ -537,28 +526,6 @@ jit_symtab_open_impl (struct gdb_symbol_callbacks *cb, >> return symtab; >> } >> >> -/* Returns true if the block corresponding to old should be placed >> - before the block corresponding to new in the final blockvector. */ >> - >> -static int >> -compare_block (const struct gdb_block *const old, >> - const struct gdb_block *const newobj) >> -{ >> - if (old == NULL) >> - return 1; >> - if (old->begin < newobj->begin) >> - return 1; >> - else if (old->begin == newobj->begin) >> - { >> - if (old->end > newobj->end) >> - return 1; >> - else >> - return 0; >> - } >> - else >> - return 0; >> -} >> - >> /* Called by readers to open a new gdb_block. This function also >> inserts the new gdb_block in the correct place in the corresponding >> gdb_symtab. */ >> @@ -570,37 +537,15 @@ jit_block_open_impl (struct gdb_symbol_callbacks *cb, >> { >> struct gdb_block *block = XCNEW (struct gdb_block); >> >> - block->next = symtab->blocks; >> block->begin = (CORE_ADDR) begin; >> block->end = (CORE_ADDR) end; >> block->name = name ? xstrdup (name) : NULL; >> block->parent = parent; >> >> - /* Ensure that the blocks are inserted in the correct (reverse of >> - the order expected by blockvector). */ >> - if (compare_block (symtab->blocks, block)) >> - { >> - symtab->blocks = block; >> - } >> - else >> - { >> - struct gdb_block *i = symtab->blocks; >> - >> - for (;; i = i->next) >> - { >> - /* Guaranteed to terminate, since compare_block (NULL, _) >> - returns 1. */ >> - if (compare_block (i->next, block)) >> - { >> - block->next = i->next; >> - i->next = block; >> - break; >> - } >> - } >> - } >> - symtab->nblocks++; >> - >> - return block; >> + /* Place the block at the end of the vector, it will be sorted when the >> + symtab is finalized. */ >> + symtab->blocks.push_back (block); >> + return symtab->blocks.back (); >> } >> >> /* Readers call this to add a line mapping (from PC to line number) to >> @@ -646,14 +591,21 @@ static void >> finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile) >> { >> struct compunit_symtab *cust; >> - struct gdb_block *gdb_block_iter; >> - struct block *block_iter; >> - int actual_nblocks, i; >> size_t blockvector_size; >> CORE_ADDR begin, end; >> struct blockvector *bv; >> >> - actual_nblocks = FIRST_LOCAL_BLOCK + stab->nblocks; >> + int actual_nblocks = FIRST_LOCAL_BLOCK + stab->blocks.size (); >> + >> + /* Sort the blocks in the order they should appear in the blockvector. >> */ >> + std::sort (stab->blocks.begin (), stab->blocks.end (), >> + [] (const gdb_block *a, const gdb_block *b) >> + { >> + if (a->begin != b->begin) >> + return a->begin < b->begin; >> + >> + return b->begin < a->begin; >> > > This doesn't look right? Should this look at end or something? Oh my, indeed, thank you so much for spotting this. I meant this, of course: std::sort (stab->blocks.begin (), stab->blocks.end (), [] (const gdb_block *a, const gdb_block *b) { if (a->begin != b->begin) return a->begin < b->begin; return b->end < a->end; }); Or do you find it more readable this way below instead? It's a bit subtle that "a" and "b" are reversed, otherwise std::sort (stab->blocks.begin (), stab->blocks.end (), [] (const gdb_block *a, const gdb_block *b) { if (a->begin != b->begin) return a->begin < b->begin; return a->end > b->end; }); Simon