| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
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
|
|
Introduce an RSP packet, 'x', for reading from the remote server
memory in binary format. The binary write packet, 'X' already exists.
The 'x' packet is essentially the same as 'm', except that the
returned data is in binary format. For transferring relatively large
data from the memory of the remote process, the 'x' packet can reduce the
transfer costs.
For example, without this patch, fetching ~100MB of data from a remote
target takes
(gdb) dump binary memory temp.o 0x00007f3ba4c576c0 0x00007f3bab709400
2024-03-13 16:17:42.626 - command started
2024-03-13 16:18:24.151 - command finished
Command execution time: 32.136785 (cpu), 41.525515 (wall)
(gdb)
whereas with this patch, we obtain
(gdb) dump binary memory temp.o 0x00007fec39fce6c0 0x00007fec40a80400
2024-03-13 16:20:48.609 - command started
2024-03-13 16:21:16.873 - command finished
Command execution time: 20.447970 (cpu), 28.264202 (wall)
(gdb)
We see improvements not only when reading bulk data as above, but also
when making a large number of small memory access requests.
For example, without this patch:
(gdb) pipe x/100000xw $pc | wc -l
2024-03-13 16:04:57.112 - command started
25000
2024-03-13 16:05:10.798 - command finished
Command execution time: 9.952364 (cpu), 13.686581 (wall)
With this patch:
(gdb) pipe x/100000xw $pc | wc -l
2024-03-13 16:06:48.160 - command started
25000
2024-03-13 16:06:57.750 - command finished
Command execution time: 6.541425 (cpu), 9.589839 (wall)
(gdb)
Another example, where we create a core file of a GDB process.
(gdb) gcore /tmp/core.1
...
Command execution time: 85.496967 (cpu), 133.224373 (wall)
vs.
(gdb) gcore /tmp/core.1
...
Command execution time: 48.328885 (cpu), 115.032289 (wall)
Regression-tested on X86-64 using the unix (default) and
native-extended-gdbserver board files.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Approved-By: Tom Tromey <tom@tromey.com>
|
|
When started with the --debug=remote flag, gdbserver enables the debug
logs for the received and sent remote packets. If the packet contents
are too long or contain verbatim binary data, printing the contents
may create noise in the logs or even distortion in the terminal output.
Introduce a function, `suppress_next_putpkt_log`, that allows omitting
the contents of a sent package in the logs. This can be useful when a
certain packet handler knows that it is sending binary data.
My first attempt was to implement this mechanism by passing an extra
parameter to putpt_binary_1 that could be controlled by the caller,
putpkt_binary or putpkt. However, all qxfer handlers, regardless of
whether they send binary or ascii data, cause the data to be sent via
putpkt_binary. Hence, the solution was going to be either too
suppressive or too intrusive. I opted for the approach where a package
handler would suppress the log explicitly.
Approved-By: Tom Tromey <tom@tromey.com>
|
|
This patch is the result of running check-include-guards.py on the
current tree. Running it a second time causes no changes.
Reviewed-By: Tom de Vries <tdevries@suse.de>
|
|
In regcache_raw_read_unsigned, we unconditionally return REG_VALID as
the register status. This does not seem right, since the register may
in fact be in another state, such as REG_UNAVAILABLE. Return the
tracked status.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
The registers_valid field of the regcache struct is used for tracking
whether we have attempted to fetch all the registers from the target.
Its name does not reflect this well, I think. It falsely gives the
impression that all the registers are valid. This may conflict an
individual register status, which could be REG_UNAVAILABLE. To better
reflect the purpose, rename the field to "registers_fetched".
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
The registers_valid and registers_owned fields of the regcache struct
are of type int. Make them bool.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
A regcache can be initialized with a register value buffer, in which
case, the register_status pointer is null. This condition is checked
in set_register_status, but not in get_register_status. Do this check
for consistence and safety.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
Convert the free `regcache_cpy` function to a method of the
regcache struct.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
Boolify the 'fetch' parameter of the get_thread_regcache function.
All of the current uses pass true for this parameter. Therefore, define
its default value as true and remove the argument from the uses.
We still keep the parameter, though, to give downstream targets the
option to obtain a regcache without having to fetch the whole
contents. Our (Intel) downstream target is an example.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
The `get_thread_regcache` function has a `fetch` option to skip
fetching the registers from the target. It seems this option is set
to false only at uses where we just need to access the tdesc through
the regcache of the current thread, as in
struct regcache *regcache = get_thread_regcache (current_thread, 0);
... regcache->tdesc ...
Since the tdesc of a regcache is set from the process of the thread
that owns the regcache, we can simplify the code to access the tdesc
via the process, as in
... current_process ()->tdesc ...
This is intended to be a refactoring with no behavioral change.
Tested only for the linux-x86-low target.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
I noticed that gdbreplay --help is rather sparse -- it doesn't even
mention the names of the options it accepts.
This patch updates the help output to be more complete.
Approved-By: Andrew Burgess <aburgess@redhat.com>
|
|
I needed to use gdbreplay today. It didn't work quite right, and
using "set debug remote 1" showed that gdb was rejecting some
responses like:
[remote] Sending packet: $vCont?#49
[remote] Junk: #vCont?
[remote] Junk: 8vCont?
[remote] Junk: 3vCont?
[remote] Received Ack
The checksum recalculation seems to have gone wrong. Looking at the
code, it seems like 'where_csum' is calculated inconsistently: in the
main loop it is after the '#' but in the "== 0" case it is before the
'#'.
This patch fixes the problem and also avoids a string copy.
CC: Alexandra Hájková <ahajkova@redhat.com>
Approved-By: Andrew Burgess <aburgess@redhat.com>
|
|
Remove the 'struct' keyword in occurrences of 'struct thread_info'.
This is a code clean-up.
Tested by rebuilding.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
In the spirit of encapsulation, I'm looking to remove the need for
external code to access the "ptid -> thread" map of process_info, making
it an internal implementation detail. The only remaining use is in
function clear_inferiors, and it led me down this rabbit hole:
- clear_inferiors is really only used by the Windows port and doesn't
really make sense in the grand scheme of things, I think (when would
you want to remove all threads of all processes, without removing
those processes?)
- ok, so let's remove clear_inferiors and inline the code where it's
called, in function win32_clear_inferiors
- the Windows port does not support multi-process, so it's not really
necessary to loop over all processes like this:
for_each_process ([] (process_info *process)
{
process->thread_list ().clear ();
process->thread_map ().clear ();
});
We could just do:
current_process ()->thread_list ().clear ();
current_process ()->thread_map ().clear ();
(or pass down the process from the caller, but it's not important
right now)
- so, the code that we've inlined in win32_clear_inferiors does 3
things:
- clear the process' thread list and map (which deletes the
thread_info objects)
- clear the dll list, which just basically frees some objects
- switch to no current process / no current thread
- let's now look at where this win32_clear_inferiors function is used:
- in win32_process_target::kill, where the process is removed just
after
- in win32_process_target::detach, where the process is removed
just after
- in win32_process_target::wait, when handling a process exit.
After this returns, we could be in handle_target_event (if async)
or resume (if sync), both in `server.cc`. In both of these
cases, target_mourn_inferior gets called, we end up in
win32_process_target::mourn, which removes the process
- in all 3 cases above, we end up removing the process, which takes
care of the 3 actions listed above:
- the thread list and map get cleared when the process gets
destroyed
- same with the dll list
- remove_process switches to no current process / current thread
if the process being removed is the current one
- I conclude that it's probably unnecessary to do the cleanup in
win32_clear_inferiors, because it's going to get done right after
anyway.
Therefore, this patch does:
- remove clear_inferiors, remove the call in win32_clear_inferiors
- remove clear_dlls, which is now unused
- remove process_info::thread_map, which is now unused
- rename win32_clear_inferiors to win32_clear_process, which seems more
accurate
win32_clear_inferiors also does:
for_each_thread (delete_thread_info);
which also makes sure to delete all threads, but it also deletes the
Windows private data object (windows_thread_info), so I'll leave this
one there for now. But if we could make the thread private data
destruction automatic, on thread destruction, it could be removed, I
think.
There should be no user-visible change with this patch. Of course,
operations don't happen in the same order as before, so there might be
some important detail I'm missing. I'm only able to build-test this, if
someone could give it a test run on Windows, it would be appreciated.
Change-Id: I4a560affe763a2c965a97754cc02f3083dbe6fbf
|
|
Remove some includes reported as unused by clangd. Add some to files
that actually need it.
Change-Id: I01c61c174858c1ade5cb54fd7ee1f582b17c3363
|
|
Currently we have duplicate code for each place where
windows_thread_info::context is touched, since for WOW64 processes
it has to do the equivalent with wow64_context instead.
For example this code...:
#ifdef __x86_64__
if (windows_process.wow64_process)
{
th->wow64_context.ContextFlags = WOW64_CONTEXT_ALL;
CHECK (Wow64GetThreadContext (th->h, &th->wow64_context));
...
}
else
#endif
{
th->context.ContextFlags = CONTEXT_DEBUGGER_DR;
CHECK (GetThreadContext (th->h, &th->context));
...
}
...changes to look like this instead:
windows_process.with_context (th, [&] (auto *context)
{
context->ContextFlags = WindowsContext<decltype(context)>::all;
CHECK (get_thread_context (th->h, context));
...
}
The actual choice if context or wow64_context are used, is handled by
this new function in windows_process_info:
template<typename Function>
auto with_context (windows_thread_info *th, Function function)
{
#ifdef __x86_64__
if (wow64_process)
return function (th != nullptr ? th->wow64_context : nullptr);
else
#endif
return function (th != nullptr ? th->context : nullptr);
}
The other parts to make this work are the templated WindowsContext class
which give the appropriate ContextFlags for both types.
And there are also overloaded helper functions, like in the case of
get_thread_context here, call either GetThreadContext or
Wow64GetThreadContext.
According git log --stat, this results in 120 lines less code.
Approved-By: Tom Tromey <tom@tromey.com>
|
|
Fix this:
gdbserver/win32-low.cc: In function ‘void child_delete_thread(DWORD, DWORD)’:
gdbserver/win32-low.cc:192:7: error: ‘all_threads’ was not declared in this scope; did you mean ‘using_threads’?
192 | if (all_threads.size () == 1)
| ^~~~~~~~~~~
| using_threads
Commit 9f77b3aa0bfc ("gdbserver: change 'all_processes' and
'all_threads' list type") changed the type of `all_thread` to an
intrusive_list, without changing this particular use, which broke the
build because an intrusive_list doesn't know its size, so it doesn't
have a `size()` method. The subsequent commit removed `all_threads`,
leading to the error above.
Fix it by using the number of threads of the concerned process instead.
My rationale: as far as I know, GDBserver on Windows only supports one
process at a time, so there's no need to iterate over all processes. If
we made GDBserver for Windows support multiple processes, my intuition
is that we'd want this check to use the number of threads of the
concerned process, not the number of threads overall.
Add the method `process_info::thread_count`, to get the number of
threads of the process.
I'm not really sure what this check is for in the first place, Hannes
Domani said that this check didn't seem to trigger on Windows 7 and 11.
Perhaps it was necessary before.
Change-Id: I84d6226532b887d99248cf3be90f5065fb7a074a
Tested-By: Hannes Domani <ssbssa@yahoo.de>
|
|
Add an overload of `process_info::find_thread` that finds a thread by
ptid. Use it in two spots.
Change-Id: I2b7fb819bf4f83f7bd37f8641c38e878119b3814
|
|
Make the field private, change the free function to be a method.
Change-Id: I05010e7d1bd58ce3895802eb263c029528427758
Approved-By: Tom Tromey <tom@tromey.com>
|
|
thread_info
Make the field private, change the free functions to be methods.
Change-Id: Ifd8ed2775dddefc73a0e00126182e1db02688af4
Approved-By: Tom Tromey <tom@tromey.com>
|
|
Replace the macro with a static inline function.
Change-Id: I1cccf5b38d6a412d251763f0316902b07cc28d16
Approved-By: Tom Tromey <tom@tromey.com>
|
|
I was thinking of changing this macro to a function, but I don't think
it adds much value over just accessing the field directly.
Change-Id: I5dc63e9db0773549c5b55a1279212e2d1213f50c
Approved-By: Tom Tromey <tom@tromey.com>
|
|
This makes the lists easier sort read and modify. There are no changes
in the generated config.h files, so I'm confident this brings no
functional changes.
Change-Id: Ib6b7fc532bcd662af7dbb230070fb1f4fc75f86b
|
|
Intel has EOL'ed the Nios II architecture, and it's time to remove support
from all toolchain components before it gets any more bit-rotten from
lack of maintenance or regular testing.
|
|
Recalculate the checksum and replace whatever is at the end
of the packet with the newly calculated checksum. Then
replay the packet with the checksum added.
The motivation for this change is that I'd like to add a TCL test
which starts a communication with gdbsever setting the remotelog file.
Then, it modifies the remotelog, injects an error message instead of
the expected replay to some packet in order to test GDB reacts to
the error response properly.
Approved-By: Tom Tromey <tom@tromey.com>
|
|
Eli mentioned [1] that given that we use US English spelling in our
documentation, we should use "behavior" instead of "behaviour".
In wikipedia-common-misspellings.txt there's a rule:
...
behavour->behavior, behaviour
...
which leaves this as a choice.
Add an overriding rule to hardcode the choice to common-misspellings.txt:
...
behavour->behavior
...
and add a rule to rewrite behaviour into behavior:
...
behaviour->behavior
...
and re-run spellcheck.sh on gdb*.
Tested on x86_64-linux.
[1] https://sourceware.org/pipermail/gdb-patches/2024-November/213371.html
|
|
When I implemented debugging of Wow64 processes, I missed that there are
extra ContextFlags defines for them.
It's a bit surprising that the wrong ones actually worked, except that
CONTEXT_EXTENDED_REGISTERS is not available for x86_64, and they are
needed for i686, since that's where the xmm registers are stored.
So this replaces the ContextFlags values with their WOW64_* equivalents.
On gdbserver this also duplicates the fallback logic if the
GetThreadContext call failed with CONTEXT_EXTENDED_REGISTERS.
Fixes these fails:
FAIL: gdb.arch/i386-sse.exp: check float contents of %xmm0
FAIL: gdb.arch/i386-sse.exp: check int8 contents of %xmm0
FAIL: gdb.arch/i386-sse.exp: check float contents of %xmm1
FAIL: gdb.arch/i386-sse.exp: check int8 contents of %xmm1
FAIL: gdb.arch/i386-sse.exp: check float contents of %xmm2
FAIL: gdb.arch/i386-sse.exp: check int8 contents of %xmm2
FAIL: gdb.arch/i386-sse.exp: check float contents of %xmm3
FAIL: gdb.arch/i386-sse.exp: check int8 contents of %xmm3
FAIL: gdb.arch/i386-sse.exp: check float contents of %xmm4
FAIL: gdb.arch/i386-sse.exp: check int8 contents of %xmm4
FAIL: gdb.arch/i386-sse.exp: check float contents of %xmm5
FAIL: gdb.arch/i386-sse.exp: check int8 contents of %xmm5
FAIL: gdb.arch/i386-sse.exp: check float contents of %xmm6
FAIL: gdb.arch/i386-sse.exp: check int8 contents of %xmm6
FAIL: gdb.arch/i386-sse.exp: check float contents of %xmm7
FAIL: gdb.arch/i386-sse.exp: check int8 contents of %xmm7
FAIL: gdb.arch/i386-sse.exp: check contents of data[0]
FAIL: gdb.arch/i386-sse.exp: check contents of data[1]
FAIL: gdb.arch/i386-sse.exp: check contents of data[2]
FAIL: gdb.arch/i386-sse.exp: check contents of data[3]
FAIL: gdb.arch/i386-sse.exp: check contents of data[4]
FAIL: gdb.arch/i386-sse.exp: check contents of data[5]
FAIL: gdb.arch/i386-sse.exp: check contents of data[6]
FAIL: gdb.arch/i386-sse.exp: check contents of data[7]
Approved-By: Tom Tromey <tom@tromey.com>
|
|
We have gdb::handle_eintr, which allows us to rewrite:
...
ssize_t ret;
do
{
errno = 0;
ret = ::write (pipe[1], "+", 1);
}
while (ret == -1 && errno == EINTR);
...
into:
...
ssize_t ret = gdb::handle_eintr (-1, ::write, pipe[1], "+", 1);
...
However, the call to write got a bit mangled, requiring effort to decode it
back to its original form.
Instead, add a new function gdb::write that allows us to write:
...
ssize_t ret = gdb::write (pipe[1], "+", 1);
...
Likewise for waitpid, read and close.
Tested on x86_64-linux.
|
|
Problem Description
-------------------
On a Windows machine I built gdbserver, configured for the target
'x86_64-w64-mingw32', then on a GNU/Linux machine I built GDB with
support for all target (--enable-targets=all).
On the Windows machine I start gdbserver with a small test binary:
$ gdbserver 192.168.129.25:54321 C:\some\directory\executable.exe
On the GNU/Linux machine I start GDB without the test binary, and
connect to gdbserver.
As I have not given GDB the test binary, my expectation is that GDB
would connect to gdbserver and then download the file over the remote
protocol, but instead I was presented with this message:
(gdb) target remote 192.168.129.25:54321
Remote debugging using 192.168.129.25:54321
warning: C:\some\directory\executable.exe: No such file or directory.
0x00007ffa3e1e1741 in ?? ()
(gdb)
What I found is that if I told GDB where to find the binary, like
this:
(gdb) file target:C:/some/directory/executable.exe
A program is being debugged already.
Are you sure you want to change the file? (y or n) y
Reading C:/some/directory/executable.exe from remote target...
warning: File transfers from remote targets can be slow. Use "set sysroot" to access files locally instead.
Reading C:/some/directory/executable.exe from remote target...
Reading symbols from target:C:/some/directory/executable.exe...
(gdb)
then GDB would download the executable.
The Actual Issue
----------------
I tracked the problem down to exec_file_find (solib.c). The remote
target was passing an absolute Windows filename (beginning with "C:/"
in this case), but in exec_file_find GDB was failing the
IS_TARGET_ABSOLUTE_PATH call, and so was treating the filename as
relative.
The IS_TARGET_ABSOLUTE_PATH call was failing because GDB thought that
the file system kind was "unix", and as the filename didn't start with
a "/" it assumed the filename was not absolute.
But I'm connecting to a Windows target and 'target-file-system-kind'
was set to "auto", so GDB should be figuring out that the target
file-system is "dos-based".
Looking in effective_target_file_system_kind (filesystem.c), we find
that the logic of "auto" is delegated to the current gdbarch. However
in windows-tdep.c we see:
set_gdbarch_has_dos_based_file_system (gdbarch, 1);
So if we are using a Windows gdbarch we should have "dos-based"
filesystems. What this means is that after connecting to the remote
target GDB has selected the wrong gdbarch.
What's happening is that the target description sent back by the
remote target only includes the x86-64 registers. There's no
information about which OS we're on. As a consequence, GDB picks the
first x86-64 gdbarch which can handle the provided register set, which
happens to be a GNU/Linux gdbarch.
And indeed, there doesn't appear to be anywhere in gdbserver that sets
the osabi on the target descriptions. Some target descriptions do have
their osabi set when the description is created, e.g. in:
gdb/arch/amd64.c - Sets GNU/Linux osabi when appropriate.
gdb/arch/i386.c - Likewise.
gdb/arch/tic6x.c - Always set GNU/Linux osabi.
There are also some cases in gdb/features/*.c where the tdesc is set,
but these locations are only called from GDB, not from gdbserver.
This means that many target descriptions are created without an osabi,
gdbserver does nothing to fix this, and the description is returned to
GDB without an osabi included. This leaves GDB having to guess what
the target osabi is, and in some cases, GDB can get this wrong.
Proposed Solution
-----------------
I propose to change init_target_desc so that it requires an gdb_osabi
to be passed in, this will then be used to set the target_desc osabi
field.
I believe that within gdbserver init_target_desc is called for every
target_desc, so this should mean that every target_desc has an
opportunity to set the osabi to something sane.
I did consider passing the osabi into the code which creates the
target_desc objects, but that would require updating far more code, as
each target has its own code for creating target descriptions.
The approach taken here requires minimal changes and forces every
user of init_target_desc to think about what the correct osabi is.
In some cases, e.g. amd64, where the osabi is already set when the
target_desc is created, the init_target_desc call will override the
current value, however, we should always be replacing it with the same
actual value. i.e. if the target_desc is created with the osabi set
to GNU/Linux, then this should only happen when gdbserver is built for
GNU/Linux, in which case the init_target_desc should also be setting
the osabi to GNU/Linux.
The Tricky Bits
---------------
Some targets, like amd64, use a features based approach for creating
target_desc objects, there's a function in arch/amd64.c which creates
a target_desc, adds features too it, and returns the new target_desc.
This target_desc is then passed to an init_target_desc call within
gdbserver. This is the easy case to handle.
Then there are other targets which instead have a fixed set of xml
files, each of which is converted into a .dat file, which is then used
to generate a .cc file, which is compiled into gdbserver. The
generated .cc file creates the target_desc object and calls
init_target_desc on it. In this case though the target description
that is sent to GDB isn't generated from the target_desc object, but
is instead the contents of the fixed xml file. For this case the
osabi which we pass to init_target_desc should match the osabi that
exists in the fixed xml file.
Luckily, in the previous commit I copied the osabi information from
the fixed xml files into the .dat files. So in this commit I have
extended regdat.sh to read the osabi from the .dat file and use it in
the generated init_target_desc call.
The problem with some of these .dat base targets is that their fixed
xml files don't currently contain any osabi information, and the file
names don't indicate that they are Linux only (despite them currently
only being used from gdbserver for Linux targets), so I don't
currently feel confident adding any osabi information to these files.
An example would be features/rs6000/powerpc-64.xml. For now I've just
ignored these cases. The init_target_desc will use GDB_OSABI_UNKNOWN
which is the default. This means that for these targets nothing
changes from the current behaviour. But many other targets do now
pass the osabi back. Targets that do pass the osabi back are
improved with this commit.
Conclusion
----------
Now when I connect to the Windows remote the target description
returned includes the osabi name. With this extra information GDB
selects the correct gdbarch object, which means that GDB understands
the target has a "dos-based" file-system. With that correct GDB
understands that the filename it was given is absolute, and so fetches
the file from the remote as we'd like.
Reviewed-By: Kevin Buettner <kevinb@redhat.com>
|
|
In gdb/aarch64-linux-tdep.c we find:
...
gdb::byte_vector za_zeroed (za_bytes, 0);
regcache->raw_supply (tdep->sme_za_regnum, za_zeroed);
...
We can't use reg_buffer::raw_supply_zeroed here because only part of the
register is written.
Add raw_supply_part_zeroed, and use it instead.
Likewise elsewhere in AArch64 tdep code.
Tested on aarch64-linux.
Approved-By: Luis Machado <luis.machado@arm.com>
|
|
This function doesn't seem so useful, use `process_info::pid` directly
instead.
Change-Id: I55d592f38b32a197957ed4c569993cd23a818cb4
Reviewed-By: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
|
