| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
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.
|
|
Spotted a minor type, a missing ')', in a debug message.
|
|
Add a new convenience variable $_inferior_thread_count that contains
the number of live (non-exited) threads in the current inferior. This
can be used in command scripts, or breakpoint conditions, etc to
adjust the behaviour for multi-threaded inferiors.
This value is only stable in all-stop mode. In non-stop mode, where
new threads can be started, and existing threads exit, at any time,
this convenience variable can give a different value each time it is
evaluated.
|
|
Add some extra 'threads' debug in a couple of places in thread.c.
I've also added an additional gdb_assert in one case.
|
|
Turns out we'll be gaining a new use of this function very soon, the
incoming AMDGPU port needs it. Let's add it back, as it isn't really
hurting anything.
This reverts commit 39b8a8090ed7e8967ceca3655aa5f3a2ae91219d.
|
|
delete_thread_silent is no longer used anywhere. Delete it.
Change-Id: Iafcec12339861d5ab2e29c14d7b1f884c9e11c0f
|
|
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.
|
|
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
|
|
While working on function calls, I realized that the thread_fsm member
of struct thread_info is a raw pointer to a resource it owns. This
commit changes the type of the thread_fsm member to a std::unique_ptr in
order to signify this ownership relationship and slightly ease resource
management (no need to manually call delete).
To ensure consistent use, the field is made a private member
(m_thread_fsm). The setter method (set_thread_fsm) can then check
that it is incorrect to associate a FSM to a thread_info object if
another one is already in place. This is ensured by an assertion.
The function run_inferior_call takes an argument as a pointer to a
call_thread_fsm and installs it in it in a thread_info instance. Also
change this function's signature to accept a unique_ptr in order to
signify that the ownership of the call_thread_fsm is transferred during
the call.
No user visible change expected after this commit.
Tested on x86_64-linux with no regression observed.
Change-Id: Ia1224f72a4afa247801ce6650ce82f90224a9ae8
|
|
This moves the gdb_regex convenience class to gdbsupport.
|
|
Same idea as 0fab79556484 ("gdb: use ptid_t::to_string in infrun debug
messages"), but throughout GDB.
Change-Id: I62ba36eaef29935316d7187b9b13d7b88491acc1
|
|
While working on another patch I wanted to add some extra debug
information to the attach_command function. This required me to add a
new function to convert the thread_info::state variable to a string.
The new debug might be useful to others, and the state to string
function might be useful in other locations, so I thought I'd merge
it.
|
|
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.
|
|
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`).
|
|
This adds a 'task apply' command, which is the Ada tasking analogue of
'thread apply'. Unlike 'thread apply', it doesn't offer the
'ascending' flag; but otherwise it's essentially the same.
|
|
I stumbled on a bug caused by the fact that a code path read
target_waitstatus::value::sig (expecting it to contain a gdb_signal
value) while target_waitstatus::kind was TARGET_WAITKIND_FORKED. This
meant that the active union field was in fact
target_waitstatus::value::related_pid, and contained a ptid. The read
signal value was therefore garbage, and that caused GDB to crash soon
after. Or, since that GDB was built with ubsan, this nice error
message:
/home/simark/src/binutils-gdb/gdb/linux-nat.c:1271:12: runtime error: load of value 2686365, which is not a valid value for type 'gdb_signal'
Despite being a large-ish change, I think it would be nice to make
target_waitstatus safe against that kind of bug. As already done
elsewhere (e.g. dynamic_prop), validate that the type of value read from
the union matches what is supposed to be the active field.
- Make the kind and value of target_waitstatus private.
- Make the kind initialized to TARGET_WAITKIND_IGNORE on
target_waitstatus construction. This is what most users appear to do
explicitly.
- Add setters, one for each kind. Each setter takes as a parameter the
data associated to that kind, if any. This makes it impossible to
forget to attach the associated data.
- Add getters, one for each associated data type. Each getter
validates that the data type fetched by the user matches the wait
status kind.
- Change "integer" to "exit_status", "related_pid" to "child_ptid",
just because that's more precise terminology.
- Fix all users.
That last point is semi-mechanical. There are a lot of obvious changes,
but some less obvious ones. For example, it's not possible to set the
kind at some point and the associated data later, as some users did.
But in any case, the intent of the code should not change in this patch.
This was tested on x86-64 Linux (unix, native-gdbserver and
native-extended-gdbserver boards). It was built-tested on x86-64
FreeBSD, NetBSD, MinGW and macOS. The rest of the changes to native
files was done as a best effort. If I forgot any place to update in
these files, it should be easy to fix (unless the change happens to
reveal an actual bug).
Change-Id: I0ae967df1ff6e28de78abbe3ac9b4b2ff4ad03b7
|
|
The pattern for using execute_command_to_string is:
...
std::string output;
output = execute_fn_to_string (fn, term_out);
...
This results in a problem when using it in a try/catch:
...
try
{
output = execute_fn_to_string (fn, term_out)
}
catch (const gdb_exception &e)
{
/* Use output. */
}
...
If an expection was thrown during execute_fn_to_string, then the output
remains unassigned, while it could be worthwhile to known what output was
generated by gdb before the expection was thrown.
Fix this by returning the string using a parameter instead:
...
execute_fn_to_string (output, fn, term_out)
...
Also add a variant without string parameter, to support places where the
function is used while ignoring the result:
...
execute_fn_to_string (fn, term_out)
...
Tested on x86_64-linux.
|
|
This started out as changing thread_info::name to a unique_xmalloc_ptr.
That showed that almost all users of that field had the same logic to
get a thread's name: use thread_info::name if non-nullptr, else ask the
target. Factor out this logic in a new thread_name free function. Make
the field private (rename to m_name) and add some accessors.
Change-Id: Iebdd95f4cd21fbefc505249bd1d05befc466a2fc
|
|
Currently the stop_pc field of thread_suspect_state is a CORE_ADDR and
when we want to indicate that there is no stop_pc available we set
this field back to a special value.
There are actually two special values used, in post_create_inferior
the stop_pc is set to 0. This is a little unfortunate, there are
plenty of embedded targets where 0 is a valid pc value. The more
common special value for stop_pc though, is set in
thread_info::set_executing, where the value (~(CORE_ADDR) 0) is used.
This commit changes things so that the stop_pc is instead a
gdb::optional. We can now explicitly reset the field to an
uninitialised state, we also have asserts that we don't read the
stop_pc when its in an uninitialised state (both in
gdbsupport/gdb_optional.h, when compiling with _GLIBCXX_DEBUG
defined, and in thread_info::stop_pc).
One situation where a thread will not have a stop_pc value is when the
thread is stopped as a consequence of GDB being in all stop mode, and
some other thread stopped at an interesting event. When GDB brings
all the other threads to a stop those other threads will not have a
stop_pc set (thus avoiding an unnecessary read of the pc register).
Previously, when GDB passed through handle_one (in infrun.c) the
threads executing flag was set to false and the stop_pc field was left
unchanged, i.e. it would (previous) have been left as ~0.
Now, handle_one leaves the stop_pc with no value.
This caused a problem when we later try to set these threads running
again, in proceed() we compare the current pc with the cached stop_pc.
If the thread was stopped via handle_one then the stop_pc would have
been left as ~0, and the compare (in proceed) would (likely) fail.
Now however, this compare tries to read the stop_pc when it has no
value and this would trigger an assert.
To resolve this I've added thread_info::stop_pc_p() which returns true
if the thread has a cached stop_pc. We should only ever call
thread_info::stop_pc() if we know that there is a cached stop_pc,
however, this doesn't mean that every call to thread_info::stop_pc()
needs to be guarded with a call to thread_info::stop_pc_p(), in most
cases we know that the thread we are looking at stopped due to some
interesting event in that thread, and so, we know that the stop_pc is
valid.
After running the testsuite I've seen no other situations where
stop_pc is read uninitialised.
There should be no user visible changes after this commit.
|
|
Rename thread_info::executing to thread_info::m_executing, and make it
private. Add a new get/set member functions, and convert GDB to make
use of these.
The only real change of interest in this patch is in thread.c where I
have deleted the helper function set_executing_thread, and now just
use the new set function thread_info::set_executing. However, the old
helper function set_executing_thread included some code to reset the
thread's stop_pc, so I moved this code into the new function
thread_info::set_executing. However, I don't believe there is
anywhere that this results in a change of behaviour, previously the
executing flag was always set true through a call to
set_executing_thread anyway.
|
|
If I debug a single-thread program and look at the infrun debug logs, I
see:
[infrun] start_step_over: stealing global queue of threads to step, length = 2
That makes no sense... turns out there's a buglet in
thread_step_over_chain_length, "num" should be initialized to 0. I
think this bug is a leftover from an earlier version of the code (not
merged upstream) that manually walked the list, where the first item was
implicitly counted (hence the 1).
Change-Id: I0af03aa93509aed36528be5076894dc156a0b5ce
|
|
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
|
|
A following patch will want to take some action when a pending wait
status is set on or removed from a thread. Add a getter and a setter on
thread_info for the pending waitstatus, so that we can add some code in
the setter later.
The thing is, the pending wait status field is in the
thread_suspend_state, along with other fields that we need to backup
before and restore after the thread does an inferior function call.
Therefore, make the thread_suspend_state member private
(thread_info::suspend becomes thread_info::m_suspend), and add getters /
setters for all of its fields:
- pending wait status
- stop signal
- stop reason
- stop pc
For the pending wait status, add the additional has_pending_waitstatus
and clear_pending_waitstatus methods.
I think this makes the thread_info interface a bit nicer, because we
now access the fields as:
thread->stop_pc ()
rather than
thread->suspend.stop_pc
The stop_pc field being in the `suspend` structure is an implementation
detail of thread_info that callers don't need to be aware of.
For the backup / restore of the thread_suspend_state structure, add
save_suspend_to and restore_suspend_from methods. You might wonder why
`save_suspend_to`, as opposed to a simple getter like
thread_suspend_state &suspend ();
I want to make it clear that this is to be used only for backing up and
restoring the suspend state, _not_ to access fields like:
thread->suspend ()->stop_pc
Adding some getters / setters allows adding some assertions. I find
that this helps understand how things are supposed to work. Add:
- When getting the pending status (pending_waitstatus method), ensure
that there is a pending status.
- When setting a pending status (set_pending_waitstatus method), ensure
there is no pending status.
There is one case I found where this wasn't true - in
remote_target::process_initial_stop_replies - which needed adjustments
to respect that contract. I think it's because
process_initial_stop_replies is kind of (ab)using the
thread_info::suspend::waitstatus to store some statuses temporarily, for
its internal use (statuses it doesn't intent on leaving pending).
process_initial_stop_replies pulls out stop replies received during the
initial connection using target_wait. It always stores the received
event in `evthread->suspend.waitstatus`. But it only sets
waitstatus_pending_p, if it deems the event interesting enough to leave
pending, to be reported to the core:
if (ws.kind != TARGET_WAITKIND_STOPPED
|| ws.value.sig != GDB_SIGNAL_0)
evthread->suspend.waitstatus_pending_p = 1;
It later uses this flag a bit below, to choose which thread to make the
"selected" one:
if (selected == NULL
&& thread->suspend.waitstatus_pending_p)
selected = thread;
And ultimately that's used if the user-visible mode is all-stop, so that
we print the stop for that interesting thread:
/* In all-stop, we only print the status of one thread, and leave
others with their status pending. */
if (!non_stop)
{
thread_info *thread = selected;
if (thread == NULL)
thread = lowest_stopped;
if (thread == NULL)
thread = first;
print_one_stopped_thread (thread);
}
But in any case (all-stop or non-stop), print_one_stopped_thread needs
to access the waitstatus value of these threads that don't have a
pending waitstatus (those that had TARGET_WAITKIND_STOPPED +
GDB_SIGNAL_0). This doesn't work with the assertions I've
put.
So, change the code to only set the thread's wait status if it is an
interesting one that we are going to leave pending. If the thread
stopped due to a non-interesting event (TARGET_WAITKIND_STOPPED +
GDB_SIGNAL_0), don't store it. Adjust print_one_stopped_thread to
understand that if a thread has no pending waitstatus, it's because it
stopped with TARGET_WAITKIND_STOPPED + GDB_SIGNAL_0.
The call to set_last_target_status also uses the pending waitstatus.
However, given that the pending waitstatus for the thread may have been
cleared in print_one_stopped_thread (and that there might not even be a
pending waitstatus in the first place, as explained above), it is no
longer possible to do it at this point. To fix that, move the call to
set_last_target_status in print_one_stopped_thread. I think this will
preserve the existing behavior, because set_last_target_status is
currently using the current thread's wait status. And the current
thread is the last one for which print_one_stopped_thread is called. So
by calling set_last_target_status in print_one_stopped_thread, we'll get
the same result. set_last_target_status will possibly be called
multiple times, but only the last call will matter. It just means
possibly more calls to set_last_target_status, but those are cheap.
Change-Id: Iedab9653238eaf8231abcf0baa20145acc8b77a7
|
|
A following patch will want to do things when a thread's resumed state
changes. Make the `resumed` field private (renamed to `m_resumed`) and
add a getter and a setter for it. The following patch in question will
therefore be able to add some code to the setter.
Change-Id: I360c48cc55a036503174313261ce4e757d795319
|
|
The threads that need a step-over are currently linked using an
hand-written intrusive doubly-linked list, so that seems a very good
candidate for intrusive_list, convert it.
For this, we have a use case of appending a list to another one (in
start_step_over). Based on the std::list and Boost APIs, add a splice
method. However, only support splicing the other list at the end of the
`this` list, since that's all we need.
Add explicit default assignment operators to
reference_to_pointer_iterator, which are otherwise implicitly deleted.
This is needed because to define thread_step_over_list_safe_iterator, we
wrap reference_to_pointer_iterator inside a basic_safe_iterator, and
basic_safe_iterator needs to be able to copy-assign the wrapped
iterator. The move-assignment operator is therefore not needed, only
the copy-assignment operator is. But for completeness, add both.
Change-Id: I31b2ff67c7b78251314646b31887ef1dfebe510c
|
|
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
|
|
The alias creation functions currently accept a name to specify the
target command. They pass this to add_alias_cmd, which needs to lookup
the target command by name.
Given that:
- We don't support creating an alias for a command before that command
exists.
- We always use add_info_alias just after creating that target command,
and therefore have access to the target command's cmd_list_element.
... change add_com_alias to accept the target command as a
cmd_list_element (other functions are done in subsequent patches). This
ensures we don't create the alias before the target command, because you
need to get the cmd_list_element from somewhere when you call the alias
creation function. And it avoids an unecessary command lookup. So it
seems better to me in every aspect.
gdb/ChangeLog:
* command.h (add_com_alias): Accept target as
cmd_list_element. Update callers.
Change-Id: I24bed7da57221cc77606034de3023fedac015150
|
|
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.
|
|
gdb/ChangeLog:
* ui-out.h (class ui_out): Add ui_out::text accepting a constant
reference to a std::string. Fix all callers using
std::string::c_str.
* ui-out.c (ui_out::text): Ditto.
|
|
Previously, the prefixname field of struct cmd_list_element was manually
set for prefix commands. This seems verbose and error prone as it
required every single call to functions adding prefix commands to
specify the prefix name while the same information can be easily
generated.
Historically, this was not possible as the prefix field was null for
many commands, but this was fixed in commit
3f4d92ebdf7f848b5ccc9e8d8e8514c64fde1183 by Philippe Waroquiers, so
we can rely on the prefix field being set when generating the prefix
name.
This commit also fixes a use after free in this scenario:
* A command gets created via Python (using the gdb.Command class).
The prefix name member is dynamically allocated.
* An alias to the new command is created. The alias's prefixname is set
to point to the prefixname for the original command with a direct
assignment.
* A new command with the same name as the Python command is created.
* The object for the original Python command gets freed and its
prefixname gets freed as well.
* The alias is updated to point to the new command, but its prefixname
is not updated so it keeps pointing to the freed one.
gdb/ChangeLog:
* command.h (add_prefix_cmd): Remove the prefixname argument as
it can now be generated automatically. Update all callers.
(add_basic_prefix_cmd): Ditto.
(add_show_prefix_cmd): Ditto.
(add_prefix_cmd_suppress_notification): Ditto.
(add_abbrev_prefix_cmd): Ditto.
* cli/cli-decode.c (add_prefix_cmd): Ditto.
(add_basic_prefix_cmd): Ditto.
(add_show_prefix_cmd): Ditto.
(add_prefix_cmd_suppress_notification): Ditto.
(add_prefix_cmd_suppress_notification): Ditto.
(add_abbrev_prefix_cmd): Ditto.
* cli/cli-decode.h (struct cmd_list_element): Replace the
prefixname member variable with a method which generates the
prefix name at runtime. Update all code reading the prefix
name to use the method, and remove all code setting it.
* python/py-cmd.c (cmdpy_destroyer): Remove code to free the
prefixname member as it's now a method.
(cmdpy_function): Determine if the command is a prefix by
looking at prefixlist, not prefixname.
|
|
Running gdb-term.exp against gdbserver with "maint set target-non-stop
on", runs into this:
[infrun] fetch_inferior_event: exit
[infrun] fetch_inferior_event: enter
/home/pedro/gdb/binutils-gdb/src/gdb/thread.c:72: internal-error: thread_info* inferior_thread(): Assertion `current_thread_ != nullptr' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
This is a bug, please report it. For instructions, see:
<https://www.gnu.org/software/gdb/bugs/>.
FAIL: gdb.base/gdb-sigterm.exp: expect eof #2 (GDB internal error)
Resyncing due to internal error.
ERROR: : spawn id exp9 not open
while executing
"expect {
-i exp9 -timeout 10
-re "Quit this debugging session\\? \\(y or n\\) $" {
send_gdb "n\n" answer
incr count
}
-re "Create ..."
("uplevel" body line 1)
invoked from within
"uplevel $body" NONE : spawn id exp9 not open
ERROR: Could not resync from internal error (timeout)
gdb.base/gdb-sigterm.exp: expect eof #2: stepped 0 times
UNRESOLVED: gdb.base/gdb-sigterm.exp: 50 SIGTERM passes
The assertion fails here:
...
#5 0x000055af4b4a7164 in internal_error (file=0x55af4b5e5de8 "/home/pedro/gdb/binutils-gdb/src/gdb/thread.c", line=72, fmt=0x55af4b5e5ce9 "%s: Assertion `%s' failed.") at /home/pedro/gdb/binutils-gdb/src/gdbsupport/errors.cc:55
#6 0x000055af4b25fc43 in inferior_thread () at /home/pedro/gdb/binutils-gdb/src/gdb/thread.c:72
#7 0x000055af4b26177e in any_thread_of_inferior (inf=0x55af4cf874f0) at /home/pedro/gdb/binutils-gdb/src/gdb/thread.c:638
#8 0x000055af4b26eec8 in kill_or_detach (inf=0x55af4cf874f0, from_tty=0) at /home/pedro/gdb/binutils-gdb/src/gdb/top.c:1665
#9 0x000055af4b26f37f in quit_force (exit_arg=0x0, from_tty=0) at /home/pedro/gdb/binutils-gdb/src/gdb/top.c:1767
#10 0x000055af4b2f72a7 in quit () at /home/pedro/gdb/binutils-gdb/src/gdb/utils.c:633
#11 0x000055af4b2f730b in maybe_quit () at /home/pedro/gdb/binutils-gdb/src/gdb/utils.c:657
#12 0x000055af4b1adb74 in ser_base_wait_for (scb=0x55af4d02e460, timeout=0) at /home/pedro/gdb/binutils-gdb/src/gdb/ser-base.c:236
#13 0x000055af4b1adf0f in do_ser_base_readchar (scb=0x55af4d02e460, timeout=0) at /home/pedro/gdb/binutils-gdb/src/gdb/ser-base.c:365
#14 0x000055af4b1ae06d in generic_readchar (scb=0x55af4d02e460, timeout=0, do_readchar=0x55af4b1adeb1 <do_ser_base_readchar(serial*, int)>) at /home/pedro/gdb/binutils-gdb/src/gdb/ser-base.c:444
...
The bug is that any_thread_of_inferior incorrectly assumes that
there's always a selected thread. This fixes it.
gdb/ChangeLog:
* thread.c (any_thread_of_inferior): Check if there's a selected
thread before calling inferior_thread().
Change-Id: Ica4b9ec746121a7a7c22bef09baea72103b3853d
|
|
Unlike the previous patch, I don't propose that we take this patch into
gdb-10-branch.
This patch adds two asserts, prompted by investigating and fixing the
bug fixed by the previous patch.
The assert in find_thread_ptid would have caught the original issue
before the segfault (I think it's slightly more use friendly).
The assert in add_thread_silent would have made it clear that the
solution proposed in [1] isn't the right one. The solution ended up
passing nullptr as a target to add_thread. We don't want that, because
add_thread_silent uses it to look up the inferior to which to add the
thread. If the target is nullptr, we could find an inferior with the
same pid, but belonging to an unrelated target. So we always want a
non-nullptr target in add_thread_silent.
gdb/ChangeLog:
* thread.c (add_thread_silent): Add assert.
(find_thread_ptid): Add assert.
[1] https://sourceware.org/pipermail/gdb-patches/2021-February/176202.html
Change-Id: Ie593ee45c5eb02235e8e9fbcda612d48ce883852
|
|
I spotted that every time thr_try_catch_cmd is called GDB has already
switched to the required thread. The call to switch_to_thread at the
head of thr_try_catch_cmd is therefore redundant.
This commit replaces the call to switch_to_thread with an assertion
that we already have the required thread selected.
I also extended the header comment on thr_try_catch_cmd to make it
clearer when this function could throw an exception.
There should be no user visible changes after this commit.
gdb/ChangeLog:
* thread.c (thr_try_catch_cmd): Replace swith_to_thread with an
assert. Extend the header comment.
|
|
This commits the result of running gdb/copyright.py as per our Start
of New Year procedure...
gdb/ChangeLog
Update copyright year range in copyright header of all GDB files.
|
|
We can remove scoped_inc_dec_ref by changing the sole user to instead
keep a vector of thread_info_ref objects. This removes some manual
reference counting and simplifies the code a bit.
gdb/ChangeLog
2020-12-11 Tom Tromey <tom@tromey.com>
* thread.c (class scoped_inc_dec_ref): Remove.
(tp_array_compar_ascending, tp_array_compar_descending): Change
parameter types.
(thread_apply_all_command): Use thread_info_ref.
|
|
displaced steps
Today, GDB only allows a single displaced stepping operation to happen
per inferior at a time. There is a single displaced stepping buffer per
inferior, whose address is fixed (obtained with
gdbarch_displaced_step_location), managed by infrun.c.
In the case of the AMD ROCm target [1] (in the context of which this
work has been done), it is typical to have thousands of threads (or
waves, in SMT terminology) executing the same code, hitting the same
breakpoint (possibly conditional) and needing to to displaced step it at
the same time. The limitation of only one displaced step executing at a
any given time becomes a real bottleneck.
To fix this bottleneck, we want to make it possible for threads of a
same inferior to execute multiple displaced steps in parallel. This
patch builds the foundation for that.
In essence, this patch moves the task of preparing a displaced step and
cleaning up after to gdbarch functions. This allows using different
schemes for allocating and managing displaced stepping buffers for
different platforms. The gdbarch decides how to assign a buffer to a
thread that needs to execute a displaced step.
On the ROCm target, we are able to allocate one displaced stepping
buffer per thread, so a thread will never have to wait to execute a
displaced step.
On Linux, the entry point of the executable if used as the displaced
stepping buffer, since we assume that this code won't get used after
startup. From what I saw (I checked with a binary generated against
glibc and musl), on AMD64 we have enough space there to fit two
displaced stepping buffers. A subsequent patch makes AMD64/Linux use
two buffers.
In addition to having multiple displaced stepping buffers, there is also
the idea of sharing displaced stepping buffers between threads. Two
threads doing displaced steps for the same PC could use the same buffer
at the same time. Two threads stepping over the same instruction (same
opcode) at two different PCs may also be able to share a displaced
stepping buffer. This is an idea for future patches, but the
architecture built by this patch is made to allow this.
Now, the implementation details. The main part of this patch is moving
the responsibility of preparing and finishing a displaced step to the
gdbarch. Before this patch, preparing a displaced step is driven by the
displaced_step_prepare_throw function. It does some calls to the
gdbarch to do some low-level operations, but the high-level logic is
there. The steps are roughly:
- Ask the gdbarch for the displaced step buffer location
- Save the existing bytes in the displaced step buffer
- Ask the gdbarch to copy the instruction into the displaced step buffer
- Set the pc of the thread to the beginning of the displaced step buffer
Similarly, the "fixup" phase, executed after the instruction was
successfully single-stepped, is driven by the infrun code (function
displaced_step_finish). The steps are roughly:
- Restore the original bytes in the displaced stepping buffer
- Ask the gdbarch to fixup the instruction result (adjust the target's
registers or memory to do as if the instruction had been executed in
its original location)
The displaced_step_inferior_state::step_thread field indicates which
thread (if any) is currently using the displaced stepping buffer, so it
is used by displaced_step_prepare_throw to check if the displaced
stepping buffer is free to use or not.
This patch defers the whole task of preparing and cleaning up after a
displaced step to the gdbarch. Two new main gdbarch methods are added,
with the following semantics:
- gdbarch_displaced_step_prepare: Prepare for the given thread to
execute a displaced step of the instruction located at its current PC.
Upon return, everything should be ready for GDB to resume the thread
(with either a single step or continue, as indicated by
gdbarch_displaced_step_hw_singlestep) to make it displaced step the
instruction.
- gdbarch_displaced_step_finish: Called when the thread stopped after
having started a displaced step. Verify if the instruction was
executed, if so apply any fixup required to compensate for the fact
that the instruction was executed at a different place than its
original pc. Release any resources that were allocated for this
displaced step. Upon return, everything should be ready for GDB to
resume the thread in its "normal" code path.
The displaced_step_prepare_throw function now pretty much just offloads
to gdbarch_displaced_step_prepare and the displaced_step_finish function
offloads to gdbarch_displaced_step_finish.
The gdbarch_displaced_step_location method is now unnecessary, so is
removed. Indeed, the core of GDB doesn't know how many displaced step
buffers there are nor where they are.
To keep the existing behavior for existing architectures, the logic that
was previously implemented in infrun.c for preparing and finishing a
displaced step is moved to displaced-stepping.c, to the
displaced_step_buffer class. Architectures are modified to implement
the new gdbarch methods using this class. The behavior is not expected
to change.
The other important change (which arises from the above) is that the
core of GDB no longer prevents concurrent displaced steps. Before this
patch, start_step_over walks the global step over chain and tries to
initiate a step over (whether it is in-line or displaced). It follows
these rules:
- if an in-line step is in progress (in any inferior), don't start any
other step over
- if a displaced step is in progress for an inferior, don't start
another displaced step for that inferior
After starting a displaced step for a given inferior, it won't start
another displaced step for that inferior.
In the new code, start_step_over simply tries to initiate step overs for
all the threads in the list. But because threads may be added back to
the global list as it iterates the global list, trying to initiate step
overs, start_step_over now starts by stealing the global queue into a
local queue and iterates on the local queue. In the typical case, each
thread will either:
- have initiated a displaced step and be resumed
- have been added back by the global step over queue by
displaced_step_prepare_throw, because the gdbarch will have returned
that there aren't enough resources (i.e. buffers) to initiate a
displaced step for that thread
Lastly, if start_step_over initiates an in-line step, it stops
iterating, and moves back whatever remaining threads it had in its local
step over queue to the global step over queue.
Two other gdbarch methods are added, to handle some slightly annoying
corner cases. They feel awkwardly specific to these cases, but I don't
see any way around them:
- gdbarch_displaced_step_copy_insn_closure_by_addr: in
arm_pc_is_thumb, arm-tdep.c wants to get the closure for a given
buffer address.
- gdbarch_displaced_step_restore_all_in_ptid: when a process forks
(at least on Linux), the address space is copied. If some displaced
step buffers were in use at the time of the fork, we need to restore
the original bytes in the child's address space.
These two adjustments are also made in infrun.c:
- prepare_for_detach: there may be multiple threads doing displaced
steps when we detach, so wait until all of them are done
- handle_inferior_event: when we handle a fork event for a given
thread, it's possible that other threads are doing a displaced step at
the same time. Make sure to restore the displaced step buffer
contents in the child for them.
[1] https://github.com/ROCm-Developer-Tools/ROCgdb
gdb/ChangeLog:
* displaced-stepping.h (struct
displaced_step_copy_insn_closure): Adjust comments.
(struct displaced_step_inferior_state) <step_thread,
step_gdbarch, step_closure, step_original, step_copy,
step_saved_copy>: Remove fields.
(struct displaced_step_thread_state): New.
(struct displaced_step_buffer): New.
* displaced-stepping.c (displaced_step_buffer::prepare): New.
(write_memory_ptid): Move from infrun.c.
(displaced_step_instruction_executed_successfully): New,
factored out of displaced_step_finish.
(displaced_step_buffer::finish): New.
(displaced_step_buffer::copy_insn_closure_by_addr): New.
(displaced_step_buffer::restore_in_ptid): New.
* gdbarch.sh (displaced_step_location): Remove.
(displaced_step_prepare, displaced_step_finish,
displaced_step_copy_insn_closure_by_addr,
displaced_step_restore_all_in_ptid): New.
* gdbarch.c: Re-generate.
* gdbarch.h: Re-generate.
* gdbthread.h (class thread_info) <displaced_step_state>: New
field.
(thread_step_over_chain_remove): New declaration.
(thread_step_over_chain_next): New declaration.
(thread_step_over_chain_length): New declaration.
* thread.c (thread_step_over_chain_remove): Make non-static.
(thread_step_over_chain_next): New.
(global_thread_step_over_chain_next): Use
thread_step_over_chain_next.
(thread_step_over_chain_length): New.
(global_thread_step_over_chain_enqueue): Add debug print.
(global_thread_step_over_chain_remove): Add debug print.
* infrun.h (get_displaced_step_copy_insn_closure_by_addr):
Remove.
* infrun.c (get_displaced_stepping_state): New.
(displaced_step_in_progress_any_inferior): Remove.
(displaced_step_in_progress_thread): Adjust.
(displaced_step_in_progress): Adjust.
(displaced_step_in_progress_any_thread): New.
(get_displaced_step_copy_insn_closure_by_addr): Remove.
(gdbarch_supports_displaced_stepping): Use
gdbarch_displaced_step_prepare_p.
(displaced_step_reset): Change parameter from inferior to
thread.
(displaced_step_prepare_throw): Implement using
gdbarch_displaced_step_prepare.
(write_memory_ptid): Move to displaced-step.c.
(displaced_step_restore): Remove.
(displaced_step_finish): Implement using
gdbarch_displaced_step_finish.
(start_step_over): Allow starting more than one displaced step.
(prepare_for_detach): Handle possibly multiple threads doing
displaced steps.
(handle_inferior_event): Handle possibility that fork event
happens while another thread displaced steps.
* linux-tdep.h (linux_displaced_step_prepare): New.
(linux_displaced_step_finish): New.
(linux_displaced_step_copy_insn_closure_by_addr): New.
(linux_displaced_step_restore_all_in_ptid): New.
(linux_init_abi): Add supports_displaced_step parameter.
* linux-tdep.c (struct linux_info) <disp_step_buf>: New field.
(linux_displaced_step_prepare): New.
(linux_displaced_step_finish): New.
(linux_displaced_step_copy_insn_closure_by_addr): New.
(linux_displaced_step_restore_all_in_ptid): New.
(linux_init_abi): Add supports_displaced_step parameter,
register displaced step methods if true.
(_initialize_linux_tdep): Register inferior_execd observer.
* amd64-linux-tdep.c (amd64_linux_init_abi_common): Add
supports_displaced_step parameter, adjust call to
linux_init_abi. Remove call to
set_gdbarch_displaced_step_location.
(amd64_linux_init_abi): Adjust call to
amd64_linux_init_abi_common.
(amd64_x32_linux_init_abi): Likewise.
* aarch64-linux-tdep.c (aarch64_linux_init_abi): Adjust call to
linux_init_abi. Remove call to
set_gdbarch_displaced_step_location.
* arm-linux-tdep.c (arm_linux_init_abi): Likewise.
* i386-linux-tdep.c (i386_linux_init_abi): Likewise.
* alpha-linux-tdep.c (alpha_linux_init_abi): Adjust call to
linux_init_abi.
* arc-linux-tdep.c (arc_linux_init_osabi): Likewise.
* bfin-linux-tdep.c (bfin_linux_init_abi): Likewise.
* cris-linux-tdep.c (cris_linux_init_abi): Likewise.
* csky-linux-tdep.c (csky_linux_init_abi): Likewise.
* frv-linux-tdep.c (frv_linux_init_abi): Likewise.
* hppa-linux-tdep.c (hppa_linux_init_abi): Likewise.
* ia64-linux-tdep.c (ia64_linux_init_abi): Likewise.
* m32r-linux-tdep.c (m32r_linux_init_abi): Likewise.
* m68k-linux-tdep.c (m68k_linux_init_abi): Likewise.
* microblaze-linux-tdep.c (microblaze_linux_init_abi): Likewise.
* mips-linux-tdep.c (mips_linux_init_abi): Likewise.
* mn10300-linux-tdep.c (am33_linux_init_osabi): Likewise.
* nios2-linux-tdep.c (nios2_linux_init_abi): Likewise.
* or1k-linux-tdep.c (or1k_linux_init_abi): Likewise.
* riscv-linux-tdep.c (riscv_linux_init_abi): Likewise.
* s390-linux-tdep.c (s390_linux_init_abi_any): Likewise.
* sh-linux-tdep.c (sh_linux_init_abi): Likewise.
* sparc-linux-tdep.c (sparc32_linux_init_abi): Likewise.
* sparc64-linux-tdep.c (sparc64_linux_init_abi): Likewise.
* tic6x-linux-tdep.c (tic6x_uclinux_init_abi): Likewise.
* tilegx-linux-tdep.c (tilegx_linux_init_abi): Likewise.
* xtensa-linux-tdep.c (xtensa_linux_init_abi): Likewise.
* ppc-linux-tdep.c (ppc_linux_init_abi): Adjust call to
linux_init_abi. Remove call to
set_gdbarch_displaced_step_location.
* arm-tdep.c (arm_pc_is_thumb): Call
gdbarch_displaced_step_copy_insn_closure_by_addr instead of
get_displaced_step_copy_insn_closure_by_addr.
* rs6000-aix-tdep.c (rs6000_aix_init_osabi): Adjust calls to
clear gdbarch methods.
* rs6000-tdep.c (struct ppc_inferior_data): New structure.
(get_ppc_per_inferior): New function.
(ppc_displaced_step_prepare): New function.
(ppc_displaced_step_finish): New function.
(ppc_displaced_step_restore_all_in_ptid): New function.
(rs6000_gdbarch_init): Register new gdbarch methods.
* s390-tdep.c (s390_gdbarch_init): Don't call
set_gdbarch_displaced_step_location, set new gdbarch methods.
gdb/testsuite/ChangeLog:
* gdb.arch/amd64-disp-step-avx.exp: Adjust pattern.
* gdb.threads/forking-threads-plus-breakpoint.exp: Likewise.
* gdb.threads/non-stop-fair-events.exp: Likewise.
Change-Id: I387cd235a442d0620ec43608fd3dc0097fcbf8c8
|
|
Rename step_over_queue_head to global_thread_step_over_chain_head, to
make it more obvious when reading code that we are touching the global
queue. Rename all functions that operate on it to have "global" in
their name, to make it clear on which chain they operate on. Also, in a
subsequent patch, we'll need both global and non-global versions of
these functions, so it will be easier to do the distinction if they are
named properly.
Normalize the naming to use "chain" everywhere instead of sometimes
"queue", sometimes "chain".
I also reworded a few comments in gdbthread.h. They implied that the
step over chain is per-inferior, when in reality there is only one
global chain, not one per inferior, as far as I understand.
gdb/ChangeLog:
* gdbthread.h (thread_step_over_chain_enqueue): Rename to...
(global_thread_step_over_chain_enqueue): ... this. Update all
users.
(thread_step_over_chain_remove): Rename to...
(global_thread_step_over_chain_remove): ... this. Update all
users.
(thread_step_over_chain_next): Rename to...
(global_thread_step_over_chain_next): ... this. Update all
users.
* infrun.h (step_over_queue_head): Rename to...
(global_thread_step_over_chain_head): ... this. Update all
users.
* infrun.c (step_over_queue_head): Rename to...
(global_thread_step_over_chain_head): ... this. Update all
users.
* thread.c (step_over_chain_remove): Rename to...
(thread_step_over_chain_remove): ... this. Update all users.
(thread_step_over_chain_next): Rename to...
(global_thread_step_over_chain_next): ... this. Update all
users.
(thread_step_over_chain_enqueue): Rename to...
(global_thread_step_over_chain_enqueue): ... this. Update all
users.
(thread_step_over_chain_remove): Rename to...
(global_thread_step_over_chain_remove): ... this. Update all
users.
Change-Id: Iabbf57d83c01321ca199d83fadb57f5b04e4d6d9
|
|
This function is now external, and isn't really threads related. Move
it to frame.c.
gdb/ChangeLog:
* thread.c (lookup_selected_frame): Move ...
* frame.c (lookup_selected_frame): ... here.
Change-Id: Ia96b79c15767337c68efd3358bcc715ce8e26c15
|
|
If the remote target closes while we're reading registers/memory for
restoring the selected frame in scoped_restore_current_thread's dtor,
the corresponding TARGET_CLOSE_ERROR error is swallowed by the
scoped_restore_current_thread's dtor, because letting exceptions
escape from a dtor is bad. It isn't great to lose that errors like
that, though. I've been thinking about how to avoid it, and I came up
with this patch.
The idea here is to make scoped_restore_current_thread's dtor do as
little as possible, to avoid any work that might throw in the first
place. And to do that, instead of having the dtor call
restore_selected_frame, which re-finds the previously selected frame,
just record the frame_id/level of the desired selected frame, and have
get_selected_frame find the frame the next time it is called. In
effect, this implements most of Cagney's suggestion, here:
/* On demand, create the selected frame and then return it. If the
selected frame can not be created, this function prints then throws
an error. When MESSAGE is non-NULL, use it for the error message,
otherwize use a generic error message. */
/* FIXME: cagney/2002-11-28: At present, when there is no selected
frame, this function always returns the current (inner most) frame.
It should instead, when a thread has previously had its frame
selected (but not resumed) and the frame cache invalidated, find
and then return that thread's previously selected frame. */
extern struct frame_info *get_selected_frame (const char *message);
The only thing missing to fully implement that would be to make
reinit_frame_cache just clear selected_frame instead of calling
select_frame(NULL), and the call select_frame(NULL) explicitly in the
places where we really wanted reinit_frame_cache to go back to the
current frame too. That can done separately, though, I'm not
proposing to do that in this patch.
Note that this patch renames restore_selected_frame to
lookup_selected_frame, and adds a new restore_selected_frame function
that doesn't throw, to be paired with the also-new save_selected_frame
function.
There's a restore_selected_frame function in infrun.c that I think can
be replaced by the new one in frame.c.
Also done in this patch is make the get_selected_frame's parameter be
optional, so that we don't have to pass down nullptr explicitly all
over the place.
lookup_selected_frame should really move from thread.c to frame.c, but
I didn't do that here, just to avoid churn in the patch while it
collects comments. I did make it extern and declared it in frame.h
already, preparing for the move. I will do the move as a follow up
patch if people agree with this approach.
Incidentally, this patch alone would fix the crashes fixed by the
previous patches in the series, because with this,
scoped_restore_current_thread's constructor doesn't throw either.
gdb/ChangeLog:
* blockframe.c (block_innermost_frame): Use get_selected_frame.
* frame.c
(scoped_restore_selected_frame::scoped_restore_selected_frame):
Use save_selected_frame. Save language as well.
(scoped_restore_selected_frame::~scoped_restore_selected_frame):
Use restore_selected_frame, and restore language as well.
(selected_frame_id, selected_frame_level): New.
(selected_frame): Update comments.
(save_selected_frame, restore_selected_frame): New.
(get_selected_frame): Use lookup_selected_frame.
(get_selected_frame_if_set): Delete.
(select_frame): Record selected_frame_level and selected_frame_id.
* frame.h (scoped_restore_selected_frame) <m_level, m_lang>: New
fields.
(get_selected_frame): Make 'message' parameter optional.
(get_selected_frame_if_set): Delete declaration.
(select_frame): Update comments.
(save_selected_frame, restore_selected_frame)
(lookup_selected_frame): Declare.
* gdbthread.h (scoped_restore_current_thread) <m_lang>: New field.
* infrun.c (struct infcall_control_state) <selected_frame_level>:
New field.
(save_infcall_control_state): Use save_selected_frame.
(restore_selected_frame): Delete.
(restore_infcall_control_state): Use restore_selected_frame.
* stack.c (select_frame_command_core, frame_command_core): Use
get_selected_frame.
* thread.c (restore_selected_frame): Rename to ...
(lookup_selected_frame): ... this and make extern. Select the
current frame if the frame level is -1.
(scoped_restore_current_thread::restore): Also restore the
language.
(scoped_restore_current_thread::~scoped_restore_current_thread):
Don't try/catch.
(scoped_restore_current_thread::scoped_restore_current_thread):
Save the language as well. Use save_selected_frame.
Change-Id: I73fd1cfc40d8513c28e5596383b7ecd8bcfe700f
|
|
The help text of 'info threads' is below:
(gdb) help info threads
Display currently known threads.
Usage: info threads [OPTION]... [ID]...
Options:
-gid
Show global thread IDs.If ID is given, it is a space-separated list of IDs of threads to display.
Otherwise, all threads are displayed.
(gdb)
I think the "If ID is given ..." info should have come right below
the the usage line. This patch reorganizes the text so that we get
(gdb) help info threads
Display currently known threads.
Usage: info threads [OPTION]... [ID]...
If ID is given, it is a space-separated list of IDs of threads to display.
Otherwise, all threads are displayed.
Options:
-gid
Show global thread IDs.
(gdb)
gdb/ChangeLog:
2020-10-19 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
* thread.c (_initialize_thread): Fine-tune the help text of
'info threads'.
|
|
This removes the target_has_registers object-like macro, replacing it
with the underlying function.
gdb/ChangeLog
2020-09-28 Tom Tromey <tom@tromey.com>
* tui/tui-regs.c (tui_get_register)
(tui_data_window::show_registers): Update.
* thread.c (scoped_restore_current_thread::restore)
(scoped_restore_current_thread::scoped_restore_current_thread):
Update.
* regcache-dump.c (regcache_print): Update.
* python/py-finishbreakpoint.c (bpfinishpy_detect_out_scope_cb):
Update.
* mi/mi-main.c (mi_cmd_data_write_register_values): Update.
* mep-tdep.c (current_me_module, current_options): Update.
* linux-thread-db.c (thread_db_load): Update.
* infcmd.c (registers_info, info_vector_command)
(info_float_command): Update.
* ia64-tdep.c (ia64_frame_prev_register)
(ia64_sigtramp_frame_prev_register): Update.
* ia64-libunwind-tdep.c (libunwind_frame_prev_register): Update.
* gcore.c (derive_stack_segment): Update.
* frame.c (get_current_frame, has_stack_frames): Update.
* findvar.c (language_defn::read_var_value): Update.
* arm-tdep.c (arm_pc_is_thumb): Update.
* target.c (target_has_registers): Rename from
target_has_registers_1.
* target.h (target_has_registers): Remove macro.
(target_has_registers): Rename from target_has_registers_1.
|
|
This removes the target_has_stack object-like macro, replacing it with
the underlying function.
gdb/ChangeLog
2020-09-28 Tom Tromey <tom@tromey.com>
* windows-tdep.c (tlb_make_value): Update.
* tui/tui-regs.c (tui_data_window::show_registers): Update.
* thread.c (scoped_restore_current_thread::restore)
(scoped_restore_current_thread::scoped_restore_current_thread)
(thread_command): Update.
* stack.c (backtrace_command_1, frame_apply_level_command)
(frame_apply_all_command, frame_apply_command): Update.
* infrun.c (siginfo_make_value, restore_infcall_control_state):
Update.
* gcore.c (derive_stack_segment): Update.
* frame.c (get_current_frame, has_stack_frames): Update.
* auxv.c (info_auxv_command): Update.
* ada-tasks.c (ada_build_task_list): Update.
* target.c (target_has_stack): Rename from target_has_stack_1.
* target.h (target_has_stack): Remove macro.
(target_has_stack): Rename from target_has_stack_1.
|
|
This removes the target_has_memory object-like macro, replacing it
with the underlying function.
gdb/ChangeLog
2020-09-28 Tom Tromey <tom@tromey.com>
* target.c (target_has_memory): Rename from target_has_memory_1.
* tui/tui-regs.c (tui_data_window::show_registers): Update.
* thread.c (scoped_restore_current_thread::restore)
(scoped_restore_current_thread::scoped_restore_current_thread):
Update.
* frame.c (get_current_frame, has_stack_frames): Update.
* target.h (target_has_memory): Remove macro.
(target_has_memory): Rename from target_has_memory_1.
|
|
"thread find" with multiple inferiors got broken with the multi-target
work:
Thread 1 "gdb" hit Breakpoint 1, internal_error (...) at ../../src/gdbsupport/errors.cc:51
51 {
(top-gdb) bt
#0 internal_error (file=0xffffd4d0 <error: Cannot access memory at address 0xffffd4d0>, line=0, fmt=0x555556330320 "en_US.UTF-8") at ../../src/gdbsupport/errors.cc:51
#1 0x0000555555bca4c7 in target_thread_name (info=0x555556801290) at ../../src/gdb/target.c:2035
#2 0x0000555555beb07a in thread_find_command (arg=0x7fffffffe08e "1", from_tty=0) at ../../src/gdb/thread.c:1959
#3 0x000055555572ec49 in do_const_cfunc (c=0x555556786bc0, args=0x7fffffffe08e "1", from_tty=0) at ../../src/gdb/cli/cli-decode.c:95
#4 0x0000555555732abd in cmd_func (cmd=0x555556786bc0, args=0x7fffffffe08e "1", from_tty=0) at ../../src/gdb/cli/cli-decode.c:2181
#5 0x0000555555bf1245 in execute_command (p=0x7fffffffe08e "1", from_tty=0) at ../../src/gdb/top.c:664
#6 0x00005555559cad10 in catch_command_errors (command=0x555555bf0c31 <execute_command(char const*, int)>, arg=0x7fffffffe082 "thread find 1", from_tty=0) at ../../src/gdb/main.c:457
#7 0x00005555559cc33d in captured_main_1 (context=0x7fffffffdb60) at ../../src/gdb/main.c:1218
#8 0x00005555559cc571 in captured_main (data=0x7fffffffdb60) at ../../src/gdb/main.c:1243
#9 0x00005555559cc5e8 in gdb_main (args=0x7fffffffdb60) at ../../src/gdb/main.c:1268
#10 0x0000555555623816 in main (argc=17, argv=0x7fffffffdc78) at ../../src/gdb/gdb.c:32
The problem is that we're not switching to the inferior/target before
calling target methods, which trips on an assertion put in place
exactly to catch this sort of problem.
gdb/testsuite/ChangeLog:
PR gdb/26631
* gdb.multi/multi-target-thread-find.exp: New file.
gdb/ChangeLog:
PR gdb/26631
* thread.c (thread_find_command): Switch inferior before calling
target methods.
|
|
I noticed what I think is a potential bug. I did not observe it nor was
I able to reproduce it using actual debugging. It's quite unlikely,
because it involves multi-target and ptid clashes. I added selftests
that demonstrate it though.
The thread_ptid_changed observer says that thread with OLD_PTID now has
NEW_PTID. Now, if for some reason we happen to have two targets
defining a thread with OLD_PTID, the observers don't know which thread
this is about.
regcache::regcache_thread_ptid_changed changes all regcaches with
OLD_PTID. If there is a regcache for a thread with ptid OLD_PTID, but
that belongs to a different target, this regcache will be erroneously
changed.
Similarly, infrun_thread_ptid_changed updates inferior_ptid if
inferior_ptid matches OLD_PTID. But if inferior_ptid currently refers
not to the thread is being changed, but to a thread with the same ptid
belonging to a different target, then inferior_ptid will erroneously be
changed.
This patch adds a `process_stratum_target *` parameter to the
`thread_ptid_changed` observable and makes the two observers use it.
Tests for both are added, which would fail if the corresponding fix
wasn't done.
gdb/ChangeLog:
* observable.h (thread_ptid_changed): Add parameter
`process_stratum_target *`.
* infrun.c (infrun_thread_ptid_changed): Add parameter
`process_stratum_target *` and use it.
(selftests): New namespace.
(infrun_thread_ptid_changed): New function.
(_initialize_infrun): Register selftest.
* regcache.c (regcache_thread_ptid_changed): Add parameter
`process_stratum_target *` and use it.
(regcache_thread_ptid_changed): New function.
(_initialize_regcache): Register selftest.
* thread.c (thread_change_ptid): Pass target to
thread_ptid_changed observable.
Change-Id: I0599e61224b6d154a7b55088a894cb88298c3c71
|
|
Running the testsuite against an Asan-enabled build of GDB makes
gdb.base/multi-target.exp expose this bug.
scoped_restore_current_thread's ctor calls get_frame_id to record the
selected frame's ID to restore later. If the frame ID hasn't been
computed yet, it will be computed on the spot, and that will usually
require accessing the target's memory and registers. If the remote
connection closes, while we're computing the frame ID, the remote
target exits its inferiors, unpushes itself, and throws a
TARGET_CLOSE_ERROR error. Exiting the inferiors deletes the
inferior's threads.
scoped_restore_current_thread increments the current thread's refcount
to prevent the thread from being deleted from under its feet.
However, the code that does that isn't considering the case of the
thread being deleted from within get_frame_id. It only increments the
refcount _after_ get_frame_id returns. So if the current thread is
indeed deleted, the
tp->incref ();
statement references a stale TP pointer.
Incrementing the refcounts earlier fixes it.
We should probably also let the TARGET_CLOSE_ERROR error propagate in
this case. That alone would fix it, though it seems better to tweak
the refcount handling too. And to avoid having to manually decref
before throwing, convert to use gdb::ref_ptr.
Unfortunately, we can't define inferior_ref in inferior.h and then use
it in scoped_restore_current_thread, because
scoped_restore_current_thread is defined before inferior is
(inferior.h includes gdbthread.h). To break that dependency, we would
have to move scoped_restore_current_thread to its own header. I'm not
doing that here.
gdb/ChangeLog:
* gdbthread.h (inferior_ref): Define.
(scoped_restore_current_thread) <m_thread>: Now a thread_info_ref.
(scoped_restore_current_thread) <m_inf>: Now an inferior_ref.
* thread.c
(scoped_restore_current_thread::restore):
Adjust to gdb::ref_ptr.
(scoped_restore_current_thread::~scoped_restore_current_thread):
Remove manual decref handling.
(scoped_restore_current_thread::scoped_restore_current_thread):
Adjust to use
inferior_ref::new_reference/thread_info_ref::new_reference.
Incref the thread before calling get_frame_id instead of after.
Let TARGET_CLOSE_ERROR propagate.
|
|
In PR 25412, Simon noticed that after the multi-target series, the
tid-reuse.exp testcase manages to create a duplicate thread in the
thread list. Or rather, two threads with the same PTID.
add_thread_silent has code in place to detect the case of a new thread
reusing some older thread's ptid, but it doesn't work correctly
anymore when the old thread is NOT the current thread and it has a
refcount higher than 0. Either condition prevents a thread from being
deleted, but the refcount case wasn't being considered. I think the
reason that case wasn't considered is that that code predates
thread_info refcounting. Back when it was originally written,
delete_thread always deleted the thread.
That add_thread_silent code in question has some now-unnecessary
warts, BTW. For instance, this:
/* Make switch_to_thread not read from the thread. */
new_thr->state = THREAD_EXITED;
... used to be required because switch_to_thread would update
'stop_pc' otherwise. I.e., it would read registers from an exited
thread otherwise. switch_to_thread no longer reads the stop_pc, since:
commit f2ffa92bbce9dd5fbedc138ac2a3bc8a88327d09
Author: Pedro Alves <palves@redhat.com>
AuthorDate: Thu Jun 28 20:18:24 2018 +0100
gdb: Eliminate the 'stop_pc' global
Also, if the ptid of the now-gone current thread is reused, we
currently return from add_thread_silent with the current thread
pointing at the _new_ thread. Either pointing at the old thread, or
at no thread selected would be reasonable. But pointing at an
unrelated thread (the new thread that happens to reuse the ptid) is
just broken. Seems like I was the one who wrote it like that but I
have no clue why, FWIW.
Currently, an exited thread kept in the thread list still holds its
original ptid. The idea was that we need the ptid to be able to
temporarily switch to another thread and then switch back to the
original thread, because thread switching is really inferior_ptid
switching. Switching back to the original thread requires a ptid
lookup.
Now, in order to avoid exited threads with the same ptid as a live
thread in the same thread list, one thing I considered (and tried) was
to change an exited thread's ptid to minus_one_ptid. However, with
that, there's a case that we won't handle well, which is if we end up
with more than one exited thread in the list, since then all exited
threads will all have the same ptid. Since inferior_thread() relies
on inferior_ptid, may well return the wrong thread.
My next attempt to address this, was to switch an exited thread's ptid
to a globally unique "exited" ptid, which is a ptid with pid == -1 and
tid == 'the thread's global GDB thread number'. Note that GDB assumes
that the GDB global thread number is monotonically increasing and
doesn't wrap around. (We should probably make GDB thread numbers
64-bit to prevent that happening in practice; they're currently signed
32-bit.) This attempt went a long way, but still ran into a number of
issues. It was a major hack too, obviously.
My next attempt is the one that I'm proposing, which is to bite the
bullet and break the connection between inferior_ptid and
inferior_thread(), aka the current thread. I.e., make the current
thread be a global thread_info pointer that is written to directly by
switch_to_thread, etc., and making inferior_thread() return that
pointer, instead of having inferior_thread() lookup up the
inferior_ptid thread, by ptid_t. You can look at this as a
continuation of the effort of using more thread_info pointers instead
of ptids when possible.
By making the current thread a global thread_info pointer, we can make
switch_to_thread simply write to the global thread pointer, which
makes scoped_restore_current_thread able to restore back to an exited
thread without relying on unrelyable ptid look ups. I.e., this makes
it not a real problem to have more than one thread with the same ptid
in the thread list. There will always be only one live thread with a
given ptid, so code that looks up a live thread by ptid will always be
able to find the right one.
This change required auditing the whole codebase for places where we
were writing to inferior_ptid directly to change the current thread,
and change them to use switch_to_thread instead or one of its
siblings, because otherwise inferior_thread() would return a thread
unrelated to the changed-to inferior_ptid. That was all (hopefully)
done in previous patches.
After this, inferior_ptid is mainly used by target backend code. It
is also relied on by a number of target methods. E.g., the
target_resume interface and the memory reading routines -- we still
need it there because we need to be able to access memory off of
processes for which we don't have a corresponding inferior/thread
object, like when handling forks. Maybe we could pass down a context
explicitly to target_read_memory, etc.
gdb/ChangeLog:
2020-06-18 Pedro Alves <palves@redhat.com>
PR gdb/25412
* gdbthread.h (delete_thread, delete_thread_silent)
(find_thread_ptid): Update comments.
* thread.c (current_thread_): New global.
(is_current_thread): Move higher, and reimplement.
(inferior_thread): Reimplement.
(set_thread_exited): Use bool. Add assertions.
(add_thread_silent): Simplify thread-reuse handling by always
calling delete_thread.
(delete_thread): Remove intro comment.
(find_thread_ptid): Skip exited threads.
(switch_to_thread_no_regs): Write to current_thread_.
(switch_to_no_thread): Check CURRENT_THREAD_ instead of
INFERIOR_PTID. Clear current_thread_.
|
