From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24326 invoked by alias); 13 Dec 2019 16:08:28 -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 24310 invoked by uid 89); 13 Dec 2019 16:08:28 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-29.9 required=5.0 tests=AWL,BAYES_00,ENV_AND_HDR_SPF_MATCH,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,HTML_MESSAGE,RCVD_IN_DNSWL_NONE,SPF_PASS,USER_IN_DEF_SPF_WL autolearn=ham version=3.3.1 spammy=H*c:alternative X-HELO: mail-ot1-f65.google.com Received: from mail-ot1-f65.google.com (HELO mail-ot1-f65.google.com) (209.85.210.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 13 Dec 2019 16:08:25 +0000 Received: by mail-ot1-f65.google.com with SMTP id k14so7035035otn.4 for ; Fri, 13 Dec 2019 08:08:25 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=HHGth3033n1BCjTLfvw5PJewcARBHCSovZ7uEUrZHt4=; b=PrUOhDfk7mb7w3PFsXkj1qRPCXqiiZAyByF4OQXEVMz3JqP0m9a42Stabo7w1VHbcs KxRjEAp4Twx8UpvpgUig/Xp4TlhauJ1QDeC0PSnJXdO3jtR9Tc41Z3TR56McA9g3eCAU +XeeD8RSxhJQkrxeS6MzZQs+oHVn3YrLv0y3ZF8aQp4BbCloKhSlSdMu6V6DtFskwFQW laE17tXyhaftMEE8krAz8cgE9NQjs1xtmmby5hiY1VHIUjMn8aadFolVUQ0R/e4flm+a ZLwh3hcg0QDZj34ib69hsxoRvTF/Ed2v2FevDR2kve0bDg3F/1jc3Z3KWxTjlTzJkbZP a70A== MIME-Version: 1.0 References: <20191213060323.1799590-1-simon.marchi@polymtl.ca> <20191213060323.1799590-5-simon.marchi@polymtl.ca> <4eb506cf-b407-82b8-8f5d-e2c0481c431f@polymtl.ca> In-Reply-To: <4eb506cf-b407-82b8-8f5d-e2c0481c431f@polymtl.ca> From: "Christian Biesinger via gdb-patches" Reply-To: Christian Biesinger Date: Fri, 13 Dec 2019 16:08:00 -0000 Message-ID: Subject: Re: [PATCH 4/7] jit: make gdb_symtab::blocks a vector To: Simon Marchi Cc: gdb-patches Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2019-12/txt/msg00600.txt.bz2 On Fri, Dec 13, 2019, 11:02 Simon Marchi wrote: > 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; > }); > I personally prefer this one. And maybe add a comment that the block that encloses the other one should come first? > Simon >