Unfortunately the background DWARF reading series introduced a number
of races, as repored by thread sanitizer. This patch changes gdb to
disable this feature for the time being -- in particular for the gdb
15 release.
I've filed a bug and linked all the known races to it. Once those are
fixed we can re-enable this feature by default.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31751
The added check fixes the case when an unqualified lookup
name without template arguments causes expansion of many CUs
which contain the name with template arguments.
This is similar to what dw2_expand_symtabs_matching_symbol does
before expanding the CU.
In the referenced issue the lookup name was wxObjectDataPtr and many
CUs had names like wxObjectDataPtr<wxBitmapBundleImpl>. This caused
their expansion and the lookup took around a minute. The added check
helps to avoid the expansion and makes the symbol lookup to return in
a second or so.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30520
If threads are disabled, either by --disable-threading explicitely, or by
missing std::thread support, you get the following ASAN error when
loading symbols:
==7310==ERROR: AddressSanitizer: heap-use-after-free on address 0x614000002128 at pc 0x00000098794a bp 0x7ffe37e6af70 sp 0x7ffe37e6af68
READ of size 1 at 0x614000002128 thread T0
#0 0x987949 in index_cache_store_context::store() const ../../gdb/dwarf2/index-cache.c:163
#1 0x943467 in cooked_index_worker::write_to_cache(cooked_index const*, deferred_warnings*) const ../../gdb/dwarf2/cooked-index.c:601
#2 0x1705e39 in std::function<void ()>::operator()() const /gcc/9/include/c++/9.2.0/bits/std_function.h:690
#3 0x1705e39 in gdb::task_group::impl::~impl() ../../gdbsupport/task-group.cc:38
0x614000002128 is located 232 bytes inside of 408-byte region [0x614000002040,0x6140000021d8)
freed by thread T0 here:
#0 0x7fd75ccf8ea5 in operator delete(void*, unsigned long) ../../.././libsanitizer/asan/asan_new_delete.cc:177
#1 0x9462e5 in cooked_index::index_for_writing() ../../gdb/dwarf2/cooked-index.h:689
#2 0x9462e5 in operator() ../../gdb/dwarf2/cooked-index.c:657
#3 0x9462e5 in _M_invoke /gcc/9/include/c++/9.2.0/bits/std_function.h:300
It's happening because cooked_index_worker::wait always returns true in
this case, which tells cooked_index::wait it can delete the m_state
cooked_index_worker member, but cooked_index_worker::write_to_cache tries
to access it immediately afterwards.
Fixed by making cooked_index_worker::wait only return true if desired_state
is CACHE_DONE, same as if threading was enabled, so m_state will not be
prematurely deleted.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31694
Approved-By: Tom Tromey <tom@tromey.com>
Currently, read_attribute_value calls dwarf2_per_objfile::adjust on
any address. This seems wrong, because the address may not even be in
the text section.
Luckily, this call is also not needed, because read_func_scope calls
'relocate', which does the same work.
read_call_site_scope does not need to call 'adjust', because in
general the call site is not a symbol address, but rather just the
address of some particular call.
As with the previous patch, this patch removes some calls to
dwarf2_per_objfile::adjust. These calls are not needed by the cooked
indexer, as it does not create symbols or look up symbols by address.
The call in dwarf2_ranges_read is similarly not needed, as it is only
used to update an addrmap; and in any case I believe this particular
call is only reached by the indexer.
dwarf2_per_objfile::adjust applies gdbarch_adjust_dwarf2_addr to an
address, leaving the result unrelocated. However, this adjustment is
only needed for text-section symbols -- it isn't needed for any sort
of address mapping. Therefore, these calls can be removed from
read_addrmap_from_aranges and create_addrmap_from_gdb_index.
Approved-By: Andrew Burgess <aburgess@redhat.com>
In commit 1d45d90934 ("[gdb/symtab] Work around PR gas/29517") we added a
workaround for PR gas/29517.
The problem is present in gas version 2.39, and fixed in 2.40, so the
workaround is only active for gas version == 2.39.
However, the problem in gas is only fixed for dwarf version >= 3, which
supports DW_TAG_unspecified_type.
Fix this by also activating the workaround for dwarf version == 2.
Tested on x86_64-linux.
Approved-by: Kevin Buettner <kevinb@redhat.com>
PR symtab/31689
https://sourceware.org/bugzilla/show_bug.cgi?id=31689
Most files including gdbcmd.h currently rely on it to access things
actually declared in cli/cli-cmds.h (setlist, showlist, etc). To make
things easy, replace all includes of gdbcmd.h with includes of
cli/cli-cmds.h. This might lead to some unused includes of
cli/cli-cmds.h, but it's harmless, and much faster than going through
the 170 or so files by hand.
Change-Id: I11f884d4d616c12c05f395c98bbc2892950fb00f
Approved-By: Tom Tromey <tom@tromey.com>
Move some declarations related to the "quit" machinery from defs.h to
event-top.h. Most of the definitions associated to these declarations
are in event-top.c. The exceptions are `quit()` and `maybe_quit()`,
that are defined in utils.c. For consistency, move these two
definitions to event-top.c.
Include "event-top.h" in many files that use these things.
Change-Id: I6594f6df9047a9a480e7b9934275d186afb14378
Approved-By: Tom Tromey <tom@tromey.com>
When building with this clang:
$ c++ --version
FreeBSD clang version 16.0.6 (https://github.com/llvm/llvm-project.git llvmorg-16.0.6-0-g7cbf1a259152)
I see:
$ gmake
CXX dwarf2/read.o
/home/smarchi/src/binutils-gdb/gdb/dwarf2/read.c:4890:6: error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move]
std::move (thread_storage.release_parent_map ()));
^
/home/smarchi/src/binutils-gdb/gdb/dwarf2/read.c:4890:6: note: remove std::move call here
std::move (thread_storage.release_parent_map ()));
^~~~~~~~~~~ ~
The compiler seems right, there is not need to std::move the result of
`release_parent_map ()`, it's already going to be an rvalue. Remove the
std::move.
The issue isn't FreeBSD-specific, I see it on Linux as well when
building hwith clang, I just noticed it on a FreeBSD build first.
Change-Id: I7aa20a4db56c799f20d838ad08099a01653bba19
Approved-By: Tom Tromey <tom@tromey.com>
Nothing in defs.h actually uses this.
Add some includes for some spots using things from hashtab.h. Note that
if the GDB build doesn't use libxxhash, hashtab.h is included by
gdbsupport/common-utils.h, so all files still see hashtab.h. It puzzled
me for some time why I didn't see build failures in my build (which
didn't use libxxhash) but the buildbot gave build failures (it uses
libxxhash).
Change-Id: I8efd68decdaf579f048941c7537cd689885caa2a
Approved-By: John Baldwin <jhb@FreeBSD.org>
Move the declarations out of defs.h, and the implementations out of
findvar.c.
I opted for a new file, because this functionality of converting
integers to bytes and vice-versa seems a bit to generic to live in
findvar.c.
Change-Id: I524858fca33901ee2150c582bac16042148d2251
Approved-By: John Baldwin <jhb@FreeBSD.org>
There is no reason the callers of these functions need to change the
returned string, so change the `char *` return types to `const char *`.
Update a few callers to also use `const char *`.
Change-Id: I94adff574d5e1b326e8cc688cf1817a15b408b96
Approved-By: Tom Tromey <tom@tromey.com>
A bug points out that a certain error message in read_str_index uses a
hard-coded section name. This patch changes it to use
dwarf2_section_info::get_name instead, like the other errors in the
function.
No test because it didn't seem worthwhile.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31639
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Tom de Vries pointed out that the combination of sharding,
multi-threading, and per-CU "racing" means that sometimes a cross-CU
DIE reference might not be correctly resolved. However, it's
important to handle this correctly, due to some unfortunate aspects of
DWARF.
This patch implements this by arranging to preserve each worker's DIE
map through the end of index finalization. The extra data is
discarded when finalization is done. This approach also allows the
parent name resolution to be sharded, by integrating it into the
existing entry finalization loop.
In an earlier review, I remarked that addrmap couldn't be used here.
However, I was mistaken. A *mutable* addrmap cannot be used, as those
are based on splay trees and restructure the tree even during lookups
(and thus aren't thread-safe). A fixed addrmap, on the other hand, is
just a vector and is thread-safe.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30846
This changes the DIE range map from a raw addrmap to a custom class.
A new type is used to represent the ranges, in an attempt to gain a
little type safety as well.
Note that the new code includes a map-of-maps type. This is not used
yet, but will be used in the next patch.
Co-Authored-By: Tom de Vries <tdevries@suse.de>
Currently the DWARF scanner will enter enumeration constants into the
same namespace as the DW_TAG_enumeration_type itself. This is the
right thing to do, but the implementation may result in strange
entries being added to the addrmap that maps DIE ranges to entries.
This came up when debugging an earlier version of this series; and
while I don't think this should impact the current series, it seems
better to clean this up anyway.
In the new code, rather than pass the "wrong" scope down through
recursive calls to the scanner, the correct scope is always passed,
and then the parent handling is done when creating the enumerator
entry.
In scan_attributes there's code:
...
if (new_reader->cu == reader->cu
&& new_info_ptr > watermark_ptr
&& *parent_entry == nullptr)
...
else if (*parent_entry == nullptr)
...
...
that uses the "*parent_entry == nullptr" condition twice.
Make this somewhat more readable by factoring out the condition:
...
if (*parent_entry == nullptr)
{
if (new_reader->cu == reader->cu
&& new_info_ptr > watermark_ptr)
...
else
...
}
...
This also allows us to factor out "form_addr (origin_offset, origin_is_dwz)".
Tested on x86_64-linux.
I noticed that add_using_directive's 'copy_names' parameter is only
used by a single caller. This patch removes the parameter and changes
that caller to copy the names itself. I chose to use intern here
since I suspect the names may well be repeated in a given objfile.
Approved-By: John Baldwin <jhb@FreeBSD.org>
The mingw build currently issues a warning:
./../../src/gdb/utils.h:378:56: warning: ignoring attributes on template argument 'void(const char*, va_list)' {aka 'void(const char*, char*)'} [-Wignored-attributes]
This patch fixes the problem as suggested by Simon:
https://sourceware.org/pipermail/gdb-patches/2024-April/207908.html
...that is, by changing the warning interceptor to a class with a
single 'warn' method.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
I recent change (e9b738dfbd "Avoid race when reading dwz file") moved
the call to dwarf2_read_dwz_file from dwarf2_initialize_objfile to
dwarf2_has_info.
Before that patch, dwarf2_initialize_objfile was only called when
dwarf2_has_info returned true, and since that patch it is always called.
When reading a file that has no debug info (.debug_info/.debug_abbrev
sections), but has a .gnu_debugaltlink section, GDB’s behavior is
different. I can observe this when loading
/lib/x86_64-linux-gnu/libtinfo.so on Ubuntu 22.04 (or while debugging
any program dynamically loading this library).
Before e9b738dfbd, we had:
$ ./gdb/gdb -data-directory ./gdb/data-directory -q /lib/x86_64-linux-gnu/libtinfo.so
Reading symbols from /lib/x86_64-linux-gnu/libtinfo.so...
(No debugging symbols found in /lib/x86_64-linux-gnu/libtinfo.so)
(gdb)
while after we have:
$ ./gdb/gdb -data-directory ./gdb/data-directory -q /lib/x86_64-linux-gnu/libtinfo.so
Reading symbols from /lib/x86_64-linux-gnu/libtinfo.so...
warning: could not find '.gnu_debugaltlink' file for /usr/lib/x86_64-linux-gnu/libtinfo.so.6.3
(No debugging symbols found in /lib/x86_64-linux-gnu/libtinfo.so)
(gdb)
This patch restores the previous behavior of only trying to load the
DWZ file for objfiles when the main part of the debuginfo is present
(i.e. when dwarf2_has_info returns true). We still make sure that
dwarf2_read_dwz_file is called at most once per objfile.
A consequence of this change is that the per_bfd->dwz_file optional
object can now remain empty (instead of containing a nullptr), so also
this patch also adjusts dwarf2_get_dwz_file to account for this
possibility. This effectively reverts the changes to
dwarf2_get_dwz_file done by e9b738dfbd.
Regression tested on x86_64-linux-gnu Ubuntu 22.04.
Approved-By: Tom Tromey <tom@tromey.com>
Now that defs.h, server.h and common-defs.h are included via the
`-include` option, it is no longer necessary for source files to include
them. Remove all the inclusions of these files I could find. Update
the generation scripts where relevant.
Change-Id: Ia026cff269c1b7ae7386dd3619bc9bb6a5332837
Approved-By: Pedro Alves <pedro@palves.net>
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.
On arm-linux, with gas 2.40, I run into:
...
(gdb) x /i main+8^M
0x4e1 <main+7>: vrhadd.u16 d14, d14, d31^M
(gdb) FAIL: gdb.arch/pr25124.exp: disassemble thumb instruction (1st try)
...
This is a regression due to PR gas/31115, which makes gas produce a low_pc
with the thumb bit set (0x4d8 & 0x1):
...
<1><24>: Abbrev Number: 2 (DW_TAG_subprogram)
<25> DW_AT_name : main
<29> DW_AT_external : 1
<29> DW_AT_type : <0x2f>
<2a> DW_AT_low_pc : 0x4d9
<2e> DW_AT_high_pc : 12
...
The regression was introduced in 2.39, and is also present in 2.40 and 2.41,
and hasn't been fixed yet.
Work around this in read_func_scope, by using gdbarch_addr_bits_remove on
low_pc and high_pc.
Tested on arm-linux and x86_64-linux.
PR tdep/31453
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31453
quirk_rust_enum makes string copies to store in an unordered_map.
However, the original strings have an appropriate lifetime, and so no
copying is needed. With C++17, we can rely on string_view having a
std::hash specialization, so this patch changes this code to use
string_view rather than string.
The background DWARF reader changes introduced a race when writing to
the index cache. The problem here is that constructing the
index_cache_store_context object should only happen on the main
thread, to ensure that the various value captures do not race.
This patch adds an assert to the construct to that effect, and then
arranges for this object to be constructed by the cooked_index_worker
constructor -- which is only invoked on the main thread.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31262
I think it is cleaner for 'store' to be a method on
index_cache_store_context rather than on the global index cache
itself. This patch makes this change.
This changes index_cache_store_context to also capture the per-BFD
object when it is constructed. This is used when storing to the
cache, and this approach makes the code a little simpler.
I noticed that index_cache_store_context captures the 'enabled'
setting, but not the index cache directory. This patch makes this
change, which avoids a possible race -- with background reading, the
user could possibly change this directory at the exact moment the
writer examines the variable.
PR gdb/31260 points out a race introduced by the background reading
changes. If a given objfile is re-opened when it is already being
read, dwarf2_initialize_objfile will call dwarf2_read_dwz_file again,
causing the 'dwz_file' to be reset.
This patch fixes the problem by arranging to open the dwz just once:
when the dwarf2_per_bfd object is created.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31260
Today I realized that while the .debug_names writer uses DW_FORM_udata
for the DIE offset, DW_FORM_ref_addr would be more appropriate here.
This patch makes this change.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31361
We currently pass frames to function by value, as `frame_info_ptr`.
This is somewhat expensive:
- the size of `frame_info_ptr` is 64 bytes, which is a bit big to pass
by value
- the constructors and destructor link/unlink the object in the global
`frame_info_ptr::frame_list` list. This is an `intrusive_list`, so
it's not so bad: it's just assigning a few points, there's no memory
allocation as if it was `std::list`, but still it's useless to do
that over and over.
As suggested by Tom Tromey, change many function signatures to accept
`const frame_info_ptr &` instead of `frame_info_ptr`.
Some functions reassign their `frame_info_ptr` parameter, like:
void
the_func (frame_info_ptr frame)
{
for (; frame != nullptr; frame = get_prev_frame (frame))
{
...
}
}
I wondered what to do about them, do I leave them as-is or change them
(and need to introduce a separate local variable that can be
re-assigned). I opted for the later for consistency. It might not be
clear why some functions take `const frame_info_ptr &` while others take
`frame_info_ptr`. Also, if a function took a `frame_info_ptr` because
it did re-assign its parameter, I doubt that we would think to change it
to `const frame_info_ptr &` should the implementation change such that
it doesn't need to take `frame_info_ptr` anymore. It seems better to
have a simple rule and apply it everywhere.
Change-Id: I59d10addef687d157f82ccf4d54f5dde9a963fd0
Approved-By: Andrew Burgess <aburgess@redhat.com>
When rewriting the DWARF scanner, I forgot to remove
dwarf2_cu::load_all_dies and dwarf2_cu::partial_dies. This patch
corrects the oversight and fixes up a couple other spots that mention
partial DIEs, which no longer exist.
Approved-By: Tom de Vries <tdevries@suse.de>
I noticed that attr_to_dynamic_prop allocates a dwarf_block, when no
allocation is required. This patch stack-allocates the object
instead.
Reviewed-By: Tom de Vries <tdevries@suse.de>
Starting with C++17, emplace_back returns a reference to the new
object. This patch changes code that uses emplace_back followed by a
call to back() to simply use this reference instead.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
If you have set up a backtrace limit, and the backtrace stops
because of this in an inline frame with arguments, you get an
assertion failure:
```
(gdb) bt
(gdb) set backtrace limit 2
(gdb) bt
C:/src/repos/binutils-gdb.git/gdb/frame.c:3346: internal-error: reinflate: Assertion `m_cached_level >= -1' failed.
```
And if this one is fixed, there is another one as well:
```
(gdb) bt
C:/src/repos/binutils-gdb.git/gdb/dwarf2/loc.c:1160: internal-error: dwarf_expr_reg_to_entry_parameter: Assertion `frame != NULL' failed.
```
The reason for both of them is this kind of loop:
```
while (get_frame_type (frame) == INLINE_FRAME)
frame = get_prev_frame (frame);
```
Since get_prev_frame respects the backtrace limit, it will return
NULL, and from there on you can't continue.
This changes these loops to use get_prev_frame_always instead, so
you always get a non-inline frame in the end.
With this backtrace works:
```
(gdb) bt
(gdb)
```
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29865
Approved-By: Andrew Burgess <aburgess@redhat.com>
This changes some of the Ada code to simplify symbol searches. For
example, if a function is being looked for, the search is narrowed to
use SEARCH_FUNCTION_DOMAIN rather than SEARCH_VFT. In one spot, a
search of the "struct" domain is removed, because Ada does not have a
tag domain.
This patch changes the DWARF reader to use the new symbol domains. It
also adjusts many bits of associated code to adapt to this change.
The non-DWARF readers are updated on a best-effort basis. This is
somewhat simpler since most of them only support C and C++. I have no
way to test a few of these.
I went back and forth a few times on how to handle the "tag"
situation. The basic problem is that C has a special namespace for
tags, which is separate from the type namespace. Other languages
don't do this. So, the question is, should a DW_TAG_structure_type
end up in the tag domain, or the type domain, or should it be
language-dependent?
I settled on making it language-dependent using a thought experiment.
Suppose there was a Rust compiler that only emitted nameless
DW_TAG_structure_type objects, and specified all structure type names
using DW_TAG_typedef. This DWARF would be correct, in that it
faithfully represents the source language -- but would not work with a
purely struct-domain implementation in gdb. Therefore gdb would be
wrong.
Now, this approach is a little tricky for C++, which uses tags but
also enters a typedef for them. I notice that some other readers --
like stabsread -- actually emit a typedef symbol as well. And, I
think this is a reasonable approach. It uses more memory, but it
makes the internals simpler. However, DWARF never did this for
whatever reason, and so in the interest of keeping the series slightly
shorter, I've left some C++-specific hacks in place here.
Note that this patch includes language_minimal as a language that uses
tags. I did this to avoid regressing gdb.dwarf2/debug-names-tu.exp,
which doesn't specify the language for a type unit. Arguably this
test case is wrong.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30164
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 changes quick_symbol_functions::lookup_global_symbol_language to
accept domain_search_flags rather than just a domain_enum, and fixes
up the fallout.
To avoid introducing any regressions, any code passing VAR_DOMAIN now
uses SEARCH_VFT.
That is, no visible changes should result from this patch. However,
it sets the stage to refine some searches later on.
