Currently, nothing in the tree ever calls mdict_free. However, code
does heap-allocate some multidicts. A simple way to see this is to
use valgrind, run "gdb -readnow" on the executable created by
gdb.dwarf2/struct-with-sig.exp, and then use "file" to clear the
objfile list. This yields:
==1522843== 144 (16 direct, 128 indirect) bytes in 1 blocks are definitely lost in loss record 905 of 3,005
==1522843== at 0x4843866: malloc (vg_replace_malloc.c:446)
==1522843== by 0x48E397: xmalloc (alloc.c:52)
==1522843== by 0x59DE66: multidictionary* xnew<multidictionary>() (poison.h:102)
==1522843== by 0x59CFF4: mdict_create_hashed_expandable(language) (dictionary.c:965)
==1522843== by 0x50A269: buildsym_compunit::finish_block_internal(symbol*, pending**, pending_block*, dynamic_prop const*, unsigned long, unsigned long, int, int) (buildsym.c:221)
==1522843== by 0x50AE04: buildsym_compunit::end_compunit_symtab_get_static_block(unsigned long, int, int) (buildsym.c:818)
==1522843== by 0x50C4CF: buildsym_compunit::end_expandable_symtab(unsigned long) (buildsym.c:1037)
==1522843== by 0x61DBC6: process_full_type_unit (read.c:4970)
This patch fixes the leaks by calling mdict_free when a blockvector is
destroyed.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
This commit adds blockvector::block_less_than() predicate that defines
required ordering of blocks within blockvector.
It orders blocks so that blocks with lower start address come before
blocks with higher start address. If two blocks start at the same
address, enclosing (larger) block should come before nested (smaller)
block.
This ordering is depended upon in find_block_in_blockvector(). Although
its comment did not say so, find_block_in_blockvector() is called from
blockvector_for_pc_sect() which is explicit about it. While at it, I
changed the comment of find_block_in_blockvector() to say so explicitly
too.
As Andrew pointed out, buildsym.c sorts block slightly differently,
taking only the start address into account. The comment there says
blocks with same start address should not be reordered as they are in
correct order already and that order is needed. It is unclear to me
if buildsym.c arranges blocks starting at the same address in required
order before sorting them or this happens "by chance". I did modify
buildsym_compunit::make_blockvector() to assert blocks are properly
ordered and running testsuite did not show any regressions.
Approved-By: Tom Tromey <tom@tromey.com>
This patch changes blockvector to be allocated on the heap (using 'new')
and changes internal implementation to use std::vector<> rather than
flexible array to add blocks to existing blockvector. This is needed for
lazy CU expansion and for Python API to build objfiles, compunits and
symtabs dynamically (similarly to JIT reader API).
The downside is higher memory consumption. The size of std::vector is
24 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 16*4593 = 73488 bytes which I hope is acceptable.
While at it, add blockvector::append_block() to add more block at the
end of block vector. This is currently used only in mdebugread.c.
Approved-By: Tom Tromey <tom@tromey.com>
There are two ways to build an iterator_range:
- Using the variadic constructor, where the arguments you pass are used
to construct the "begin" underlying iterator. The "end" iterator is
obtained by default-constructing the underlying iterator.
- Using the other constructor, by explicitly providing the "begin" and
"end" iterators.
My experience is that using the variadic constructor is very confusing,
especially when you have multiple layers of iterator wrappers. It's not
obvious where the arguments you provide end up. When you have a
compilation error, it is hard to decipher.
I propose to remove the variadicity of the first constructor of
iterator_range, and subsequently of the other iterator wrappers. This
requires callers to be more verbose, explicitly instantiate all the
layers. But since we only instantiate these iterator wrappers in
factory functions, I think it's fine. If there is a compilation error,
it will be much easier to find and fix the problem.
Using the new one-argument constructor, it is still assumed that the end
iterator can be obtained by default-constructing the underlying iterator
type, which I think is fine and not too confusing.
Change-Id: I54d6fdef18bcd7e308825064e0fc18fadd7ca717
Approved-By: Tom Tromey <tom@tromey.com>
This adds a new best_symbol_tracker struct. This is used to implement
the "best symbol" logic that is used sometimes in symtab.c. This
approach makes it simpler and more efficient to track the "best"
symbol when searching across multiple blocks.
Acked-By: Simon Marchi <simon.marchi@efficios.com>
This changes block_lookup_symbol_primary to accept a lookup_name_info.
This follows the general trend of hoisting these objects to the
outermost layer where they make sense -- somewhat reducing the cost of
using them.
Acked-By: Simon Marchi <simon.marchi@efficios.com>
This updates the copyright headers to include 2025. I did this by
running gdb/copyright.py and then manually modifying a few files as
noted by the script.
Approved-By: Eli Zaretskii <eliz@gnu.org>
This patch is the result of running check-include-guards.py on the
current tree. Running it a second time causes no changes.
Reviewed-By: Tom de Vries <tdevries@suse.de>
The entry PC for a DIE, e.g. an inline function, might not be the base
address of the DIE. Currently though, in block::entry_pc(), GDB
always returns the base address (low-pc or the first address of the
first range) as the entry PC.
This commit extends the block class to carry the entry PC as a
separate member variable. Then the DWARF reader is extended to read
and set the entry PC for the block. Now in block::entry_pc(), if the
entry PC has been set, this is the value returned.
If the entry-pc has not been set to a specific value then the old
behaviour of block::entry_pc() remains, GDB will use the block's base
address. Not every DIE will set the entry-pc, but GDB still needs to
have an entry-pc for every block, so the existing logic supplies the
entry-pc for any block where the entry-pc was not set.
The DWARF-5 spec for reading the entry PC is a super-set of the spec
as found in DWARF-4. For example, if there is no DW_AT_entry_pc then
DWARF-4 says to use DW_AT_low_pc while DWARF-5 says to use the base
address, which is DW_AT_low_pc or the first address in the first range
specified by DW_AT_ranges if there is no DW_AT_low_pc.
I have taken the approach of just implementing the DWARF-5 spec for
everyone. There doesn't seem to be any benefit to deliberately
ignoring a ranges based entry PC value for DWARF-4. If some naughty
compiler has emitted that, then lets use it.
Similarly, DWARF-4 says that DW_AT_entry_pc is an address. DWARF-5
allows an address or a constant, where the constant is an offset from
the base address. I allow both approaches for all DWARF versions.
There doesn't seem to be any downsides to this approach.
I ran into an issue when testing this patch where GCC would have the
DW_AT_entry_pc point to an empty range. When GDB parses the ranges
any empty ranges are ignored. As a consequence, the entry-pc appears
to be outside the address range of a block.
The empty range problem is certainly something that we can, and should
address, but that is not the focus of this patch, so for now I'm
ignoring that problem. What I have done is added a check: if the
DW_AT_entry_pc is outside the range of a block then the entry-pc is
ignored, GDB will then fall-back to its default algorithm for
computing the entry-pc.
If/when in the future we address the empty range problem, these
DW_AT_entry_pc attributes will suddenly become valid and GDB will
start using them. Until then, GDB continues to operate as it always
has.
An early version of this patch stored the entry-pc within the block
like this:
std::optional<CORE_ADDR> m_entry_pc;
However, a concern was raised that this, on a 64-bit host, effectively
increases the size of block by 16-bytes (8-bytes for the CORE_ADDR,
and 8-bytes for the std::optional's bool plus padding).
If we remove the std::optional part and just use a CORE_ADDR then we
need to have a "special" address to indicate if m_entry_pc is in use
or not. I don't really like using special addresses; different
targets can access different address ranges, even zero is a valid
address on some targets.
However, Bernd Edlinger suggested storing the entry-pc as an offset,
and I think that will resolve my concerns. So, we store the entry-pc
as a signed offset from the block's base address (the first address of
the first range, or the start() address value if there are now
ranges). Remember, ranges can be out of order, in which case the
first address of the first range might be greater than the entry-pc.
When GDB needs to read the entry-pc we can add the offset onto the
blocks base address to recalculate it.
With this done, on a 64-bit host, block only needs to increase by
8-bytes.
The inline-entry.exp test was originally contributed by Bernd here:
https://inbox.sourceware.org/gdb-patches/AS1PR01MB94659E4D9B3F4A6006CC605FE4922@AS1PR01MB9465.eurprd01.prod.exchangelabs.com
though I have made some edits, making more use of lib/gdb.exp
functions, making the gdb_test output patterns a little tighter, and
updating the test to run with Clang. I also moved the test to
gdb.opt/ as that seemed like a better home for it.
Co-Authored-By: Bernd Edlinger <bernd.edlinger@hotmail.de>
This patch changes block::get_using to return an iterator range. This
seemed cleaner to me than the current approach of returning a pointer
to the first using directive; all the callers actually use this to
iterate.
Some refactors around struct global_block, all in one patch because they
all tie in together and are relatively trivial.
- Make block::global_block() and blockvector::global_block() return
`global_block *`, instead of `block *`. There is no cost in doing
so, and it's a bit more precise. Callers of these methods that need
a `global_block *` won't need to cast themselves.
- Add some block::as_global_block methods, as a way to get a
`global_block *` from a `block *` when you know it's a global block.
This is basically a static cast with an assert.
- Move set_compunit_symtab to global_block, since it requires the
block to be a global block anyway. Rename to just `set_compunit` (I
think that compunit_symtab should just be renamed compunit...).
- Move the get_block_compunit_symtab free function to be a method of
global_block.
- Make global_block::compunit_symtab private and rename.
- Simplify initialize_block_iterator.
Change-Id: I1667a86b5c1a02d0d460cfad55b5d3d48867583d
Approved-By: Tom Tromey <tom@tromey.com>
This patch makes allocate_on_obstack a little bit safer, by enforcing
the rule that objects allocated on an obstack must have a trivial
destructor.
The static assert is done in a method -- doing it inside the class
itself won't work because the class is incomplete at that point.
There are a few spots in the tree that use 'addrmap' where only an
addrmap_fixed will ever really be seen. This patch changes this code
to use the more specific type.
I noticed that basic_lookup_transparent_type calls two different
functions that both proceed to create a lookup_name_info. It's more
efficient to create this object in the outermost layer possible.
Making this change required a few related changes, resulting in this
patch.
There are still more changes of this sort that could be made.
Regression tested on x86-64 Fedora 38.
This changes lookup_symbol and associated APIs to accept
domain_search_flags rather than a domain_enum.
Note that this introduces some new constants to Python and Guile. I
chose to break out the documentation patch for this, because the
internals here do not change until a later patch, and it seemed
simpler to patch the docs just once, rather than twice.
This commit is the result of the following actions:
- Running gdb/copyright.py to update all of the copyright headers to
include 2024,
- Manually updating a few files the copyright.py script told me to
update, these files had copyright headers embedded within the
file,
- Regenerating gdbsupport/Makefile.in to refresh it's copyright
date,
- Using grep to find other files that still mentioned 2023. If
these files were updated last year from 2022 to 2023 then I've
updated them this year to 2024.
I'm sure I've probably missed some dates. Feel free to fix them up as
you spot them.
block_find_symbol takes a callback function, but only two callbacks
are ever passed to it -- and they are similar enough that it seems
cleaner to just have block_find_symbol do the work itself. Also,
block_find_symbol can take a lookup_name_info as an argument,
following the general idea of pushing the construction of these
objects as high in the call chain as feasible.
Regression tested on x86-64 Fedora 38.
Tested-By: Alexandra Petlanova Hajkova <ahajkova@redhat.com>
This changes the block_iterator to store the 'name' that is used by
block_iter_match_next. This avoids any problem where the name could
be passed inconsistently, and also makes the subsequent patches easier
to understand.
This converts block_static_block and block_global_block to be methods.
This was mostly written by script. It was simpler to convert them at
the same time because they're often used near each other.
This converts block_scope, block_set_scope, block_using, and
block_set_using to be methods. These are all done at once to make it
easier to also convert block_initialize_namespace at the same time.
This was mostly written by script.
This commit is the result of running the gdb/copyright.py script,
which automated the update of the copyright year range for all
source files managed by the GDB project to be updated to include
year 2023.
This commit:
commit f5cb8afdd2
Date: Sun Feb 6 22:27:53 2022 -0500
gdb: remove BLOCK_RANGES macro
introduces a potential nullptr dereference in block::ranges, this is
breaking most tests, e.g. gdb.base/break.exp is failing for me.
In the above patch BLOCK_CONTIGUOUS_P is changed from this:
#define BLOCK_CONTIGUOUS_P(bl) (BLOCK_RANGES (bl) == nullptr \
|| BLOCK_NRANGES (bl) <= 1)
to this:
#define BLOCK_CONTIGUOUS_P(bl) ((bl)->ranges ().size () == 0 \
|| (bl)->ranges ().size () == 1)
So, before the commit we checked for the block ranges being nullptr,
but afterwards we just call block::ranges() in all cases.
The problem is that block::ranges() looks like this:
/* Return a view on this block's ranges. */
gdb::array_view<blockrange> ranges ()
{ return gdb::make_array_view (m_ranges->range, m_ranges->nranges); }
where m_ranges is:
struct blockranges *m_ranges;
And so, we see that the nullptr check has been lost, and we might end
up dereferencing a nullptr.
My proposed fix is to move the nullptr check into block::ranges, and
return an explicit empty array_view if m_ranges is nullptr.
After this, everything seems fine again.
Replace with calls to blockvector::blocks, and the appropriate method
call on the returned array_view.
Change-Id: I04d1f39603e4d4c21c96822421431d9a029d8ddd
