From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id fAwVJL5l7miCwzEAWB0awg (envelope-from ) for ; Tue, 14 Oct 2025 11:01:18 -0400 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=IXnF8saR; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 866FD1E0B6; Tue, 14 Oct 2025 11:01:18 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-3.4 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_VALIDITY_CERTIFIED_BLOCKED, RCVD_IN_VALIDITY_RPBL_BLOCKED,RCVD_IN_VALIDITY_SAFE_BLOCKED autolearn=ham autolearn_force=no version=4.0.1 Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange x25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 944F91E047 for ; Tue, 14 Oct 2025 11:01:16 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 533CC385625D for ; Tue, 14 Oct 2025 15:01:16 +0000 (GMT) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTP id 37CA43858D38 for ; Tue, 14 Oct 2025 15:00:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 37CA43858D38 Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 37CA43858D38 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1760454050; cv=none; b=QeMrdMzCMTxjHKhwwYs6nNO0LivAyoHx1MZfsIx46kzUCrK83iiPZR+w9GVCdqDp5xzzDp3m7wCeyyWQPKkaN1XuejmInFPTDaNalv9b7ME/JLeolXrZPNadsYrVgHWuvQRdawI+0iUSA621mxKTU+Aqf+r5YKSL5w1uJyOcEME= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1760454050; c=relaxed/simple; bh=WtFOIhtEjpesDvCWzE9pTlq/SwC9U5oZMVJ+MCgzUj0=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=scjHdHBT8iYo9u2EDiLcFUI5bAA9OCziwET5wxiRLkNTgHAKOpjCGpkP7VWBix4nj8NPfvojXrTrzZiyhrM4fUN0sq08PzCZIjjk3IwHyiXa9YTDfm6DL1Wg8tcCT4JLRQKd9vOvfPNUDPYd49yBaU4EMRXPNUbadD6DaWwY3NA= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 37CA43858D38 Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=IXnF8saR DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1760454049; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Vqm8dNr84TT6xh5vz4hme9tm4rxH/ismGCiWQW+LLuY=; b=IXnF8saRpThzydk0Ek25p7Rgpp8jmZlkkb7cXFZW91eh4VemCy9Y7XivlA6fjjDoPLeRz9 qNo1aIocrnY3BQ6cNa03wDVWLPnz+89p6TJj19K4k+bxMdWYnXrt7TwLHClrjdie4EuCty yAFOgLq3Wx94arOUBAAh5lPzDa8j7zQ= Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-323-6YmUguX3M_yoi16FnOKQwQ-1; Tue, 14 Oct 2025 11:00:48 -0400 X-MC-Unique: 6YmUguX3M_yoi16FnOKQwQ-1 X-Mimecast-MFC-AGG-ID: 6YmUguX3M_yoi16FnOKQwQ_1760454047 Received: by mail-wm1-f70.google.com with SMTP id 5b1f17b1804b1-46e32eb4798so28254345e9.2 for ; Tue, 14 Oct 2025 08:00:48 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1760454047; x=1761058847; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=Vqm8dNr84TT6xh5vz4hme9tm4rxH/ismGCiWQW+LLuY=; b=CJRD9bDWklZNH8WRWaLESwB4hOZ+ciF1mj5TezP3gmF10ErVzIXPpYWA2ziwn9wNLH X5Jt7m+cegvBj7ZYMxH+a8WGaN1aX1hx0LED2EeDOO3S+lHG/ZST3osfZw+atCv7imEO 0vZtCdRg2xinnhSQx4teMPv/GRzK+GtvU4jLEEiWwo2DAPviAyDKOdT6nL5OW+LnPAJB wcWJTwj23BFoEbafnDqiBQuSJMFnZ0Z0rdis9X63uf6D48w7jFm2L6Uwkrouxj18iKlQ 1KCzRPEurtIMz+NmbBuApmEvWcp/rK2uu3joU4YXeX2trxqNtYGuzzys/L/VI+xxikrk Gl9A== X-Forwarded-Encrypted: i=1; AJvYcCURx2rfmp3uZrP9UJNqszp6gSLfMN4B8OH3VCBaxoePvmFMDMPaCRTXoTRwzviZ7ztYDRW/w0VgJRc3vQ==@sourceware.org X-Gm-Message-State: AOJu0YyWLdb5laltc3VUrWPBt9g39jJnaWtcl+1fMDP0GizQgRLw/IPW sVXiw6f+oGSfJyu9CDCQtYBJpfhBPh33lH2DsSZFWSp42p87LRYinq08gQj2y8nvVZSg1Y8LGex Uziqqp17cM4byvCrYdt43+zTlWQuMEQkFk2R6sDYpyfq3xR3p25+4LlqBHLZwbac= X-Gm-Gg: ASbGnctBgovpgB1eJ1BCI7Zv0QjJNT8OEGo49b5LFNsoGwmpuF6tyFCQTZ6FAAPANPQ Qyp1YPpKF1mw4s1BmwXJchb2DqUKcZzePAzwOyRhthrTayeu6IPJbeQ+3cbmzokFtsjdui301V8 TH8TlTt5CDEYdOEHyl3+1O6EbNaz0jU2Q57ukosqOSk/GPbTVFS5y6Asbeiur0dk6YcVsmdTRaM qemZp1TkNl7aJMM7VFtgw8xy4EX6DvJgJP6SRshUCkQLFCh1ErPL02eCYnagZAz8AkDL8rJ+KET BE0kU8vV89ApujVTl1QLNLpva+YinygKXI4= X-Received: by 2002:a05:600c:a148:b0:46f:c55a:5a8c with SMTP id 5b1f17b1804b1-46fc55a5bbamr54920145e9.4.1760454046834; Tue, 14 Oct 2025 08:00:46 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFrMZEutLDe43pK3ipIIysbkNhFBEzu6wcoQ8J0A2Mw6dXETC/rpPnpVBqxJh2/nhqtmS/vJA== X-Received: by 2002:a05:600c:a148:b0:46f:c55a:5a8c with SMTP id 5b1f17b1804b1-46fc55a5bbamr54919515e9.4.1760454045800; Tue, 14 Oct 2025 08:00:45 -0700 (PDT) Received: from localhost ([31.111.84.207]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-426ce5cf6b4sm24172570f8f.25.2025.10.14.08.00.45 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 14 Oct 2025 08:00:45 -0700 (PDT) From: Andrew Burgess To: Jan Vrany , gdb-patches@sourceware.org Cc: Jan Vrany Subject: Re: [PATCH 2/2] gdb: use std::vector<> to hold on blocks in struct blockvector In-Reply-To: <20251013182318.1045138-3-jan.vrany@labware.com> References: <20251013182318.1045138-1-jan.vrany@labware.com> <20251013182318.1045138-3-jan.vrany@labware.com> Date: Tue, 14 Oct 2025 16:00:43 +0100 Message-ID: <87v7kha5pg.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: N_CBoTWsZLa3wC7LX62t4YmEp0k05sEbZSNBajj8ZQg_1760454047 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org Hi Jan, Thanks for working on this. I think that this commit should be merged with the previous one. For me at least, I'm unable to build the previous commit as I get compiler errors about malloc/free being poisoned. Not sure why this didn't impact you, but I didn't look too closely once I realised patch #2 removes those calls. Given the overlap of the patches, I think the easiest solution would be to just merge the two. Jan Vrany writes: > This commit changes internal implementation of struct blockvector to use > std::vector<> rather than flexible array, allowing one to easily add > blocks to existing blockvector. > > This is needed to for lazy CU expansion and for Python API to build > objfiles, compunits and symtabs dynamically (similarly to JIT reader > API). > > To avoid extensive reallocation when adding a lot of blocks, ctor takes > initial number of blocks to be stored in blockvector. > > While at is, it also adds blockvector::add_block() to add new blocks > and changes mdebugread.c to use it. > > The downside is higher memory consumption. The size of std::vector with > obstack allocator is 32 bytes (GCC 14) compared to 8 bytes used > currently to store the number of blocks (m_num_blocks). Stopping gdb at > its main(), followed by "maint expand-symtabs" results in 4593 > compunit symtabs so in this case the overhead is 24*4593 = 110232 bytes > which I hope is acceptable. > --- > gdb/block.c | 36 +++++++++++++++++++++++++++++ > gdb/block.h | 59 +++++++++++++++++++++++------------------------- > gdb/buildsym.c | 2 +- > gdb/jit.c | 2 +- > gdb/mdebugread.c | 30 +++--------------------- > 5 files changed, 69 insertions(+), 60 deletions(-) > > diff --git a/gdb/block.c b/gdb/block.c > index e09bccdcf86..a9191c3fdff 100644 > --- a/gdb/block.c > +++ b/gdb/block.c > @@ -17,6 +17,7 @@ > You should have received a copy of the GNU General Public License > along with this program. If not, see . */ > > +#include I couldn't see why this should be needed. > #include "block.h" > #include "symtab.h" > #include "symfile.h" > @@ -809,6 +810,41 @@ make_blockranges (struct objfile *objfile, > return blr; > } > > +static bool > +block_ordering_predicate (struct block *b1, struct block *b2) Functions should have a comment above them. > +{ > + CORE_ADDR start1 = b1->start (); > + CORE_ADDR start2 = b2->start (); > + > + if (start1 != start2) > + return start1 < start2; > + return (b2->end () < b1->end ()); > +} I notice that we use a slightly different ordering function in buildsym.c, one that doesn't consider the end address. Probably doesn't matter, but maybe it's at least worth mentioning in a comment or the commit message, that way, if someone is looking at the code in the future they can understand why these things are different. Even if the reason is just "we have no motivating example for why these should be the same right now". > + > +/* See block.h. */ > + > +void > +blockvector::add_block (struct block *block) > +{ > + if (num_blocks () <= FIRST_LOCAL_BLOCK) > + { > + /* No blocks (except global and static block). */ > + m_blocks.push_back (block); > + } > + else > + { > + /* blockvector already contains some blocks. Insert new block > + to a correct place. */ The indentation of the second line is wrong here. > + auto first = m_blocks.begin () + FIRST_LOCAL_BLOCK; > + auto last = m_blocks.end (); > + > + auto insert_before = std::upper_bound (first, last, block, > + block_ordering_predicate); > + > + m_blocks.insert (insert_before, block); > + } > +} > + > /* Implement 'maint info blocks' command. If passed an argument then > print a list of all blocks at the given address. With no arguments > then list all blocks at the current address of the current inferior. */ > diff --git a/gdb/block.h b/gdb/block.h > index d1ca41d7482..c86650753a4 100644 > --- a/gdb/block.h > +++ b/gdb/block.h > @@ -20,6 +20,7 @@ > #ifndef GDB_BLOCK_H > #define GDB_BLOCK_H > > +#include > #include "dictionary.h" > #include "gdbsupport/array-view.h" > #include "gdbsupport/next-iterator.h" > @@ -420,59 +421,58 @@ struct global_block : public block > > struct blockvector > { > - void *operator new (size_t size, size_t num_blocks) > - { > - size_t size_in_bytes = size + (num_blocks - 1) * (sizeof (struct block *)); > - return std::malloc (size_in_bytes); > - } > - > - void operator delete (void *memory) > - { > - std::free (memory); > - } > - > blockvector (int nblocks) > : m_map (nullptr), > - m_num_blocks (nblocks) > - { > - std::memset (m_blocks, 0, nblocks * sizeof (struct block *)); > - } > + m_blocks (nblocks, nullptr) > + {} > > /* Return a view on the blocks of this blockvector. */ > gdb::array_view blocks () > { > - return gdb::array_view (m_blocks, m_num_blocks); > + return gdb::array_view (m_blocks.data (), > + m_blocks.size ()); > } > > /* Const version of the above. */ > gdb::array_view blocks () const > { > - const struct block **blocks = (const struct block **) m_blocks; > - return gdb::array_view (blocks, m_num_blocks); > + const struct block **blocks = (const struct block **) m_blocks.data (); > + return gdb::array_view (blocks, > + m_blocks.size ()); > } > > /* Return the block at index I. */ > struct block *block (size_t i) > - { return this->blocks ()[i]; } > + { > + return m_blocks[i]; > + } > > /* Const version of the above. */ > const struct block *block (size_t i) const > - { return this->blocks ()[i]; } > + { > + return m_blocks[i]; > + } > > /* Set the block at index I. */ > void set_block (int i, struct block *block) > { m_blocks[i] = block; } > > - /* Set the number of blocks of this blockvector. > - > - The storage of blocks is done using a flexible array member, so the number > - of blocks set here must agree with what was effectively allocated. */ > - void set_num_blocks (int num_blocks) > - { m_num_blocks = num_blocks; } > + /* Add BLOCK, making sure blocks are ordered by code-addresses > + as required. Update global and static block start and end > + adresses accordingly. */ > + void add_block (struct block *block); Maybe this comment needs tweaking. In the general case the global and/or static blocks are not updated, but I can see why you would consider them as updated if we started from an empty block list. But I think expanding this comment would make things clearer. > > /* Return the number of blocks in this blockvector. */ > int num_blocks () const > - { return m_num_blocks; } > + { > + return m_blocks.size (); > + } > + > + /* Set the number of blocks of this blockvector. */ > + void set_num_blocks (int num_blocks) > + { > + m_blocks.resize (num_blocks, nullptr); > + } > > /* Return the global block of this blockvector. */ > struct global_block *global_block () > @@ -511,11 +511,8 @@ struct blockvector > enough. */ > addrmap_fixed *m_map; > > - /* Number of blocks in the list. */ > - int m_num_blocks; > - > /* The blocks themselves. */ > - struct block *m_blocks[1]; > + std::vector m_blocks; > }; > > extern const struct blockvector *blockvector_for_pc (CORE_ADDR, > diff --git a/gdb/buildsym.c b/gdb/buildsym.c > index a61ef68ada1..8daa68d296b 100644 > --- a/gdb/buildsym.c > +++ b/gdb/buildsym.c > @@ -427,7 +427,7 @@ buildsym_compunit::make_blockvector () > { > } > > - blockvector = new (i) struct blockvector (i); > + blockvector = new struct blockvector (i); > > /* Copy the blocks into the blockvector. This is done in reverse > order, which happens to put the blocks into the proper order > diff --git a/gdb/jit.c b/gdb/jit.c > index d95fe542a29..4e3abd1802c 100644 > --- a/gdb/jit.c > +++ b/gdb/jit.c > @@ -553,7 +553,7 @@ finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile) > filetab->set_linetable (new_table); > } > > - bv = new (actual_nblocks) blockvector (actual_nblocks); > + bv = new blockvector (actual_nblocks); > cust->set_blockvector (bv); > > /* At the end of this function, (begin, end) will contain the PC range this > diff --git a/gdb/mdebugread.c b/gdb/mdebugread.c > index 3cedf0e4fc2..90bbcd81ed2 100644 > --- a/gdb/mdebugread.c > +++ b/gdb/mdebugread.c > @@ -256,8 +256,6 @@ static legacy_psymtab *new_psymtab (const char *, psymtab_storage *, > static void mdebug_expand_psymtab (legacy_psymtab *pst, > struct objfile *objfile); > > -static void add_block (struct block *, struct symtab *); > - > static void add_symbol (struct symbol *, struct symtab *, struct block *); > > static int add_line (struct linetable *, int, CORE_ADDR, int); > @@ -808,7 +806,7 @@ parse_symbol (SYMR *sh, union aux_ext *ax, char *ext_sh, int bigend, > b->set_start (sh->value); > b->set_end (sh->value); > b->set_superblock (top_stack->cur_block); > - add_block (b, top_stack->cur_st); > + top_stack->cur_st->compunit ()->blockvector ()->add_block (b); I don't see it being called out anywhere that this change is now going to reorder the blocks. I have no idea if this change is important or not, but I think it is usually a good idea to talk about this sort thing in the commit message -- imagine in a few months someone hits a bug in ECOFF support related to block ordering, they look at this commit, and they have to figure out why the block ordering changed.... Thanks, Andrew > > /* Not if we only have partial info. */ > if (SC_IS_UNDEF (sh->sc) || sh->sc == scNil) > @@ -1136,7 +1134,7 @@ parse_symbol (SYMR *sh, union aux_ext *ax, char *ext_sh, int bigend, > b->set_start (sh->value + top_stack->procadr); > b->set_superblock (top_stack->cur_block); > top_stack->cur_block = b; > - add_block (b, top_stack->cur_st); > + top_stack->cur_st->compunit ()->blockvector ()->add_block (b); > break; > > case stEnd: /* end (of anything) */ > @@ -4492,28 +4490,6 @@ add_symbol (struct symbol *s, struct symtab *symtab, struct block *b) > mdict_add_symbol (b->multidict (), s); > } > > -/* Add a new block B to a symtab S. */ > - > -static void > -add_block (struct block *b, struct symtab *s) > -{ > - /* Cast away "const", but that's ok because we're building the > - symtab and blockvector here. */ > - struct blockvector *old_bv > - = (struct blockvector *) s->compunit ()->blockvector (); > - > - struct blockvector *new_bv = new (old_bv->num_blocks () + 1) > - blockvector (old_bv->num_blocks () + 1); > - > - s->compunit ()->set_blockvector (old_bv); > - > - for (int i = 0; i < old_bv->num_blocks (); i++) > - new_bv->set_block (i, old_bv->block (i)); > - new_bv->set_block (new_bv->num_blocks () - 1, b); > - > - delete old_bv; > -} > - > /* Add a new linenumber entry (LINENO,ADR) to a linevector LT. > MIPS' linenumber encoding might need more than one byte > to describe it, LAST is used to detect these continuation lines. > @@ -4635,7 +4611,7 @@ new_symtab (const char *name, int maxlines, struct objfile *objfile) > lang = cust->language (); > > /* All symtabs must have at least two blocks. */ > - bv = new (2) blockvector (2); > + bv = new blockvector (2); > bv->set_block (GLOBAL_BLOCK, new_block (objfile, NON_FUNCTION_BLOCK, lang)); > bv->set_block (STATIC_BLOCK, new_block (objfile, NON_FUNCTION_BLOCK, lang)); > bv->static_block ()->set_superblock (bv->global_block ()); > -- > 2.51.0