Change most of regcache (and base classes) to use array_view when
possible, instead of raw pointers. By propagating the use of array_view
further, it enables having some runtime checks to make sure the what we
read from or write to regcaches has the expected length (such as the one
in the `copy(array_view, array_view)` function. It also integrates well
when connecting with other APIs already using gdb::array_view.
Add some overloads of the methods using raw pointers to avoid having to
change all call sites at once (which is both a lot of work and risky).
I tried to do this change in small increments, but since many of these
functions use each other, it ended up simpler to do it in one shot than
having a lot of intermediary / transient changes.
This change extends into gdbserver as well, because there is some part
of the regcache interface that is shared.
Changing the reg_buffer_common interface to use array_view caused some
build failures in nat/aarch64-scalable-linux-ptrace.c. That file
currently "takes advantage" of the fact that
reg_buffer_common::{raw_supply,raw_collect} operates on `void *`, which
IMO is dangerous. It uses raw_supply/raw_collect directly on
uint64_t's, which I guess is fine because it is expected that native
code will have the same endianness as the debugged process. To
accomodate that, add some overloads of raw_collect and raw_supply that
work on uint64_t.
This file also uses raw_collect and raw_supply on `char` pointers.
Change it to use `gdb_byte` pointers instead. Add overloads of
raw_collect and raw_supply that work on `gdb_byte *` and make an
array_view on the fly using the register's size. Those call sites could
be converted to use array_view with not much work, in which case these
overloads could be removed, but I didn't want to do it in this patch, to
avoid starting to dig in arch-specific code.
During development, I inadvertently changed reg_buffer::raw_compare's
behavior to not accept an offset equal to the register size. This
behavior (effectively comparing 0 bytes, returning true) change was
caught by the AArch64 SME core tests. Add a selftest to make sure that
this raw_compare behavior is preserved in the future.
Change-Id: I9005f04114543ddff738949e12d85a31855304c2
Reviewed-By: John Baldwin <jhb@FreeBSD.org>
Right now, gdbsupport/common-regcache.h contains two abstractons for a
regcache. An opaque type `regcache` (gdb and gdbserver both have their
own regcache that is the concrete version of this) and an abstract base
class `reg_buffer_common`, that is the base of regcaches on both sides.
These abstractions allow code to be written for both gdb and gdbserver,
for instance in the gdb/arch sub-directory.
However, having two
different abstractions is impractical. If some common code has a regcache,
and wants to use an operation defined on reg_buffer_common, it can't.
It would be better to have just one. Change all instances of `regcache
*` in gdbsupport/common-regcache.h to be `reg_buffer_common *`, then fix
fallouts.
Implementations in gdb and gdbserver now need to down-cast (using
gdb::checked_static_cast) from reg_buffer_common to their concrete
regcache type. Some of them could be avoided by changing free functions
(like regcache_register_size) to be virtual methods on
reg_buffer_common. I tried it, it seems to work, but I did not include
it in this series to avoid adding unnecessary changes.
Change-Id: Ia5503adb6b5509a0f4604bd2a68b4642cc5283fd
Reviewed-by: John Baldwin <jhb@FreeBSD.org>
This commit enables the early initialization commands (92e4e97a9f) to
modify the number of threads used by gdb's thread pool.
The motivation here is to prevent gdb from spawning a detrimental number
of threads on many-core systems under environments with restrictive
ulimits.
With gdb before this commit, the thread pool takes the following sizes:
1. Thread pool size is initialized to 0.
2. After the maintenance commands are defined, the thread pool size is
set to the number of system cores (if it has not already been set).
3. Using early initialization commands, the thread pool size can be
changed using "maint set worker-threads".
4. After the first prompt, the thread pool size can be changed as in the
previous step.
Therefore after step 2. gdb has potentially launched hundreds of threads
on a many-core system.
After this change, step 2 and 3 are reversed so there is an opportunity
to set the required number of threads without needing to default to the
number of system cores first.
There does exist a configure option (added in 261b07488b) to disable
multithreading, but this does not allow for an already deployed gdb to
be configured.
Additionally, the default number of worker threads is clamped at eight
to control the number of worker threads spawned on many-core systems.
This value was chosen as testing recorded on bugzilla issue 29959
indicates that parallel efficiency drops past this point.
GDB built with GCC 13.
No test suite regressions detected. Compilers: GCC, ACfL, Intel, Intel
LLVM, NVHPC; Platforms: x86_64, aarch64.
The scenario that interests me the most involves preventing GDB from
spawning any worker threads at all. This was tested by counting the
number of clones observed by strace:
strace -e clone,clone3 gdb/gdb -q \
--early-init-eval-command="maint set worker-threads 0" \
-ex q ./gdb/gdb |& grep --count clone
The new test relies on "gdb: install CLI uiout while processing early
init files" developed by Andrew Burgess. This patch will need pushing
prior to this change.
The clamping was tested on machines with both 16 cores and a single
core. "maint show worker-threads" correctly reported eight and one
respectively.
Approved-By: Tom Tromey <tom@tromey.com>
gdbsupport/scope-exit.h has a couple of comments about being able to
rely on copy elision in C++17. This patch makes the change.
Approved-By: Pedro Alves <pedro@palves.net>
gdbsupport/new-op.cc has a comment about relying on the C++-17 <new>
header. This patch implements the suggestion.
Approved-By: Pedro Alves <pedro@palves.net>
This changes the various gdb-related directories to use
-Wimplicit-fallthrough=5, meaning that only the fallthrough attribute
can be used in switches -- special 'fallthrough' comments will no
longer be usable.
Approved-By: Pedro Alves <pedro@palves.net>
This changes gdb to use the C++17 [[fallthrough]] attribute rather
than special comments.
This was mostly done by script, but I neglected a few spellings and so
also fixed it up by hand.
I suspect this fixes the bug mentioned below, by switching to a
standard approach that, presumably, clang supports.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=23159
Approved-By: John Baldwin <jhb@FreeBSD.org>
Approved-By: Luis Machado <luis.machado@arm.com>
Approved-By: Pedro Alves <pedro@palves.net>
When running test-case gdb.base/vfork-follow-parent.exp on powerpc64 (likewise
on s390x), I run into:
...
(gdb) PASS: gdb.base/vfork-follow-parent.exp: \
exec_file=vfork-follow-parent-exit: target-non-stop=on: non-stop=off: \
resolution_method=schedule-multiple: print unblock_parent = 1
continue^M
Continuing.^M
Reading symbols from vfork-follow-parent-exit...^M
^M
^M
Fatal signal: Segmentation fault^M
----- Backtrace -----^M
0x1027d3e7 gdb_internal_backtrace_1^M
src/gdb/bt-utils.c:122^M
0x1027d54f _Z22gdb_internal_backtracev^M
src/gdb/bt-utils.c:168^M
0x1057643f handle_fatal_signal^M
src/gdb/event-top.c:889^M
0x10576677 handle_sigsegv^M
src/gdb/event-top.c:962^M
0x3fffa7610477 ???^M
0x103f2144 for_each_block^M
src/gdb/dcache.c:199^M
0x103f235b _Z17dcache_invalidateP13dcache_struct^M
src/gdb/dcache.c:251^M
0x10bde8c7 _Z24target_dcache_invalidatev^M
src/gdb/target-dcache.c:50^M
...
or similar.
The root cause for the segmentation fault is that linux_is_uclinux gives an
incorrect result: it should always return false, given that we're running on a
regular linux system, but instead it returns first true, then false.
In more detail, the segmentation fault happens as follows:
- a program space with an address space is created
- a second program space is about to be created. maybe_new_address_space
is called, and because linux_is_uclinux returns true, maybe_new_address_space
returns false, and no new address space is created
- a second program space with the same address space is created
- a program space is deleted. Because linux_is_uclinux now returns false,
gdbarch_has_shared_address_space (current_inferior ()->arch ()) returns
false, and the address space is deleted
- when gdb uses the address space of the remaining program space, we run into
the segfault, because the address space is deleted.
Hardcoding linux_is_uclinux to false makes the test-case pass.
We leave addressing the root cause for the following commit in this series.
For now, prevent the segmentation fault by making the address space a refcounted
object.
This was already suggested here [1]:
...
A better solution might be to have the address spaces be reference counted
...
Tested on top of trunk on x86_64-linux and ppc64le-linux.
Tested on top of gdb-14-branch on ppc64-linux.
Co-Authored-By: Simon Marchi <simon.marchi@polymtl.ca>
PR gdb/30547
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30547
[1] https://sourceware.org/pipermail/gdb-patches/2023-October/202928.html
Given that GDB now requires C++17, we can replace gdb::invoke_result
with std::invoke_result which is provided by <type_traits>.
This patch also removes gdbsupport/invoke-result.h as it is not used
anymore.
Change-Id: I7e567356d38d6b3d85d8797d61cfc83f6f933f22
Approved-By: Tom Tromey <tom@tromey.com>
Approved-By: Pedro Alves <pedro@palves.net>
Now that all places using gdb::string_view have been updated to use
std::string_view, this patch drops the gdb::string_view implementation
and the tests which came with it.
As this drops the unittests/string_view-selftests.c, this also
implicitly solves PR build/23676, as pointed-out by Tom Tromey.
Change-Id: Idf5479b09e0ac536917b3f0e13aca48424b90df0
Approved-By: Tom Tromey <tom@tromey.com>
Approved-By: Pedro Alves <pedro@palves.net>
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=23676
Given that GDB now requires a C++17, replace all uses of
gdb::string_view with std::string_view.
This change has mostly been done automatically:
- gdb::string_view -> std::string_view
- #include "gdbsupport/gdb_string_view.h" -> #include <string_view>
One things which got brought up during review is that gdb::stging_view
does support being built from "nullptr" while std::sting_view does not.
Two places are manually adjusted to account for this difference:
gdb/tui/tui-io.c:tui_getc_1 and
gdbsupport/format.h:format_piece::format_piece.
The above automatic change transformed
"gdb::to_string (const gdb::string_view &)" into
"gdb::to_string (const std::string_view &)". The various direct users
of this function are now explicitly including
"gdbsupport/gdb_string_view.h". A later patch will remove the users of
gdb::to_string.
The implementation and tests of gdb::string_view are unchanged, they will
be removed in a following patch.
Change-Id: Ibb806a7e9c79eb16a55c87c6e41ad396fecf0207
Approved-By: Tom Tromey <tom@tromey.com>
Approved-By: Pedro Alves <pedro@palves.net>
The previous patch migrated all the uses of gdb::optional to use
std::optional instead, so gdb::optional can be removed entirely
as well as the self-tests which came with it.
Change-Id: I96ecd67b850b01be10ef00eb85a78ac647d5adc7
Approved-By: Tom Tromey <tom@tromey.com>
Approved-By: Pedro Alves <pedro@palves.net>
Since GDB now requires C++17, we don't need the internally maintained
gdb::optional implementation. This patch does the following replacing:
- gdb::optional -> std::optional
- gdb::in_place -> std::in_place
- #include "gdbsupport/gdb_optional.h" -> #include <optional>
This change has mostly been done automatically. One exception is
gdbsupport/thread-pool.* which did not use the gdb:: prefix as it
already lives in the gdb namespace.
Change-Id: I19a92fa03e89637bab136c72e34fd351524f65e9
Approved-By: Tom Tromey <tom@tromey.com>
Approved-By: Pedro Alves <pedro@palves.net>
gdb::make_unique is a wrapper around std::make_unique when compiled with
C++17. Now that C++17 is required, use std::make_unique directly in the
codebase, and remove gdb::make_unique.
Change-Id: I80b615e46e4b7c097f09d78e579a9bdce00254ab
Approved-By: Tom Tromey <tom@tromey.com>
Approved-By: Pedro Alves <pedro@palves.net
common-defs.h has a few defines that I suspect were used during the
transition to C++. These aren't needed any more, so remove them.
Tested by rebuilding.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Approved-By: Andrew Burgess <aburgess@redhat.com>
I (almost) had a bug where I did:
buffer.slice (...)
but I meant:
buffer = buffer.slice (...)
The first one does nothing, it creates a new array_view but without
using it, it's useless. Mark the slice methods with [[nodiscard]]
(which is standard C++17) so that error would generate a warning.
I guess that many functions could be marked as nodiscard, essentially
function that is pure (doesn't have side-effects). But this one seems
particularly easy to mis-use.
Change-Id: Ib39a0a65a5728a3cfd68a02ae31635810baeaccb
Approved-By: Tom Tromey <tom@tromey.com>
Since "maint selftest" now runs quite a lot of tests (especially in an
all-targets build), I thought it would be useful to print a summary at
the end of what failed. So, implement that.
Print the summary before the "Ran %d unit tests, %zu failed\n" line, so
that that one remains the last line, and the gdb.gdb/unittest.exp
doesn't need to be changed.
The output looks like (if I force a failure in a test):
(gdb) maint selftest
...
Running selftest value_copy.
Running selftest xml_escape_text.
Running selftest xml_escape_text_append.
Failures:
aarch64-analyze-prologue
Ran 4134 unit tests, 1 failed
(gdb)
Change-Id: If3aaabdd6f8078d0e6e50e8d08f3e558ab85277e
Approved-By: Tom Tromey <tom@tromey.com>
This patch proposes to require a C++17 compiler to build gdb /
gdbsupport / gdbserver. Before this patch, GDB required a C++11
compiler.
The general policy regarding bumping C++ language requirement in GDB (as
stated in [1]) is:
Our general policy is to wait until the oldest compiler that
supports C++NN is at least 3 years old.
Rationale: We want to ensure reasonably widespread compiler
availability, to lower barrier of entry to GDB contributions, and to
make it easy for users to easily build new GDB on currently
supported stable distributions themselves. 3 years should be
sufficient for latest stable releases of distributions to include a
compiler for the standard, and/or for new compilers to appear as
easily installable optional packages. Requiring everyone to build a
compiler first before building GDB, which would happen if we
required a too-new compiler, would cause too much inconvenience.
See the policy proposal and discussion
[here](https://sourceware.org/ml/gdb-patches/2016-10/msg00616.html).
The first GCC release which with full C++17 support is GCC-9[2],
released in 2019[3], which is over 4 years ago. Clang has had C++17
support since Clang-5[4] released in 2018[5].
A discussions with many distros showed that a C++17-able compiler is
always available, meaning that this no hard requirement preventing us to
require it going forward.
[1] https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#When_is_GDB_going_to_start_requiring_C.2B-.2B-NN_.3F
[2] https://gcc.gnu.org/projects/cxx-status.html#cxx17
[3] https://gcc.gnu.org/gcc-9/
[4] https://clang.llvm.org/cxx_status.html
[5] https://releases.llvm.org/
Change-Id: Id596f5db17ea346e8a978668825787b3a9a443fd
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Approved-By: Tom Tromey <tom@tromey.com>
Approved-By: Pedro Alves <pedro@palves.net>
This patch upgrades gdb/ax_cxx_compile_stdcxx.m4 to follow changes
available in [1] and regenerates the configure script.
[1] https://www.gnu.org/software/autoconf-archive/ax_cxx_compile_stdcxx.html
Change-Id: I5b16adc65c9e48a13ad65202d58ab7a9d487214e
Approved-By: Tom Tromey <tom@tromey.com>
Approved-By: Pedro Alves <pedro@palves.net>
It seems to me like the code should used the defined type aliases, for
consistency.
Change-Id: Ib52493ff18ad29464405275bc10a0c6704ed39e9
Approved-By: Pedro Alves <pedro@palves.net>
Reviewed-By: Reviewed-By: Lancelot Six <lancelot.six@amd.com>
A user pointed out that the -lsocket check in gdb should also apply to
gdbserver -- otherwise it can't find the Solaris socketpair. This
patch makes the change. It also removes a couple of redundant
function checks from gdb's configure.ac.
This was tested by the person who reported the bug.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30927
Approved-By: Pedro Alves <pedro@palves.net>
I noticed a comment by an include and remembered that I think these
don't really provide much value -- sometimes they are just editorial,
and sometimes they are obsolete. I think it's better to just remove
them. Tested by rebuilding.
Approved-By: Andrew Burgess <aburgess@redhat.com>
A user pointed out that the build failed with GCC 4.8. The problem
was that the form used by the std::hash specialization of ptid_t was
not accepted. This patch rewrites this code into a form that is
acceptable to the older compiler.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
This changes hash_ptid to instead be a specialization of std::hash.
This makes it a little easier to use with standard containers.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Following the example of private_thread_info and private_inferior, turn
struct btrace_target_info into a small class hierarchy.
Also merge btrace_tinfo_bts with btrace_tinfo_pt and inline into
linux_btrace_target_info.
Fixes PR gdb/30751.
The standard layout of the XSAVE extended state area consists of three
regions. The first 512 bytes (legacy region) match the layout of the
FXSAVE instruction including floating point registers, MMX registers,
and SSE registers. The next 64 bytes (XSAVE header) contains a header
with a fixed layout. The final region (extended region) contains zero
or more optional state components. Examples of these include the
upper 128 bits of YMM registers for AVX.
These optional state components generally have an
architecturally-fixed size, but they are not assigned architectural
offsets in the extended region. Instead, processors provide
additional CPUID leafs describing the size and offset of each
component in the "standard" layout for a given CPU. (There is also a
"compact" format which uses an alternate layout, but existing OS's
currently export the "standard" layout when exporting XSAVE data via
ptrace() and core dumps.)
To date, GDB has assumed the layout used on current Intel processors
for state components in the extended region and hardcoded those
offsets in the tables in i387-tdep.c and i387-fp.cc. However, this
fails on recent AMD processors which use a different layout.
Specifically, AMD Zen3 and later processors do not leave space for the
MPX register set in between the AVX and AVX512 register sets.
To rectify this, add an x86_xsave_layout structure which contains the
total size of the XSAVE extended state area as well as the offset of
each known optional state component.
Subsequent commits will modify XSAVE parsing in both gdb and gdbserver
to use x86_xsave_layout.
Co-authored-by: Aleksandar Paunovic <aleksandar.paunovic@intel.com>
Approved-By: Simon Marchi <simon.marchi@efficios.com>
While GDB is still C++11, lets add a gdb::make_unique template
function that can be used to create std::unique_ptr objects, just like
the C++14 std::make_unique.
If GDB is being compiled with a C++14 compiler then the new
gdb::make_unique function will delegate to the std::make_unique. I
checked with gcc, and at -O1 and above gdb::make_unique will be
optimised away completely in this case.
If C++14 (or later) becomes our minimum, then it will be easy enough
to go through the code and replace gdb::make_unique with
std::make_unique later on.
I've make use of this function in all the places I think this can
easily be used, though I'm sure I've probably missed some.
Should be no user visible changes after this commit.
Approved-By: Tom Tromey <tom@tromey.com>
When building gdb with -std=c++20, I run into:
...
gdbsupport/default-init-alloc.h:52:12: error: ‘construct’ has not been \
declared in ‘class std::allocator<unsigned char>’
52 | using A::construct;
| ^~~~~~~~~
...
Indeed, std::allocator::construct has been deprecated in c++17 and removed in
c++20.
Fix this by using instead std::pmr::polymorphic_allocator for c++20.
Tested on x86_64-linux.
When building gdb with clang 15 and -std=c++20, I run into:
...
gdbsupport/poison.h:52:11: error: 'is_pod<timeval>' is deprecated: use \
is_standard_layout && is_trivial instead [-Werror,-Wdeprecated-declarations]
std::is_pod<T>>
^
...
Fix this by following the suggestion.
Likewise in gdb/unittests/ptid-selftests.c.
Tested on x86_64-linux.
When building gdb with clang 15 and -std=c++20, I run into:
...
gdbsupport/common-exceptions.h:203:32: error: arithmetic between different \
enumeration types ('const enum return_reason' and 'const enum errors') is \
deprecated [-Werror,-Wdeprecated-enum-enum-conversion]
size_t result = exc.reason + exc.error;
~~~~~~~~~~ ^ ~~~~~~~~~
...
Fix this by using to_underlying.
Likewise in a few other places.
Tested on x86_64-linux.
This regenerates config files changed by the previous 44 commits.
Note that subject lines in these commits mostly match the gcc git
originating commit.
unrelocated_addr is currently defined in symtab.h, but in order to
avoid having to include that in more places, I wanted to move the type
elsewhere. I considered defs.h, but it seemed reasonable to have it
next to CORE_ADDR, which is what this patch does.
This commit adds a new format for the printf and dprintf commands:
'%V'. This new format takes any GDB expression and formats it as a
string, just as GDB would for a 'print' command, e.g.:
(gdb) print a1
$a = {2, 4, 6, 8, 10, 12, 14, 16, 18, 20}
(gdb) printf "%V\n", a1
{2, 4, 6, 8, 10, 12, 14, 16, 18, 20}
(gdb)
It is also possible to pass the same options to %V as you might pass
to the print command, e.g.:
(gdb) print -elements 3 -- a1
$4 = {2, 4, 6...}
(gdb) printf "%V[-elements 3]\n", a1
{2, 4, 6...}
(gdb)
This new feature would effectively replace an existing feature of GDB,
the $_as_string builtin convenience function. However, the
$_as_string function has a few problems which this new feature solves:
1. $_as_string doesn't currently work when the inferior is not
running, e.g:
(gdb) printf "%s", $_as_string(a1)
You can't do that without a process to debug.
(gdb)
The reason for this is that $_as_string returns a value object with
string type. When we try to print this we call value_as_address,
which ends up trying to push the string into the inferior's address
space.
Clearly we could solve this problem, the string data exists in GDB, so
there's no reason why we have to push it into the inferior, but this
is an existing problem that would need solving.
2. $_as_string suffers from the fact that C degrades arrays to
pointers, e.g.:
(gdb) printf "%s\n", $_as_string(a1)
0x404260 <a1>
(gdb)
The implementation of $_as_string is passed a gdb.Value object that is
a pointer, it doesn't understand that it's actually an array. Solving
this would be harder than issue #1 I think. The whole array to
pointer transformation is part of our expression evaluation. And in
most cases this is exactly what we want. It's not clear to me how
we'd (easily) tell GDB that we didn't want this reduction in _some_
cases. But I'm sure this is solvable if we really wanted to.
3. $_as_string is a gdb.Function sub-class, and as such is passed
gdb.Value objects. There's no super convenient way to pass formatting
options to $_as_string. By this I mean that the new %V feature
supports print formatting options. Ideally, we might want to add this
feature to $_as_string, we might imagine it working something like:
(gdb) printf "%s\n", $_as_string(a1,
elements = 3,
array_indexes = True)
where the first item is the value to print, while the remaining
options are the print formatting options. However, this relies on
Python calling syntax, which isn't something that convenience
functions handle. We could possibly rely on strictly positional
arguments, like:
(gdb) printf "%s\n", $_as_string(a1, 3, 1)
But that's clearly terrible as there's far more print formatting
options, and if you needed to set the 9th option you'd need to fill in
all the previous options.
And right now, the only way to pass these options to a gdb.Function is
to have GDB first convert them all into gdb.Value objects, which is
really overkill for what we want.
The new %V format solves all these problems: the string is computed
and printed entirely on the GDB side, we are able to print arrays as
actual arrays rather than pointers, and we can pass named format
arguments.
Finally, the $_as_string is sold in the manual as allowing users to
print the string representation of flag enums, so given:
enum flags
{
FLAG_A = (1 << 0),
FLAG_B = (1 << 1),
FLAG_C = (1 << 1)
};
enum flags ff = FLAG_B;
We can:
(gdb) printf "%s\n", $_as_string(ff)
FLAG_B
This works just fine with %V too:
(gdb) printf "%V\n", ff
FLAG_B
So all functionality of $_as_string is replaced by %V. I'm not
proposing to remove $_as_string, there might be users currently
depending on it, but I am proposing that we don't push $_as_string in
the documentation.
As %V is a feature of printf, GDB's dprintf breakpoints naturally gain
access to this feature too. dprintf breakpoints can be operated in
three different styles 'gdb' (use GDB's printf), 'call' (call a
function in the inferior), or 'agent' (perform the dprintf on the
remote).
The use of '%V' will work just fine when dprintf-style is 'gdb'.
When dprintf-style is 'call' the format string and arguments are
passed to an inferior function (printf by default). In this case GDB
doesn't prevent use of '%V', but the documentation makes it clear that
support for '%V' will depend on the inferior function being called.
I chose this approach because the current implementation doesn't place
any restrictions on the format string when operating in 'call' style.
That is, the user might already be calling a function that supports
custom print format specifiers (maybe including '%V') so, I claim, it
would be wrong to block use of '%V' in this case. The documentation
does make it clear that users shouldn't expect this to "just work"
though.
When dprintf-style is 'agent' then GDB does no support the use of
'%V' (right now). This is handled at the point when GDB tries to
process the format string and send the dprintf command to the remote,
here's an example:
Reading symbols from /tmp/hello.x...
(gdb) dprintf call_me, "%V", a1
Dprintf 1 at 0x401152: file /tmp/hello.c, line 8.
(gdb) set sysroot /
(gdb) target remote | gdbserver --once - /tmp/hello.x
Remote debugging using | gdbserver --once - /tmp/hello.x
stdin/stdout redirected
Process /tmp/hello.x created; pid = 3088822
Remote debugging using stdio
Reading symbols from /lib64/ld-linux-x86-64.so.2...
(No debugging symbols found in /lib64/ld-linux-x86-64.so.2)
0x00007ffff7fd3110 in _start () from /lib64/ld-linux-x86-64.so.2
(gdb) set dprintf-style agent
(gdb) c
Continuing.
Unrecognized format specifier 'V' in printf
Command aborted.
(gdb)
This is exactly how GDB would handle any other invalid format
specifier, for example:
Reading symbols from /tmp/hello.x...
(gdb) dprintf call_me, "%Q", a1
Dprintf 1 at 0x401152: file /tmp/hello.c, line 8.
(gdb) set sysroot /
(gdb) target remote | gdbserver --once - /tmp/hello.x
Remote debugging using | gdbserver --once - /tmp/hello.x
stdin/stdout redirected
Process /tmp/hello.x created; pid = 3089193
Remote debugging using stdio
Reading symbols from /lib64/ld-linux-x86-64.so.2...
(No debugging symbols found in /lib64/ld-linux-x86-64.so.2)
0x00007ffff7fd3110 in _start () from /lib64/ld-linux-x86-64.so.2
(gdb) set dprintf-style agent
(gdb) c
Continuing.
Unrecognized format specifier 'Q' in printf
Command aborted.
(gdb)
The error message isn't the greatest, but improving that can be put
off for another day I hope.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Acked-By: Simon Marchi <simon.marchi@efficios.com>
This is the same idea as the previous patch, but for filtered_iterator.
Without this patch, I would see this when applying the patch that
removes reference_to_pointer_iterator from breakpoint_range:
CXX breakpoint.o
/home/smarchi/src/binutils-gdb/gdb/breakpoint.c: In function ‘void download_tracepoint_locations()’:
/home/smarchi/src/binutils-gdb/gdb/breakpoint.c:11007:41: error: cannot allocate an object of abstract type ‘breakpoint’
11007 | for (breakpoint &b : all_tracepoints ())
| ^
In file included from /home/smarchi/src/binutils-gdb/gdb/gdbthread.h:26,
from /home/smarchi/src/binutils-gdb/gdb/infrun.h:21,
from /home/smarchi/src/binutils-gdb/gdb/gdbarch.h:28,
from /home/smarchi/src/binutils-gdb/gdb/arch-utils.h:23,
from /home/smarchi/src/binutils-gdb/gdb/breakpoint.c:21:
/home/smarchi/src/binutils-gdb/gdb/breakpoint.h:619:8: note: because the following virtual functions are pure within ‘breakpoint’:
619 | struct breakpoint : public intrusive_list_node<breakpoint>
| ^~~~~~~~~~
/home/smarchi/src/binutils-gdb/gdb/breakpoint.c:250:1: note: ‘virtual breakpoint::~breakpoint()’
250 | breakpoint::~breakpoint ()
| ^~~~~~~~~~
Change-Id: I05285ff27d21cb0ab80cba392ec4e959167e3cd7
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
Using the following patch that removes the reference_to_pointer_iterator
from breakpoint_range, I would get:
CXX breakpoint.o
/home/smarchi/src/binutils-gdb/gdb/breakpoint.c: In function ‘void breakpoint_program_space_exit(program_space*)’:
/home/smarchi/src/binutils-gdb/gdb/breakpoint.c:3030:46: error: cannot allocate an object of abstract type ‘breakpoint’
3030 | for (breakpoint &b : all_breakpoints_safe ())
| ^
In file included from /home/smarchi/src/binutils-gdb/gdb/gdbthread.h:26,
from /home/smarchi/src/binutils-gdb/gdb/infrun.h:21,
from /home/smarchi/src/binutils-gdb/gdb/gdbarch.h:28,
from /home/smarchi/src/binutils-gdb/gdb/arch-utils.h:23,
from /home/smarchi/src/binutils-gdb/gdb/breakpoint.c:21:
/home/smarchi/src/binutils-gdb/gdb/breakpoint.h:619:8: note: because the following virtual functions are pure within ‘breakpoint’:
619 | struct breakpoint : public intrusive_list_node<breakpoint>
| ^~~~~~~~~~
/home/smarchi/src/binutils-gdb/gdb/breakpoint.c:250:1: note: ‘virtual breakpoint::~breakpoint()’
250 | breakpoint::~breakpoint ()
| ^~~~~~~~~~
This is because the operator* method of the basic_safe_iterator iterator
wrapper returns a value_type. So, even if the method of the underlying
iterator (breakpoint_iterator, an intrusive_list iterator) returns a
`breakpoint &`, the method of the wrapper returns a `breakpoint`.
I think it would make sense for iterator wrappers such as
basic_safe_iterator to return the exact same thing as the iterator they
wrap. At least, it fixes my problem.
Change-Id: Ibbcd390ac03d2fb6ae4854923750c8d7c3c04e8a
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
Using the following patch, I would get this build failure:
CXX breakpoint.o
In file included from /usr/include/c++/13.1.1/bits/stl_algobase.h:66,
from /usr/include/c++/13.1.1/bits/hashtable_policy.h:36,
from /usr/include/c++/13.1.1/bits/hashtable.h:35,
from /usr/include/c++/13.1.1/bits/unordered_map.h:33,
from /usr/include/c++/13.1.1/unordered_map:41,
from /usr/include/c++/13.1.1/functional:63,
from /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/ptid.h:35,
from /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/common-defs.h:206,
from /home/smarchi/src/binutils-gdb/gdb/defs.h:26,
from /home/smarchi/src/binutils-gdb/gdb/breakpoint.c:20:
/usr/include/c++/13.1.1/bits/stl_iterator_base_funcs.h: In instantiation of ‘constexpr void std::__advance(_BidirectionalIterator&, _Distance, bidirectional_iterator_tag) [with _BidirectionalIterator = reference_to_pointer_iterator<intrusive_list_iterator<bp_location, intrusive_base_node<bp_location> > >; _Distance = long int]’:
/usr/include/c++/13.1.1/bits/stl_iterator_base_funcs.h:224:21: required from ‘constexpr void std::advance(_InputIterator&, _Distance) [with _InputIterator = reference_to_pointer_iterator<intrusive_list_iterator<bp_location, intrusive_base_node<bp_location> > >; _Distance = long int]’
/usr/include/c++/13.1.1/bits/stl_iterator_base_funcs.h:237:19: required from ‘constexpr _InputIterator std::next(_InputIterator, typename iterator_traits<_Iter>::difference_type) [with _InputIterator = reference_to_pointer_iterator<intrusive_list_iterator<bp_location, intrusive_base_node<bp_location> > >; typename iterator_traits<_Iter>::difference_type = long int]’
/home/smarchi/src/binutils-gdb/gdb/breakpoint.c:1073:19: required from here
/usr/include/c++/13.1.1/bits/stl_iterator_base_funcs.h:179:11: error: no match for ‘operator--’ (operand type is ‘reference_to_pointer_iterator<intrusive_list_iterator<bp_location, intrusive_base_node<bp_location> > >’)
179 | --__i;
| ^~~~~
This points out that while intrusive_list_iterator has an operator--,
the reference_to_pointer_iterator wrapper does not. I'm not to sure why
the compiler chooses the overload of __advance that accepts a
_BidirectionalIterator, given that reference_to_pointer_iterator can't
be decremented, but adding those operators seems like the right thing to
do in any case, for completeness.
Change-Id: I8e2044b6734fadf0f21093047cf35bb7080dbdc3
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
Add a checked_static_cast overload that works with references. A bad
dynamic cast with references throws std::bad_cast, it would be possible
to implement the new overload based on that, but it seemed simpler to
just piggy back off the existing function.
I found some potential uses of this new overload in amd-dbgapi-target.c,
update them to illustrate the use of the new overload. To build
amd-dbgapi-target.c, on needs the amd-dbgapi library, which I don't
expect many people to have. But I have it, and it builds fine here. I
did test the new overload by making a purposely bad cast and it did
catch it.
Change-Id: Id6b6a7db09fe3b4aa43cddb60575ff5f46761e96
Reviewed-By: Lancelot SIX <lsix@lancelotsix.com>
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
Latest libc++[1] causes transitive include to <locale> when
<mutex> or <thread> header is included. This causes
gdb to not build[2] since <locale> defines isupper/islower etc.
functions that are explicitly macroed-out in safe-ctype.h to
prevent their use.
Use the suggestion from libc++ to include <locale> internally when
building in C++ mode to avoid build errors.
Use safe-gdb-ctype.h as the include instead of "safe-ctype.h"
to keep this isolated to gdb since rest of binutils
does not seem to use much C++.
[1]: https://reviews.llvm.org/D144331
[2]: https://issuetracker.google.com/issues/277967395
It was pointed out during review of another patch that the function
displaced_step_dump_bytes really isn't specific to displaced stepping,
and should really get a more generic name and move into gdbsupport/.
This commit does just that. The function is renamed to
bytes_to_string and is moved into gdbsupport/common-utils.{cc,h}. The
function implementation doesn't really change. Much...
... I have updated the function to take an array view, which makes it
slightly easier to call in a couple of places where we already have a
gdb::bytes_vector. I've then added an inline wrapper to convert a raw
pointer and length into an array view, which is used in places where
we don't easily have a gdb::bytes_vector (or similar).
Updated all users of displaced_step_dump_bytes.
There should be no user visible changes after this commit.
Finally, I ended up having to add an include of gdb_assert.h into
array-view.h. When I include array-view.h into common-utils.h I ran
into build problems because array-view.h calls gdb_assert.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
When building with clang 16, we get:
CXX gdb.o
In file included from /home/smarchi/src/binutils-gdb/gdb/gdb.c:19:
In file included from /home/smarchi/src/binutils-gdb/gdb/defs.h:65:
/home/smarchi/src/binutils-gdb/gdb/../gdbsupport/enum-flags.h:95:52: error: integer value -1 is outside the valid range of values [0, 15] for this enumeration type [-Wenum-constexpr-conversion]
integer_for_size<sizeof (T), static_cast<bool>(T (-1) < T (0))>::type
^
The error message does not make it clear in the context of which enum
flag this fails (i.e. what is T in this context), but it doesn't really
matter, we have similar warning/errors for many of them, if we let the
build go through.
clang is right that the value -1 is invalid for the enum type we cast -1
to. However, we do need this expression in order to select an integer
type with the appropriate signedness. That is, with the same signedness
as the underlying type of the enum.
I first wondered if that was really needed, if we couldn't use
std::underlying_type for that. It turns out that the comment just above
says:
/* Note that std::underlying_type<enum_type> is not what we want here,
since that returns unsigned int even when the enum decays to signed
int. */
I was surprised, because std::is_signed<std::underlying_type<enum_type>>
returns the right thing. So I tried replacing all this with
std::underlying_type, see if that would work. Doing so causes some
build failures in unittests/enum-flags-selftests.c:
CXX unittests/enum-flags-selftests.o
/home/smarchi/src/binutils-gdb/gdb/unittests/enum-flags-selftests.c:254:1: error: static assertion failed due to requirement 'gdb::is_same<selftests::enum_flags_tests::check_valid_expr254::archetype<enum_flags<s
elftests::enum_flags_tests::RE>, selftests::enum_flags_tests::RE, enum_flags<selftests::enum_flags_tests::RE2>, selftests::enum_flags_tests::RE2, enum_flags<selftests::enum_flags_tests::URE>, selftests::enum_fla
gs_tests::URE, int>, selftests::enum_flags_tests::check_valid_expr254::archetype<enum_flags<selftests::enum_flags_tests::RE>, selftests::enum_flags_tests::RE, enum_flags<selftests::enum_flags_tests::RE2>, selfte
sts::enum_flags_tests::RE2, enum_flags<selftests::enum_flags_tests::URE>, selftests::enum_flags_tests::URE, unsigned int>>::value == true':
CHECK_VALID (true, int, true ? EF () : EF2 ())
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/smarchi/src/binutils-gdb/gdb/unittests/enum-flags-selftests.c:91:3: note: expanded from macro 'CHECK_VALID'
CHECK_VALID_EXPR_6 (EF, RE, EF2, RE2, UEF, URE, VALID, EXPR_TYPE, EXPR)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/smarchi/src/binutils-gdb/gdb/../gdbsupport/valid-expr.h:105:3: note: expanded from macro 'CHECK_VALID_EXPR_6'
CHECK_VALID_EXPR_INT (ESC_PARENS (typename T1, typename T2, \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/smarchi/src/binutils-gdb/gdb/../gdbsupport/valid-expr.h:66:3: note: expanded from macro 'CHECK_VALID_EXPR_INT'
static_assert (gdb::is_detected_exact<archetype<TYPES, EXPR_TYPE>, \
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
This is a bit hard to decode, but basically enumerations have the
following funny property that they decay into a signed int, even if
their implicit underlying type is unsigned. This code:
enum A {};
enum B {};
int main() {
std::cout << std::is_signed<std::underlying_type<A>::type>::value
<< std::endl;
std::cout << std::is_signed<std::underlying_type<B>::type>::value
<< std::endl;
auto result = true ? A() : B();
std::cout << std::is_signed<decltype(result)>::value << std::endl;
}
produces:
0
0
1
So, the "CHECK_VALID" above checks that this property works for enum flags the
same way as it would if you were using their underlying enum types. And
somehow, changing integer_for_size to use std::underlying_type breaks that.
Since the current code does what we want, and I don't see any way of doing it
differently, ignore -Wenum-constexpr-conversion around it.
Change-Id: Ibc82ae7bbdb812102ae3f1dd099fc859dc6f3cc2
