| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
When running test-case gdb.base/vfork-follow-parent.exp on powerpc64 (likewise
on s390x), I run into:
...
(gdb) PASS: gdb.base/vfork-follow-parent.exp: \
exec_file=vfork-follow-parent-exit: target-non-stop=on: non-stop=off: \
resolution_method=schedule-multiple: print unblock_parent = 1
continue^M
Continuing.^M
Reading symbols from vfork-follow-parent-exit...^M
^M
^M
Fatal signal: Segmentation fault^M
----- Backtrace -----^M
0x1027d3e7 gdb_internal_backtrace_1^M
src/gdb/bt-utils.c:122^M
0x1027d54f _Z22gdb_internal_backtracev^M
src/gdb/bt-utils.c:168^M
0x1057643f handle_fatal_signal^M
src/gdb/event-top.c:889^M
0x10576677 handle_sigsegv^M
src/gdb/event-top.c:962^M
0x3fffa7610477 ???^M
0x103f2144 for_each_block^M
src/gdb/dcache.c:199^M
0x103f235b _Z17dcache_invalidateP13dcache_struct^M
src/gdb/dcache.c:251^M
0x10bde8c7 _Z24target_dcache_invalidatev^M
src/gdb/target-dcache.c:50^M
...
or similar.
The root cause for the segmentation fault is that linux_is_uclinux gives an
incorrect result: it should always return false, given that we're running on a
regular linux system, but instead it returns first true, then false.
In more detail, the segmentation fault happens as follows:
- a program space with an address space is created
- a second program space is about to be created. maybe_new_address_space
is called, and because linux_is_uclinux returns true, maybe_new_address_space
returns false, and no new address space is created
- a second program space with the same address space is created
- a program space is deleted. Because linux_is_uclinux now returns false,
gdbarch_has_shared_address_space (current_inferior ()->arch ()) returns
false, and the address space is deleted
- when gdb uses the address space of the remaining program space, we run into
the segfault, because the address space is deleted.
Hardcoding linux_is_uclinux to false makes the test-case pass.
We leave addressing the root cause for the following commit in this series.
For now, prevent the segmentation fault by making the address space a refcounted
object.
This was already suggested here [1]:
...
A better solution might be to have the address spaces be reference counted
...
Tested on top of trunk on x86_64-linux and ppc64le-linux.
Tested on top of gdb-14-branch on ppc64-linux.
Co-Authored-By: Simon Marchi <simon.marchi@polymtl.ca>
PR gdb/30547
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30547
[1] https://sourceware.org/pipermail/gdb-patches/2023-October/202928.html
|
|
Since GDB now requires C++17, we don't need the internally maintained
gdb::optional implementation. This patch does the following replacing:
- gdb::optional -> std::optional
- gdb::in_place -> std::in_place
- #include "gdbsupport/gdb_optional.h" -> #include <optional>
This change has mostly been done automatically. One exception is
gdbsupport/thread-pool.* which did not use the gdb:: prefix as it
already lives in the gdb namespace.
Change-Id: I19a92fa03e89637bab136c72e34fd351524f65e9
Approved-By: Tom Tromey <tom@tromey.com>
Approved-By: Pedro Alves <pedro@palves.net>
|
|
In the following commit I ran into a problem. The next commit aims to
improve GDB's handling of the main executable being a file on a remote
target (i.e. one with a 'target:' prefix).
To do this I have replaced a system 'stat' call with a bfd_stat call.
However, doing this caused a regression in gdb.base/attach.exp.
The problem is that the bfd library caches open FILE* handles for bfd
objects that it has accessed, which is great for short-lived, non
interactive programs (e.g. the assembler, or objcopy, etc), however,
for GDB this caching causes us a problem.
If we open the main executable as a bfd then the bfd library will
cache the open FILE*. If some time passes, maybe just sat at the GDB
prompt, or with the inferior running, and then later we use bfd_stat
to check if the underlying, on-disk file has changed, then the bfd
library will actually use fstat on the underlying file descriptor.
This is of course slightly different than using system stat on with
the on-disk file name.
If the on-disk file has changed then system stat will give results for
the current on-disk file. But, if the bfd cache is still holding open
the file descriptor for the original on-disk file (from before the
change) then fstat will return a result based on the original file,
and so show no change as having happened.
This is a known problem in GDB, and so far this has been solved by
scattering bfd_cache_close_all() calls throughout GDB. But, as I
said, in the next commit I've made a change and run into a
problem (gdb.base/attach.exp) where we are apparently missing a
bfd_cache_close_all() call.
Now I could solve this problem by adding a bfd_cache_close_all() call
before the bfd_stat call that I plan to add in the next commit, that
would for sure solve the problem, but feels a little crude.
Better I think would be to track down where the bfd is being opened
and add a corresponding bfd_cache_close_all() call elsewhere in GDB
once we've finished doing whatever it is that caused us to open the
bfd in the first place.
This second solution felt like the better choice, so I tracked the
problem down to elf_locate_base and fixed that. But that just exposed
another problem in gdb_bfd_map_section which was also re-opening the
bfd, so I fixed this (with another bfd_cache_close_all() call), and
that exposed another issue in gdbarch_lookup_osabi... and at this
point I wondered if I was approaching this problem the wrong way...
.... And so, I wonder, is there a _better_ way to handle these
bfd_cache_close_all() calls?
I see two problems with the current approach:
1. It's fragile. Folk aren't always aware that they need to clear
the bfd cache, and this feels like something that is easy to
overlook in review. So adding new code to GDB can innocently touch
a bfd, which populates the cache, which will then be a bug that can
lie hidden until an on-disk file just happens to change at the wrong
time ... and GDB fails to spot the change. Additionally,
2. It's in efficient. The caching is intended to stop the bfd
library from continually having to re-open the on-disk file. If we
have a function that touches a bfd then often that function is the
obvious place to call bfd_cache_close_all. But if a single GDB
command calls multiple functions, each of which touch the bfd, then
we will end up opening and closing the same on-disk file multiple
times. It feels like we would be better postponing the
bfd_cache_close_all call until some later point, then we can benefit
from the bfd cache.
So, in this commit I propose a new approach. We now clear the bfd
cache in two places:
(a) Just before we display a GDB prompt. We display a prompt after
completing a command, and GDB is about to enter an idle state
waiting for further input from the user (or in async mode, for an
inferior event). If while we are in this idle state the user
changes the on-disk file(s) then we would like GDB to notice this
the next time it leaves its idle state, e.g. the next time the user
executes a command, or when an inferior event arrives,
(b) When we resume the inferior. In synchronous mode, resuming the
inferior is another time when GDB is blocked and sitting idle, but
in this case we don't display a prompt. As with (a) above, when an
inferior event arrives we want GDB to notice any changes to on-disk
files.
It turns out that there are existing observers for both of these
cases (before_prompt and target_resumed respectively), so my initial
thought was that I should attach to these observers in gdb_bfd.c, and
in both cases call bfd_cache_close_all().
And this does indeed solve the gdb.base/attach.exp problem that I see
with the following commit.
However, I see a problem with this solution.
Both of the observers I'm using are exposed through the Python API as
events that a user can hook into. The user can potentially run any
GDB command (using gdb.execute), so Python code might end up causing
some bfds to be reopened, and inserted into the cache.
To solve this one solution would be to add a bfd_cache_close_all()
call into gdbpy_enter::~gdbpy_enter(). Unfortunately, there's no
similar enter/exit object for Guile, though right now Guile doesn't
offer the same event API, so maybe we could just ignore that
problem... but this doesn't feel great.
So instead, I think a better solution might be to not use observers
for the bfd_cache_close_all() calls. Instead, I'll call
bfd_cache_close_all() directly from core GDB after we've notified the
before_prompt and target_resumed observers, this was we can be sure
that the cache is cleared after the observers have run, and before GDB
enters an idle state.
This commit also removes all of the other bfd_cache_close_all() calls
from GDB. My claim is that these are no longer needed.
Approved-By: Tom Tromey <tom@tromey.com>
|
|
This commit replaces the architecture_changed observer with a
new_architecture observer.
Currently the only user of the architecture_changed observer is the
Python code, which uses this observer to register the Python unwinder
with the architecture.
The problem is that the architecture_changed observer is triggered
from inferior::set_arch(), which only sees the inferior-wide gdbarch
value. For targets that use thread-specific architectures, these
never trigger the architecture_changed observer, and so never have the
Python unwinder registered with them.
When it comes to unwinding GDB makes use of the frame's gdbarch, which
is based on the thread's regcache gdbarch, which is set in
get_thread_regcache to the value returned from
target_thread_architecture, which is not always the inferiors gdbarch
value, it might be a thread-specific gdbarch which has not passed
through inferior::set_arch().
The new_architecture observer will be triggered from
gdbarch_find_by_info, whenever a new gdbarch is created and
initialised. As GDB caches and reuses gdbarch values, we should
expect to see each new architecture trigger the new_architecture
observer just once.
After this commit, targets that make use of thread-specific
architectures should be able to make use of Python unwinders.
As I don't have access to a machine that makes use of thread-specific
architectures right now, I asked Luis to confirm that an AArch64
target that uses SVE/SME can't use the Python unwinders in threads
that are using a thread-specific architectures, and he confirmed that
this is indeed the case, see this discussion:
https://inbox.sourceware.org/gdb/87wmvsat8i.fsf@redhat.com
Tested-By: Lancelot Six <lancelot.six@amd.com>
Tested-By: Luis Machado <luis.machado@arm.com>
Reviewed-By: Luis Machado <luis.machado@arm.com>
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
inferior::set_arch calls registers_changed, which invalidates all
regcaches. It would be enough to invalidate only regcaches of threads
belonging to this inferior. Call registers_changed_ptid instead, with
the proper process target / ptid. If the inferior does not have a
process target, there should be no regcaches for that inferior, so no
need to invalidate anything.
Change-Id: Id8b5500acb7f373b01a534f16d3a7d028dc0d882
Reviewed-By: John Baldwin <jhb@FreeBSD.org>
Approved-By: Andrew Burgess <aburgess@redhat.com>
|
|
set_target_gdbarch is basically a setter for the current inferior's
arch, that notifies other parts of GDB of the architecture change. Move
the code of set_target_gdbarch to the inferior::set_arch method.
Add gdbarch_initialized_p, so we can keep the assertion.
Change-Id: I276e28eafd4740c94bc5233c81a86c01b4a6ae90
Reviewed-By: John Baldwin <jhb@FreeBSD.org>
Approved-By: Andrew Burgess <aburgess@redhat.com>
|
|
Make the inferior's gdbarch field private, and add getters and setters.
This helped me by allowing putting breakpoints on set_arch to know when
the inferior's arch was set. A subsequent patch in this series also
adds more things in set_arch.
Change-Id: I0005bd1ef4cd6b612af501201cec44e457998eec
Reviewed-By: John Baldwin <jhb@FreeBSD.org>
Approved-By: Andrew Burgess <aburgess@redhat.com>
|
|
The following patch makes the amdgpu port transfer a property from the
original inferior to the new inferior when using the clone-inferior
command. Add the inferior_cloned observable to help with this.
Change-Id: Id845a799813ec49b1b7b2fcb97b07d0a1e5e2631
Approved-By: Tom Tromey <tom@tromey.com>
|
|
Currently, each target backend is responsible for printing "[Thread
...exited]" before deleting a thread. This leads to unnecessary
differences between targets, like e.g. with the remote target, we
never print such messages, even though we do print "[New Thread ...]".
E.g., debugging the gdb.threads/attach-many-short-lived-threads.exp
with gdbserver, letting it run for a bit, and then pressing Ctrl-C, we
currently see:
(gdb) c
Continuing.
^C[New Thread 3850398.3887449]
[New Thread 3850398.3887500]
[New Thread 3850398.3887551]
[New Thread 3850398.3887602]
[New Thread 3850398.3887653]
...
Thread 1 "attach-many-sho" received signal SIGINT, Interrupt.
0x00007ffff7e6a23f in __GI___clock_nanosleep (clock_id=clock_id@entry=0, flags=flags@entry=0, req=req@entry=0x7fffffffda80, rem=rem@entry=0x7fffffffda80)
at ../sysdeps/unix/sysv/linux/clock_nanosleep.c:78
78 in ../sysdeps/unix/sysv/linux/clock_nanosleep.c
(gdb)
Above, we only see "New Thread" notifications, even though threads
were deleted.
After this patch, we'll see:
(gdb) c
Continuing.
^C[Thread 3558643.3577053 exited]
[Thread 3558643.3577104 exited]
[Thread 3558643.3577155 exited]
[Thread 3558643.3579603 exited]
...
[New Thread 3558643.3597415]
[New Thread 3558643.3600015]
[New Thread 3558643.3599965]
...
Thread 1 "attach-many-sho" received signal SIGINT, Interrupt.
0x00007ffff7e6a23f in __GI___clock_nanosleep (clock_id=clock_id@entry=0, flags=flags@entry=0, req=req@entry=0x7fffffffda80, rem=rem@entry=0x7fffffffda80)
at ../sysdeps/unix/sysv/linux/clock_nanosleep.c:78
78 in ../sysdeps/unix/sysv/linux/clock_nanosleep.c
(gdb) q
This commit fixes this by moving the thread exit printing to common
code instead, triggered from within delete_thread (or rather,
set_thread_exited).
There's one wrinkle, though. While most targest want to print:
[Thread ... exited]
the Windows target wants to print:
[Thread ... exited with code <exit_code>]
... and sometimes wants to suppress the notification for the main
thread. To address that, this commits adds a delete_thread_with_code
function, only used by that target (so far).
This fix was originally posted as part of a larger series:
https://inbox.sourceware.org/gdb-patches/20221212203101.1034916-1-pedro@palves.net/
But didn't really need to be part of that series. In order to get
this fix merged sooner, I (Andrew Burgess) have rebased this commit
outside of the original series. Any bugs introduced while splitting
this patch out and rebasing, are entirely my own.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30129
Co-Authored-By: Andrew Burgess <aburgess@redhat.com>
|
|
After the previous commit, exit_inferior_1 no longer makes use of the
silent parameter. This commit removes this parameter and cleans up
the callers.
After doing this exit_inferior_1, exit_inferior, and
exit_inferior_silent are all equivalent, so rename exit_inferior_1 to
exit_inferior and delete exit_inferior_silent, update all the callers.
Also I spotted the declaration exit_inferior_num_silent in inferior.h,
but this function is not defined anywhere, so I deleted the
declaration.
There should be no user visible changes after this commit.
|
|
After this commit:
commit a78ef8757418105c35685c5d82b9fdf79459321b
Date: Wed Jun 22 18:10:00 2022 +0100
Always emit =thread-exited notifications, even if silent
The function mi_interp::on_thread_exited (or mi_thread_exit as the
function was called back then) no longer makes use of the "silent"
parameter.
As a result there is no difference between inferior::clear_thread_list
with silent true or false, because:
- None of the interpreter ::on_thread_exited functions rely on the
silent parameter, and
- None of GDB's thread_exit observers rely on the silent parameter
either.
This commit removes the silent parameter from
inferior::clear_thread_list, and makes the function always silent.
This commit was originally part of a larger series:
https://inbox.sourceware.org/gdb-patches/20221212203101.1034916-1-pedro@palves.net/
But didn't really need to be part of that series. I had an interest
in seeing this patch merged:
https://inbox.sourceware.org/gdb-patches/20221212203101.1034916-31-pedro@palves.net/
Which also didn't really need to be part of the larger series, but
does depend, at least a little, on this commit. In order to get the
fix I'm interested in merged quicker, I (Andrew Burgess) have rebased
this commit outside of the original series. Any bugs introduced while
splitting this patch out and rebasing, are entirely my own.
There should be no user visible changes after this commit.
Co-Authored-By: Andrew Burgess <aburgess@redhat.com>
|
|
Same idea as previous patches, but for inferior_removed.
Change-Id: I7971840bbbdcfabf77e2ded7584830c9dfdd10d0
|
|
Same idea as previous patches, but for inferior_disappeared.
For symmetry with on_inferior_appeared, I named this one
on_inferior_disappeared, despite the observer being called
inferior_exit. This is called when detaching an inferior, so I think
that calling it "disappeared" is a bit less misleading (the observer
should probably be renamed later).
Change-Id: I372101586bc9454997953c1e540a2a6685f53ef6
|
|
Same idea as previous patches, but for inferior_appeared.
Change-Id: Ibe4feba34274549a886b1dfb5b3f8d59ae79e1b5
|
|
Same idea as previous patches, but for inferior_added.
mi_interp::init avoided using mi_inferior_added, since, as the comment
used to say, it would notify all MI interpreters. Now, it's easy to
only notify the new interpreter, so it's possible to just call the
on_inferior_added method in mi_interp::init.
Change-Id: I0eddbd5367217d1c982516982089913019ef309f
|
|
Same as previous patches, but for user_selected_context_changed.
Change-Id: I40de15be897671227d4bcf3e747f0fd595f0d5be
|
|
This patch turns set_inferior_args_vector into an overload of
inferior::set_args.
Regression tested on x86-64 Fedora 36.
|
|
Add the maybe_switch_inferior function, which ensures that the given
inferior is the current one. Return an instantiated
scoped_restore_current_thread object only we actually needed to switch
inferior.
Returning a scoped_restore_current_thread requires it to be
move-constructible, so give it a move constructor.
Change-Id: I1231037102ed6166f2530399e8257ad937fb0569
Reviewed-By: Pedro Alves <pedro@palves.net>
|
|
Make find_thread_ptid (the overload that takes an inferior) a method of
struct inferior.
Change-Id: Ie5b9fa623ff35aa7ddb45e2805254fc8e83c9cd4
Reviewed-By: Tom Tromey <tom@tromey.com>
|
|
Move the implementation over to target_desc_info. Remove the
target_desc_info forward declaration in target-descriptions.h, it's no
longer needed.
Change-Id: Ic95060341685afe0b73af591ca6efe32f5e7e892
|
|
This function is now trivial, we can just copy inferior::tdesc_info
where needed.
Change-Id: I25185e2cd4ba1ef24a822d9e0eebec6e611d54d6
|
|
I initially made this field a unique pointer, to have automatic memory
management. But I then thought that the field didn't really need to be
allocated separately from struct inferior. So make it a regular
non-pointer field of inferior.
Remove target_desc_info_free, as it's no longer needed.
Change-Id: Ica2b97071226f31c40e86222a2f6922454df1229
|
|
This commit is the result of running the gdb/copyright.py script,
which automated the update of the copyright year range for all
source files managed by the GDB project to be updated to include
year 2023.
|
|
This commit:
commit 53cf95c3389a3ecd97276d322e4a60fe3396a201
Date: Wed Dec 14 14:17:44 2022 +0000
gdb: make more use of make_target_connection_string
Introduced a couple of inefficient uses of std::string, both of which
are fixed in this commit.
There should be no user visible changes after this commit.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
I noticed that we have a function make_target_connection_string which
wraps all the logic for creating a string that describes a target
connection - but in some places we are not calling this function,
instead we duplicate the function's logic.
This commit cleans this up, and calls make_target_connection_string
where possible.
There should be no user visible changes after this commit.
|
|
Now that the inferiors target_stack automatically manages target
reference counts, we might think that we don't need to unpush targets
when an inferior is deleted...
...unfortunately that is not the case. The inferior::unpush function
can do some work depending on the type of target, so it is important
that we still pass through this function.
To ensure that this is the case, in this commit I've added an assert
to inferior::~inferior that ensures the inferior's target_stack is
empty (except for the ever present dummy_target).
I've then added a pop_all_targets call to delete_inferior, otherwise
the new assert will fire in, e.g. the gdb.python/py-inferior.exp test.
|
|
This commit removes the global functions pop_all_targets,
pop_all_targets_above, and pop_all_targets_at_and_above, and makes
them methods on the inferior class.
As the pop_all_targets functions will unpush each target, which
decrements the targets reference count, it is possible that the target
might be closed.
Right now, closing a target, in some cases, depends on the current
inferior being set correctly, that is, to the inferior from which the
target was popped.
To facilitate this I have used switch_to_inferior_no_thread within the
new methods. Previously it was the responsibility of the caller to
ensure that the correct inferior was selected.
In a couple of places (event-top.c and top.c) I have been able to
remove a previous switch_to_inferior_no_thread call.
In remote_unpush_target (remote.c) I have left the
switch_to_inferior_no_thread call as it is required for the
generic_mourn_inferior call.
|
|
Some class members were changed to bool, but there was
still some assignments or comparisons using 0/1.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
This rewrites registry.h, removing all the macros and replacing it
with relatively ordinary template classes. The result is less code
than the previous setup. It replaces large macros with a relatively
straightforward C++ class, and now manages its own cleanup.
The existing type-safe "key" class is replaced with the equivalent
template class. This approach ended up requiring relatively few
changes to the users of the registry code in gdb -- code using the key
system just required a small change to the key's declaration.
All existing users of the old C-like API are now converted to use the
type-safe API. This mostly involved changing explicit deletion
functions to be an operator() in a deleter class.
The old "save/free" two-phase process is removed, and replaced with a
single "free" phase. No existing code used both phases.
The old "free" callbacks took a parameter for the enclosing container
object. However, this wasn't truly needed and is removed here as
well.
|
|
Now that filtered and unfiltered output can be treated identically, we
can unify the printf family of functions. This is done under the name
"gdb_printf". Most of this patch was written by script.
|
|
A number of spots call printf_unfiltered only because they are in code
that should not be interrupted by the pager. However, I believe these
cases are all handled by infrun's blanket ban on paging, and so can be
converted to the default (_filtered) API.
After this patch, I think all the remaining _unfiltered calls are ones
that really ought to be. A few -- namely in complete_command -- could
be replaced by a scoped assignment to pagination_enabled, but for the
remainder, the code seems simple enough like this.
|
|
Prior to the multi-target support commit:
commit 5b6d1e4fa4fc6827c7b3f0e99ff120dfa14d65d2
Date: Fri Jan 10 20:06:08 2020 +0000
Multi-target support
When a new inferior was added using the MI -add-inferior command, the
new inferior would be using the same target as all the other
inferiors. This makes sense, GDB only supported a single target stack
at a time.
After the above commit, each inferior has its own target stack.
To maintain backward compatibility, for the CLI add-inferior command,
when a new inferior is added the above commit has the new inferior
inherit a copy of the target stack from the current inferior.
Unfortunately, this same backward compatibility is missing for the MI.
This commit fixes this oversight.
Now, when the -add-inferior MI command is used, the new inferior will
inherit a copy of the target stack from the current inferior.
|
|
No kind of internal var uses it remove it. This makes the transition to
using a variant easier, since we don't need to think about where this
should be called (in a destructor or not), if it can throw, etc.
Change-Id: Iebbc867d1ce6716480450d9790410d6684cbe4dd
|
|
I found a few spots where filename styling ought to be applied, but is
not.
|
|
This moves the gdb_argv class to a new header in gdbsupport.
|
|
This commit brings all the changes made by running gdb/copyright.py
as per GDB's Start of New Year Procedure.
For the avoidance of doubt, all changes in this commits were
performed by the script.
|
|
This commit ensures that the following settings are cloned from one
inferior to the new one when processing the clone-inferior command:
- inferior-tty
- environment variables
- cwd
- args
Some of those parameters can be passed as command line arguments to GDB
(-args and -tty), so one could expect the clone-inferior to respect
those flags. The following debugging session illustrates that:
gdb -nx -quiet -batch \
-ex "show args" \
-ex "show inferior-tty" \
-ex "clone-inferior" \
-ex "inferior 2" \
-ex "show args" \
-ex "show inferior-tty" \
-tty=/some/tty \
-args echo foo bar
Argument list to give program being debugged when it is started is "foo bar".
Terminal for future runs of program being debugged is "/some/tty".
[New inferior 2]
Added inferior 2.
[Switching to inferior 2 [<null>] (/bin/echo)]
Argument list to give program being debugged when it is started is "".
Terminal for future runs of program being debugged is "".
The other properties this commit copies on clone (i.e. CWD and the
environment variables) are included since they are related (in the sense
that they influence the runtime behavior of the program) even if they
cannot be directly set using command line switches.
There is a chance that this patch changes existing user workflow. I
think that this change is mostly harmless. If users want to start a new
inferior based on an existing one, they probably already propagate those
settings to the new inferior in some way.
Tested on x86_64-linux.
Change-Id: I3b1f28b662f246228b37bb24c2ea1481567b363d
|
|
Add new commands:
set debug threads on|off
show debug threads
Prints additional debug information relating to thread creation and
deletion.
GDB already announces when threads are created of course.... most of
the time, but sometimes threads are added silently, in which case this
debug message is the only mechanism to see the thread being added.
Also, though GDB does announce when a thread exits, it doesn't
announce when the thread object is deleted, I've added a debug message
for that.
Additionally, having message printed through the debug system will
cause the messages to be nested to an appropriate depth when other
debug sub-systems are turned on (especially things like `infrun` and
`lin-lwp`).
|
|
Same idea as the previous patch, but for m_terminal.
Change-Id: If9367d5db8c976a4336680adca4ea5bc31ab64d2
|
|
When debugging a large number of threads (thousands), looking up a
thread by ptid_t using the inferior::thread_list linked list can add up.
Add inferior::thread_map, an std::unordered_map indexed by ptid_t, and
change the find_thread_ptid function to look up a thread using
std::unordered_map::find, instead of iterating on all of the
inferior's threads. This should make it faster to look up a thread
from its ptid.
Change-Id: I3a8da0a839e18dee5bb98b8b7dbeb7f3dfa8ae1c
Co-Authored-By: Pedro Alves <pedro@palves.net>
|
|
status
Looking up threads that are both resumed and have a pending wait
status to report is something that we do quite often in the fast path
and is expensive if there are many threads, since it currently requires
walking whole thread lists.
The first instance is in maybe_set_commit_resumed_all_targets. This is
called after handling each event in fetch_inferior_event, to see if we
should ask targets to commit their resumed threads or not. If at least
one thread is resumed but has a pending wait status, we don't ask the
targets to commit their resumed threads, because we want to consume and
handle the pending wait status first.
The second instance is in random_pending_event_thread, where we want to
select a random thread among all those that are resumed and have a
pending wait status. This is called every time we try to consume
events, to see if there are any pending events that we we want to
consume, before asking the targets for more events.
To allow optimizing these cases, maintain a per-process-target list of
threads that are resumed and have a pending wait status.
In maybe_set_commit_resumed_all_targets, we'll be able to check in O(1)
if there are any such threads simply by checking whether the list is
empty.
In random_pending_event_thread, we'll be able to use that list, which
will be quicker than iterating the list of threads, especially when
there are no resumed with pending wait status threads.
About implementation details: using the new setters on class
thread_info, it's relatively easy to maintain that list. Any time the
"resumed" or "pending wait status" property is changed, we check whether
that should cause the thread to be added or removed from the list.
In set_thread_exited, we try to remove the thread from the list, because
keeping an exited thread in that list would make no sense (especially if
the thread is freed). My first implementation assumed that a process
stratum target was always present when set_thread_exited is called.
That's however, not the case: in some cases, targets unpush themselves
from an inferior and then call "exit_inferior", which exits all the
threads. If the target is unpushed before set_thread_exited is called
on the threads, it means we could mistakenly leave some threads in the
list. I tried to see how hard it would be to make it such that targets
have to exit all threads before unpushing themselves from the inferior
(that would seem logical to me, we don't want threads belonging to an
inferior that has no process target). That seemed quite difficult and
not worth the time at the moment. Instead, I changed
inferior::unpush_target to remove all threads of that inferior from the
list.
As of this patch, the list is not used, this is done in the subsequent
patches.
The debug messages in process-stratum-target.c need to print some ptids.
However, they can't use target_pid_to_str to print them without
introducing a dependency on the current inferior (the current inferior
is used to get the current target stack). For debug messages, I find it
clearer to print the spelled out ptid anyway (the pid, lwp and tid
values). Add a ptid_t::to_string method that returns a string
representation of the ptid that is meant for debug messages, a bit like
we already have frame_id::to_string.
Change-Id: Iad8f93db2d13984dd5aa5867db940ed1169dbb67
|
|
Change inferior_list, the global list of inferiors, to use
intrusive_list. I think most other changes are somewhat obvious
fallouts from this change.
There is a small change in behavior in scoped_mock_context. Before this
patch, constructing a scoped_mock_context would replace the whole
inferior list with only the new mock inferior. Tests using two
scoped_mock_contexts therefore needed to manually link the two inferiors
together, as the second scoped_mock_context would bump the first mock
inferior from the thread list. With this patch, a scoped_mock_context
adds its mock inferior to the inferior list on construction, and removes
it on destruction. This means that tests run with mock inferiors in the
inferior list in addition to any pre-existing inferiors (there is always
at least one). There is no possible pid clash problem, since each
scoped mock inferior uses its own process target, and pids are per
process target.
Co-Authored-By: Simon Marchi <simon.marchi@efficios.com>
Change-Id: I7eb6a8f867d4dcf8b8cd2dcffd118f7270756018
|
|
GDB currently has several objects that are put in a singly linked list,
by having the object's type have a "next" pointer directly. For
example, struct thread_info and struct inferior. Because these are
simply-linked lists, and we don't keep track of a "tail" pointer, when
we want to append a new element on the list, we need to walk the whole
list to find the current tail. It would be nice to get rid of that
walk. Removing elements from such lists also requires a walk, to find
the "previous" position relative to the element being removed. To
eliminate the need for that walk, we could make those lists
doubly-linked, by adding a "prev" pointer alongside "next". It would be
nice to avoid the boilerplate associated with maintaining such a list
manually, though. That is what the new intrusive_list type addresses.
With an intrusive list, it's also possible to move items out of the
list without destroying them, which is interesting in our case for
example for threads, when we exit them, but can't destroy them
immediately. We currently keep exited threads on the thread list, but
we could change that which would simplify some things.
Note that with std::list, element removal is O(N). I.e., with
std::list, we need to walk the list to find the iterator pointing to
the position to remove. However, we could store a list iterator
inside the object as soon as we put the object in the list, to address
it, because std::list iterators are not invalidated when other
elements are added/removed. However, if you need to put the same
object in more than one list, then std::list<object> doesn't work.
You need to instead use std::list<object *>, which is less efficient
for requiring extra memory allocations. For an example of an object
in multiple lists, see the step_over_next/step_over_prev fields in
thread_info:
/* Step-over chain. A thread is in the step-over queue if these are
non-NULL. If only a single thread is in the chain, then these
fields point to self. */
struct thread_info *step_over_prev = NULL;
struct thread_info *step_over_next = NULL;
The new intrusive_list type gives us the advantages of an intrusive
linked list, while avoiding the boilerplate associated with manually
maintaining it.
intrusive_list's API follows the standard container interface, and thus
std::list's interface. It is based the API of Boost's intrusive list,
here:
https://www.boost.org/doc/libs/1_73_0/doc/html/boost/intrusive/list.html
Our implementation is relatively simple, while Boost's is complicated
and intertwined due to a lot of customization options, which our version
doesn't have.
The easiest way to use an intrusive_list is to make the list's element
type inherit from intrusive_node. This adds a prev/next pointers to
the element type. However, to support putting the same object in more
than one list, intrusive_list supports putting the "node" info as a
field member, so you can have more than one such nodes, one per list.
As a first guinea pig, this patch makes the per-inferior thread list use
intrusive_list using the base class method.
Unlike Boost's implementation, ours is not a circular list. An earlier
version of the patch was circular: the intrusive_list type included an
intrusive_list_node "head". In this design, a node contained pointers
to the previous and next nodes, not the previous and next elements.
This wasn't great for when debugging GDB with GDB, as it was difficult
to get from a pointer to the node to a pointer to the element. With the
design proposed in this patch, nodes contain pointers to the previous
and next elements, making it easy to traverse the list by hand and
inspect each element.
The intrusive_list object contains pointers to the first and last
elements of the list. They are nullptr if the list is empty.
Each element's node contains a pointer to the previous and next
elements. The first element's previous pointer is nullptr and the last
element's next pointer is nullptr. Therefore, if there's a single
element in the list, both its previous and next pointers are nullptr.
To differentiate such an element from an element that is not linked into
a list, the previous and next pointers contain a special value (-1) when
the node is not linked. This is necessary to be able to reliably tell
if a given node is currently linked or not.
A begin() iterator points to the first item in the list. An end()
iterator contains nullptr. This makes iteration until end naturally
work, as advancing past the last element will make the iterator contain
nullptr, making it equal to the end iterator. If the list is empty,
a begin() iterator will contain nullptr from the start, and therefore be
immediately equal to the end.
Iterating on an intrusive_list yields references to objects (e.g.
`thread_info&`). The rest of GDB currently expects iterators and ranges
to yield pointers (e.g. `thread_info*`). To bridge the gap, add the
reference_to_pointer_iterator type. It is used to define
inf_threads_iterator.
Add a Python pretty-printer, to help inspecting intrusive lists when
debugging GDB with GDB. Here's an example of the output:
(top-gdb) p current_inferior_.m_obj.thread_list
$1 = intrusive list of thread_info = {0x61700002c000, 0x617000069080, 0x617000069400, 0x61700006d680, 0x61700006eb80}
It's not possible with current master, but with this patch [1] that I
hope will be merged eventually, it's possible to index the list and
access the pretty-printed value's children:
(top-gdb) p current_inferior_.m_obj.thread_list[1]
$2 = (thread_info *) 0x617000069080
(top-gdb) p current_inferior_.m_obj.thread_list[1].ptid
$3 = {
m_pid = 406499,
m_lwp = 406503,
m_tid = 0
}
Even though iterating the list in C++ yields references, the Python
pretty-printer yields pointers. The reason for this is that the output
of printing the thread list above would be unreadable, IMO, if each
thread_info object was printed in-line, since they contain so much
information. I think it's more useful to print pointers, and let the
user drill down as needed.
[1] https://sourceware.org/pipermail/gdb-patches/2021-April/178050.html
Co-Authored-By: Simon Marchi <simon.marchi@efficios.com>
Change-Id: I3412a14dc77f25876d742dab8f44e0ba7c7586c0
|
|
While reviewing another patch, I realized that gdbarch_info_init could
easily be removed in favor of initializing gdbarch_info fields directly
in the struct declaration. The only odd part is the union. I don't
know if it's actually important for it to be zero-initialized, but I
presume it is. I added a constructor to gdbarch_info to take care of
that. A proper solution would be to use std::variant. Or, these could
also be separate fields, the little extra space required wouldn't
matter.
gdb/ChangeLog:
* gdbarch.sh (struct gdbarch_info): Initialize fields, add
constructor.
* gdbarch.h: Re-generate.
* arch-utils.h (gdbarch_info_init): Remove, delete all usages.
* arch-utils.c (gdbarch_info_init): Remove.
Change-Id: I7502e08fe0f278d84eef1667a072e8a97bda5ab5
|
|
While adding a ui_out::text () overload accepting a std::string, I
noticed that several callers of ui_out::field_string () were converting
std::string instances to char pointers even if not necessary.
gdb/ChangeLog:
* ui-out.c (ui_out::field_string): Add missing style_argument
to the overload accepting a std::string, to make it equivalent
to the char pointer version.
* ui-out.h (class ui_out): Ditto.
* break-catch-sig.c (signal_catchpoint_print_one): Do not
convert std::strings to char pointers before passing them to
ui_out::field_string ().
* break-catch-throw.c (print_one_detail_exception_catchpoint):
Ditto.
* cli/cli-setshow.c (do_show_command): Ditto.
* disasm.c (gdb_pretty_print_disassembler::pretty_print_insn):
Ditto.
* infcmd.c (print_return_value_1): Ditto.
* inferior.c (print_inferior): Ditto.
* linux-thread-db.c (info_auto_load_libthread_db): Ditto.
* mi/mi-cmd-var.c (print_varobj): Ditto.
(mi_cmd_var_set_format): Ditto.
(mi_cmd_var_info_type): Ditto.
(mi_cmd_var_info_expression): Ditto.
(mi_cmd_var_evaluate_expression): Ditto.
(mi_cmd_var_assign): Ditto.
(varobj_update_one): Ditto.
* mi/mi-main.c (list_available_thread_groups): Ditto.
(mi_cmd_data_read_memory_bytes): Ditto.
(mi_cmd_trace_frame_collected): Ditto.
* osdata.c (info_osdata): Ditto.
* probe.c (info_probes_for_spops): Ditto.
* target-connection.c (print_connection): Ditto.
* thread.c (print_thread_info_1): Ditto.
* tracepoint.c (print_one_static_tracepoint_marker): Ditto.
|
|
Use unique_xmalloc_ptr to avoid manual memory management.
gdb/ChangeLog:
* inferior.h (class inferior) <args>: Change type to
unique_xmalloc_ptr.
* inferior.c (inferior::~inferior): Don't free args.
* infcmd.c (get_inferior_args): Adjust.
(set_inferior_args): Adjust.
Change-Id: I96300e59eb2faf2d80660416a8f5694d243a944e
|
|
Turn continuations-related functions into methods of the inferior
class. This is a refactoring.
gdb/ChangeLog:
2021-04-22 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
* Makefile.in (COMMON_SFILES): Remove continuations.c.
* inferior.c (inferior::add_continuation): New method, adapted
from 'add_inferior_continuation'.
(inferior::do_all_continuations): New method, adapted from
'do_all_inferior_continuations'.
(inferior::~inferior): Clear the list of continuations directly.
* inferior.h (class inferior) <continuations>: Rename into...
<m_continuations>: ...this and make private.
* continuations.c: Remove.
* continuations.h: Remove.
* event-top.c: Don't include "continuations.h".
Update the users below.
* inf-loop.c (inferior_event_handler)
* infcmd.c (attach_command)
(notice_new_inferior): Update.
|
|
The current_top_target function is a hidden dependency on the current
inferior. Since I'd like to slowly move towards reducing our dependency
on the global current state, remove this function and make callers use
current_inferior ()->top_target ()
There is no expected change in behavior, but this one step towards
making those callers use the inferior from their context, rather than
refer to the global current inferior.
gdb/ChangeLog:
* target.h (current_top_target): Remove, make callers use the
current inferior instead.
* target.c (current_top_target): Remove.
Change-Id: Iccd457036f84466cdaa3865aa3f9339a24ea001d
|
|
Same as the previous patch, but for the push_target functions.
The implementation of the move variant is moved to a new overload of
inferior::push_target.
gdb/ChangeLog:
* target.h (push_target): Remove, update callers to use
inferior::push_target.
* target.c (push_target): Remove.
* inferior.h (class inferior) <push_target>: New overload.
Change-Id: I5a95496666278b8f3965e5e8aecb76f54a97c185
|
|
This patch makes the inferior command display information about the
current inferior when called with no argument. This behavior is similar
to the one of the thread command.
Before patch:
(gdb) info inferior
Num Description Connection Executable
* 1 process 19221 1 (native) /home/lsix/tmp/a.out
2 process 19239 1 (native) /home/lsix/tmp/a.out
(gdb) inferior 2
[Switching to inferior 2 [process 19239] (/home/lsix/tmp/a.out)]
[Switching to thread 2.1 (process 19239)]
#0 0x0000000000401146 in main ()
(gdb) inferior
Argument required (expression to compute).
After patch:
(gdb) info inferior
Num Description Connection Executable
* 1 process 18699 1 (native) /home/lsix/tmp/a.out
2 process 18705 1 (native) /home/lsix/tmp/a.out
(gdb) inferior 2
[Switching to inferior 2 [process 18705] (/home/lsix/tmp/a.out)]
[Switching to thread 2.1 (process 18705)]
#0 0x0000000000401146 in main ()
(gdb) inferior
[Current inferior is 2 [process 18705] (/home/lsix/tmp/a.out)]
gdb/doc/ChangeLog:
* gdb.texinfo (Inferiors Connections and Programs): Document the
inferior command when used without argument.
gdb/ChangeLog:
* NEWS: Add entry for the behavior change of the inferior command.
* inferior.c (inferior_command): When no argument is given to the
inferior command, display info about the currently selected
inferior.
gdb/testsuite/ChangeLog:
* gdb.base/inferior-noarg.c: New test.
* gdb.base/inferior-noarg.exp: New test.
|
