Fix this error when building with clang++-14:
CXX complaints.o
/home/smarchi/src/binutils-gdb/gdb/complaints.c:130:65: error: format string is not a string literal [-Werror,-Wformat-nonliteral]
g_complaint_interceptor->m_complaints.insert (string_vprintf (fmt, args));
^~~
Change-Id: I0ef11f970510eb8638d1651fa0d5eeecd6a9d31a
Building with clang++-14, I see:
CXX mips-tdep.o
/home/smarchi/src/binutils-gdb/gdb/mips-tdep.c:453:12: error: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
return !(MSYMBOL_TARGET_FLAG_MIPS16 (msym)
~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/smarchi/src/binutils-gdb/gdb/mips-tdep.h:54:2: note: expanded from macro 'MSYMBOL_TARGET_FLAG_MIPS16'
(sym)->target_flag_1 ()
^
/home/smarchi/src/binutils-gdb/gdb/mips-tdep.c:453:12: note: cast one or both operands to int to silence this warning
/home/smarchi/src/binutils-gdb/gdb/mips-tdep.h:54:2: note: expanded from macro 'MSYMBOL_TARGET_FLAG_MIPS16'
(sym)->target_flag_1 ()
^
That's since commit e165fcef1e7 ("gdb: remove MSYMBOL_TARGET_FLAG_{1,2}
macros"). Fix this by using the boolean || rather than the bitwise |,
since the new methods return bool values. No change in behavior
expected.
Change-Id: Ia82664135aa25db64c29c92f5c1141859d345bf7
Tromey noticed that intrusive_list_node leaves its data members
public, which seems sub-optimal.
This commit makes intrusive_list_node's data fields private.
intrusive_list_iterator, intrusive_list_reverse_iterator, and
intrusive_list do need to access the fields, so they are made friends.
Change-Id: Ia8b306b40344cc218d423c8dfb8355207a612ac5
Now that Ada is able to parse & print 0xffffffffffffffff (2^64-1) in
hex, move it to the else branch like most other languages.
Change-Id: Ib305f6bb2b6b230a1190ea783b245b865821094c
On irc, Pedro pointed out that Ada couldn't properly handle
0xffffffffffffffff. This used to work, but is a regression due to
some patches I wrote in the Ada lexer. This patch fixes the bug.
Reading a simple file compiled with :
$ gcc -DONE=1 -gdwarf-4 -g3 test.c
$ gcc --version
gcc (Ubuntu 9.4.0-1ubuntu1~20.04) 9.4.0
I get:
Reading symbols from /tmp/cwd/a.out...
/home/smarchi/src/binutils-gdb/gdb/dwarf2/cooked-index.c:332:11: runtime error: null pointer passed as argument 2, which is declared to never be null
It looks like even if the size is 0 (the size of the `entries` vector is
0), we shouldn't be passing a NULL pointer to memcpy. And
`entries.data ()` returns NULL.
Fix that by using std::vector::insert to insert the items of entries
into m_entries. I haven't checked, but it should essentially compile
down to a memcpy, since the vector elements are trivially copyiable.
Change-Id: I75f1c901e9b522e42e89eb5936e2c70d68eb21e5
Change this field to an std::vector to facilitate memory management.
Since the linetable_entry array is copied into the symtab resulting from
the subfile, it is possible to change it without changing how symtab
stores the linetable entries (which would be a much larger change).
There is a small change in buildsym_compunit::record_line to avoid
accessing a now invalid linetable_entry. Before this patch, we keep a
pointer to the last linetable entry, pop it from the vector, and then
read last->line. It works with the manually-maintained array, but since
we now use std::vector::pop_back, I am afraid that it could be flagged
as an invalid access by the various static / dynamic analysis tools to
access the linetable_entry object after popping it from the vector.
Instead, record just the line number in an optional and use it.
There are substantial changes in xcoffread.c that simplify the code, but
I can't test them. I was hesitant to do this change because of that,
but I decided to send it anyway. I don't think that an almost dead
platform should hold back improving the code in the common parts of GDB.
The changes in xcoffread.c are:
- Make arrange_linetable "arrange" the linetable passed as a parameter,
instead of returning possibly a new one, possibly the same one.
- In the "Process main file's line numbers.", I'm not too sure what
happens. We get the lintable from "main_subfile", "arrange" it, but
then assign the result to the current subfile, obtained with
get_current_subfile. I assume that the current subfile is also the
main one, so now I just call arrange_linetable on the main subfile's
line table.
- Remove that weird "Useless if!!!" FIXME comment. It's been there
forever, but the "if" is still there, so I guess the "if" can stay
there.
Change-Id: I11799006fd85189e8cf5bd3a168f8f38c2c27a80
Reduce manual memory management and make the code a bit easier to read.
This helps me a bit in the following patch.
I don't have a way to test this, it's best-effort.
Change-Id: I64af9cd756311deabc6cd95e701dfb21234a40a5
Change subfile::name to be a string, for easier memory management.
Change buildsym_compunit::m_comp_dir as well, since we move one in to
the other at some point in patch_subfile_names, so it's easier to do
both at the same time. There are various NULL checks for both fields
currently, replace them with empty checks, I think it ends up
equivalent.
I can't test the change in xcoffread.c, it's best-effort.
Change-Id: I62b5fb08b2089e096768a090627ac7617e90a016
Allocate struct subfile with new, initialize its fields instead of
memset-ing it to 0. Use a unique_ptr for the window after a subfile has
been allocated but before it is linked in the buildsym_compunit's list
of subfile (and therefore owned by the buildsym_compunit.
I can't test the change in xcoffread.c, it's best-effort. I couldn't
find where subfiles are freed in that file, I assume they were
intentionally (or not) leaked.
Change-Id: Ib3b6877de31b7e65bc466682f08dbf5840225f24
When building with -std=c++11, I get:
CXX dwarf2/read.o
/home/smarchi/src/binutils-gdb/gdb/dwarf2/read.c: In function ‘void dwarf2_build_psymtabs_hard(dwarf2_per_objfile*)’:
/home/smarchi/src/binutils-gdb/gdb/dwarf2/read.c:7130:23: error: expected type-specifier before ‘typeof’
7130 | using iter_type = typeof (per_bfd->all_comp_units.begin ());
| ^~~~~~
This is because typeof is a GNU extension. Use C++'s decltype keyword
instead.
Change-Id: Ieca2e8d25e50f71dc6c615a405a972a54de3ef14
When building with -std=c++11, I get:
In file included from /home/smarchi/src/binutils-gdb/gdb/unittests/parallel-for-selftests.c:22: /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/parallel-for.h:134:10: error: ‘result_of_t’ is not a member of ‘std’; did you mean ‘result_of’?
134 | std::result_of_t<RangeFunction (RandomIt, RandomIt)>
| ^~~~~~~~~~~
| result_of
This is because result_of_t has been introduced in C++14. Use the
equivalent result_of<...>::type instead.
result_of and result_of_t have been removed in C++20 though, so I think
we'll need some patches eventually to make the code use invoke_result
instead, depending on the C++ version.
Change-Id: I4817f361c0ebcdd4b32976898fc368bb302b61b9
Now that the psymtab reader has been removed, the
dwarf2_per_cu_data::v union is no longer needed. Instead, we can
simply move the members from dwarf2_per_cu_quick_data into
dwarf2_per_cu_data and remove the "quick" object entirely.
This patch finally enables the new indexer. It is left until this
point in the series to avoid any regressions; in particular, it has to
come after the changes to the DWARF index writer to avoid this
problem.
However, if you experiment with the series, this patch can be moved
anywhere from the patch to wire in the new reader to this point.
Moving this patch around is how I got separate numbers for the
parallelization and background finalization patches.
In the ongoing performance example, this reduces the time from the
baseline of 1.598869 to 0.903534.
This updates the .gdb_index writer to work with the new DWARF scanner.
The .debug_names writer is deferred to another patch, to make review
simpler.
This introduces a small hack to psyms_seen_size, but is
inconsequential because this function will be deleted in a subsequent
patch.
This updates the DWARF index writing code to make the addrmap-writing
a bit more generic. Now, it can handle multiple maps, and it can work
using the maps generated by the new indexer.
Note that the new addrmap_index_data::using_index field will be
deleted in a future patch, when the rest of the DWARF psymtab code is
removed.
To support the removal of partial symtabs from the DWARF index writer,
this makes a small change to have write_address_map accept the address
map as a parameter, rather than assuming it always comes from the
per-BFD object.
In order to change the DWARF index writer to avoid partial symtabs,
this patch changes the key type in psym_index_map (and renames that
type as well). Using the dwarf2_per_cu_data as the key makes it
simpler to reuse this code with the new indexer.
After scanning the CUs, the DWARF indexer merges all the data into a
single vector, canonicalizing C++ names as it proceeds. While not
necessarily single-threaded, this process is currently done in just
one thread, to keep memory costs lower.
However, this work is all done without reference to any data outside
of the indexes. This patch improves the apparent performance of GDB
by moving it to the background. All uses of the index are then made
to wait for this process to complete.
In our ongoing example, this reduces the scanning time on gdb itself
to 0.173937 (wall). Recall that before this patch, the time was
0.668923; and psymbol reader does this in 1.598869. That is, at the
end of this series, we see about a 10x speedup.
This parallelizes the new DWARF indexer. The indexer's storage was
designed so that each storage object and each indexer is fully
independent. This setup makes it simple to scan different CUs
independently.
This patch creates a new cooked index storage object per thread, and
then scans a subset of all the CUs in each such thread, using gdb's
existing thread pool.
In the ongoing "gdb gdb" example, this patch reduces the wall time
down to 0.668923, from 0.903534. (Note that the 0.903534 is the time
for the new index -- that is, when the "enable the new index" patch is
rebased to before this one. However, in the final series, that patch
appears toward the end. Hopefully this isn't too confusing.)
Because BFD is not thread-safe, we need to be sure that any section
data that is needed is read before trying to do any DWARF indexing in
the background.
This patch takes a simple approach to this -- it pre-reads the
"info"-related sections. This is done for the main file, but also any
auxiliary files as well, such as the DWO file.
This patch could be perhaps enhanced by removing some now-redundant
calls to dwarf2_section_info::read.
This introduces a new class that can be used to make the "complaint"
code thread-safe. Instantiating the class installs a new handler that
collects complaints, and then prints them all when the object is
destroyed.
This approach requires some locks. I couldn't think of a better way
to handle this, though, because the I/O system is not thread-safe.
It seemed to me that only GDB developers are likely to enable
complaints, and because the complaint macro handle this case already
(before any locks are required), I reasoned that any performance
degradation that would result here would be fine.
As an aside about complaints -- are they useful at all? I just ignore
them, myself, since mostly they seem to indicate compiler problems
that can't be solved in the GDB world anyway. I'd personally prefer
them to be in a separate tool, like a hypothetical 'dwarflint'.
This wires the new DWARF indexer into the existing reader code. That
is, this patch makes the modification necessary to enable the new
indexer. It is not actually enabled by this patch -- that will be
done later.
I did a bit of performance testing for this patch and a few others. I
copied my built gdb to /tmp, so that each test would be done on the
same executable. Then, each time, I did:
$ ./gdb -nx
(gdb) maint time 1
(gdb) file /tmp/gdb
This patch is the baseline and on one machine came in at 1.598869 wall
time.
This implements quick_symbol_functions for the cooked DWARF index.
This is the code that interfaces between the new index and the rest of
gdb. Cooked indexes still aren't created by anything.
For the most part this is straightforward. It shares some concepts
with the existing DWARF indices. However, because names are stored
pre-split in the cooked index, name lookup here is necessarily
different; see expand_symtabs_matching for the gory details.
This patch adds the code to index DWARF. This is just the scanner; it
reads the DWARF and constructs the index, but nothing calls it yet.
The indexer is split into two parts: a storage object and an indexer
object. This is done to support the parallelization of this code -- a
future patch will create a single storage object per thread.
This patch introduces the new DWARF index class. It is called
"cooked" to contrast against a "raw" index, which is mapped from disk
without extra effort.
Nothing constructs a cooked index yet. The essential idea here is
that index entries are created via the "add" method; then when all the
entries have been read, they are "finalize"d -- name canonicalization
is performed and the entries are added to a sorted vector.
Entries use the DWARF name (DW_AT_name) or linkage name, not the full
name as is done for partial symbols.
These two facets -- the short name and the deferred canonicalization
-- help improve the performance of this approach. This will become
clear in later patches, when parallelization is added.
Some special code is needed for Ada, because GNAT only emits mangled
("encoded", in the Ada lingo) names, and so we reconstruct the
hierarchical structure after the fact. This is also done in the
finalization phase.
One other aspect worth noting is that the way the "main" function is
found is different in the new code. Currently gdb will notice
DW_AT_main_subprogram, but won't recognize "main" during reading --
this is done later, via explicit symbol lookup. This is done
differently in the new code so that finalization can be done in the
background without then requiring a synchronization to look up the
symbol.
The new DIE scanner works more or less along the lines indicated by
the text for the .debug_names section, disregarding the bugs in the
specification.
While working on this, I noticed that whether a DIE is interesting is
a static property of the DIE's abbrev. It also turns out that many
abbrevs imply a static size for the DIE data, and additionally that
for many abbrevs, the sibling offset is stored at a constant offset
from the start of the DIE.
This patch changes the abbrev reader to analyze each abbrev and stash
the results on the abbrev. These combine to speed up the new indexer.
If the "interesting" flag is false, GDB knows to skip the DIE
immediately. If the sibling offset is statically known, skipping can
be done without reading any attributes; and in some other cases, the
DIE can be skipped using simple arithmetic.
The replacement for the DWARF psymbol reader works in a somewhat
different way. The current reader reads and stores all the DIEs that
might be interesting. Then, if it is missing a DIE, it re-scans the
CU and reads them all. This approach is used for both intra- and
inter-CU references.
I instrumented the partial DIE hash to see how frequently it was used:
[ 0] -> 1538165
[ 1] -> 4912
[ 2] -> 96102
[ 3] -> 175
[ 4] -> 244
That is, most DIEs are never used, and some are looked up twice -- but
this is just an artifact of the implementation of
partial_die_info::fixup, which may do two lookups.
Based on this, the new implementation doesn't try to store any DIEs,
but instead just re-scans them on demand. In order to do this,
though, it is convenient to have a cache of DWARF abbrevs. This way,
if a second CU is needed to resolve an inter-CU reference, the abbrevs
for that CU need only be computed a single time.
This changes the file_and_directory object to be able to compute and
cache the "fullname" in the same way that is done by other code, like
the psymtab reader.
This adds a std::hash specialization for gdb_exception. This lets us
store these objects in a hash table, which is used later in this
series to de-duplicate the exception output from multiple threads.
This changes gdb::parallel_for_each to return a vector of the results.
However, if the passed-in function returns void, the return type
remains 'void'. This functionality is used later, to parallelize the
new indexer.
parallel_for_each currently requires each thread to process at least
10 elements. However, when indexing, it's fine for a thread to handle
just a single CU. This patch parameterizes this, and updates the one
user.
The new DWARF scanner needs to save the entire cutu_reader object, not
just parts of it. In order to make this possible, this patch
refactors build_type_psymtabs_reader. This change is done separately
because it is easy to review in isolation and it helps make the later
patches smaller.
The new DWARF index code works by keeping names pre-split. That is,
rather than storing a symbol name like "a:🅱️:c", the names "a", "b",
and "c" will be stored separately.
This patch introduces some helper code to split a full name into its
components.
The new DWARF scanner records names as they appear in DWARF. However,
because Ada is unusual, it also decodes the Ada names to synthesize
package components for them. In order for this to work out properly,
gdb also needs a mode where ada_decode can be instructed not to decode
Ada operator names. That is what this patch implements.
This changes dwarf2_get_pc_bounds so that it does not directly access
a psymtab or psymtabs_addrmap. Instead, both the addrmap and the
desired payload are passed as parameters. This makes it suitable to
be used by the new indexer.
This adds a new member to dwarf2_per_cu_data that indicates whether
addresses have been seen for this CU. This is then set by the
.debug_aranges reader. The idea here is to detect when a CU does not
have address information, so that the new indexer will know to do
extra scanning in that case.
Tom de Vries found a failure that we tracked down to a latent bug in
read_addrmap_from_aranges (previously create_addrmap_from_aranges).
The bug is that this code can erroneously reject .debug_aranges when
dwz is in use, due to CUs at duplicate offsets. Because aranges can't
refer to a CU coming from the dwz file, the fix is to simply skip such
CUs in the loop.
thread-pool.h requires CXX_STD_THREAD in order to even be included.
However, there's no deep reason for this, and during review we found
that one patch in the new DWARF indexer series unconditionally
requires the thread pool.
Because the thread pool already allows a task to be run in the calling
thread (for example if it is configured to have no threads in the
pool), it seemed straightforward to make this code ok to use when host
threads aren't available at all.
This patch implements this idea. I built it on a thread-less host
(mingw, before my recent configure patch) and verified that the result
builds.
After the thread-pool change, parallel-for.h no longer needs any
CXX_STD_THREAD checks at all, so this patch removes these as well.
