| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
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>
|
|
Signed-off-by: David Holsgrove <david.holsgrove@petalogix.com>
Signed-off-by: Nathan Rossi <nathan.rossi@petalogix.com>
Signed-off-by: Mahesh Bodapati <mbodapat@xilinx.com>
Signed-off-by: Gopi Kumar Bulusu <gopi@sankhya.com>
Signed-off-by: Michael Eager <eager@eagercon.com>
|
|
Add a pre-commit codespell hook for directories gdbsupport and gdbserver,
which are codespell-clean:
...
$ pre-commit run codespell --all-files
codespell................................................................Passed
...
A non-trivial question is where the codespell configuration goes.
Currently we have codespell sections in gdbsupport/setup.cfg and
gdbserver/setup.cfg, but codespell doesn't automatically use those because the
pre-commit hook runs codespell at the root of the repository.
A solution would be to replace those 2 setup.cfg files with a setup.cfg in the
root of the repository. Not ideal because generally we try to avoid adding
files related to subdirectories at the root.
Another solution would be to add two codespell hooks, one using
--config gdbsupport/setup.cfg and one using --config gdbserver/setup.cfg, and
add a third one once we start supporting gdb. Not ideal because it creates
duplication, but certainly possible.
I went with the following solution: a setup.cfg file in gdb/contrib (alongside
codespell-ignore-words.txt) which is used for both gdbserver and gdbsupport.
So, what can this new setup do for us? Let's demonstrate by simulating a typo:
...
$ echo "/* aways */" >> gdbsupport/agent.cc
...
We can check unstaged changes before committing:
...
$ pre-commit run codespell --all-files
codespell................................................................Failed
- hook id: codespell
- exit code: 65
gdbsupport/agent.cc:282: aways ==> always, away
...
Likewise, staged changes (no need for the --all-files):
...
$ git add gdbsupport/agent.cc
$ pre-commit run codespell
codespell................................................................Failed
- hook id: codespell
- exit code: 65
gdbsupport/agent.cc:282: aways ==> always, away
...
Or we can try to commit, and run into the codespell failure:
...
$ git commit -a
black................................................(no files to check)Skipped
flake8...............................................(no files to check)Skipped
isort................................................(no files to check)Skipped
codespell................................................................Failed
- hook id: codespell
- exit code: 65
gdbsupport/agent.cc:282: aways ==> always, away
check-include-guards.................................(no files to check)Skipped
...
which makes the commit fail.
Approved-By: Tom Tromey <tom@tromey.com>
|
|
Fix a typo:
...
$ codespell --config gdbserver/setup.cfg gdbserver/tracepoint.cc
gdbserver/tracepoint.cc:951: mistakingly ==> mistakenly
...
|
|
The function remove_thread() was changed to a method in 2500e7d7d (gdbserver:
make remove_thread a method of process_info).
Signed-off-by: Wataru Ashihara <wsh@iij.ad.jp>
Change-Id: I4b2d8a6f84b5329b8d450b268fa9453fe424914e
|
|
The function construct_inferior_arguments (gdbsupport/common-inferior.cc)
currently escapes all special shell characters. After this commit
there will be two "levels" of quoting:
1. The current "full" quoting, where all posix shell special
characters are quoted, and
2. a new "reduced" quoting, where only the characters that GDB sees
as special (quotes and whitespace) are quoted.
After this, almost all construct_inferior_arguments calls will use the
"full" quoting, which is the current quoting. The "reduced" quoting
will be used in this commit to restore the behaviour that was lost in
the previous commit (more details below).
In the future, the reduced quoting will be useful for some additional
inferior argument that I have planned. I already posted my full
inferior argument work here:
https://inbox.sourceware.org/gdb-patches/cover.1730731085.git.aburgess@redhat.com
But that series is pretty long, and wasn't getting reviewed, so I'm
posted the series in parts now.
Before the previous commit, GDB behaved like this:
$ gdb -eiex 'set startup-with-shell off' --args /tmp/exec '$FOO'
(gdb) show args
Argument list to give program being debugged when it is started is "$FOO".
Notice that with 'startup-with-shell' off, the argument was left as
just '$FOO'. But after the previous commit, this changed to:
$ gdb -eiex 'set startup-with-shell off' --args /tmp/exec '$FOO'
(gdb) show args
Argument list to give program being debugged when it is started is "\$FOO".
Now the '$' is escaped with a backslash. This commit restores the
original behaviour, as this is (currently) the only way to unquoted
shell special characters into arguments from the GDB command line.
The series that I listed above includes a new command line option for
GDB which provides a better approach for controlling the quoting of
special shell characters, but that work requires these patches to be
merged first.
I've split out the core of construct_inferior_arguments into the new
function escape_characters, which takes a set of characters to escape.
Then the two functions escape_shell_characters and
escape_gdb_characters call escape_characters with the appropriate
character sets.
Finally, construct_inferior_arguments, now takes a boolean which
indicates if we should perform full shell escaping, or just perform
the reduced escaping.
I've updated all uses of construct_inferior_arguments to pass a
suitable value to indicate what escaping to perform (mostly just
'true', but one case in main.c is different), also I've updated
inferior::set_args to take the same boolean flag, and pass it through
to construct_inferior_arguments.
Tested-By: Guinevere Larsen <guinevere@redhat.com>
|
|
Add a few -Wunused-* diagnostic flags that look useful. Some are known
to gcc, some to clang, some to both. Fix the fallouts.
-Wunused-const-variable=1 is understood by gcc, but not clang.
-Wunused-const-variable would be undertsood by both, but for gcc at
least it would flag the unused const variables in headers. This doesn't
make sense to me, because as soon as one source file includes a header
but doesn't use a const variable defined in that header, it's an error.
With `=1`, gcc only warns about unused const variable in the main source
file. It's not a big deal that clang doesn't understand it though: any
instance of that problem will be flagged by any gcc build.
Change-Id: Ie20d99524b3054693f1ac5b53115bb46c89a5156
Approved-By: Tom Tromey <tom@tromey.com>
|
|
Put them one per line and sort alphabetically.
Change-Id: Idb6947d444dc6e556a75645b04f97a915bba7a59
Approved-By: Tom Tromey <tom@tromey.com>
|
|
In gdbserver/xtensa-xtregs.cc, there's a table:
...
const xtensa_regtable_t xtensa_regmap_table[] = {
/* gnum,gofs,cpofs,ofs,siz,cp, dbnum, name */
{ 44, 176, 0, 0, 4, -1, 0x020c, "scompare1" },
{ 0 }
};
...
on which codespell triggers:
...
$ codespell --config ./gdbserver/setup.cfg gdbserver
gdbserver/xtensa-xtregs.cc:34: siz ==> size, six
...
Fix this by laying out the table in vertical fashion, and using the full field
names instead of the abbreviations ("size" instead of "siz", "offset" instead
of "ofs", etc).
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
Add a codespell section in new config file gdbserver/setup.cfg, similar to the
one in gdbsupport/setup.cfg.
There's just one item left:
...
$ codespell --config ./gdbserver/setup.cfg gdbserver
gdbserver/xtensa-xtregs.cc:34: siz ==> size, six
...
|
|
Fix typos in gdbserver:
...
gdbreplay.cc:444: substract ==> subtract
notif.cc:35: Enque ==> Enqueue
notif.cc:42: enque ==> enqueue
i387-fp.cc:233: simplifed ==> simplified
i387-fp.cc:508: simplifed ==> simplified
linux-arc-low.cc:221: shoudn't ==> shouldn't
linux-sparc-low.cc:112: ans ==> and
linux-ppc-low.cc:1134: Followings ==> Following
linux-ppc-low.cc:1160: Followings ==> Following
linux-ppc-low.cc:1193: Followings ==> Following
linux-ppc-low.cc:1226: Followings ==> Following
configure.ac:141: defintions ==> definitions
...
Regenerate configure from configure.ac using autoconf.
|
|
I noticed a
// namespace selftests
comment, which doesn't follow our comment formatting convention. I did
a find & replace to fix all the offenders.
Change-Id: Idf8fe9833caf1c3d99e15330db000e4bab4ec66c
|
|
GDB prints the target id of a thread in various places such as the
output of the "info threads" command in the "Target Id" column or when
switching to a thread. A target can define what to print for a given
ptid by overriding the `pid_to_str` method.
The remote target is a gateway behind which one of many various
targets could be running. The remote target converts a given ptid to
a string in a uniform way, without consulting the low target at the
server-side.
In this patch we introduce a new attribute in the XML that is sent in
response to the "qXfer:threads:read" RSP packet, so that a low target
at the server side, if it wishes, can specify what to print as the
target id of a thread.
Note that the existing "name" attribute or the "extra" text provided
in the XML are not sufficient for the server-side low target to
achieve the goal. Those attributes, when present, are simply appended
to the target id by GDB.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Reviewed-By: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
Replace the few uses of `std::unordered_map` in gdbserver with
`gdb::unordered_map`.
The only one of these that is likely to ever see a lot of elements is
probably `process_info::m_ptid_thread_map`. It was added precisely to
improve performance when there are a lot of threads, so I guess using
`gdb::unordered_map` here won't hurt. I changed the others too, since
it's easy.
Change-Id: Ibc4ede5245551fdd7717cb349a012d05726f4363
Reviewed-By: Stephan Rohr <stephan.rohr@intel.com>
|
|
When the changes on the remote protocol are made,
we want to test all the corner cases to prevent
regressions. Currently it can be tricky to simulate
some corner case conditions that would expose possible
regressions. When I want to add or change the remote
protocol packet, I need to hack gdbserver to send a
corrupted packet or an error to make sure GDB is able
to handle such a case.
This test makes it easy to send a corruped packet or
an error message to GDB using the gdbreplay tool and
check GDB deals with it as we expect it to.
This test starts a communication with gdbsever setting
the remotelog file. Then, it modifies the remotelog with
update_log proc, injects an error message instead of
the expected replay to the vMustReplyEmpty packet in order
to test GDB reacts to the error response properly. After
the remotelog modification, this test restarts GDB and starts
communication with gdbreply instead of the gdbserver using
the remotelog.
Add a lib/gdbreplay-support.exp. update_log proc matches lines
from GDB to gdbserver in a remotelogfile. Once a match is found then
the custom line is used to build a replacement line to send from
gdbserver to GDB.
Approved-By: Andrew Burgess <aburgess@redhat.com>
|
|
The register_status field of regcache is declared as `unsigned char *`.
This is incorrect, because `enum register_status` from
gdbsupport/common-regcache.h is based on signed char and
REG_UNAVAILABLE is defined as -1. Fix the declared type.
Now that we are modifying the declaration, also use a unique_ptr
and make the field private.
The get/set methods already use the correct type, but we update cast
operations in two places.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
The supply_regblock function takes a pointer to a buffer as an
argument and implements two different behavior based on the pointer
being null. There are two cases where we pass nullptr, all in
tracepoint.cc, where we are essentially doing a reset on the regcache.
In fast_tracepoint_ctx::regcache, register_status array does not
even exist. Hence, that use simply boils down to zeroing of register
data. Do this at the time of creating the buffer and remove the call
to supply_regblock.
In fetch_traceframe_registers, inline the use with a call to `reset`.
Hence, there are no more cases left, where a nullptr would be passed
to supply_regblock. Assert that the buffer argument is non-null and
simplify the implementation.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
Define a `reset` method for a regcache and use it for code
simplification. This patch allows further simplification in the next
patch.
The reset method fills the register data with zeroes. For the use in
get_thread_regcache, this is added behavior, making the patch not a
pure refactoring, and may look like extra overhead. However, it is
better to avoid having arbitrary values left in the data buffer.
Hence, it is considered a behavioral improvement.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
When a regcache is initialized, the values of registers are not
fetched yet. Thus, initialize the register statuses to REG_UNKNOWN
instead of REG_UNAVAILABLE, because the latter rather means "we
attempted to fetch but could not obtain the value".
The definitions of the reg status enums (from
gdbsupport/common-regcache.h) as a reminder:
/* The register value is not in the cache, and we don't know yet
whether it's available in the target (or traceframe). */
REG_UNKNOWN = 0,
/* The register value is valid and cached. */
REG_VALID = 1,
/* The register value is unavailable. E.g., we're inspecting a
traceframe, and this register wasn't collected. Note that this
"unavailable" is different from saying the register does not
exist in the target's architecture --- in that case, the target
should have given us a target description that does not include
the register in the first place. */
REG_UNAVAILABLE = -1
Similarly, when the regcache is invalidated, change all the statuses
back to REG_UNKNOWN.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
Store the regcache pointer in thread_info as a unique_ptr. This
allows us delete the thread_info destructor.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
Convert the `free_register_cache` function into a destructor of the
regcache struct. In one place, we completely remove the call to free
the regcache object by stack-allocating the object.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
This is a refactoring that converts
init_register_cache (struct regcache *regcache,
const struct target_desc *tdesc,
unsigned char *regbuf)
into the constructor
regcache (const target_desc *tdesc, unsigned char *regbuf)
and converts
new_register_cache (const struct target_desc *tdesc)
into the constructor
regcache (const target_desc *tdesc)
Also use DISABLE_COPY_AND_ASSIGN for additional compile-time safety.
Tested by rebuilding gdbserver with '--enable-inprocess-agent=no' and
with '--enable-inprocess-agent=yes'.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
This is a continuation of the previous refactoring to use inheritance
in the definition of tracepoints contexts. Again, no behavioral change
is intended.
Different tracepoint contexts are identified by the `type` field. The
field is used only in `get_context_regcache`, where we essentially
have 2 cases, each corresponding to a tracepoint context type. Remove
the `type` field and split the `get_context_regcache` function into 2
virtual method implementations.
Tested by rebuilding gdbserver with '--enable-inprocess-agent=no' and
'--enable-inprocess-agent=yes'.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
Use inheritance in the definition of tracepoint contexts. This is a
refactoring that aims to improve the code. No behavior should be
altered.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
Before the removal of the UST/static-tracepoint support, the
`static_tracepoint_ctx` struct contained comments for its fields,
whereas `fast_tracepoint_ctx` did not. Nevertheless, those comments
also applied to `fast_tracepoint_ctx`. With the removal of
`static_tracepoint_ctx`, the comments were lost, making
`fast_tracepoint_ctx` data members completely commentless. Add back
those comments.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
In gdbserver there are a couple of places where we perform manual
memory management using a 'std::vector<char *>' with the vector owning
the strings within it. We need to take care to call
free_vector_argv() before leaving the scope to cleanup the strings
within the vector.
This commit introduces a new class gdb::argv_vec which wraps around a
'std::vector<char *>' and owns the strings within the vector, taking
care to xfree() them when the gdb::argv_vec is destroyed.
Right now I plan to use this class in gdbserver.
But this class will also be used to address review feedback on this
commit:
https://inbox.sourceware.org/gdb-patches/72227f1c5a2e350ca70b2151d1b91306a0261bdc.1736860317.git.aburgess@redhat.com
where I tried to introduce another 'std::vector<char *>' which owns
the strings. That patch will be updated to use gdb::argv_vec instead.
The obvious question is, instead of introducing this new class, could
we change the APIs to avoid having a std::vector<char *> that owns the
strings? Could we use 'std::vector<std::string>' or
'std::vector<gdb::unique_xmalloc_ptr<char>>' instead?
The answer is yes we could.
I originally posted this larger patch set:
https://inbox.sourceware.org/gdb-patches/cover.1730731085.git.aburgess@redhat.com
however, getting a 14 patch series reviewed is just not possible, so
instead, I'm posting the patches one at a time. The earlier patch I
mentioned is pulled from the larger series.
The larger series already includes changes to gdbserver which removes
the need for the 'std::vector<char *>', however, getting those changes
in depends (I think) on the patch I mention above. Hence we have a
bit of a circular dependency.
My proposal is to merge this patch (adding gdb::argv_vec) and make use
of it in gdbserver.
Then I'll update the patch above to also use gdb::argv_vec, which will
allow the above patch to get reviewed and merged.
Then I'll post, and hopefully merge additional patches from my larger
inferior argument series, which will remove the need for gdb::argv_vec
from gdbserver.
At this point, the only use of gdb::argv_vec will be in the above
patch, where I think it will remain, as I don't think that location
can avoid using 'std::vector<char *>'.
Approved-By: Tom Tromey <tom@tromey.com>
|
|
This mailing list discussion:
https://inbox.sourceware.org/gdb-patches/CAOp6jLYD0g-GUsx7jhO3g8H_4pHkB6dkh51cbyDT-5yMfQwu+A@mail.gmail.com
highlighted the following issue with GDB's 'x' packet implementation.
Unfortunately, LLDB also has an 'x' packet, but their implementation
is different to GDB's and so targets that have implemented LLDB's 'x'
packet are incompatible with GDB.
The above thread is specifically about the 'rr' tool, but there could
be other remote targets out there that have this problem.
The difference between LLDB and GDB is that GDB expects a 'b' prefix
on the reply data, while LLDB does not. The 'b' is important as it
allows GDB to distinguish between an empty reply (which will be a 'b'
prefix with no trailing data) and an unsupported packet (which will be
a completely empty packet). It is not clear to me how LLDB
distinguishes these two cases.
See for discussion of the 'x' packet:
https://inbox.sourceware.org/gdb-patches/cover.1710343840.git.tankut.baris.aktemur@intel.com/#r
with the part specific to the 'b' marker in:
https://inbox.sourceware.org/gdb-patches/87msq82ced.fsf@redhat.com/
I propose that we add a new feature 'binary-upload' which can be
reported by a stub in its qSupported reply. By default this feature
is "off", meaning GDB will not use the 'x' packet unless a stub
advertises this feature.
I have updated gdbserver to send 'binary-upload+', and when I examine
the gdbserver log I can see this feature being sent back, and then GDB
will use the 'x' packet.
When connecting to an older gdbserver, the feature is not sent, and
GDB does not try to use the 'x' packet at all.
I also built the latest version of `rr` and tested using current HEAD
of master, where I see problems like this:
(rr) x/10i main
0x401106 <main>: Cannot access memory at address 0x401106
Then tested using this patched version of GDB, and now I see:
(rr) x/10i main
0x401106 <main>: push %rbp
0x401107 <main+1>: mov %rsp,%rbp
0x40110a <main+4>: mov 0x2f17(%rip),%rax # 0x404028 <global_ptr>
... etc ...
and looking in the remote log I see GDB is now using the 'm' packet
instead of the 'x' packet.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32593
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Reviewed-By: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
|
|
Commit 3470a0e144df6c01f8479fa649f43aa907936e7e inadvertently broke
the build on MIPS because it's passing a non-existent "pid" argument
to "proc->for_each_thread". This commit fixes the problem by removing
the argument from the call.
Signed-off-by: Sergio Durigan Junior <sergiodj@sergiodj.net>
|
|
The comment
/* Functions local to this file. */
has somehow been positioned above struct definitions, not functions.
Some static function declarations are given after the structs, to
where the comment could be moved, but the comment is not really
helpful. Therefore remove it.
|
|
Remove the unnecessary forward declaration for `struct tracepoint_hit_ctx`.
|
|
LoongArch defines hardware watchpoint functions for fetch and load/store
operations, the related support for gdb was added in the following two
commit c1cdee0e2c17 ("gdb: LoongArch: Add support for hardware watchpoint")
commit 6ced1278fc00 ("gdb: LoongArch: Add support for hardware breakpoint")
Now, add hardware watchpoint and breakpoint support for gdbserver on
LoongArch.
Here is a simple example
$ cat test.c
#include <stdio.h>
int a = 0;
int b = 0;
int main()
{
printf("start test\n");
a = 1;
printf("a = %d\n", a);
a = 2;
printf("a = %d\n", a);
b = 2;
printf("b = %d\n", b);
return 0;
}
$ gcc -g test.c -o test
Execute on the target machine:
$ gdbserver 192.168.1.100:1234 ./test
Execute on the host machine:
$ gdb ./test
...
(gdb) target remote 192.168.1.100:1234
...
(gdb) b main
Breakpoint 1 at 0x1200006b8: file test.c, line 6.
(gdb) c
Continuing.
...
Breakpoint 1, main () at test.c:6
6 printf("start test\n");
(gdb) watch a
Hardware watchpoint 2: a
(gdb) hbreak 11
Hardware assisted breakpoint 3 at 0x120000700: file test.c, line 11.
(gdb) c
Continuing.
Hardware watchpoint 2: a
Old value = 0
New value = 1
main () at test.c:8
8 printf("a = %d\n", a);
(gdb) c
Continuing.
Hardware watchpoint 2: a
Old value = 1
New value = 2
main () at test.c:10
10 printf("a = %d\n", a);
(gdb) c
Continuing.
Breakpoint 3, main () at test.c:11
11 b = 2;
(gdb) c
Continuing.
[Inferior 1 (process 696656) exited normally]
Output on the target machine:
Process ./test created; pid = 696708
Listening on port 1234
Remote debugging from host 192.168.1.200, port 60742
start test
a = 1
a = 2
b = 2
Child exited with status 0
Signed-off-by: Hui Li <lihui@loongson.cn>
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
|
|
This commit changes how gdbserver stores the inferior arguments from
being a vector of separate arguments into a single string with all of
the arguments combined together.
Making this change might feel a little strange; intuitively it feels
like we would be better off storing the arguments as a vector, but
this change is part of a larger series of work that aims to improve
GDB's inferior argument handling. The full series was posted here:
https://inbox.sourceware.org/gdb-patches/cover.1730731085.git.aburgess@redhat.com
But asking people to review a 14 patch series in unreasonable, so I'm
instead posting the patches in smaller batches. This patch can stand
alone, and I do think this change makes sense on its own:
First, GDB already stores the inferior arguments as a single string,
so doing this moves gdbserver into line with GDB. The common code
into which gdbserver calls requires the arguments to be a single
string, so currently each target's create_inferior implementation
merged the arguments anyway, so all this commit really does is move
the merging up the call stack, and store the merged result rather than
storing the separate parts.
However, the biggest reason for why this commit is needed, is an issue
with passing arguments from GDB to gdbserver when starting a new
inferior.
Consider:
(gdb) set args $VAR
(gdb) run
...
When using a native target the inferior will see the value of $VAR
expanded by the shell GDB uses to start the inferior. However, if
using an extended-remote target the inferior will see literally $VAR,
the unexpanded name of the variable, the reason for this is that,
although GDB sends '$VAR' to gdbserver, when gdbserver receives this,
it converts this to '\$VAR', which prevents the variable from being
expanded by the shell.
The reason for this is that construct_inferior_arguments escapes all
special shell characters within its arguments, and it is
construct_inferior_arguments that is used to combine the separate
arguments into a single string.
In the future I will change construct_inferior_arguments so that
it can apply different escaping strategies. When this happens we will
want to escape arguments coming from the gdbserver command line
differently than arguments coming from GDB (via a vRun packet), which
means we need to call construct_inferior_arguments earlier, at the
point where we know if the arguments came from the gdbserver command
line, or from the vRun packet.
This argument escaping issue is discussed in PR gdb/28392.
This commit doesn't fix any issues, nor does it change
construct_inferior_arguments to actually do different escaping, that
will all come later. This is purely a restructuring.
There should be no user visible changes after this commit.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28392
Tested-By: Guinevere Larsen <guinevere@redhat.com>
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
Now that static tracepoint support is removed from gdbserver, it makes
sense to remove handling of the 'L' tracepoint action, too. The code
that checks received actions already has a default case that tolerates
unrecognized actions:
default:
trace_debug ("unknown trace action '%c', ignoring...", *act);
In case 'L' is unexpectedly received, we would at least be able to see
this in the logs.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
As a continuation of the previous patches that remove UST from
gdbserver, remove the `static_tracepoint` enum value from
`tracepoint_type` and all the associated code.
Now that the last use of `write_e_static_tracepoints_not_supported`
is gone, also remove that function.
The handling of the 'S' option, where the `static_tracepoint` enum
value was being used, is removed completely, because recognizing that
option makes sense only when static tracepoint support is announced.
This patch is easier to view with "git show -w".
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
Remove the announcement that `qXfer:statictrace:read` and
`StaticTracepoints` are supported. Associated to this, remove the
handling of "qTfSTM", "qTsSTM", and "qTSTMat" packets and the
qXfer:statictrace:read handling.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
With the removal of UST, the `in_process_agent_supports_ust` query
would essentially always be false. Remove the function and adjust
the uses, comments, and warning/error messages.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
UST support in gdbserver is substantially outdated. Simon says:
...[having HAVE_UST defined] never happens nowadays because it used
a version of lttng-ust that has been deprecated for a loooong time
(the 0.x series). So everything in HAVE_UST just bitrots. It might
be possible to update all this code to use lttng-ust 2.x (1.x never
existed), but I don't think it's going to happen unless somebody
specifically asks for it. I would suggest removing support for UST
from gdbserver. ...If we ever want to resurrect the support for UST
and port to 2.x, we can get the code from the git history.
This patch removes the support, mostly mechanically by deleting code
guarded by `#ifdef HAVE_UST`. After these removals, `struct
static_tracepoint_ctx` becomes unused. So, remove it, too.
The following patches remove more code.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
Introduce and use a setter method in regcache to set the status of a
register. There already exists get_register_status. So, it made
sense to add the setter to control access to the register_status
field.
In two places, we also do cosmetic improvements to for-loops.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
Fix 'collect_register_as_string' so that unavailable registers are
dumped as 'xx...x' instead of arbitrary values, in particular when
reporting expedited registers in a resume reply packet. This change
gives the opportunity that we can reuse 'collect_register_as_string'
in 'registers_to_string' for additional code simplification.
Reviewed-By: Luis Machado <luis.machado@arm.com>
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
The regcache_register_size function has one implementation in GDB, and
one in gdbserver. Both of them have a gdb::checked_static_cast to their
corresponding regcache class. This can be avoided by defining a
pure virtual register_size method in the
reg_buffer_common class, which is then implemented by the reg_buffer
class in GDB, and by the regcache class in gdbserver.
Calls to the register_size () function from methods of classes in the
reg_buffer_common hierarchy need to be changed to calls to the newly
defined method, otherwise the compiler complains that a matching method
cannot be found.
Co-Authored-By: Simon Marchi <simon.marchi@efficios.com>
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Reviewed-By: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Change-Id: I7f4f74a51e96c42604374e87321ca0e569bc07a3
|
