I happened to notice that one "show" callback was printing to
gdb_stdout rather than to the passed-in ui_file parameter. I went
through all such callbacks and fixed them to consistently use the
ui_file.
Regression tested on x86-64 Fedora 34.
Bug PR gdb/28405 reports a regression when using attach with an
extended-remote target. In this case the target is not including a
thread-id in the stop packet it sends back after the attach.
The regression was introduced with this commit:
commit 8f66807b98
Date: Wed Jan 13 20:26:58 2021 -0500
gdb: better handling of 'S' packets
The problem is that when GDB processes the stop packet, it sees that
there is no thread-id and so has to "guess" which thread the stop
should apply to.
In this case the target only has one thread, so really, there's no
guessing needed, but GDB still runs through the same process, this
shouldn't cause us any problems.
However, after the above commit, GDB now expects itself to be more
internally consistent, specifically, only a thread that GDB thinks is
resumed, can be a candidate for having stopped.
It turns out that, when GDB attaches to a process through an
extended-remote target, the threads of the process being attached too,
are not, initially, marked as resumed.
And so, when GDB tries to figure out which thread the stop might apply
too, it finds no threads in the processes marked resumed, and so an
assert triggers.
In extended_remote_target::attach we create a new thread with a call
to add_thread_silent, rather than remote_target::remote_add_thread,
the reason is that calling the latter will result in a call to
'add_thread' rather than 'add_thread_silent'. However,
remote_target::remote_add_thread includes additional
actions (i.e. calling remote_thread_info::set_resumed and set_running)
which are missing from extended_remote_target::attach. These missing
calls are what would serve to mark the new thread as resumed.
In this commit I propose that we add an extra parameter to
remote_target::remote_add_thread. This new parameter will force the
new thread to be added with a call to add_thread_silent. We can now
call remote_add_thread from the ::attach method, the extra
actions (listed above) will now be performed, and the thread will be
left in the correct state.
Additionally, in PR gdb/28405, a segfault is reported. This segfault
triggers when 'set debug remote 1' is used before trying to reproduce
the original assertion failure. The cause of this is in
remote_target::select_thread_for_ambiguous_stop_reply, where we do
this:
remote_debug_printf ("first resumed thread is %s",
pid_to_str (first_resumed_thread->ptid).c_str ());
remote_debug_printf ("is this guess ambiguous? = %d", ambiguous);
gdb_assert (first_resumed_thread != nullptr);
Notice that when debug printing is on we dereference
first_resumed_thread before we assert that the pointer is not
nullptr. This is the cause of the segfault, and is resolved by moving
the assert before the debug printing code.
I've extended an existing test, ext-attach.exp, so that the original
test is run multiple times; we run in the original mode, as normal,
but also, we now run with different packets disabled in gdbserver. In
particular, disabling Tthread would trigger the assertion as it was
reported in the original bug. I also run the test in all-stop and
non-stop modes now for extra coverage, we also run the tests with
target-async enabled, and disabled.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28405
While working on another patch I ended up in a situation where I had
async mode disabled (with 'maint set target-async off'), but the async
event token got marked anyway.
In this situation GDB was continually calling into
remote_target::wait, however, the async token would never become
unmarked as the unmarking is guarded by target_is_async_p.
We could just unconditionally unmark the token, but that would feel
like just ignoring a bug, so, instead, lets assert that if
!target_is_async_p, then the async token should not be marked.
This assertion would have caught my earlier mistake.
There should be no user visible changes with this commit.
While working on another patch relating to remote targets, I wanted to
test with 'maint set target-async off' in place. Unfortunately I ran
into some problems. This commit is an attempt to fix one of the
issues I hit.
In my particular case I was actually running with:
maint set target-async off
maint set target-non-stop off
that is, we're telling GDB to force the targets to operate in
non-async mode, and in all-stop mode. Here's my GDB session showing
the problem:
(gdb) maintenance set target-async off
(gdb) maintenance set target-non-stop off
(gdb) target extended-remote :54321
Remote debugging using :54321
(gdb) attach 2365960
Attaching to process 2365960
No unwaited-for children left.
(gdb)
Notice the 'No unwaited-for children left.' error, this is the
problem. There's no reason why GDB should not be able to attach to
the process.
The problem is this:
1. The user runs 'attach PID' and this sends GDB into attach_command
in infcmd.c. From here we call the ::attach method on the attach
target, which will be the extended_remote_target.
2. In extended_remote_target::attach, we attach to the remote target
and get the first reply (which is a stop packet). We put off
processing the stop packet until the end of ::attach. We setup the
inferior and thread to represent the process we attached to, and
download the target description. Finally, we process the initial
stop packet.
If '!target_is_non_stop_p ()' and '!target_can_async_p ()', which is
the case for us given the maintenance commands we used, we cache the
stop packet within the remote_state::buf for later processing.
3. Back in attach_command, if 'target_is_non_stop_p ()' then we
request that the target stops. This will either process any cached
stop replies, or request that the target stops, and process the stop
replies. However, this code is not what we use due to non-stop mode
being disabled. So, we skip to the next step which is to call
validate_exec_file.
4. Calling validate_exec_file can cause packets to be sent to the
remote target, and replies received, the first path I hit is the
call to target_pid_to_exec_file, which calls
remote_target::pid_to_exec_file, which can then try to read the
executable from the remote. Sending an receiving packets will make
use of the remote_state::buf object.
5. The attempt to attach continues, but the damage is already done...
So, the problem is that, in step #2 we cache a stop reply in the
remote_state::buf, and then in step #4 we reuse the remote_state::buf
object, discarding any cached stop reply. As a result, the initial
stop, which is sent when GDB first attaches to the target, is lost.
This problem can clearly be seen, I feel, by looking at the
remote_state::cached_wait_status flag. This flag tells GDB if there
is a wait status cached in remote_state::buf. However, in
remote_target::putpkt_binary and remote_target::getpkt_or_notif_sane_1
this flag is just set back to 0, doing this immediately discards any
cached data.
I don't know if this scheme ever made sense, looking at commit
2d717e4f8a, where the cached_wait_status flag was added, it appears
that there was nothing between where the stop was cached, and where
the stop was consumed, so, I suspect, there never was a situation
where we ended up in putpkt_binary or getpkt_or_notif_sane_1 and
needed to clear to the flag, maybe the clearing was added "just in
case". Whatever the history, I claim that this clearing this flag is
no longer a good idea.
So, my first step toward fixing this issue was to replace the two
instances of 'rs->cached_wait_status = 0;' in ::putpkt_binary and
::getpkt_or_notif_sane_1 with 'gdb_assert (rs->cached_wait_status ==
0);', this, at least would show me when GDB was doing something
dangerous, and indeed, this assert is now hit in my test case above.
I did play with using some kind of scoped restore to backup, and
restore the remote_state::buf object in all the places within remote.c
that I was hitting where the ::buf was being corrupted. The first
problem with this is that, where the ::cached_wait_status flag is
reset is _not_ where ::buf is corrupted. For the ::putpkt_binary
case, by the time we get to the method the buffer has already been
corrupted in many cases, so we end up needing to add the scoped
save/restore within the callers, which means we need the save/restore
in _lots_ of places.
Plus, using this save/restore model feels like the wrong solution. I
don't think that it's obvious that the buffer might be holding cached
data, and I think it would be too easy for new corruptions of the
buffer to be introduced, which could easily go unnoticed for a long
time.
So, I really wanted a solution that didn't require us to cache data in
the ::buf object.
Luckily, I think we already have such a solution in place, the
remote_state::stop_reply_queue, it seems like this does exactly the
same task, just in a slightly different way. With the
::stop_reply_queue, the stop packets are processed upon receipt and
the stop_reply object is added to the queue. With the ::buf cache
solution, the unprocessed stop reply is cached in the ::buf, and
processed later.
So, finally, in this commit, I propose to remove the
remote_state::cached_wait_status flag and to stop using the ::buf to
cache stop replies. Instead, stop replies will now always be stored
in the ::stop_reply_queue.
There are two places where we use the ::buf to hold a cached stop
reply, the first is in the ::attach method, and the second is in
remote_target::start_remote, however, the second of these cases is far
less problematic, as after caching the stop reply in ::buf we call the
global start_remote function, which does very little work before
calling normal_stop, which processes the cached stop reply. However,
my plan is to switch both users over to using ::stop_reply_queue so
that the old (unsafe) ::cached_wait_status mechanism can be completely
removed.
The next problem is that the ::stop_reply_queue is currently only used
for async-mode, and so, in remote_target::push_stop_reply, where we
push stop_reply objects into the ::stop_reply_queue, we currently also
mark the async event token. I've modified this so we only mark the
async event token if 'target_is_async_p ()' - note, _is_, not _can_
here. The ::push_stop_reply method is called in places where async
mode has been temporarily disabled, but, when async mode is switched
back on (see remote_target::async) we will mark the event token if
there are events in the queue.
Another change of interest is in remote_target::remote_interrupt_as.
Previously this code checked ::cached_wait_status, but didn't check
for events in the ::stop_reply_queue. Now that ::cached_wait_status
has been removed we now check the queue length instead, which should
have the same result.
Finally, in remote_target::wait_as, I've tried to merge the processing
of the ::stop_reply_queue with how we used to handle the
::cached_wait_status flag.
Currently, when processing the ::stop_reply_queue we call
process_stop_reply and immediately return. However, when handling
::cached_wait_status we run through the whole of ::wait_as, and return
at the end of the function.
If we consider a standard stop packet, the two differences I see are:
1. Resetting of the remote_state::waiting_for_stop_reply, flag; this
is not currently done when processing a stop from the
::stop_reply_queue.
2. The final return value has the possibility of being adjusted at
the end of ::wait_as, as well as there being calls to
record_currthread, non of which are done if we process a stop from
the ::stop_reply_queue.
After discussion on the mailing list:
https://sourceware.org/pipermail/gdb-patches/2021-December/184535.html
it was suggested that, when an event is pushed into the
::stop_reply_queue, the ::waiting_for_stop_reply flag is never going
to be set. As a result, we don't need to worry about the first
difference. I have however, added a gdb_assert to validate the
assumption that the flag is never going to be set. If in future the
situation ever changes, then we should find out pretty quickly.
As for the second difference, I have resolved this by having all stop
packets taken from the ::stop_reply_queue, pass through the return
value adjustment code at the end of ::wait_as.
An example of a test that reveals the benefits of this commit is:
make check-gdb \
RUNTESTFLAGS="--target_board=native-extended-gdbserver \
GDBFLAGS='-ex maint\ set\ target-async\ off \
-ex maint\ set\ target-non-stop\ off' \
gdb.base/attach.exp"
For testing I've been running test on x86-64/GNU Linux, and run with
target boards unix, native-gdbserver, and native-extended-gdbserver.
For each board I've run with the default GDBFLAGS, as well as with:
GDBFLAGS='-ex maint\ set\ target-async\ off \
-ex maint\ set\ target-non-stop\ off' \
Though running with the above GDBFLAGS is clearly a lot more unstable
both before and after my patch, I'm not seeing any consistent new
failures with my patch, except, with the native-extended-gdbserver
board, where I am seeing new failures, but only because more tests are
now running. For that configuration alone I see the number of
unresolved go down by 49, the number of passes goes up by 446, and the
number of failures also increases by 144. All of the failures are new
tests as far as I can tell.
While working with pending fork events, I wondered what would happen if
the user detached an inferior while a thread of that inferior had a
pending fork event. What happens with the fork child, which is
ptrace-attached by the GDB process (or by GDBserver), but not known to
the core? Sure enough, neither the core of GDB or the target detach the
child process, so GDB (or GDBserver) just stays ptrace-attached to the
process. The result is that the fork child process is stuck, while you
would expect it to be detached and run.
Make GDBserver detach of fork children it knows about. That is done in
the generic handle_detach function. Since a process_info already exists
for the child, we can simply call detach_inferior on it.
GDB-side, make the linux-nat and remote targets detach of fork children
known because of pending fork events. These pending fork events can be
stored in:
- thread_info::pending_waitstatus, if the core has consumed the event
but then saved it for later (for example, because it got the event
while stopping all threads, to present an all-stop stop on top of a
non-stop target)
- thread_info::pending_follow: if we ran to a "catch fork" and we
detach at that moment
Additionally, pending fork events can be in target-specific fields:
- For linux-nat, they can be in lwp_info::status and
lwp_info::waitstatus.
- For the remote target, they could be stored as pending stop replies,
saved in `remote_state::notif_state::pending_event`, if not
acknowledged yet, or in `remote_state::stop_reply_queue`, if
acknowledged. I followed the model of remove_new_fork_children for
this: call remote_notif_get_pending_events to process /
acknowledge any unacknowledged notification, then look through
stop_reply_queue.
Update the gdb.threads/pending-fork-event.exp test (and rename it to
gdb.threads/pending-fork-event-detach.exp) to try to detach the process
while it is stopped with a pending fork event. In order to verify that
the fork child process is correctly detached and resumes execution
outside of GDB's control, make that process create a file in the test
output directory, and make the test wait $timeout seconds for that file
to appear (it happens instantly if everything goes well).
This test catches a bug in linux-nat.c, also reported as PR 28512
("waitstatus.h:300: internal-error: gdb_signal target_waitstatus::sig()
const: Assertion `m_kind == TARGET_WAITKIND_STOPPED || m_kind ==
TARGET_WAITKIND_SIGNALLED' failed.). When detaching a thread with a
pending event, get_detach_signal unconditionally fetches the signal
stored in the waitstatus (`tp->pending_waitstatus ().sig ()`). However,
that is only valid if the pending event is of type
TARGET_WAITKIND_STOPPED, and this is now enforced using assertions (iit
would also be valid for TARGET_WAITKIND_SIGNALLED, but that would mean
the thread does not exist anymore, so we wouldn't be detaching it). Add
a condition in get_detach_signal to access the signal number only if the
wait status is of kind TARGET_WAITKIND_STOPPED, and use GDB_SIGNAL_0
instead (since the thread was not stopped with a signal to begin with).
Add another test, gdb.threads/pending-fork-event-ns.exp, specifically to
verify that we consider events in pending stop replies in the remote
target. This test has many threads constantly forking, and we detach
from the program while the program is executing. That gives us some
chance that we detach while a fork stop reply is stored in the remote
target. To verify that we correctly detach all fork children, we ask
the parent to exit by sending it a SIGUSR1 signal and have it write a
file to the filesystem before exiting. Because the parent's main thread
joins the forking threads, and the forking threads wait for their fork
children to exit, if some fork child is not detach by GDB, the parent
will not write the file, and the test will time out. If I remove the
new remote_detach_pid calls in remote.c, the test fails eventually if I
run it in a loop.
There is a known limitation: we don't remove breakpoints from the
children before detaching it. So the children, could hit a trap
instruction after being detached and crash. I know this is wrong, and
it should be fixed, but I would like to handle that later. The current
patch doesn't fix everything, but it's a step in the right direction.
Change-Id: I6d811a56f520e3cb92d5ea563ad38976f92e93dd
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28512
In preparation for a following patch, refactor a few things that I did
find a bit awkward, and to make them a bit more reusable.
- Pass an inferior to kill_new_fork_children instead of a pid. That
allows iterating on only this inferior's threads and avoid further
filtering on the thread's pid.
- Change thread_pending_fork_status to return a non-nullptr value only
if the thread does have a pending fork status.
- Remove is_pending_fork_parent_thread, as one can just use
thread_pending_fork_status and check for nullptr.
- Replace is_pending_fork_parent with is_fork_status, which just
returns if the given target_waitkind if a fork or a vfork. Push
filtering on the pid to the callers, when it is necessary.
Change-Id: I0764ccc684d40f054e39df6fa5458cc4c5d1cd7b
Move the stop_reply and a few functions up. Some code above them in the
file will need to use them in a following patch. No behavior changes
expected here.
Change-Id: I3ca57d0e3ec253f56e1ba401289d9d167de14ad2
When running test-case gdb.base/cached-source-file.exp with target board
readnow, we run into:
...
FAIL: gdb.base/cached-source-file.exp: rerun program (the program exited)
...
The problem is that when rereading, the readnow is ignored.
Fix this by copying the readnow handling code from symbol_file_add_with_addrs
to reread_symbols.
Tested on x86_64-linux.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=26800
This commit:
commit 288712bbac
Date: Mon Nov 22 15:16:27 2021 +0000
gdb/remote: use scoped_restore to control starting_up flag
introduced a use after free bug. The scoped restore added in the
above commit resets a flag within a remote_target's remote_state
object.
However, in some situations, the remote_target can be unpushed before
the error is thrown. If the only reference to the target is the one
in the target stack, then unpushing the target will cause the
remote_target to be deleted, which, in turn, will delete the
remote_state object. The scoped restore will then try to reset the
flag within a deleted object.
This problem was caught in the gdb.server/server-connect.exp test,
which, when run with the address sanitizer enabled, highlights the
write after free bug described above.
This commit resolves this issue by adding a new class specifically for
the purpose of managing the starting_up flag. As well as setting, and
then clearing the starting_up flag, this new class increments, and
then decrements the reference count on the remote_target object. This
prevents the remote_target from being deleted until after the flag has
been reset.
The gdb.server/server-connect.exp now runs cleanly with the address
sanitizer enabled.
This commit makes use of a scoped_restore object to control the
remote_state::starting_up flag within the remote_target::start_remote
method.
Ideally I would have liked to create the scoped_restore inside
start_remote and just leave the restore in place until the end of the
scope, however, I'm worried that doing this would change the behaviour
of GDB. Specifically, in start_remote, the following code is executed
once the starting_up flag has been restored to its previous value:
if (breakpoints_should_be_inserted_now ())
insert_breakpoints ();
I think (but am not 100% sure) that calling install_breakpoints could
end up back inside remote_target::can_download_tracepoint, which does
check the value of remote_state::starting_up. And so, I'm concerned
that leaving the scoped_restore in place until the end of start_remote
will cause a possible change in behaviour.
To avoid this, and to leave things as close to the current behaviour
as possible, I've split remote_target::start_remote into two, there's
the main function body which moves into remote_target::start_remote_1,
this function uses the scoped_restore to change the ::starting_up
flag, then there's the old remote_target::start_remote, which now just
calls ::start_remote_1, and then does the insert_breakpoints call.
There should be no user visible changes after this commit, unless
there's a situation where the ::starting_up flag could previously have
been left set, if this was the case, then this situation should no
longer be possible.
This commits adds a new sub-class of gdb.TargetConnection,
gdb.RemoteTargetConnection. This sub-class is created for all
'remote' and 'extended-remote' targets.
This new sub-class has one additional method over its base class,
'send_packet'. This new method is equivalent to the 'maint
packet' CLI command, it allows a custom packet to be sent to a remote
target.
The outgoing packet can either be a bytes object, or a Unicode string,
so long as the Unicode string contains only ASCII characters.
The result of calling RemoteTargetConnection.send_packet is a bytes
object containing the reply that came from the remote.
In a later commit I will add a Python API to access the 'maint packet'
functionality, that is, sending a user specified packet to the target.
To make implementing this easier, this commit refactors how this
command is currently implemented so that the packet_command function
is now global.
The new global send_remote_packet function takes an object that is an
implementation of an abstract interface. Two functions within this
interface are then called, one just before a packet is sent to the
remote target, and one when the reply has been received from the
remote target. Using an interface object in this way allows (1) for
the error checking to be done before the first callback is made, this
means we only print out what packet it being sent once we know we are
going to actually send it, and (2) we don't need to make a copy of the
reply if all we want to do is print it.
One user visible changes after this commit are the error
messages, which I've changed to be less 'maint packet' command
focused, this will make them (I hope) better for when
send_remote_packet can be called from Python code.
So: "command can only be used with remote target"
Becomes: "packets can only be sent to a remote target"
And: "remote-packet command requires packet text as argument"
Becomes: "a remote packet must not be empty"
Additionally, in this commit, I've added support for packet replies
that contain binary data. Before this commit, the code that printed
the reply treated the reply as a C string, it assumed that the string
only contained printable characters, and had a null character only at
the end.
One way to show the problem with this is if we try to read the auxv
data from a remote target, the auxv data is binary, so, before this
commit:
(gdb) target remote :54321
...
(gdb) maint packet qXfer:auxv:read::0,1000
sending: "qXfer:auxv:read::0,1000"
received: "l!"
(gdb)
And after this commit:
(gdb) target remote :54321
...
(gdb) maint packet qXfer:auxv:read::0,1000
sending: "qXfer:auxv:read::0,1000"
received: "l!\x00\x00\x00\x00\x00\x00\x00\x00\xf0\xfc\xf7\xff\x7f\x00\x00\x10\x00\x00\x00\x00\x00\x00\x00\xff\xf>
(gdb)
The binary contents of the reply are now printed as escaped hex.
This commit introduced a test failure in gdb.server/attach-flag.exp.
I didn't spot this failure originally as the problem is fixed by this,
as yet unpushed patch:
https://sourceware.org/pipermail/gdb-patches/2021-November/183768.html
I unfortunately didn't test each patch in the original series
independently. I'll repost this patch after the above patch has been
merged.
This reverts commit 32b1f5e8d6.
While working on another patch I ended up in a situation where I had
async mode disabled (with 'maint set target-async off'), but the async
event token got marked anyway.
In this situation GDB was continually calling into
remote_target::wait, however, the async token would never become
unmarked as the unmarking is guarded by target_is_async_p.
We could just unconditionally unmark the token, but that would feel
like just ignoring a bug, so, instead, lets assert that if
!target_is_async_p, then the async token should not be marked.
This assertion would have caught my earlier mistake.
There should be no user visible changes with this commit.
This commit simplifies remote_target::is_async_p by removing the
target_async_permitted check.
In previous commits I have added additional assertions around the
target_async_permitted flag into target.c, as a result we should now
be confident that if target_can_async_p returns false, a target will
never have async mode enabled. Given this, it should not be necessary
to check target_async_permitted in remote_target::is_async_p, if this
flag is false ::is_async_p should return false anyway. There is an
assert to this effect in target_is_async_p.
There should be no user visible change after this commit.
This commit moves the target_async_permitted check out of each targets
::can_async_p method and into the target_can_async_p wrapper function.
I've left some asserts in the two ::can_async_p methods that I
changed, which will hopefully catch any direct calls to these methods
that might be added in the future.
There should be no user visible changes after this commit.
While working on target_waitstatus changes, I noticed a few places where
const target_waitstatus objects could be passed by reference instead of
by pointers. And in some cases, places where a target_waitstatus could
be passed as const, but was not. Convert them as much as possible.
Change-Id: Ied552d464be5d5b87489913b95f9720a5ad50c5a
Change gdb_assert_not_reached to accept a format string plus
corresponding arguments. This allows giving more precise messages.
Because the format string passed by the caller is prepended with a "%s:"
to add the function name, the callers can no longer pass a translated
string (`_(...)`). Make the gdb_assert_not_reached include the _(),
just like the gdb_assert_fail macro just above.
Change-Id: Id0cfda5a57979df6cdaacaba0d55dd91ae9efee7
The motivation is to reduce the number of places where unmanaged
pointers are returned from allocation type routines. All of the
callers are updated.
There should be no user visible changes after this commit.
The getter and setter in struct setting always receive and return values
by const reference. This is not necessary for scalar values (like bool
and int), but more importantly it makes it a bit annoying to write a
getter, you have to use a scratch static variable or something similar
that you can refer to:
const bool &
my_getter ()
{
static bool value;
value = function_returning_bool ();
return value;
}
Change the getter and setter function signatures to receive and return
value by value instead of by reference, when the underlying data type is
scalar. This means that string-based settings will still use
references, but all others will be by value. The getter above would
then be re-written as:
bool
my_getter ()
{
return function_returning_bool ();
}
This is useful for a patch later in this series that defines a boolean
setting with a getter and a setter.
Change-Id: Ieca3a2419fcdb75a6f75948b2c920b548a0af0fd
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
String-like settings (var_string, var_filename, var_optional_filename,
var_string_noescape) currently take a pointer to a `char *` storage
variable (typically global) that holds the setting's value. I'd like to
"mordernize" this by changing them to use an std::string for storage.
An obvious reason is that string operations on std::string are often
easier to write than with C strings. And they avoid having to do any
manual memory management.
Another interesting reason is that, with `char *`, nullptr and an empty
string often both have the same meaning of "no value". String settings
are initially nullptr (unless initialized otherwise). But when doing
"set foo" (where `foo` is a string setting), the setting now points to
an empty string. For example, solib_search_path is nullptr at startup,
but points to an empty string after doing "set solib-search-path". This
leads to some code that needs to check for both to check for "no value".
Or some code that converts back and forth between NULL and "" when
getting or setting the value. I find this very error-prone, because it
is very easy to forget one or the other. With std::string, we at least
know that the variable is not "NULL". There is only one way of
representing an empty string setting, that is with an empty string.
I was wondering whether the distinction between NULL and "" would be
important for some setting, but it doesn't seem so. If that ever
happens, it would be more C++-y and self-descriptive to use
optional<string> anyway.
Actually, there's one spot where this distinction mattered, it's in
init_history, for the test gdb.base/gdbinit-history.exp. init_history
sets the history filename to the default ".gdb_history" if it sees that
the setting was never set - if history_filename is nullptr. If
history_filename is an empty string, it means the setting was explicitly
cleared, so it leaves it as-is. With the change to std::string, this
distinction doesn't exist anymore. This can be fixed by moving the code
that chooses a good default value for history_filename to
_initialize_top. This is ran before -ex commands are processed, so an
-ex command can then clear that value if needed (what
gdb.base/gdbinit-history.exp tests).
Another small improvement, in my opinion is that we can now easily
give string parameters initial values, by simply initializing the global
variables, instead of xstrdup-ing it in the _initialize function.
In Python and Guile, when registering a string-like parameter, we
allocate (with new) an std::string that is owned by the param_smob (in
Guile) and the parmpy_object (in Python) objects.
This patch started by changing all relevant add_setshow_* commands to
take an `std::string *` instead of a `char **` and fixing everything
that failed to build. That includes of course all string setting
variable and their uses.
string_option_def now uses an std::string also, because there's a
connection between options and settings (see
add_setshow_cmds_for_options).
The add_path function in source.c is really complex and twisted, I'd
rather not try to change it to work on an std::string right now.
Instead, I added an overload that copies the std:string to a `char *`
and back. This means more copying, but this is not used in a hot path
at all, so I think it is acceptable.
Change-Id: I92c50a1bdd8307141cdbacb388248e4e4fc08c93
Co-authored-by: Lancelot SIX <lsix@lancelotsix.com>
cmd_list_element can contain a pointer to data that can be set and / or
shown. This is achieved with the void* VAR member which points to the
data that can be accessed, while the VAR_TYPE member (of type enum
var_types) indicates how to interpret the data pointed to.
With this pattern, the user of the cmd_list_element needs to know what
is the storage type associated with a given VAR_TYPES in order to do
the proper casting. No automatic safeguard is available to prevent
miss-use of the pointer. Client code typically looks something like:
switch (c->var_type)
{
case var_zuinteger:
unsigned int v = *(unsigned int*) c->var;
...
break;
case var_boolean:
bool v = *(bool *) c->var;
...
break;
...
}
This patch proposes to add an abstraction around the var_types and void*
pointer pair. The abstraction is meant to prevent the user from having
to handle the cast and verify that the data is read or written as a type
that is coherent with the setting's var_type. This is achieved by
introducing the struct setting which exposes a set of templated get /
set member functions. The template parameter is the type of the
variable that holds the referred variable.
Using those accessors allows runtime checks to be inserted in order to
ensure that the data pointed to has the expected type. For example,
instantiating the member functions with bool will yield something
similar to:
const bool &get<bool> () const
{
gdb_assert (m_var_type == var_boolean);
gdb_assert (m_var != nullptr);
return *static_cast<bool *> (m_var);
}
void set<bool> (const bool &var)
{
gdb_assert (m_var_type == var_boolean);
gdb_assert (m_var != nullptr);
*static_cast<bool *> (m_var) = var;
}
Using the new abstraction, our initial example becomes:
switch (c->var_type)
{
case var_zuinteger:
unsigned int v = c->var->get<unsigned int> ();
...
break;
case var_boolean:
bool v = c->var->get<bool> ();
...
break;
...
}
While the call site is still similar, the introduction of runtime checks
help ensure correct usage of the data.
In order to avoid turning the bulk of add_setshow_cmd_full into a
templated function, and following a suggestion from Pedro Alves, a
setting can be constructed from a pre validated type erased reference to
a variable. This is what setting::erased_args is used for.
Introducing an opaque abstraction to describe a setting will also make
it possible to use callbacks to retrieve or set the value of the setting
on the fly instead of pointing to a static chunk of memory. This will
be done added in a later commit.
Given that a cmd_list_element may or may not reference a setting, the
VAR and VAR_TYPES members of the struct are replaced with a
gdb::optional<setting> named VAR.
Few internal function signatures have been modified to take into account
this new abstraction:
-The functions value_from_setting, str_value_from_setting and
get_setshow_command_value_string used to have a 'cmd_list_element *'
parameter but only used it for the VAR and VAR_TYPE member. They now
take a 'const setting &' parameter instead.
- Similarly, the 'void *' and a 'enum var_types' parameters of
pascm_param_value and gdbpy_parameter_value have been replaced with a
'const setting &' parameter.
No user visible change is expected after this patch.
Tested on GNU/Linux x86_64, with no regression noticed.
Co-authored-by: Simon Marchi <simon.marchi@polymtl.ca>
Change-Id: Ie1d08c3ceb8b30b3d7bf1efe036eb8acffcd2f34
The following scenario hangs:
- maint set target-non-stop on
- `gdbserver --attach`
- a multi-threaded program
For example:
Terminal 1:
$ gnome-calculator&
[1] 495731
$ ../gdbserver/gdbserver --once --attach :1234 495731
Attached; pid = 495731
Listening on port 1234
Terminal 2:
$ ./gdb -nx -q --data-directory=data-directory /usr/bin/gnome-calculator -ex "maint set target-non-stop on" -ex "tar rem :1234"
Reading symbols from /usr/bin/gnome-calculator...
(No debugging symbols found in /usr/bin/gnome-calculator)
Remote debugging using :1234
* hangs *
What happens is:
- The protocol between gdb and gdbserver is in non-stop mode, but the
user-visible behavior is all-stop
- On connect, gdbserver sends one stop reply for one thread that is
stops, the others stay running
- In process_initial_stop_replies, gdb calls stop_all_threads to stop
these other threads, because we are using the all-stop user-visible
mode
- stop_all_threads sends a stop request for all the running threads and
then waits for resulting events
- At this point, the remote target is in target_async(0) mode, which
makes stop_all_threads not consider it for events
- stop_all_threads loops indefinitely (it does not even block
indefinitely, it is in an infinite busy loop) because there are no
event sources. wait_one_event returns a TARGET_WAITKIND_NO_RESUMED
wait status.
Fix that by making the remote target async around the stop_all_threads
call.
I haven't implemented it because I'm not sure how to do it, but I think
it would be a good idea to have, in stop_all_threads / wait_one /
handle_one, an assert to check that if we are expecting one or more
event, then there are some targets that are in a state where they can
supply some events. Otherwise, we'll necessarily be stuck in this
infinite loop, and it's probably due to a bug in GDB. I'm not too sure
where to put this or how to express it though. Perhaps in
stop_all_threads, here:
for (int i = 0; i < waits_needed; i++)
{
wait_one_event event = wait_one ();
*here*
if (handle_one (event))
break;
}
If at that point, the returned event is TARGET_WAITKIND_NO_RESUMED,
there's a problem. We expect some event, because we've asked some
threads to stop, but all targets are answering that they won't have any
events for us. That's a contradiction, and a sign that something has
gone wrong. It could perhaps event be:
gdb_assert (event.ws.kind != TARGET_WAITKIND_NO_RESUMED);
in handle_one, as the idea is the same in prepare_for_detach.
A bit more sophisticated would be: we know which targets we are
expecting waits from, since we know which threads we have asked to
stop. So if any of these targets returns TARGET_WAITKIND_NO_RESUMED,
something is fishy.
Add a test that tests attaching with gdbserver's --attach flag to a
multi-threaded program, and then connecting to it. Without the fix, the
test reproduces the hang.
Change-Id: If6f6690a4887ca66693ef1af64791dda4c65f24f
get_ada_task_ptid currently takes a 'long' as its 'thread' parameter
type. However, on some platforms this is actually a pointer, and
using 'long' can sometimes end up with the value being sign-extended.
This sign extension can cause problems later, if the tid is then later
used as an address again.
This patch changes the parameter type to ULONGEST and updates all the
uses. This approach preserves sign extension on the targets where it
is apparently intended, while avoiding it on others.
Co-Authored-By: John Baldwin <jhb@FreeBSD.org>
The ptid_t 'tid' member is normally used as an address in gdb -- both
bsd-uthread and ravenscar-thread use it this way. However, because
the type is 'long', this can cause problems with sign extension.
This patch changes the type to ULONGEST to ensure that sign extension
does not occur.
I wanted to find, and potentially modify, all the spots where the
'tid' parameter to the ptid_t constructor was used. So, I temporarily
removed this parameter and then rebuilt.
In order to make it simpler to search through the "real" (nonzero)
uses of this parameter, something I knew I'd have to do multiple
times, I removed any ", 0" from constructor calls.
Co-Authored-By: John Baldwin <jhb@FreeBSD.org>
In the context of ROCm-gdb [1], the ROCm target sits on top of the
linux-nat target. when a process forks, it needs to carry over some
data from the forking inferior to the fork child inferior. Ideally, the
ROCm target would implement the follow_fork target_ops method, but there
are some small problems. This patch fixes these, which helps the ROCm
target, but also makes things more consistent and a bit nicer in
general, I believe.
The main problem is: when follow-fork-mode is "parent",
target_follow_fork is called with the parent as the current inferior.
When it's "child", target_follow_fork is called with the child as the
current inferior. This means that target_follow_fork is sometimes
called on the parent's target stack and sometimes on the child's target
stack.
The parent's target stack may contain targets above the process target,
such as the ROCm target. So if follow-fork-child is "parent", the ROCm
target would get notified of the fork and do whatever is needed. But
the child's target stack, at that moment, only contains the exec and
process target copied over from the parent. The child's target stack is
set up by follow_fork_inferior, before calling target_follow_fork. In
that case, the ROCm target wouldn't get notified of the fork.
For consistency, I think it would be good to always call
target_follow_fork on the parent inferior's target stack. I think it
makes sense as a way to indicate "this inferior has called fork, do
whatever is needed". The desired outcome of the fork (whether an
inferior is created for the child, do we need to detach from the child)
can be indicated by passed parameter.
I therefore propose these changes:
- make follow_fork_inferior always call target_follow_fork with the
parent as the current inferior. That lets all targets present on the
parent's target stack do some fork-related handling and push
themselves on the fork child's target stack if needed.
For this purpose, pass the child inferior down to target_follow_fork
and follow_fork implementations. This is nullptr if no inferior is
created for the child, because we want to detach from it.
- as a result, in follow_fork_inferior, detach from the parent inferior
(if needed) only after the target_follow_fork call. This is needed
because we want to call target_follow_fork before the parent's
target stack is torn down.
- hand over to the targets in the parent's target stack (including the
process target) the responsibility to push themselves, if needed, to
the child's target stack. Also hand over the responsibility to the
process target, at the same time, to create the child's initial
thread (just like we do for follow_exec).
- pass the child inferior to exec_on_vfork, so we don't need to swap
the current inferior between parent and child. Nothing in
exec_on_vfork depends on the current inferior, after this change.
Although this could perhaps be replaced with just having the exec
target implement follow_fork and push itself in the child's target
stack, like the process target does... We would just need to make
sure the process target calls beneath()->follow_fork(...). I'm not
sure about this one.
gdb/ChangeLog:
* target.h (struct target_ops) <follow_fork>: Add inferior*
parameter.
(target_follow_fork): Likewise.
* target.c (default_follow_fork): Likewise.
(target_follow_fork): Likewise.
* fbsd-nat.h (class fbsd_nat_target) <follow_fork>: Likewise.
(fbsd_nat_target::follow_fork): Likewise, and call
inf_ptrace_target::follow_fork.
* linux-nat.h (class linux_nat_target) <follow_fork>: Likewise.
* linux-nat.c (linux_nat_target::follow_fork): Likewise, and
call inf_ptrace_target::follow_fork.
* obsd-nat.h (obsd_nat_target) <follow_fork>: Likewise.
* obsd-nat.c (obsd_nat_target::follow_fork): Likewise, and call
inf_ptrace_target::follow_fork.
* remote.c (class remote_target) <follow_fork>: Likewise.
(remote_target::follow_fork): Likewise, and call
process_stratum_target::follow_fork.
* process-stratum-target.h (class process_stratum_target)
<follow_fork>: New.
* process-stratum-target.c
(process_stratum_target::follow_fork): New.
* target-delegates.c: Re-generate.
[1] https://github.com/ROCm-Developer-Tools/ROCgdb
Change-Id: I460bd0af850f0485e8aed4b24c6d8262a4c69929
Same idea as the previous patch, but for m_cwd.
To keep things consistent across the board, change get_inferior_cwd as
well, which is shared with GDBserver. So update the related GDBserver
code too.
Change-Id: Ia2c047fda738d45f3d18bc999eb67ceb8400ce4e
Add cwd/set_cwd to the inferior class, remove set_inferior_args.
Keep get_inferior_args, because it is used from fork_inferior, in shared
code. The cwd could eventually be passed as a parameter eventually,
though, I think that would be cleaner.
Change-Id: Ifb72ea865d7e6f9a491308f0d5c1595579d8427e
Commit 408f66864a ("detach in all-stop
with threads running") regressed "detach" with "target remote":
(gdb) detach
Detaching from program: target:/any/program, process 3671843
Detaching from process 3671843
Ending remote debugging.
[Inferior 1 (process 3671843) detached]
In main
terminate called after throwing an instance of 'gdb_exception_error'
Aborted (core dumped)
Here's the exception above being thrown:
(top-gdb) bt
#0 throw_error (error=TARGET_CLOSE_ERROR, fmt=0x555556035588 "Remote connection closed") at src/gdbsupport/common-exceptions.cc:222
#1 0x0000555555bbaa46 in remote_target::readchar (this=0x555556a11040, timeout=10000) at src/gdb/remote.c:9440
#2 0x0000555555bbb9e5 in remote_target::getpkt_or_notif_sane_1 (this=0x555556a11040, buf=0x555556a11058, forever=0, expecting_notif=0, is_notif=0x0) at src/gdb/remote.c:9928
#3 0x0000555555bbbda9 in remote_target::getpkt_sane (this=0x555556a11040, buf=0x555556a11058, forever=0) at src/gdb/remote.c:10030
#4 0x0000555555bc0e75 in remote_target::remote_hostio_send_command (this=0x555556a11040, command_bytes=13, which_packet=14, remote_errno=0x7fffffffcfd0, attachment=0x0, attachment_len=0x0) at src/gdb/remote.c:12137
#5 0x0000555555bc1b6c in remote_target::remote_hostio_close (this=0x555556a11040, fd=8, remote_errno=0x7fffffffcfd0) at src/gdb/remote.c:12455
#6 0x0000555555bc1bb4 in remote_target::fileio_close (During symbol reading: .debug_line address at offset 0x64f417 is 0 [in module build/gdb/gdb]
this=0x555556a11040, fd=8, remote_errno=0x7fffffffcfd0) at src/gdb/remote.c:12462
#7 0x0000555555c9274c in target_fileio_close (fd=3, target_errno=0x7fffffffcfd0) at src/gdb/target.c:3365
#8 0x000055555595a19d in gdb_bfd_iovec_fileio_close (abfd=0x555556b9f8a0, stream=0x555556b11530) at src/gdb/gdb_bfd.c:439
#9 0x0000555555e09e3f in opncls_bclose (abfd=0x555556b9f8a0) at src/bfd/opncls.c:599
#10 0x0000555555e0a2c7 in bfd_close_all_done (abfd=0x555556b9f8a0) at src/bfd/opncls.c:847
#11 0x0000555555e0a27a in bfd_close (abfd=0x555556b9f8a0) at src/bfd/opncls.c:814
#12 0x000055555595a9d3 in gdb_bfd_close_or_warn (abfd=0x555556b9f8a0) at src/gdb/gdb_bfd.c:626
#13 0x000055555595ad29 in gdb_bfd_unref (abfd=0x555556b9f8a0) at src/gdb/gdb_bfd.c:715
#14 0x0000555555ae4730 in objfile::~objfile (this=0x555556515540, __in_chrg=<optimized out>) at src/gdb/objfiles.c:573
#15 0x0000555555ae955a in std::_Sp_counted_ptr<objfile*, (__gnu_cxx::_Lock_policy)2>::_M_dispose (this=0x555556c20db0) at /usr/include/c++/9/bits/shared_ptr_base.h:377
#16 0x000055555572b7c8 in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release (this=0x555556c20db0) at /usr/include/c++/9/bits/shared_ptr_base.h:155
#17 0x00005555557263c3 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count (this=0x555556bf0588, __in_chrg=<optimized out>) at /usr/include/c++/9/bits/shared_ptr_base.h:730
#18 0x0000555555ae745e in std::__shared_ptr<objfile, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr (this=0x555556bf0580, __in_chrg=<optimized out>) at /usr/include/c++/9/bits/shared_ptr_base.h:1169
#19 0x0000555555ae747e in std::shared_ptr<objfile>::~shared_ptr (this=0x555556bf0580, __in_chrg=<optimized out>) at /usr/include/c++/9/bits/shared_ptr.h:103
#20 0x0000555555b1c1dc in __gnu_cxx::new_allocator<std::_List_node<std::shared_ptr<objfile> > >::destroy<std::shared_ptr<objfile> > (this=0x5555564cdd60, __p=0x555556bf0580) at /usr/include/c++/9/ext/new_allocator.h:153
#21 0x0000555555b1bb1d in std::allocator_traits<std::allocator<std::_List_node<std::shared_ptr<objfile> > > >::destroy<std::shared_ptr<objfile> > (__a=..., __p=0x555556bf0580) at /usr/include/c++/9/bits/alloc_traits.h:497
#22 0x0000555555b1b73e in std::__cxx11::list<std::shared_ptr<objfile>, std::allocator<std::shared_ptr<objfile> > >::_M_erase (this=0x5555564cdd60, __position=std::shared_ptr<objfile> (expired, weak count 1) = {get() = 0x555556515540}) at /usr/include/c++/9/bits/stl_list.h:1921
#23 0x0000555555b1afeb in std::__cxx11::list<std::shared_ptr<objfile>, std::allocator<std::shared_ptr<objfile> > >::erase (this=0x5555564cdd60, __position=std::shared_ptr<objfile> (expired, weak count 1) = {get() = 0x555556515540}) at /usr/include/c++/9/bits/list.tcc:158
#24 0x0000555555b19576 in program_space::remove_objfile (this=0x5555564cdd20, objfile=0x555556515540) at src/gdb/progspace.c:210
#25 0x0000555555ae4502 in objfile::unlink (this=0x555556515540) at src/gdb/objfiles.c:487
#26 0x0000555555ae5a12 in objfile_purge_solibs () at src/gdb/objfiles.c:875
#27 0x0000555555c09686 in no_shared_libraries (ignored=0x0, from_tty=1) at src/gdb/solib.c:1236
#28 0x00005555559e3f5f in detach_command (args=0x0, from_tty=1) at src/gdb/infcmd.c:2769
So frame #28 already detached the remote process, and then we're
purging the shared libraries. GDB had opened remote shared libraries
via the target: sysroot, so it tries closing them. GDBserver is
tearing down already, so remote communication breaks down and we close
the remote target and throw TARGET_CLOSE_ERROR.
Note frame #14:
#14 0x0000555555ae4730 in objfile::~objfile (this=0x555556515540, __in_chrg=<optimized out>) at src/gdb/objfiles.c:573
That's a dtor, thus noexcept. That's the reason for the
std::terminate.
Stepping back a bit, why do we still have open remote files if we've
managed to detach already, and, we're debugging with "target remote"?
The reason is that commit 408f66864a
makes detach_command hold a reference to the target, so the remote
target won't be finally closed until frame #28 returns. It's closing
the target that invalidates target file I/O handles.
This commit fixes the issue by not relying on target_close to
invalidate the target file I/O handles, instead invalidate them
immediately in remote_unpush_target. So when GDB purges the solibs,
and we end up in target_fileio_close (frame #7 above), there's nothing
to do, and we don't try to talk with the remote target anymore.
The regression isn't seen when testing with
--target_board=native-gdbserver, because that does "set sysroot" to
disable the "target:" sysroot, for test run speed reasons. So this
commit adds a testcase that explicitly tests detach with "set sysroot
target:".
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <pedro@palves.net>
PR gdb/28080
* remote.c (remote_unpush_target): Invalidate file I/O target
handles.
* target.c (fileio_handles_invalidate_target): Make extern.
* target.h (fileio_handles_invalidate_target): Declare.
gdb/testsuite/ChangeLog:
yyyy-mm-dd Pedro Alves <pedro@palves.net>
PR gdb/28080
* gdb.base/detach-sysroot-target.exp: New.
* gdb.base/detach-sysroot-target.c: New.
Reported-By: Jonah Graham <jonah@kichwacoders.com>
Change-Id: I851234910172f42a1b30e731161376c344d2727d
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 remote_state::starting_up member variable is already of type bool,
but in some places we still write to it using 1 and 0. This commit
just updates things to use true and false.
There should be no user visible change after this commit.
gdb/ChangeLog:
* remote.c (remote_target::start_remote): Set 'starting_up' using
boolean values instead of integers.
gdb/remote.c:14541:5: warning: misleading indentation; statement is not part of the previous 'if' [-Wmisleading-indentation]
if (current_inferior ()->in_initial_library_scan)
^
gdb/remote.c:14527:3: note: previous statement is here
if (remote == nullptr)
^
gdb/ChangeLog:
* remote.c (remote_new_objfile): Fix indentation.
When trying to attach to a pthread process on a Linux system with glibc 2.33,
we get:
$ ./gdb -q -nx --data-directory=data-directory -p 1472010
Attaching to process 1472010
[New LWP 1472013]
[New LWP 1472014]
[New LWP 1472015]
Error while reading shared library symbols for /usr/lib/libpthread.so.0:
Cannot find user-level thread for LWP 1472015: generic error
0x00007ffff6d3637f in poll () from /usr/lib/libc.so.6
(gdb)
When attaching to a process (or handling a fork child, an operation very
similar to attaching), GDB reads the shared library list from the
process. For each shared library (if "set auto-solib-add" is on), it
reads its symbols and calls the "new_objfile" observable.
The libthread-db code monitors this observable, and if it sees an
objfile named somewhat like "libpthread.so" go by, it tries to load
libthread_db.so in the GDB process itself. libthread_db knows how to
navigate libpthread's data structures to get information about the
existing threads.
To locate these data structures, libthread_db calls ps_pglobal_lookup
(implemented in proc-service.c), passing in a symbol name and expecting
an address in return.
Before glibc 2.33, libthread_db always asked for symbols found in
libpthread. There was no ordering problem: since we were always trying
to load libthread_db in reaction to processing libpthread (and reading
in its symbols) and libthread_db only asked symbols from libpthread, the
requested symbols could always be found. Starting with glibc 2.33,
libthread_db now asks for a symbol name that can be found in
/lib/ld-linux-x86-64.so.2 (_rtld_global). And the ordering in which GDB
reads the shared libraries from the inferior when attaching is
unfortunate, in that libpthread is processed before ld-linux. So when
loading libthread_db in reaction to processing libpthread, and
libthread_db requests the symbol that is from ld-linux, GDB is not yet
able to supply it.
That problematic symbol lookup happens in the thread_from_lwp function,
when we call td_ta_map_lwp2thr_p, and an exception is thrown at this
point:
#0 0x00007ffff6681012 in __cxxabiv1::__cxa_throw (obj=0x60e000006100, tinfo=0x555560033b50 <typeinfo for gdb_exception_error>, dest=0x55555d9404bc <gdb_exception_error::~gdb_exception_error()>) at /build/gcc/src/gcc/libstdc++-v3/libsupc++/eh_throw.cc:78
#1 0x000055555e5d3734 in throw_it(return_reason, errors, const char *, typedef __va_list_tag __va_list_tag *) (reason=RETURN_ERROR, error=GENERIC_ERROR, fmt=0x55555f0c5360 "Cannot find user-level thread for LWP %ld: %s", ap=0x7fffffffaae0) at /home/simark/src/binutils-gdb/gdbsupport/common-exceptions.cc:200
#2 0x000055555e5d37d4 in throw_verror (error=GENERIC_ERROR, fmt=0x55555f0c5360 "Cannot find user-level thread for LWP %ld: %s", ap=0x7fffffffaae0) at /home/simark/src/binutils-gdb/gdbsupport/common-exceptions.cc:208
#3 0x000055555e0b0ed2 in verror (string=0x55555f0c5360 "Cannot find user-level thread for LWP %ld: %s", args=0x7fffffffaae0) at /home/simark/src/binutils-gdb/gdb/utils.c:171
#4 0x000055555e5e898a in error (fmt=0x55555f0c5360 "Cannot find user-level thread for LWP %ld: %s") at /home/simark/src/binutils-gdb/gdbsupport/errors.cc:43
#5 0x000055555d06b4bc in thread_from_lwp (stopped=0x617000035d80, ptid=...) at /home/simark/src/binutils-gdb/gdb/linux-thread-db.c:418
#6 0x000055555d07040d in try_thread_db_load_1 (info=0x60c000011140) at /home/simark/src/binutils-gdb/gdb/linux-thread-db.c:912
#7 0x000055555d071103 in try_thread_db_load (library=0x55555f0c62a0 "libthread_db.so.1", check_auto_load_safe=false) at /home/simark/src/binutils-gdb/gdb/linux-thread-db.c:1014
#8 0x000055555d072168 in try_thread_db_load_from_sdir () at /home/simark/src/binutils-gdb/gdb/linux-thread-db.c:1091
#9 0x000055555d072d1c in thread_db_load_search () at /home/simark/src/binutils-gdb/gdb/linux-thread-db.c:1146
#10 0x000055555d07365c in thread_db_load () at /home/simark/src/binutils-gdb/gdb/linux-thread-db.c:1203
#11 0x000055555d07373e in check_for_thread_db () at /home/simark/src/binutils-gdb/gdb/linux-thread-db.c:1246
#12 0x000055555d0738ab in thread_db_new_objfile (objfile=0x61300000c0c0) at /home/simark/src/binutils-gdb/gdb/linux-thread-db.c:1275
#13 0x000055555bd10740 in std::__invoke_impl<void, void (*&)(objfile*), objfile*> (__f=@0x616000068d88: 0x55555d073745 <thread_db_new_objfile(objfile*)>) at /usr/include/c++/10.2.0/bits/invoke.h:60
#14 0x000055555bd02096 in std::__invoke_r<void, void (*&)(objfile*), objfile*> (__fn=@0x616000068d88: 0x55555d073745 <thread_db_new_objfile(objfile*)>) at /usr/include/c++/10.2.0/bits/invoke.h:153
#15 0x000055555bce0392 in std::_Function_handler<void (objfile*), void (*)(objfile*)>::_M_invoke(std::_Any_data const&, objfile*&&) (__functor=..., __args#0=@0x7fffffffb4a0: 0x61300000c0c0) at /usr/include/c++/10.2.0/bits/std_function.h:291
#16 0x000055555d3595c0 in std::function<void (objfile*)>::operator()(objfile*) const (this=0x616000068d88, __args#0=0x61300000c0c0) at /usr/include/c++/10.2.0/bits/std_function.h:622
#17 0x000055555d356b7f in gdb::observers::observable<objfile*>::notify (this=0x555566727020 <gdb::observers::new_objfile>, args#0=0x61300000c0c0) at /home/simark/src/binutils-gdb/gdb/../gdbsupport/observable.h:106
#18 0x000055555da3f228 in symbol_file_add_with_addrs (abfd=0x61200001ccc0, name=0x6190000d9090 "/usr/lib/libpthread.so.0", add_flags=..., addrs=0x7fffffffbc10, flags=..., parent=0x0) at /home/simark/src/binutils-gdb/gdb/symfile.c:1131
#19 0x000055555da3f763 in symbol_file_add_from_bfd (abfd=0x61200001ccc0, name=0x6190000d9090 "/usr/lib/libpthread.so.0", add_flags=<error reading variable: Cannot access memory at address 0xffffffffffffffb0>, addrs=0x7fffffffbc10, flags=<error reading variable: Cannot access memory at address 0xffffffffffffffc0>, parent=0x0) at /home/simark/src/binutils-gdb/gdb/symfile.c:1167
#20 0x000055555d95f9fa in solib_read_symbols (so=0x6190000d8e80, flags=...) at /home/simark/src/binutils-gdb/gdb/solib.c:681
#21 0x000055555d96233d in solib_add (pattern=0x0, from_tty=0, readsyms=1) at /home/simark/src/binutils-gdb/gdb/solib.c:987
#22 0x000055555d93646e in enable_break (info=0x608000008f20, from_tty=0) at /home/simark/src/binutils-gdb/gdb/solib-svr4.c:2238
#23 0x000055555d93cfc0 in svr4_solib_create_inferior_hook (from_tty=0) at /home/simark/src/binutils-gdb/gdb/solib-svr4.c:3049
#24 0x000055555d96610d in solib_create_inferior_hook (from_tty=0) at /home/simark/src/binutils-gdb/gdb/solib.c:1195
#25 0x000055555cdee318 in post_create_inferior (from_tty=0) at /home/simark/src/binutils-gdb/gdb/infcmd.c:318
#26 0x000055555ce00e6e in setup_inferior (from_tty=0) at /home/simark/src/binutils-gdb/gdb/infcmd.c:2439
#27 0x000055555ce59c34 in handle_one (event=...) at /home/simark/src/binutils-gdb/gdb/infrun.c:4887
#28 0x000055555ce5cd00 in stop_all_threads () at /home/simark/src/binutils-gdb/gdb/infrun.c:5064
#29 0x000055555ce7f0da in stop_waiting (ecs=0x7fffffffd170) at /home/simark/src/binutils-gdb/gdb/infrun.c:8006
#30 0x000055555ce67f5c in handle_signal_stop (ecs=0x7fffffffd170) at /home/simark/src/binutils-gdb/gdb/infrun.c:6062
#31 0x000055555ce63653 in handle_inferior_event (ecs=0x7fffffffd170) at /home/simark/src/binutils-gdb/gdb/infrun.c:5727
#32 0x000055555ce4f297 in fetch_inferior_event () at /home/simark/src/binutils-gdb/gdb/infrun.c:4105
#33 0x000055555cdbe3bf in inferior_event_handler (event_type=INF_REG_EVENT) at /home/simark/src/binutils-gdb/gdb/inf-loop.c:42
#34 0x000055555d018047 in handle_target_event (error=0, client_data=0x0) at /home/simark/src/binutils-gdb/gdb/linux-nat.c:4060
#35 0x000055555e5ea77e in handle_file_event (file_ptr=0x60600008b1c0, ready_mask=1) at /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:575
#36 0x000055555e5eb09c in gdb_wait_for_event (block=0) at /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:701
#37 0x000055555e5e8d19 in gdb_do_one_event () at /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:212
#38 0x000055555dd6e0d4 in wait_sync_command_done () at /home/simark/src/binutils-gdb/gdb/top.c:528
#39 0x000055555dd6e372 in maybe_wait_sync_command_done (was_sync=0) at /home/simark/src/binutils-gdb/gdb/top.c:545
#40 0x000055555d0ec7c8 in catch_command_errors (command=0x55555ce01bb8 <attach_command(char const*, int)>, arg=0x7fffffffe28d "1472010", from_tty=1, do_bp_actions=false) at /home/simark/src/binutils-gdb/gdb/main.c:452
#41 0x000055555d0f03ad in captured_main_1 (context=0x7fffffffdd10) at /home/simark/src/binutils-gdb/gdb/main.c:1149
#42 0x000055555d0f1239 in captured_main (data=0x7fffffffdd10) at /home/simark/src/binutils-gdb/gdb/main.c:1232
#43 0x000055555d0f1315 in gdb_main (args=0x7fffffffdd10) at /home/simark/src/binutils-gdb/gdb/main.c:1257
#44 0x000055555bb70cf9 in main (argc=7, argv=0x7fffffffde88) at /home/simark/src/binutils-gdb/gdb/gdb.c:32
The exception is caught here:
#0 __cxxabiv1::__cxa_begin_catch (exc_obj_in=0x60e0000060e0) at /build/gcc/src/gcc/libstdc++-v3/libsupc++/eh_catch.cc:84
#1 0x000055555d95fded in solib_read_symbols (so=0x6190000d8e80, flags=...) at /home/simark/src/binutils-gdb/gdb/solib.c:689
#2 0x000055555d96233d in solib_add (pattern=0x0, from_tty=0, readsyms=1) at /home/simark/src/binutils-gdb/gdb/solib.c:987
#3 0x000055555d93646e in enable_break (info=0x608000008f20, from_tty=0) at /home/simark/src/binutils-gdb/gdb/solib-svr4.c:2238
#4 0x000055555d93cfc0 in svr4_solib_create_inferior_hook (from_tty=0) at /home/simark/src/binutils-gdb/gdb/solib-svr4.c:3049
#5 0x000055555d96610d in solib_create_inferior_hook (from_tty=0) at /home/simark/src/binutils-gdb/gdb/solib.c:1195
#6 0x000055555cdee318 in post_create_inferior (from_tty=0) at /home/simark/src/binutils-gdb/gdb/infcmd.c:318
#7 0x000055555ce00e6e in setup_inferior (from_tty=0) at /home/simark/src/binutils-gdb/gdb/infcmd.c:2439
#8 0x000055555ce59c34 in handle_one (event=...) at /home/simark/src/binutils-gdb/gdb/infrun.c:4887
#9 0x000055555ce5cd00 in stop_all_threads () at /home/simark/src/binutils-gdb/gdb/infrun.c:5064
#10 0x000055555ce7f0da in stop_waiting (ecs=0x7fffffffd170) at /home/simark/src/binutils-gdb/gdb/infrun.c:8006
#11 0x000055555ce67f5c in handle_signal_stop (ecs=0x7fffffffd170) at /home/simark/src/binutils-gdb/gdb/infrun.c:6062
#12 0x000055555ce63653 in handle_inferior_event (ecs=0x7fffffffd170) at /home/simark/src/binutils-gdb/gdb/infrun.c:5727
#13 0x000055555ce4f297 in fetch_inferior_event () at /home/simark/src/binutils-gdb/gdb/infrun.c:4105
#14 0x000055555cdbe3bf in inferior_event_handler (event_type=INF_REG_EVENT) at /home/simark/src/binutils-gdb/gdb/inf-loop.c:42
#15 0x000055555d018047 in handle_target_event (error=0, client_data=0x0) at /home/simark/src/binutils-gdb/gdb/linux-nat.c:4060
#16 0x000055555e5ea77e in handle_file_event (file_ptr=0x60600008b1c0, ready_mask=1) at /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:575
#17 0x000055555e5eb09c in gdb_wait_for_event (block=0) at /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:701
#18 0x000055555e5e8d19 in gdb_do_one_event () at /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:212
#19 0x000055555dd6e0d4 in wait_sync_command_done () at /home/simark/src/binutils-gdb/gdb/top.c:528
#20 0x000055555dd6e372 in maybe_wait_sync_command_done (was_sync=0) at /home/simark/src/binutils-gdb/gdb/top.c:545
#21 0x000055555d0ec7c8 in catch_command_errors (command=0x55555ce01bb8 <attach_command(char const*, int)>, arg=0x7fffffffe28d "1472010", from_tty=1, do_bp_actions=false) at /home/simark/src/binutils-gdb/gdb/main.c:452
#22 0x000055555d0f03ad in captured_main_1 (context=0x7fffffffdd10) at /home/simark/src/binutils-gdb/gdb/main.c:1149
#23 0x000055555d0f1239 in captured_main (data=0x7fffffffdd10) at /home/simark/src/binutils-gdb/gdb/main.c:1232
#24 0x000055555d0f1315 in gdb_main (args=0x7fffffffdd10) at /home/simark/src/binutils-gdb/gdb/main.c:1257
#25 0x000055555bb70cf9 in main (argc=7, argv=0x7fffffffde88) at /home/simark/src/binutils-gdb/gdb/gdb.c:32
Catching the exception at this point means that the thread_db_info
object for this inferior will be left in place, despite the failure to
load libthread_db. This means that there won't be further attempts at
loading libthread_db, because thread_db_load will think that
libthread_db is already loaded for this inferior and will always exit
early. To fix this, add a try/catch around calling try_thread_db_load_1
in try_thread_db_load, such that if some exception is thrown while
trying to load libthread_db, we reset / delete the thread_db_info for
that inferior. That alone makes attach work fine again, because
check_for_thread_db is called again in the thread_db_inferior_created
observer (that happens after we learned about all shared libraries and
their symbols), and libthread_db is successfully loaded then.
When attaching, I think that the inferior_created observer is a good
place to try to load libthread_db: it is called once everything has
stabilized, when we learned about all shared libraries.
The only problem then is that when we first try (and fail) to load
libthread_db, in reaction to learning about libpthread, we show this
warning:
warning: Unable to find libthread_db matching inferior's thread library, thread debugging will not be available.
This is misleading, because we do succeed in loading it later. So when
attaching, I think we shouldn't try to load libthread_db in reaction to
the new_objfile events, we should wait until we have learned about all
shared libraries (using the inferior_created observable). To do so, add
an `in_initial_library_scan` flag to struct inferior. This flag is used
to postpone loading libthread_db if we are attaching or handling a fork
child.
When debugging remotely with GDBserver, the same problem happens, except
that the qSymbol mechanism (allowing the remote side to ask GDB for
symbols values) is involved. The fix there is the same idea, we make
GDB wait until all shared libraries and their symbols are known before
sending out a qSymbol packet. This way, we never present the remote
side a state where libpthread.so's symbols are known but ld-linux's
symbols aren't.
gdb/ChangeLog:
* inferior.h (class inferior) <in_initial_library_scan>: New.
* infcmd.c (post_create_inferior): Set in_initial_library_scan.
* infrun.c (follow_fork_inferior): Likewise.
* linux-thread-db.c (try_thread_db_load): Catch exception thrown
by try_thread_db_load_1
(thread_db_load): Return early if in_initial_library_scan is
set.
* remote.c (remote_new_objfile): Return early if
in_initial_library_scan is set.
Change-Id: I7a279836cfbb2b362b4fde11b196b4aab82f5efb
I needed more debug output from:
remote_target::select_thread_for_ambiguous_stop_reply
I thought this would be useful for others too.
gdb/ChangeLog:
* remote.c (remote_target)
<select_thread_for_ambiguous_stop_reply>: Add additional debug
output.
I spotted some indentation issues where we had some spaces followed by
tabs at beginning of line, that I wanted to fix. So while at it, I did
a quick grep to find and fix all I could find.
gdb/ChangeLog:
* Fix tab after space indentation issues throughout.
Change-Id: I1acb414dd9c593b474ae2b8667496584df4316fd
Add the breakpoint::locations method, which returns a range that can be
used to iterate over a breakpoint's locations. This shortens
for (bp_location *loc = b->loc; loc != nullptr; loc = loc->next)
into
for (bp_location *loc : b->locations ())
Change all the places that I found that could use it.
gdb/ChangeLog:
* breakpoint.h (bp_locations_range): New.
(struct breakpoint) <locations>: New. Use where possible.
Change-Id: I1ba2f7d93d57e544e1f8609124587dcf2e1da037
Same idea as previous patch, but for add_alias_cmd. Remove the overload
that accepts the target command as a string (the target command name),
leaving only the one that takes the cmd_list_element.
gdb/ChangeLog:
* command.h (add_alias_cmd): Accept target as
cmd_list_element. Update callers.
Change-Id: I546311f411e9e7da9302322d6ffad4e6c56df266
Remove a few instances where we look up a command by name, but could
just use the return value of a previous "add command" function call
instead.
gdb/ChangeLog:
* mi/mi-main.c (_initialize_mi_main):
* python/py-auto-load.c (gdbpy_initialize_auto_load):
* remote.c (_initialize_remote):
Change-Id: I6d06f9ca636e340c88c1064ae870483ad392607d
Convert a couple of local variables from int to bool. There should be
no user visible changes after this commit.
gdb/ChangeLog:
* remote.c (check_pending_events_prevent_wildcard_vcont): Change
argument type, update and re-wrap, header comment.
(remote_target::commit_resumed): Convert any_process_wildcard and
may_global_wildcard_vcont from int to bool.
On "exec", some targets need to unpush themselves from the inferior,
and do some bookkeeping, like forgetting the data associated to the
exec'ing inferior.
One such example is the thread-db target. It does so in
a special case in thread_db_target::wait, just before returning the
TARGET_WAITKIND_EXECD event to its caller.
We have another such case in the context of rocm-gdb [1], where the
"rocm" target is pushed on top of the linux-nat target. When an exec
happens, we want to unpush the rocm target from the exec'ing inferior to
close some file descriptors that refer to the pre-exec address space and
forget about that inferior. We then want to push the target on the
inferior in which execution continues, to open the file descriptors for
the post-exec address space.
I think that a good way to address this cleanly is to do all this in the
target_ops::follow_exec implementations. Make the
process_stratum_target::follow_exec implementation have the default
behavior of pushing itself to the new inferior's target stack (if
execution continues in a new inferior) and add the initial thread.
remote_target::follow_exec is an example of process target that wants to
do a bit more than the default behavior. So it calls
process_stratum_target::follow_exec first and does the extra work
second.
linux-thread-db (a non-process target) implements follow_exec to do some
bookeeping (forget about that process' data), before handing down the
event down to the process target (which hits
process_stratum_target::follow_exec).
gdb/ChangeLog:
* target.h (struct target_ops) <follow_exec>: Add ptid_t
parameter.
(target_follow_exec): Likewise.
* target.c (target_follow_exec): Add ptid_t parameter.
* infrun.c (follow_exec): Adjust call to target_follow_exec,
don't push target nor create thread.
* linux-thread-db.c (class thread_db_target) <follow_exec>: New.
(thread_db_target::wait): Just return on TARGET_WAITKIND_EXECD.
(thread_db_target::follow_exec): New.
* remote.c (class remote_target) <follow_exec>: Add ptid_t parameter.
(remote_target::follow_exec): Call
process_stratum_target::follow_exec.
* target-delegates.c: Re-generate.
Change-Id: I3f96d0ba3ea0dde6540b7e1b4d5cdb01635088c8
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
3f4d92ebdf 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.
Change int parameter to bool in remote_notice_new_inferior (remote.c)
and notice_new_inferior (infcmd.c), and update the callers.
There should be no user visible changes after this commit.
gdb/ChangeLog:
* infcmd.c (notice_new_inferior): Change parameter type.
* inferior.h (notice_new_inferior): Change parameter type.
* remote.c (remote_notice_new_inferior): Change parameter type to
bool. Also update type of local variable to bool.
(remote_target::update_thread_list): Change type of local variable
to bool.
(remote_target::process_stop_reply): Pass bool instead of int to
remote_notice_new_inferior.
Give a name to each observer, this will help produce more meaningful
debug message.
gdbsupport/ChangeLog:
* observable.h (class observable) <struct observer> <observer>:
Add name parameter.
<name>: New field.
<attach>: Add name parameter, update all callers.
Change-Id: Ie0cc4664925215b8d2b09e026011b7803549fba0
Occassionally I run into the following assert:
...
(gdb) PASS: gdb.multi/multi-target-continue.exp: inferior 5
Remote debugging from host ::1, port 49990^M
Process multi-target-continue created; pid = 31241^M
src/gdb/remote-notif.c:113: internal-error: \
void remote_async_get_pending_events_handler(gdb_client_data): \
Assertion `target_is_non_stop_p ()' failed.^M
...
The assert checks target_is_non_stop_p, which is related to the current
target.
Fix this by changing the assert such that it checks non-stopness related to
the event it's handling.
Tested on x86_64-linux.
gdb/ChangeLog:
2021-04-22 Simon Marchi <simon.marchi@polymtl.ca>
Tom de Vries <tdevries@suse.de>
PR remote/27710
* remote.c (remote_target_is_non_stop_p): New function.
* remote.h (remote_target_is_non_stop_p): Declare.
* remote-notif.c (remote_async_get_pending_events_handler): Fix assert
to check non-stopness using notif_state->remote rather current target.
