| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
Add indent-with-non-tab for *.def in gdb*/.gitattributes.
Fix whitespace in the *.def files in gdb*, and add these files to the clean
list in gdb/contrib/check-whitespace-pre-commit.py.
Tested on x86_64-linux.
Approved-By: Tom Tromey <tom@tromey.com>
|
|
This is the result of:
...
$ git rm -f $(find gdb* -name "*.ac")
$ git rm -f $(find gdb* -name "*.m4")
$ git commit -a -m tmp
$ git revert HEAD
$ git rebase --whitespace=fix HEAD^
$ git reset --soft HEAD^
$ git commit --amend
...
and running autoreconf -f in directories gdb, gdb/testsuite, gdbsupport and
gdbserver.
Tested on x86_64-linux.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
Since commit 52ca3d3fe61 ("toplevel: unify the GCC and GDB/binutils .editorconfig
files"), .editorconfig has settings for .ac and .m4 files:
...
[*.{ac,m4}]
indent_style = tab
indent_size = 2
trim_trailing_whitespace = true
...
There are no setting for those files in .gitattributes, so the whitespace
attribute defaults to trailing-space (shorthand for blank-at-eol,
blank-at-eof) and space-before-tab.
Since according to .editorconfig the indentation style is tab, add
indent-with-non-tab as well.
Since aclocal.m4 is generated, unset the whitespace attribute. Likewise for
configure.
Probably, this eventually needs to be added to root level .gitattributes, but
for now try this in gdb*/.gitattributes.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
Propagate the use of target_desc unique pointers further. Basically,
avoid releasing target_desc objects, except in some spots in gdbserver
(e.g. netbsd) where we don't currently have a persistent container for
created target descriptions (and the target desc just leaks). Introduce
const_target_desc_up to change some `const target_desc *` to a unique
pointer without loss of constness.
Architectures that use the old regformats/regdat.sh don't need to be
changed, because their target_desc objects are statically allocated in
the generated files.
I was able to built-test these native configs:
- Linux AArch64
- Linux ARC
- Linux AMD64
- Linux ARM
- Linux LoongArch
- Linux M68K
- Linux Microblaze
- Linux MIPS (32 and 64)
- Linux or1k
- Linux PPC (32 and 64)
- Linux RISC-V
- Linux s390x
- Linux SH4
- Linux Sparc (32 and 64)
- Linux Xtensa
- FreeBSD AMD64
- NetBSD AMD64
- Windows i686
- Windows AMD64
For the rest, I did my best by staring at the code long enough. I
probably missed or messed some spots, but that shouldn't be difficult to
fix.
Change-Id: I8db8790c57942edd2bfe890987157e2dc0c67879
Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
|
|
Commit 9a5d3e6c4a50 ("gdb: remove tic6x .dat files") removed the .dat
files that were used solely for selftests in linux-tic6x-low.cc.
However, I forgot to remove the things that depended on that in
linux-tic6x-low.cc (or rather, I put them in the wrong patch). As-is,
linux-tic6x-low.cc will certainly no build, because function
selftests::tdesc::tic6x_tdesc_test is not defined anywere. Remove those
remaining bits.
Change-Id: I3f307a6ae4848ef748e2a685a870b662c88180b5
|
|
At Red Hat we have an out of tree AArch64 watchpoint test which broke
after this commit:
commit cf16ab724a41e4cbaf723b5633d4e7b29f61372b
Date: Tue Mar 12 17:08:18 2024 +0100
[gdb/tdep] Fix gdb.base/watch-bitfields.exp on aarch64
The problem with AArch64 hardware watchpoints is that they (as I
understand it) are restricted to a minimum of 8 bytes.
The problem is that current AArch64 hardware has imprecise hardware
watchpoint events due to unaligned accesses. The address reported for
the watchpoint event will depend on the access size. As a result, it
is possible that multiple watchpoints could potentially account for a
single watchpoint event, which is the case in the RH test. GDB can
then miss-identify which watchpoint actually triggered.
Prior to the above commit the RH test was passing. However, the test
was relying on, in the case of ambiguity, GDB selecting the first
created watchpoint. That behaviour changed with the above commit.
Now GDB favours reporting non write breakpoints, and will only report
a write breakpoint if no non-write breakpoint exists in the same
region.
I originally posted a patch to try and tweak the existing logic to
restore enough of the original behaviour that the RH test would pass,
this can be found here (2 iterations):
https://inbox.sourceware.org/gdb-patches/65e746b6394f04faa027e778f733eda95d20f368.1753115072.git.aburgess@redhat.com
https://inbox.sourceware.org/gdb-patches/638cbe9b738c0c529f6370f90ba4a395711f63ae.1753971315.git.aburgess@redhat.com
Neither of these really resolved the problem, they fixed some cases,
but broke others.
Ultimately, the problem on AArch64 is that for a single watchpoint
trap, there could be multiple watchpoints that are potentially
responsible. The existing API defined by the target_ops methods
stopped_by_watchpoint() and stopped_data_address() only allow for two
possible options:
1. If stopped_by_watchpoint() is true then stopped_data_address()
can return true and a single address which identifies all
watchpoints at that single address, or
2. If stopped_by_watchpoint() is true then stopped_data_address()
can return false, in which case GDB will check all write
watchpoints to see if any have changed, if they have, then GDB
tells the user that that was the triggering watchpoint.
If we are in a situation where we have to choose between multiple
write and read watchpoints then the current API doesn't allow the
architecture specific code to tell GDB core about this case.
In this commit I propose that we change the target_ops API,
specifically, the method:
bool target_ops::stopped_data_address (CORE_ADDR *);
will change to:
std::vector<CORE_ADDR> target_ops::stopped_data_addresses ();
The architecture specific code can now return a set of watchpoint
addresses, allowing GDB to identify a set of watchpoints that might
have triggered. GDB core can then select the most likely watchpoint,
and present that to the user.
As with the old API, target_ops::stopped_data_addresses should only be
called when target_ops::stopped_by_watchpoint is true, in which case
it's return values can be interpreted like this:
a. An empty vector; this replaces the old case where false was
returned. GDB should check all the write watchpoints and select
the one that changed as the responsible watchpoint.
b. A single entry vector; all targets except AArch64 currently
return at most a single entry vector. The single address
indicates the watchpoint(s) that triggered.
c. A multi-entry vector; currently AArch64 only. These addresses
indicate the set of watchpoints that might have triggered. GDB
will check the write watchpoints to see which (if any) changed,
and if no write watchpoints changed, GDB will present the first
access watchpoint.
In the future, we might want to improve the handling of (c) so that
GDB tells the user that multiple access watchpoints might have
triggered, and then list all of them. This might clear up some
confusion. But I think that can be done in the future (I don't have
an immediate plan to work on this). I think this change is already a
good improvement.
The changes for this are pretty extensive, but here's a basic summary:
* Within gdb/ changing the API name from stopped_data_address to
stopped_data_addresses throughout. Comments are updated too where
needed.
* For targets other than AArch64, the existing code is retained with
as few changes as possible, we only allow for a single address to
be returned, the address is now wrapped in a vector. Where we
used to return false, we now return the empty vector.
* For AArch64, the return a vector logic is pushed through to
gdb/nat/aarch64-hw-point.{c,h}, and aarch64_stopped_data_address
changes to aarch64_stopped_data_addresses, and is updated to
return a vector of addresses.
* In infrun.c there's some updates to some debug output.
* In breakpoint.c the interesting changes are in
watchpoints_triggered. The existing code has three cases to
handle:
(i) target_stopped_by_watchpoint returns false. This case is
unchanged.
(ii) target_stopped_data_address returns false. This case is now
calling target_stopped_data_addresses, and checks for the
empty vector, but otherwise is unchanged.
(iii) target_stopped_data_address returns true, and a single
address. This code calls target_stopped_data_addresses, and
now handles the possibility of a vector containing multiple
entries. We need to first loop over every watchpoint
setting its triggered status to 'no', then we check every
address in the vector setting matching watchpoint's
triggered status to 'yes'. But the actual logic for if a
watchpoint matches an address or not is unchanged.
The important thing to notice here is that in case (iii), before
this patch, GDB could already set _multiple_ watchpoints to
triggered. For example, setting a read and write watchpoint on
the same address would result in multiple watchpoints being marked
as triggered. This patch just extends this so that multiple
watchpoints, at multiple addresses, can now be marked as
triggered.
* In remote.c there is an interesting change. We need to allow
gdbserver to pass the multiple addresses back to GDB. To achieve
this, I now allow multiple 'watch', 'rwatch', and 'awatch' tokens
in a 'T' stop reply packet. There's a new feature multi-wp-addr
which is passed in the qSupported packet to determine if the
remote is allowed to pass back multiple watchpoint stop reasons.
If the remote passed multiple watchpoint addresses then these are
collected and returned from the target_ops::stopped_data_addresses
call.
If a new GDB connects to an old gdbserver that doesn't understand
the multi-wp-addr feature, then gdbserver will continue to return
a single watchpoint address in the 'T' packet, which is what
happens before this patch.
* In gdbserver/ the changes are pretty similar. The API is renamed
from ::stopped_data_address to ::stopped_data_addresses, and
::low_stopped_data_address to ::low_stopped_data_addresses.
There's also code added to detect the new multi-wp-addr feature.
If this feature is not advertised from GDB then only a single
watchpoint address will be returned in the 'T' stop reply packet.
* In GDB and gdbserver, for all targets except AArch64, the existing
code to figure out a watchpoint address is retained, we just wrap
the single address into a vector.
* For AArch64, we call aarch64_stopped_data_addresses, which returns
the required vector.
For testing, I've built GDB on GNU/Linux for i386, x86-64, PPC64le,
ARM, and AArch64. That still leaves a lot of targets possibly
impacted by this change as untested. Which is a risk. I certainly
wouldn't want to push this patch until after GDB 17 branches so we
have time to find and fix any regressions that are introduced.
I've run a full regression test on AArch64 and x86-64 (both GNU/Linux)
with no regressions. As I said above, for other targets nothing
should really have changed, all non-AArch64 targets just return a
single watchpoint address from target_ops::stopped_data_addresses(),
so, as long as the target builds, it should run unchanged.
I also sent the branch through the sourceware CI, and everything
passed.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33240
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33252
Acked-By: Tom de Vries <tdevries@suse.de>
|
|
This commit extends GDB for x86/Linux to include the NT_I386_TLS note
in generated core files (i.e. created with `generate-core-file` or
`gcore` command). This note contains the 3 per-thread TLS related
GDT (global descriptor table) entries, and is present for i386
binaries, or those compiled on x86-64 with -m32.
The approach I have taken to achieve this, is to make the 3 GDT
entries available within 3 new registers. I added these registers to
the org.gnu.gdb.i386.linux target description feature, as this feature
seemed perfectly named. As the new registers are optional I don't see
any harm in extending this existing feature. I did consider adding a
new feature with `tls` in the name, but this seemed excessive given
the existing feature.
Which GDT entries are used for TLS varies between i386 and x86-64
running in 32-bit mode. As such the registers are named with suffixes
0, 1, and 2, and it is left to GDB or gdbserver, to find the correct
GDT entries (based on the precise target) and place the contents into
these registers.
With this done, adding the relevant regset is sufficient to get the
tls contents emitted as a core file note. Support for emitting the
note into the generated core file relies on some BFD changes which
were made in an earlier commit:
commit ea6ec00ff4520895735e4913cb90c933c7296f04
Date: Fri Jul 25 19:51:58 2025 +0100
bfd: support for NT_386_TLS notes
The three new registers are readable and writable. Writing to one of
the new registers will update the relevant kernel GDT entry.
Each TLS GDT is represented by a 'struct user_desc' (see 'man 2
get_thread_area' for details), the first 4 bytes of each 'user_desc'
is the 'entry_number' field, this is the index of the GDT within the
kernel, and cannot be modified. Attempts to write to this region of
the register will be ignored, but will not give an error.
I did consider not including this part of the user_desc within the
register value, but this becomes difficult when we consider remote
targets, GDB would then need to figure out what these indexes were so
that the core file note could be generated. Sure, we probably could
figure the correct index values out, but I figure, why bother, we can
just pass them through in the register and know for certain that we
have the correct values.
For testing, there's a new test that covers the basic functionality,
including read/write access to the new registers, and checking that
the NT_386_TLS note is added to the core file, and that the note
contents can be read by GDB.
I also manually tested opening a core file generated from an old
GDB (so no NT_386_TLS notes) using a GDB with this patch. This works
fine, the new tls registers are not created as the NT_GDB_TDESC
note (the target description) doesn't include the new registers.
Out of interest I also patched an old version of GDB to avoid creating
the NT_GDB_TDESC, and created a core file. This core file contained
neither the NT_386_TLS nor NT_GDB_TDESC. When opening this core file
with a patched GDB, the new registers do show up, but their contents
are given as <unavailable>, which is exactly what we'd expect, GDB
builds a target description based on the architecture, the
architecture says these registers should exist, but they are missing
from the core file, hence, <unavailable>.
I also tested using a patched GDB with an old version of gdbserver,
the new registers don't show up as the old gdbserver doesn't send them
in its target description. And a core file created using the gcore
command in such a setup leaves no NT_386_TLS notes added, which is
what we'd expect.
And I also tested a new gdbserver running with an old version of GDB.
As the new tls registers are now mentioned in the target description,
then obviously, the old GDB does see the registers, and present them
to the user, however GDB doesn't know how to use these registers to
create a NT_386_TLS, so that note isn't added to any core files.
Also, while a new GDB places the tls registers into the 'system'
group, an old GDB doesn't do this, so the registers end up in the
'general' group by default. This means they show up within 'info
registers' output. This isn't ideal, but there's not much that can be
done about this.
Overall, I feel the combinations of old and new tools has been tested,
and the behaviours are what we'd want or expect.
I'm tagging this commit with PR gdb/15591, even though this patch
isn't directly related. That bug is for improving GDB's testing of
TLS support in core files. The test in this commit does do some very
simple reading of a TLS variable, but there's only two threads, and
one TLS variable, so it's not extensive. Additionally, the test in
this commit is x86 only, so this should not be considered a full
resolution to that bug. But still, it's something.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=15591
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Reviewed-By: Christina Schimpe <christina.schimpe@intel.com>
Reviewed-By: Keith Seitz <keiths@redhat.com>
|
|
Add support for FPMR in gdbserver.
Approved-By: Luis Machado <luis.machado.foss@gmail.com>
|
|
In write_ptid(), a ptid's LWP member, which is declared long, is stored in
an int local variable before printing, potentially truncating it. Fix it.
In read_ptid(), both PID and LWP are read as ULONGEST and then cast to
their respective type without checking for overflows. Fix it.
In read_ptid(), an empty component is treated as zero. Diagnose that as
an error, instead.
Approved-By: Tom Tromey <tom@tromey.com>
|
|
The special thread id '-1' means 'all threads'.
The special thread id '0' means 'any thread'.
Read_ptid () currently returns
<current pid>.-1.0
and
<current pid>.0.0
respectively.
Change that to minus_one_ptid for '-1' and to null_ptid for '0'.
CC: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
Approved-By: Tom Tromey <tom@tromey.com>
|
|
I spotted this call missing an argument.
For context, init_target_desc gained this osabi parameter in this
commit:
Author: Andrew Burgess <aburgess@redhat.com>
AuthorDate: Fri Oct 4 19:30:04 2024 +0100
Commit: Andrew Burgess <aburgess@redhat.com>
CommitDate: Tue Nov 12 12:51:36 2024 +0000
gdbserver: pass osabi to GDB in more target descriptions
This bug was present in GDB 16. I wonder if anybody uses this today.
Change-Id: Id5483be3efa0ca9d238d59af8abae94e8bdbd57c
Approved-By: Tom Tromey <tom@tromey.com>
|
|
The tix6x gdbserver port was modified to use target descriptions in
commit 506fe5f49967 ("Change tic6x target descriptions"). The old
regformats .dat files were kept as a way to make sure the new target
descriptions matched the old register decsriptions. I think by now it's
not necessary to keep the .dat files.
I don't have a way to build-test this though.
Change-Id: Ia90b5ae6381234c6e95555201d3e65ed9be880ea
Approved-By: Tom Tromey <tom@tromey.com>
|
|
We had a customer bug report which was eventually tracked down to
gdbserver not fully sending a target description to gdb. (This
presented as a timeout on the gdb side.)
The customer was using the WINAPI code, which does this:
# define write(fd, buf, len) send (fd, (char *) buf, len, 0)
In this setup, I think it's possible to have a partial write.
However, gdbserver does not account for this possibility, despite the
fact that write_prim documents this.
This patch attempts to fix the problem by always writing the full
buffer in write_prim. In this case the customer fixed their bug in a
different way, so we haven't actually tested this in the wild.
v2: Return bool from write_prim.
Reviewed-by: Kévin Le Gouguec <legouguec@adacore.com>
|
|
I noticed my IDE (VSCode) starting to automatically trim trailing
whitespaces on save, despite the setting for it being disabled. I
realized that this is because the .editorconfig file now has
trim_trailing_whitespace = true
for many file types. If we have this EditorConfig setting forcing
editors to trim trailing whitespaces, I think it would make sense to
clean up trailing whitespaces from our files. Otherwise, people will
always get spurious whitespace changes when editing these files.
I did a mass cleanup using this command:
$ find gdb gdbserver gdbsupport -type f \( \
-name "*.c" -o \
-name "*.h" -o \
-name "*.cc" -o \
-name "*.texi" -o \
-name "*.exp" -o \
-name "*.tcl" -o \
-name "*.py" -o \
-name "*.s" -o \
-name "*.S" -o \
-name "*.asm" -o \
-name "*.awk" -o \
-name "*.ac" -o \
-name "Makefile*" -o \
-name "*.sh" -o \
-name "*.adb" -o \
-name "*.ads" -o \
-name "*.d" -o \
-name "*.go" -o \
-name "*.F90" -o \
-name "*.f90" \
\) -exec sed -ri 's/[ \t]+$//' {} +
I then did an autotools regen, because we don't actually want to change
the Makefile and Makefile.in files that are generated.
Change-Id: I6f91b83e3b8c4dc7d5d51a2ebf60706120efe691
|
|
This commit adds a new remote protocol packet qExecAndArgs, and
updates GDB to use it.
When gdbserver is started a user can provide an executable and
arguments, these are used (by the remote target) to start an initial
inferior, this is the inferior to which GDB first connects.
When GDB is connected in extended-remote mode, if the user does a
'run' without specifying a new 'remote exec-file' then the executable
given on the gdbserver command line is reused to start the new
inferior.
Interestingly, the arguments given on the gdbserver command line are
only used when starting the first inferior, subsequent inferiors will
be passed an empty argument string by GDB. This might catch out a
user, causing the rerun to behave differently than the first run.
In this commit I will add a new qExecAndArgs packet, which I think
will improve the experience in this area.
The new qExecAndArgs packet is sent from GDB, and gdbserver replies
with a packet that includes the executable filename and the arguments
string that were used for starting the initial inferior.
On the GDB side this information can be used to update GDB's state,
the 'show remote exec-file' will reflect how gdbserver was started,
and 'show args' will reflect the arguments used for starting the
inferior.
As a result of updating the args, if the user restarts the inferior,
then this same argument string will be passed back to the remote
target, and used for the new inferior. Thus, rerunning the inferior
will behave just like the initial inferior, which I think is a good
improvement.
Finally, GDB will warn if the user has 'set remote exec-file' and
then connects to a gdbserver that was started with some alternative
filename, like this:
(gdb) set remote exec-file /tmp/foo
(gdb) target remote | gdbserver --once - /tmp/bar
... snip ...
warning: updating 'remote exec-file' to '/tmp/bar' to match remote target
... snip ...
I made the choice to have GDB update the remote exec-file setting to
match the remote, as, after the 'target remote', we are connected to
an inferior that is running /tmp/bar (in this case), so trying to hang
onto the non-matching user supplied setting doesn't seem helpful.
There is one case where I can see this choice being a problem, if a
user does:
(gdb) set remote exec-file /tmp/foo
(gdb) target extended-remote | gdbserver --multi --once - /tmp/bar
... snip ...
warning: updating 'remote exec-file' to '/tmp/bar' to match remote target
... snip ...
(gdb) run
In this case, prior to this patch, they would 'run' /tmp/foo, while
after this patch, they will run /tmp/bar. I think it is unfortunate
that I'm breaking this use case, but, I'm not _that_ sorry -- just
start gdbserver with the correct executable, or even no executable,
and the problem goes away.
This last point is important, in extended-remote mode, it is possible
to start gdbserver without specifying an executable, like this:
$ gdbserver --multi --once :54321
In this case gdbserver doesn't start an initial inferior. When GDB
connects the qExecAndArgs reply from gdbserver indicates that no
information (executable or arguments) were set, and any existing
information is retained, as in this session:
(gdb) set sysroot
(gdb) set remote exec-file /tmp/foo
(gdb) set args a b c
(gdb) target extended-remote | ./gdbserver/gdbserver --multi --once -
Remote debugging using | ./gdbserver/gdbserver --multi --once -
Remote debugging using stdio
(gdb) show remote exec-file
The remote exec-file is "/tmp/foo".
(gdb) show args
Argument list to give program being debugged when it is started is "a b c".
(gdb)
This is the second time proposing this new packet. The first attempt
can be found here:
https://inbox.sourceware.org/gdb-patches/80d8b37d757033976b1a8ddd370c294c7aae8f8c.1692200989.git.aburgess@redhat.com
The review feedback on this patch was that the inferior arguments
should be passed back as a vector of individual strings. This makes
sense, at the time that feedback was given, GDB would pass arguments
to gdbserver as a vector of individual arguments, so it would seem
sensible that gdbserver should adopt the same approach for passing
arguments back to GDB.
However, since then I have been working on how GDB passes the inferior
arguments to gdbserver, fixing a lot of broken corner cases, which
culminated in this patch:
commit 8e28eef6cdcbd86ad61325ce1e6bd563b0fad1e1
Date: Thu Nov 23 18:46:54 2023 +0000
gdb/gdbserver: pass inferior arguments as a single string
Though we do retain the vector of individual arguments behaviour for
backward compatibility with old remote targets, the preferred approach
now is for GDB to pass arguments to gdbserver as a single string.
This removes the need for GDB/gdbserver to try and figure out what is
the correct escaping to apply to the arguments, and fixes some
argument passing corner cases.
And so, now, I think it makes sense that gdbserver should also pass
the arguments back to GDB as a single string. I've updated the
documentation a little to (I hope) explain how gdbserver should escape
things before passing them back to GDB (TLDR: no additional escaping
should be added just for sending to GDB. The argument string should
be sent to GDB as if it were being sent to the 'set args' GDB
command).
The main test for this new functionality is
gdb.server/fetch-exec-and-args.exp, but I've also added a test
gdb.replay/fetch-exec-and-args.exp, which allows me to test a corner
case that isn't currently exercised by gdbserver, this is the case for
sending pack inferior arguments, but no executable.
The qExecAndArgs reply format is 'S;exec;args;' where 'exec' and
'args' are hex encoded strings. If 'args' is empty then this is
perfectly valid, this just means there were no command line
arguments. But what if 'exec' is empty? I needed to decide what to
do in this case. The easiest choice is to treat empty 'exec' as the
executable is not set. But currently, due to how gdbserver works, it
is not possible to hit this case, so I used the gdbreplay testing
framework to exercise this instead. There were a few supporting
changes needed to write this test though.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Approved-By: Tom Tromey <tom@tromey.com>
|
|
While testing another patch I'm working on I discovered that passing
an empty program name to gdbserver would trigger an assertion, like
this:
$ gdbserver --multi :54321 ""
../../gdb/gdbserver/../gdb/nat/fork-inferior.c:240: A problem internal to GDBserver has been detected.
fork_inferior: Assertion `exec_file != nullptr' failed.
User input, no matter how weird, shouldn't be triggering an assertion,
so lets fix that.
In extended mode, it is valid to start gdbserver without an executable
name, like this:
$ gdbserver --multi :54321
Here gdbserver doesn't start an inferior, and it is up to GDB to
connect, and tell gdbserver what to run, and to then start it running.
I did wonder if the empty string case should handled like the no
executable name case, but then you get into the situation where the
user can specify command line arguments without an inferior, like:
$ gdbserver --multi :54321 "" a b c
And while there's nothing really wrong with this, and I'm sure someone
could come up with a use case for it. I'd like to propose that for
now at least, we take the simple approach of not allowing an empty
executable name, instead we should give an error, like this:
$ gdbserver --multi :54321 ""
No program to debug
Exiting
We can always relax this requirement in the future, and allow the
empty executable with or without inferior arguments, if we decide
there's a compelling reason for it. It would be simple enough to add
this in the future, but once we add support for it, it's much harder
to remove the feature in the future, so lets start simple.
The non-extended remote case works much the same. It too triggers the
assertion currently, and after this patch exits with the same error.
Of course, the non-extended remote case never supported not having an
inferior, if you did:
$ gdbserver :54321
You'd be shown the usage text and gdbserver would exit.
Approved-By: Tom Tromey <tom@tromey.com>
|
|
It affects following files:
- gdb/riscv-linux-tdep.c: a function to get syscall number.
- gdbserver/linux-riscv-low.cc: syscall trapinfo function to enable
catching syscalls on remote targets.
- gdb/syscalls/riscv-linux.xml.in: a file with syscalls, generated from
linux kernel sources using gdb/syscalls/update-linux-from-src.sh script.
- gdb/syscalls/riscv-linux.xml: a file with syscalls, patched with group
names gdb/syscalls/apply-defaults.xsl using xsltproc tool.
- gdb/syscalls/update-linux.sh: set startyear to 2025 on riscv.
- gdb/syscalls/update-linux-from-src.sh: riscv syscall table must be
generated from kernel headers.
- gdb/NEWS: catch-syscall feature is now available on riscv.
- gdb/data-directory/Makefile.in: adding file with syscalls to Makefile.
Approved-By: Tom Tromey <tom@tromey.com>
|
|
Codespell noticed two new typos:
...
$ pre-commit run --all-files codespell
codespell................................................................Failed
- hook id: codespell
- exit code: 65
gdbserver/server.cc:4255: errror ==> error
gdb/testsuite/gdb.replay/missing-thread.exp:87: Whn ==> When
...
Fix these.
|
|
By passing ':' within the optstring to getopt_long, the getopt_long
call will now return ':' for missing value errors and '?' for unknown
argument errors, rather than returning '?' for all error types.
We can now print a different error message for missing argument
values. For example:
$ gdbserver --debug-file :54321 /tmp/hello
Missing argument value for: --debug-file
Compared to:
$ gdbserver --unknown :54321 ~/tmp/hello.x
Unknown argument: --unknown
Current HEAD gdbserver treats every error as the 'Unknown argument'
error.
While I was messing with the code that prints these error messages,
I've wrapped then with _(...) to allow for internationalisation.
Approved-By: Tom Tromey <tom@tromey.com>
|
|
Now that we use getopt_long for argument processing in gdbserver, it
is relatively easy to support GNU style arguments, that is, arguments
passed without an '=' between the argument and the value.
As support for GNU style arguments is the default from getopt_long,
the first part of this commit is to remove the code which deliberately
disables the GNU argument support.
With that done, we now need to consider optional arguments. In this
case, getopt_long doesn't automatically grab the next word from ARGV
to be the argument value, so I've added some code to do this.
I've also tried to make this code a little smart. As the first
argument passed to gdbserver that doesn't have a '--' at the start is
the PORT number, the new code block I've added tries to spot if the
argument value might be the port number. If it is, then we don't
allow the port number to become the argument value, and instead, we
pretend the argument value is missing. This seems to give better
error messages.
There are going to be UI changes in how gdbserver handles incorrect
arguments after this commit. However, the behaviour for valid
command lines should be unchanged.
Approved-By: Tom Tromey <tom@tromey.com>
|
|
Remove the use of xmalloc (and the arbitrary allocation size) in
format_pieces. This turned out a bit more involved than expected, but
not too bad.
format_pieces::m_storage is a buffer with multiple concatenated
null-terminated strings, referenced by format_piece::string. Change
this to an std::string, while keeping its purpose (use the std::string
as a buffer with embedded null characters).
However, because the std::string's internal buffer can be reallocated as
it grows, and I do not want to hardcode a big reserved size like we have
now, it's not possible to store the direct pointer to the string in
format_piece::string. Those pointers would become stale as the buffer
gets reallocated. Therefore, change format_piece to hold an index into
the storage instead. Add format_pieces::piece_str for the callers to be
able to access the piece's string. This requires changing the few
callers, but in a trivial way.
The selftest also needs to be updated. I want to keep the test cases
as-is, where the expected pieces contain the expected string, and not
hard-code an expected index. To achieve this, add the
expected_format_piece structure. Note that the previous
format_piece::operator== didn't compare the n_int_args fields, while the
test provides expected values for that field. I guess that was a
mistake. The new code checks it, and the test still passes.
Change-Id: I80630ff60e01c8caaa800ae22f69a9a7660bc9e9
Reviewed-By: Keith Seitz <keiths@redhat.com>
|
|
Found by the codespell pre-commit hook.
Change-Id: Iafadd9485ce334c069dc8dbdab88ac3fb5fba674
|
|
GDB holds the inferior arguments as a single string. Currently when
GDB needs to pass the inferior arguments to a remote target as part of
a vRun packet, this is done by splitting the single argument string
into its component arguments by calling gdb::remote_args::split, which
uses the gdb_argv class to split the arguments for us.
The same gdb_argv class is used when the user has asked GDB/gdbserver
to start the inferior without first invoking a shell; the gdb_argv
class is used to split the argument string into it component
arguments, and each is passed as a separate argument to the execve
call which spawns the inferior.
There is however, a problem with using gdb_argv to split the arguments
before passing them to a remote target. To understand this problem we
must first understand how gdb_argv is used when invoking an inferior
without a shell.
And to understand how gdb_argv is used to start an inferior without a
shell, I feel we need to first look at an example of starting an
inferior with a shell.
Consider these two cases:
(a) (gdb) set args \$VAR
(b) (gdb) set args $VAR
When starting with a shell, in case (a) the user expects the inferior
to receive a literal '$VAR' string as an argument, while in case (b)
the user expects to see the shell expanded value of the variable $VAR.
If the user does 'set startup-with-shell off', then in (a) GDB will
strip the '\' while splitting the arguments, and the inferior will be
passed a literal '$VAR'. In (b) there is no '\' to strip, so also in
this case the inferior will receive a literal '$VAR', remember
startup-with-shell is off, so there is no shell that can ever expand
$VAR.
Notice, that when startup-with-shell is off, we end up with a many to
one mapping, both (a) and (b) result in the literal string $VAR being
passed to the inferior. I think this is the correct behaviour in this
case.
However, as we use gdb_argv to split the remote arguments we have the
same many to one mapping within the vRun packet. But the vRun packet
will be used when startup-with-shell is both on and off. What this
means is that when gdbserver receives a vRun packet containing '$VAR'
it doesn't know if GDB actually had '$VAR', or if GDB had '\$VAR'.
And this is a huge problem.
We can address this by making the argument splitting for remote
targets smarter, and I do have patches that try to do this in this
series:
https://inbox.sourceware.org/gdb-patches/cover.1730731085.git.aburgess@redhat.com
That series was pretty long, and wasn't getting reviewed, so I'm
pulling the individual patches out and posting them separately.
This patch doesn't try to improve remote argument splitting. I think
that splitting and then joining the arguments is a mistake which can
only introduce problems. The patch in the above series which tries to
make the splitting and joining "smarter" handles unquoted, single
quoted, and double quoted strings. But that doesn't really address
parameter substitution, command substitution, or arithmetic expansion.
And even if we did try to address these cases, what rules exactly
would we implement? Probably POSIX shell rules, but what if the
remote target doesn't have a POSIX shell? The only reason we're
talking about which shell rules to follow is because the splitting and
joining logic needs to mirror those rules. If we stop splitting and
joining then we no longer need to care about the target's shell.
Clearly, for backward compatibility we need to maintain some degree of
argument splitting and joining as we currently have; and that's why I
have a later patch (see the series above) that tries to improve that
splitting and joining a little. But I think, what we should really
do, is add a new feature flag (as used by the qSupported packet) and,
if GDB and the remote target agree, we should pass the inferior
arguments as a single string.
This solves all our problems. In the startup with shell case, we no
longer need to worry about splitting at all. The arguments are passed
unmodified to the remote target, that can then pass the arguments to
the shell directly.
In the 'startup-with-shell off' case it is now up to the remote target
to split the arguments, though in gdbserver we already did this, so
nothing really changes in this case. And if the remote target doesn't
have a POSIX shell, well GDB just doesn't need to worry about it!
Something similar to this was originally suggested in this series:
https://inbox.sourceware.org/gdb-patches/20211022071933.3478427-1-m.weghorn@posteo.de/
though this series didn't try to maintain backward compatibility,
which I think is an issue that my patch solves. Additionally, this
series only passed the arguments as a single string in some cases,
I've simplified this so that, when GDB and the remote agree, the
arguments are always passed as a single string. I think this is a
little cleaner.
I've also added documentation and some tests with this commit,
including ensuring that we test both the new single string approach,
and the fallback split/join approach.
I've credited the author of the referenced series as co-author as they
did come to a similar conclusion, though I think my implementation is
different enough that I'm happy to list myself as primary author.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28392
Co-Authored-By: Michael Weghorn <m.weghorn@posteo.de>
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Tested-By: Guinevere Larsen <guinevere@redhat.com>
Approved-by: Kevin Buettner <kevinb@redhat.com>
|
|
This introduces a new '--no-escape-args' option for gdb and gdbserver.
I (Andrew Burgess) have based this patch from work done in this
series:
https://inbox.sourceware.org/gdb-patches/20211022071933.3478427-1-m.weghorn@posteo.de/
I have changed things slightly from the original series. I think this
work is close enough that I've left the original author (Michael) in
place and added myself as co-author. Any bugs introduced by my
modifications to the original patch should be considered mine. I've
also added documentation and tests which were missing from the
originally proposed patch.
When the startup-with-shell option is enabled, arguments passed
directly as 'gdb --args <args>' or 'gdbserver <args>', are by default
escaped so that they are passed to the inferior as passed on the
command line, no globbing or variable substitution happens within the
shell GDB uses to start the inferior.
For gdbserver, this is the case since commit:
commit bea571ebd78ee29cb94adf648fbcda1e109e1be6
Date: Mon May 25 11:39:43 2020 -0400
Use construct_inferior_arguments which handles special chars
Only arguments set via 'set args <args>', 'run <args>', or through the
Python API are not escaped in standard upstream GDB right now.
For the 'gdb --args' case, directly setting unescaped args on gdb
invocation is possible e.g. by using the "--eval-command='set args
<args>'", while this possibility does not exist for gdbserver.
This commit adds a new '--no-escape-args' command line option for GDB
and gdbserver. This option is used with GDB as a replacement for the
current '--args' option, and for gdbserver this new option is a flag
which changes how gdbserver handles inferior arguments on the command
line. When '--no-escape-args' is used inferior arguments passed on
the command line will not have escaping added by GDB or gdbserver.
For gdbserver, using this new option allows having the behaviour from
before commit bea571ebd78ee29cb94adf648fbcda1e109e1be6, while keeping
the default behaviour unified between GDB and GDBserver.
For GDB the --no-escape-args option can be used as a replacement for
--args, like this:
shell> gdb --no-escape-args my-program arg1 arg2 arg3
While for gdbserver, the --no-escape-args option is a flag, which can
be used like:
shell> gdbserver --no-escape-args --once localhost:54321 \
my-program arg1 arg2 arg3
Co-Authored-By: Andrew Burgess <aburgess@redhat.com>
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28392
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Tested-By: Guinevere Larsen <guinevere@redhat.com>
|
|
PR ada/33217 points out that gdb incorrectly calls the <ctype.h>
functions. In particular, gdb feels free to pass a 'char' like:
char *str = ...;
... isdigit (*str)
This is incorrect as isdigit only accepts EOF and values that can be
represented as 'unsigned char' -- that is, a cast is needed here to
avoid undefined behavior when 'char' is signed and a character in the
string might be sign-extended. (As an aside, I think this API seems
obviously bad, but unfortunately this is what the standard says, and
some systems check this.)
Rather than adding casts everywhere, this changes all the code in gdb
that uses any <ctype.h> API to instead call the corresponding c-ctype
function.
Now, c-ctype has some limitations compared to <ctype.h>. It works as
if the C locale is in effect, so in theory some non-ASCII characters
may be misclassified. This would only affect a subset of character
sets, though, and in most places I think ASCII is sufficient -- for
example the many places in gdb that check for whitespace.
Furthermore, in practice most users are using UTF-8-based locales,
where these functions aren't really informative for non-ASCII
characters anyway; see the existing workarounds in gdb/c-support.h.
Note that safe-ctype.h cannot be used because it causes conflicts with
readline.h. And, we canot poison the <ctype.h> identifiers as this
provokes errors from some libstdc++ headers.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33217
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
This changes gdb and related programs to use the gnulib c-ctype code
rather than safe-ctype.h. The gdb-safe-ctype.h header is removed.
This changes common-defs.h to include the c-ctype header, making it
available everywhere in gdb.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33217
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
Add the org.gnu.gdb.aarch64.gcs feature with the GCSPR register, and the
org.gnu.gdb.aarch64.gcs.linux feature with "registers" to represent the
Linux kernel ptrace and prctl knobs that enable and lock specific GCS
functionality.
This code supports GCS only in Linux userspace applications, so the
GCSPR that is exposed is the one at EL0.
Also, support for calling inferior functions is enabled by adding an
implementation for the shadow_stack_push gdbarch method.
If for some reason a target description contains the
org.gnu.gdb.aarch64.gcs feature but not the
org.gnu.gdb.aarch64.gcs.linux feature then GCS support is disabled and
GDB continues the debugging session. Features that need GCS
support (for example, calling inferior functions) will not work and the
inferior will get a segmentation fault signal instead. There's a
testcase for this scenario but it only checks the native debugging case,
even though in practice this problem would only occur in remote
debugging with a broken stub or gdbserver. I tested manually with a
gdbserver hacked to send a broken target description and it worked as
described.
Testcases gdb.arch/aarch64-gcs.exp, gdb.arch/aarch64-gcs-core.exp and
gdb.arch/aarch64-gcs-wrong-tdesc.exp are included to cover the added
functionality.
Reviewed-By: Christina Schimpe <christina.schimpe@intel.com>
Approved-By: Luis Machado <luis.machado@arm.com>
|
|
This patch adds the user mode register PL3_SSP which is part of the
Intel(R) Control-Flow Enforcement Technology (CET) feature for support
of shadow stack.
For now, only native and remote debugging support for shadow stack
userspace on amd64 linux are covered by this patch including 64 bit and
x32 support. 32 bit support is not covered due to missing Linux kernel
support.
This patch requires fixing the test gdb.base/inline-frame-cycle-unwind
which is failing in case the shadow stack pointer is unavailable.
Such a state is possible if shadow stack is disabled for the current thread
but supported by HW.
This test uses the Python unwinder inline-frame-cycle-unwind.py which fakes
the cyclic stack cycle by reading the pending frame's registers and adding
them to the unwinder:
~~~
for reg in pending_frame.architecture().registers("general"):
val = pending_frame.read_register(reg)
unwinder.add_saved_register(reg, val)
return unwinder
~~~
However, in case the python unwinder is used we add a register (pl3_ssp) that is
unavailable. This leads to a NOT_AVAILABLE_ERROR caught in
gdb/frame-unwind.c:frame_unwind_try_unwinder and it is continued with standard
unwinders. This destroys the faked cyclic behavior and the stack is
further unwinded after frame 5.
In the working scenario an error should be triggered:
~~~
bt
0 inline_func () at /tmp/gdb.base/inline-frame-cycle-unwind.c:49^M
1 normal_func () at /tmp/gdb.base/inline-frame-cycle-unwind.c:32^M
2 0x000055555555516e in inline_func () at /tmp/gdb.base/inline-frame-cycle-unwind.c:45^M
3 normal_func () at /tmp/gdb.base/inline-frame-cycle-unwind.c:32^M
4 0x000055555555516e in inline_func () at /tmp/gdb.base/inline-frame-cycle-unwind.c:45^M
5 normal_func () at /tmp/gdb.base/inline-frame-cycle-unwind.c:32^M
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
(gdb) PASS: gdb.base/inline-frame-cycle-unwind.exp: cycle at level 5: backtrace when the unwind is broken at frame 5
~~~
To fix the Python unwinder, we simply skip the unavailable registers.
Also it makes the test gdb.dap/scopes.exp fail. The shadow stack feature is
disabled by default, so the pl3_ssp register which is added with my CET
shadow stack series will be shown as unavailable and we see a TCL error:
~~
>>> {"seq": 12, "type": "request", "command": "variables", "arguments": {"variablesReference": 2, "count": 85}}
Content-Length: 129^M
^M
{"request_seq": 12, "type": "response", "command": "variables", "success": false, "message": "value is not available", "seq": 25}FAIL: gdb.dap/scopes.exp: fetch all registers success
ERROR: tcl error sourcing /tmp/gdb/testsuite/gdb.dap/scopes.exp.
ERROR: tcl error code TCL LOOKUP DICT body
ERROR: key "body" not known in dictionary
while executing
"dict get $val body variables"
(file "/tmp/gdb/testsuite/gdb.dap/scopes.exp" line 152)
invoked from within
"source /tmp/gdb/testsuite/gdb.dap/scopes.exp"
("uplevel" body line 1)
invoked from within
"uplevel #0 source /tmp/gdb/testsuite/gdb.dap/scopes.exp"
invoked from within
"catch "uplevel #0 source $test_file_name" msg"
UNRESOLVED: gdb.dap/scopes.exp: testcase '/tmp/gdb/testsuite/gdb.dap/scopes.exp' aborted due to Tcl error
~~
I am fixing this by enabling the test for CET shadow stack, in case we
detect that the HW supports it:
~~~
# If x86 shadow stack is supported we need to configure GLIBC_TUNABLES
# such that the feature is enabled and the register pl3_ssp is
# available. Otherwise the reqeust to fetch all registers will fail
# with "message": "value is not available".
if { [allow_ssp_tests] } {
append_environment GLIBC_TUNABLES "glibc.cpu.hwcaps" "SHSTK"
}
~~~
Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Approved-By: Luis Machado <luis.machado@arm.com>
Approved-By: Andrew Burgess <aburgess@redhat.com>
|
|
The XSAVE function set is organized in state components, which are a set of
registers or parts of registers. So-called XSAVE-supported features are
organized using state-component bitmaps, each bit corresponding to a
single state component.
The Intel Software Developer's Manual uses the term xstate_bv for a
state-component bitmap, which is defined as XCR0 | IA32_XSS. The control
register XCR0 only contains a state-component bitmap that specifies user state
components, while IA32_XSS contains a state-component bitmap that specifies
supervisor state components.
Until now, XCR0 is used as input for target description creation in GDB.
However, a following patch will add userspace support for the CET shadow
stack feature by Intel. The CET state is configured in IA32_XSS and consists
of 2 state components:
- State component 11 used for the 2 MSRs controlling user-mode
functionality for CET (CET_U state)
- State component 12 used for the 3 MSRs containing shadow-stack pointers
for privilege levels 0-2 (CET_S state).
Reading the CET shadow stack pointer register on linux requires a separate
ptrace call using NT_X86_SHSTK. To pass the CET shadow stack enablement
state we would like to pass the xstate_bv value instead of xcr0 for target
description creation. To prepare for that, we rename the xcr0 mask
values for target description creation to xstate_bv. However, this
patch doesn't add any functional changes in GDB.
Future states specified in IA32_XSS such as CET will create a combined
xstate_bv_mask including xcr0 register value and its corresponding bit in
the state component bitmap. This combined mask will then be used to create
the target descriptions.
Reviewed-By: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
Approved-By: Luis Machado <luis.machado@arm.com>
|
|
On x86 the PTRACE_GETREGSET request is currently only used for the xstate regset.
The size of the xstate regset is initialized to 0 such that it can be reset to
the appropriate size once we know it is supported for the current target
in x86_linux_read_description.
However, this configuration would not just affect the xstate regset but any regset
with PTRACE_GETREGSET request that is added in the future. The new regset would be
misconfigured with the xstate regset size. To avoid this we add an assert for
unsupported regsets and check explicitly for the note type of the register set.
Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
Approved-By: Luis Machado <luis.machado@arm.com>
|
|
Some register sets can be activated and deactivated by the OS during the runtime of
a process. One example register is the Intel CET shadow stack pointer. This patch
adds a new type of register set to handle such cases. We shouldn't deactivate these
regsets and should not show a warning if the register set is not active but supported
by the kernel. However, it is safe to deactivate them, if they are unsupported by the
kernel. To differentiate those scenarios we can use the errno returned by the ptrace
call.
Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
Approved-By: Luis Machado <luis.machado@arm.com>
|
|
Stop using AC_HEADER_STDC since it is no longer supported in autoconf
2.72+. We require a C++ compiler that supports c++17, it's probably
safe to assume that the C compiler fully supports C89.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
The copyright notices printed by these programs still use year 2024.
Update to 2025.
It seems like a trivial patch to me, but I am posting it for review, in
case there's something wrong in the new-year process that caused them to
be missed, in which case we should address that too.
Change-Id: I7d9541bb154b8000e66cfead4e4228e33c49f18b
Approved-by: Kevin Buettner <kevinb@redhat.com>
|
|
This patch gives gdbserver the ability to omit the 's' reply to
'vCont?'. This tells gdb that hardware single-step is definitely not
supported, causing it to fall back to using software single-step.
This is useful for testing the earlier change to
maybe_software_singlestep.
Approved-By: Andrew Burgess <aburgess@redhat.com>
|
|
Update gdbserver to use getopt_long for argument processing. This
turned out to be easier than I expected.
Interesting parts of this patch are:
I pass '+' as the OPTSTRING to getopt_long, this prevents getopt_long
from reordering the contents of ARGV. This is needed so that things
like this will work:
gdbserver :54321 program --arg1 --arg2
Without the '+', getopt_long will reorder ARGV, moving '--arg1' and
'--arg2' forward and handling them as arguments to gdbserver.
Because of this (not reordering) and to maintain backward
compatibility, we retain a special case to deal with '--attach' which
can appear after the PORT, like this:
gdbserver :54321 --attach PID
I did consider adding a warning to this special case, informing the
user that placing --attach after the PORT was deprecated, but in the
end I didn't want to really change the behaviour as part of this
commit, so I've left that as an optional change for the future.
The getopt_long function supports two value passing forms, there is
'--option=value', and also '--option value'. Traditionally, gdbserver
only supports the first of these. To maintain this behaviour, after
the call to getopt_long, I spot if '--option value' was used, and if
so, modify the state so that it seems that no value was passed, and
that 'value' is the next ARGV entry to be parsed. We could, possibly,
remove this code in the future, but that would be a functional change,
which is not something I want in this commit.
Handling of "-" for stdio connection has now moved out of the argument
processing loop as '-' isn't considered a valid option by getopt_long,
this is an improvement as all PORT handling is now in one place.
I've tried as much as possible to leave user visible functionality
unchanged after this commit, and as far as I can tell from testing,
nothing has changed.
Approved-By: Tom Tromey <tom@tromey.com>
|
|
When using the command:
$ gdbserver --disable-packet
gdbserver lists all the packets that can be disabled, and then exits.
I think that this output is a helpful error message that is printed
when the user has forgotten to entry a packet name. We get similar
output if we run the command:
$ gdbserver --disable-packet=foo
where gdbserver tells us that 'foo' is invalid, and then lists the
packets that we can use.
The difference is that, in the first case, gdbserver exits with a code
of 0, and in the second, gdbserver exits with a code of 1.
I think both these cases should exit with a code of 1.
With the exception of '--help' and '--version', where we are asking
gdbserver to print some message then exit (which are, and should exit
with a code of 0), in all other cases where we do an early exit, I
think this is an indication that the user has done something
wrong (entered and invalid argument, or missed an argument value), and
gdbserver should exit with a non-zero exit code to indicate this.
This commit updates the exit code in the above case from 0 to 1.
Approved-By: Tom Tromey <tom@tromey.com>
|
|
Within gdbserver/server.cc, in captured_main, convert some locals to
bool. Move the declaration of some locals into the body of the
function.
There should be no user visible changes after this commit.
Approved-By: Tom Tromey <tom@tromey.com>
|
|
The armhf buildbot fails to build GDB, with:
../../binutils-gdb/gdbserver/server.cc: In function ‘void handle_general_set(char*)’:
../../binutils-gdb/gdbserver/server.cc:1021:23: error: loop variable ‘<structured bindings>’ creates a copy from type ‘const std::pair<thread_info*, enum_flags<gdb_thread_option> >’ [-Werror=range-loop-construct]
1021 | for (const auto [thread, options] : set_options)
| ^~~~~~~~~~~~~~~~~
../../binutils-gdb/gdbserver/server.cc:1021:23: note: use reference type to prevent copying
1021 | for (const auto [thread, options] : set_options)
| ^~~~~~~~~~~~~~~~~
| &
I did not use a reference on purpose, because the pair is very small. I
don't see the problem when building on amd64, I presume it is because
the pair is considered too big to copy on a 32-bit architecture, but not
on a 64-bit architecture.
In any case, fix it by adding a reference.
Change-Id: I8e95235d6e53f032361950cf6e0c7d46b082f951
|
|
I wanted to change one of these, so I searched for more similar
instances, while at it. I think this looks a bit tidier, over unpacking
the pairs by hand.
Change-Id: Ife4b678f7a6aed01803434197c564d2ab93532a7
Approved-By: Tom Tromey <tom@tromey.com>
|
|
This commit works around a problem introduced by commit:
commit e58beedf2c8a1e0c78e0f57aeab3934de9416bfc
Date: Tue Jan 23 16:00:59 2024 +0000
gdb: attach to a process when the executable has been deleted
The above commit extended GDB for Linux, so that, of the executable
for a process had been deleted, GDB would instead try to use
/proc/PID/exe as the executable.
This worked by updating linux_proc_pid_to_exec_file to introduce the
/proc/PID/exe fallback. However, the result of
linux_proc_pid_to_exec_file is then passed to exec_file_find to
actually find the executable, and exec_file_find, will take into
account the sysroot. In addition, if GDB is attaching to a process in
a different MNT and/or PID namespace then the executable lookup is
done within that namespace.
This all means two things:
1. Just because linux_proc_pid_to_exec_file cannot see the
executable doesn't mean that GDB is actually going to fail to
find the executable, and
2. returning /proc/PID/exe isn't useful if we know GDB is then going
to look for this within a sysroot, or within some other
namespace (where PIDs might be different).
There was an initial attempt to fix this issue here:
https://inbox.sourceware.org/gdb-patches/20250511141517.2455092-4-kilger@sec.in.tum.de/
This proposal addresses the issue in PR gdb/32955, which is all about
the namespace side of the problem. The fix in this original proposal
is to check the MNT namespace inside linux_proc_pid_to_exec_file, and
for the namespace problem this is fine. But we should also consider
the sysroot problem.
And for the sysroot problem, the fix cannot fully live inside
linux_proc_pid_to_exec_file, as linux_proc_pid_to_exec_file is shared
between GDB and gdbserver, and gdbserver has no sysroot.
And so, I propose a slightly bigger change.
Now, linux_proc_pid_to_exec_file takes a flag which indicates if
GDB (or gdbserver) will look for the inferior executable in the
local file system, where local means the same file system as GDB (or
gdbserver) is running in.
This local file system check is true if:
1. The MNT namespace of the inferior is the same as for GDB, and
2. for GDB only, the sysroot must either be empty, or 'target:'.
If the local file system check is false then GDB (or gdbserver) is
going to look elsewhere for the inferior executable, and so, falling
back to /proc/PID/exe should not be done, as GDB will end up looking
for this file in the sysroot, or within the alternative MNT
namespace (which in also likely to be a different PID namespace).
Now this is all a bit of a shame really. It would be nice if
linux_proc_pid_to_exec_file could return /proc/PID/exe in such a way
that exec_file_find would know that the file should NOT be looked for
in the sysroot, or in the alternative namespace. But fixing that
problem would be a much bigger change, so for now lets just disable
the /proc/PID/exe fallback for cases where it might not work.
For testing, the sysroot case is now tested.
I don't believe we have any alternative namespace testing. It would
certainly be interesting to add some, but I'm not proposing any with
this patch, so the code for checking the MNT namespace has been tested
manually by me, but isn't covered by a new test I'm adding here.
Author of the original fix is listed as co-author here. Credit for
identifying the original problem, and proposing a solution belongs to
them.
Co-Authored-By: Fabian Kilger <kilger@sec.in.tum.de>
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32955
|
|
Tom de Vries reported a build failure on x86_64-w64-mingw32 after
commit:
commit bd389c9515d240f55b117075b43184efdea41287
Date: Wed Jun 11 22:52:16 2025 +0200
gdb: implement linux namespace support for fileio_lstat and vFile::lstat
The build failure looks like this:
../../src/gdbserver/hostio.cc: In function 'void handle_lstat(char*, int*)':
../../src/gdbserver/hostio.cc:544:63: error: cannot convert '_stat64*' to 'stat*'
544 | ret = the_target->multifs_lstat (hostio_fs_pid, filename, &st);
| ^~~
| |
| _stat64*
In file included from ./../../src/gdbserver/server.h:58,
from <command-line>:
./../../src/gdbserver/target.h:448:74: note: initializing argument 3 of 'virtual int process_stratum_target::multifs_lstat(int, const char*, stat*)'
448 | virtual int multifs_lstat (int pid, const char *filename, struct stat *sb);
| ~~~~~~~~~~~~~^~
The problem is that in sys/stat.h for mingw, 'stat' is #defined to
_stat64, but target.h doesn't include sys/stat.h, and so doesn't see
this #define.
However, target.h does, by luck, manages to see the actual definition
of 'struct stat', which isn't in sys/stat.h itself, but is in some
other header that just happens to be pulled in by chance.
As a result of all this, the declaration of
process_stratum_target::multifs_lstat in target.h uses 'struct stat'
for its argument type, while the call in hostio.cc, uses 'struct
_stat64' as its argument type, which causes the build error seen
above.
The fix is to include sys/stat.h in target.h so that the declaration's
argument type will change to 'struct _stat64' (via the #define).
|
|
Currently gdbserver uses the require_int() function to parse the
requested offset (in vFile::pread packet and the like). This function
allows integers up to 0x7fffffff (to fit in 32-bit int), however the
offset (for the pread system call) has an off_t type which can be
larger than 32-bit.
This patch allows require_int() function to parse offset up to the
maximum value implied by the off_t type.
Approved-By: Pedro Alves <pedro@palves.net>
Change-Id: I3691bcc1ab1838c0db7f8b82d297d276a5419c8c
|
|
The new algorithm to look for a build-id-based debug file
(introduced by commit 22836ca88591ac7efacf06d5b6db191763fd8aba)
makes use of fileio_lstat. As lstat was not supported by
linux-namespace.c, all lstat calls would be performed on the host
and not inside the namespace. Fixed by adding namespace lstat
support.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32956
Approved-By: Andrew Burgess <aburgess@redhat.com>
|
|
This commit continues the work of the previous two commits.
In the following commits I added the target_fileio_stat function, and
the target_ops::fileio_stat member function:
* 08a115cc1c4 gdb: add target_fileio_stat, but no implementations yet
* 3055e3d2f13 gdb: add GDB side target_ops::fileio_stat implementation
* 6d45af96ea5 gdbserver: add gdbserver support for vFile::stat packet
* 22836ca8859 gdb: check for multiple matching build-id files
Unfortunately I messed up, despite being called 'stat' these function
actually performed an 'lstat'. The 'lstat' is the correct (required)
implementation, it's the naming that is wrong.
Additionally, to support remote targets, these commit added the
vFile::stat packet, which again, performed an 'lstat'.
In the previous two commits I changed the GDB code to replace 'stat'
with 'lstat' in the fileio function names. I then added a new
vFile:lstat packet which GDB now uses instead of vFile:stat.
And that just leaves the vFile:stat packet which is, right now,
performing an 'lstat'.
Now, clearly when I wrote this code I fully intended for this packet
to perform an lstat, it's the lstat that I needed. But now, I think,
we should "fix" vFile:stat to actually perform a 'stat'.
This is risky. This is a change in remote protocol behaviour.
Reasons why this might be OK:
- vFile:stat was only added in GDB 16, so it's not been "in the
wild" for too long yet. If we're quick, we might be able to "fix"
this before anyone realises I messed up.
- The documentation for vFile:stat is pretty vague. It certainly
doesn't explicitly say "this does an lstat". Most implementers
would (I think), given the name, start by assuming this should be
a 'stat' (given the name). Only if they ran the full GDB
testsuite, or examined GDB's implementation, would they know to
use lstat.
Reasons why this might not be OK:
- Some other debug client could be connecting to gdbserver, sending
vFile:stat and expecting to get lstat behaviour. This would break
after this patch.
- Some other remote server might have implemented vFile:stat
support, and either figured out, or copied, the lstat behaviour
from gdbserver. This remote server would technically be wrong
after this commit, but as GDB no longer uses vFile:stat, then this
will only become a problem if/when GDB or some other client starts
to use vFile:stat in the future.
Given the vague documentation for vFile:stat, and that it was only
added in GDB 16, I think we should fix it now to perform a 'stat', and
that is what this commit does.
The change in behaviour is documented in the NEWS file. I've improved
the vFile:stat documentation in the manual to better explain what is
expected from this packet, and I've extended the existing test to
cover vFile:stat.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Approved-By: Tom Tromey <tom@tromey.com>
|
|
In the following commits I added the target_fileio_stat function, and
the target_ops::fileio_stat member function:
* 08a115cc1c4 gdb: add target_fileio_stat, but no implementations yet
* 3055e3d2f13 gdb: add GDB side target_ops::fileio_stat implementation
* 6d45af96ea5 gdbserver: add gdbserver support for vFile::stat packet
* 22836ca8859 gdb: check for multiple matching build-id files
Unfortunately I messed up, despite being called 'stat' these function
actually performed an 'lstat'. The 'lstat' is the correct (required)
implementation, it's the naming that is wrong.
In the previous commit I fixed the naming within GDB, renaming 'stat'
to 'lstat' throughout.
However, in order to support target_fileio_stat (as was) on remote
targets, the above patches added the vFile:stat packet, which actually
performed an 'lstat' call. This is really quite unfortunate, and I'd
like to do as much as I can to try and clean up this mess. But I'm
mindful that changing packets is not really the done thing.
So, this commit doesn't change anything.
Instead, this commit adds vFile:lstat as a new packet.
Currently, this packet is handled identically as vFile:stat, the
packet performs an 'lstat' call.
I then update GDB to send the new vFile:lstat instead of vFile:stat
for the remote_target::fileio_lstat implementation.
After this commit GDB will never send the vFile:stat packet.
However, I have retained the 'set remote hostio-stat-packet' control
flag, just in case someone was trying to set this somewhere.
Then there's one test in the testsuite which used to disable the
vFile:stat packet, that test is updated to now disable vFile:lstat.
There's a new test that does a more direct test of vFile:lstat. This
new test can be extended to also test vFile:stat, but that is left for
the next commit.
And so, after this commit, GDB sends the new vFile:lstat packet in
order to implement target_ops::fileio_lstat. The new packet is more
clearly documented than vFile:stat is. But critically, this change
doesn't risk breaking any other clients or servers that implement
GDB's remote protocol.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Approved-By: Tom Tromey <tom@tromey.com>
|
|
Gdbsupport functions phex and phex_nz have a parameter sizeof_l:
...
extern const char *phex (ULONGEST l, int sizeof_l);
extern const char *phex_nz (ULONGEST l, int sizeof_l);
...
and a lot of calls use:
...
phex (l, sizeof (l))
...
Make this easier by reimplementing the functions as a template, allowing us to
simply write:
...
phex (l)
...
Simplify existing code using:
...
$ find gdb* -type f \
| xargs sed -i 's/phex (\([^,]*\), sizeof (\1))/phex (\1)/'
$ find gdb* -type f \
| xargs sed -i 's/phex_nz (\([^,]*\), sizeof (\1))/phex_nz (\1)/'
...
and manually review:
...
$ find gdb* -type f | xargs grep "phex (.*, sizeof.*)"
$ find gdb* -type f | xargs grep "phex_nz (.*, sizeof.*)"
...
Tested on x86_64-linux.
Approved-By: Tom Tromey <tom@tromey.com>
|
|
This is a refactoring commit. When passing inferior arguments to
gdbserver we have two actions that need to be performed, splitting and
joining.
On the GDB side, we take the inferior arguments, a single string, and
split the string into a list of individual arguments. These are then
sent to gdbserver over the remote protocol.
On the gdbserver side we receive the list of individual arguments and
join these back together into a single inferior argument string.
In the next commit I plan to add some unit testing for this remote
argument passing process. Ideally, for unit testing, we need the code
being tested to be located in some easily callable function, rather
than being inline at the site of use.
So in this commit I propose to move the splitting and joining logic
out into a separate file, we can then use this within GDB and
gdbserver when passing arguments between GDB and gdbserver, but we can
also call the same functions for some unit testing.
In this commit I'm not adding the unit tests, they will be added next,
so for now there should be no user visible changes after this commit.
Tested-By: Guinevere Larsen <guinevere@redhat.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>
|
|
Since commit 84da4a1ea0ae ("gdbserver: refactor the definition and uses of
supply_regblock") there is no case where supply_regblock is passed a
nullptr for the BUF argument, and there is even a gdb_assert to make
sure of it.
Therefore remove that part of the documentation comment.
|
|
Commit 51e6b8cfd649013ae16a3d00f1451b2531ba6bc9 fixed a
regression for SVE/SME registers on gdb's side by using a <= comparison for
regcache's raw_compare assertion check. We seem to have failed to do the same
for gdbserver's raw_compare counterpart.
With the code as it is, I'm seeing a lot of crashes for gdbserver on a machine
with SVE enabled. For instance, with the following invocation:
make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver" TESTS=gdb.base/break.exp
Running /work/builds/binutils-gdb/gdb/testsuite/../../../../repos/binutils-gdb/gdb/testsuite/gdb.base/break.exp ...
FAIL: gdb.base/break.exp: test_break: run until function breakpoint
FAIL: gdb.base/break.exp: test_break: run until breakpoint set at a line number (the program is no longer running)
FAIL: gdb.base/break.exp: test_break: run until file:function(6) breakpoint (the program is no longer running)
FAIL: gdb.base/break.exp: test_break: run until file:function(5) breakpoint (the program is no longer running)
FAIL: gdb.base/break.exp: test_break: run until file:function(4) breakpoint (the program is no longer running)
FAIL: gdb.base/break.exp: test_break: run until file:function(3) breakpoint (the program is no longer running)
FAIL: gdb.base/break.exp: test_break: run until file:function(2) breakpoint (the program is no longer running)
FAIL: gdb.base/break.exp: test_break: run until file:function(1) breakpoint (the program is no longer running)
FAIL: gdb.base/break.exp: test_break: run until quoted breakpoint (the program is no longer running)
FAIL: gdb.base/break.exp: test_break: run until file:linenum breakpoint (the program is no longer running)
FAIL: gdb.base/break.exp: test_break: breakpoint offset +1
FAIL: gdb.base/break.exp: test_break: step onto breakpoint (the program is no longer running)
FAIL: gdb.base/break.exp: test_break: setting breakpoint at }
FAIL: gdb.base/break.exp: test_break: continue to breakpoint at } (the program is no longer running)
FAIL: gdb.base/break.exp: test_no_break_on_catchpoint: runto: run to main
FAIL: gdb.base/break.exp: test_break_nonexistent_line: runto: run to main
FAIL: gdb.base/break.exp: test_break_default: runto: run to main
FAIL: gdb.base/break.exp: test_break_silent_and_more: runto: run to main
FAIL: gdb.base/break.exp: test_break_line_convenience_var: runto: run to main
FAIL: gdb.base/break.exp: test_break_user_call: runto: run to main
FAIL: gdb.base/break.exp: test_finish_arguments: runto: run to main
FAIL: gdb.base/break.exp: test_next_with_recursion: kill program
FAIL: gdb.base/break.exp: test_next_with_recursion: run to factorial(6)
FAIL: gdb.base/break.exp: test_next_with_recursion: continue to factorial(5) (the program is no longer running)
FAIL: gdb.base/break.exp: test_next_with_recursion: backtrace from factorial(5)
FAIL: gdb.base/break.exp: test_next_with_recursion: next to recursive call (the program is no longer running)
FAIL: gdb.base/break.exp: test_next_with_recursion: next over recursive call (the program is no longer running)
FAIL: gdb.base/break.exp: test_next_with_recursion: backtrace from factorial(5.1)
FAIL: gdb.base/break.exp: test_next_with_recursion: continue until exit at recursive next test (the program is no longer running)
FAIL: gdb.base/break.exp: test_break_optimized_prologue: run until function breakpoint, optimized file
FAIL: gdb.base/break.exp: test_break_optimized_prologue: run until breakpoint set at small function, optimized file (the program is no longer running)
FAIL: gdb.base/break.exp: test_rbreak_shlib: rbreak junk
Adjusting the regcache raw_compare assertion check to use <= fixes
the problem on aarch64-linux on a SVE-capable system.
This patch also adds a simple selftest to gdbserver that validates this
particular case by simulating a raw_compare operation.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32775
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
