Currently you get this assertion failure if you try to execute the
inferior after loading a saved recording, when no recording was done
earlier in the same gdb session:
```
$ gdb -q c -ex "record restore test.rec"
Reading symbols from c...
[New LWP 26428]
Core was generated by `/tmp/c'.
Restored records from core file /tmp/test.rec.
(gdb) c
Continuing.
../../gdb/inferior.c:293: internal-error: inferior* find_inferior_pid(process_stratum_target*, int): Assertion `pid != 0' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
```
The change in step-precsave.exp triggers this bug, since now the
recording is loaded in a new gdb session, where
record_full_resume_ptid was never set.
The fix is to simply set record_full_resume_ptid when resuming a loaded
recording.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31971
Approved-By: Guinevere Larsen <blarsen@redhat.com>
Remove some includes reported as unused by clangd. Add some includes in
other files that were previously relying on the transitive include.
Change-Id: Ibdd0a998b04d21362a20d0ca8e5267e21e2e133e
Most files including gdbcmd.h currently rely on it to access things
actually declared in cli/cli-cmds.h (setlist, showlist, etc). To make
things easy, replace all includes of gdbcmd.h with includes of
cli/cli-cmds.h. This might lead to some unused includes of
cli/cli-cmds.h, but it's harmless, and much faster than going through
the 170 or so files by hand.
Change-Id: I11f884d4d616c12c05f395c98bbc2892950fb00f
Approved-By: Tom Tromey <tom@tromey.com>
These functions are implemented in top.c, move their declarations to
top.h.
Change-Id: I8893ef91d955156a6530734fefe8002d78c3e5fc
Approved-By: Tom Tromey <tom@tromey.com>
Move the declarations out of defs.h, and the implementations out of
findvar.c.
I opted for a new file, because this functionality of converting
integers to bytes and vice-versa seems a bit to generic to live in
findvar.c.
Change-Id: I524858fca33901ee2150c582bac16042148d2251
Approved-By: John Baldwin <jhb@FreeBSD.org>
I spotted that the two functions:
record_full_open_1
record_full_core_open_1
both took two arguments, neither of which are used.
I stumbled onto this while reviewing how filename_completer is used.
The 'record full restore' command uses filename_completer and invokes
the cmd_record_full_restore function.
The cmd_record_full_restore function calls core_file_command and then
record_full_open, which then calls one of the above functions.
As 'record full restore' takes a filename, this is passed to
cmd_record_full_restore, which forwards the filename to both
core_file_command and record_full_open. However, record_full_open
never actually uses the filename that is passed in.
The record_full_open function is also used for 'target record-full'.
I propose that record_full_open should no longer expect to see any
user supplied arguments passed in (it doesn't use any). In fact, I've
added a check that if we do get any user supplied arguments we'll
throw an error.
Now that we know record_full_open isn't being passed any user
arguments we can stop passing the arguments to record_full_open_1 and
record_full_core_open_1, this will make no user visible difference as
these arguments were not used.
It is possible that a user was previously doing:
(gdb) target record-full blah blah blah
And this previously would work fine, the 'blah blah blah' was
ignored. Now this will give an error. Other than this case there
should be no user visible changes after this commit.
Approved-By: Tom Tromey <tom@tromey.com>
Now that defs.h, server.h and common-defs.h are included via the
`-include` option, it is no longer necessary for source files to include
them. Remove all the inclusions of these files I could find. Update
the generation scripts where relevant.
Change-Id: Ia026cff269c1b7ae7386dd3619bc9bb6a5332837
Approved-By: Pedro Alves <pedro@palves.net>
The core_bfd macro hides a use of current_program_space. Remove it, so
that we have the opportunity to get the program space from the context,
if possible. I guess that the macro was introduced at some point to
replace a global variable of the same name without changing all the
uses.
Change-Id: I971a65b29b5e5a5941f3cb7ea234547daa787268
Approved-By: Tom Tromey <tom@tromey.com>
This commit is the result of the following actions:
- Running gdb/copyright.py to update all of the copyright headers to
include 2024,
- Manually updating a few files the copyright.py script told me to
update, these files had copyright headers embedded within the
file,
- Regenerating gdbsupport/Makefile.in to refresh it's copyright
date,
- Using grep to find other files that still mentioned 2023. If
these files were updated last year from 2022 to 2023 then I've
updated them this year to 2024.
I'm sure I've probably missed some dates. Feel free to fix them up as
you spot them.
When running test-case gdb.base/vfork-follow-parent.exp on powerpc64 (likewise
on s390x), I run into:
...
(gdb) PASS: gdb.base/vfork-follow-parent.exp: \
exec_file=vfork-follow-parent-exit: target-non-stop=on: non-stop=off: \
resolution_method=schedule-multiple: print unblock_parent = 1
continue^M
Continuing.^M
Reading symbols from vfork-follow-parent-exit...^M
^M
^M
Fatal signal: Segmentation fault^M
----- Backtrace -----^M
0x1027d3e7 gdb_internal_backtrace_1^M
src/gdb/bt-utils.c:122^M
0x1027d54f _Z22gdb_internal_backtracev^M
src/gdb/bt-utils.c:168^M
0x1057643f handle_fatal_signal^M
src/gdb/event-top.c:889^M
0x10576677 handle_sigsegv^M
src/gdb/event-top.c:962^M
0x3fffa7610477 ???^M
0x103f2144 for_each_block^M
src/gdb/dcache.c:199^M
0x103f235b _Z17dcache_invalidateP13dcache_struct^M
src/gdb/dcache.c:251^M
0x10bde8c7 _Z24target_dcache_invalidatev^M
src/gdb/target-dcache.c:50^M
...
or similar.
The root cause for the segmentation fault is that linux_is_uclinux gives an
incorrect result: it should always return false, given that we're running on a
regular linux system, but instead it returns first true, then false.
In more detail, the segmentation fault happens as follows:
- a program space with an address space is created
- a second program space is about to be created. maybe_new_address_space
is called, and because linux_is_uclinux returns true, maybe_new_address_space
returns false, and no new address space is created
- a second program space with the same address space is created
- a program space is deleted. Because linux_is_uclinux now returns false,
gdbarch_has_shared_address_space (current_inferior ()->arch ()) returns
false, and the address space is deleted
- when gdb uses the address space of the remaining program space, we run into
the segfault, because the address space is deleted.
Hardcoding linux_is_uclinux to false makes the test-case pass.
We leave addressing the root cause for the following commit in this series.
For now, prevent the segmentation fault by making the address space a refcounted
object.
This was already suggested here [1]:
...
A better solution might be to have the address spaces be reference counted
...
Tested on top of trunk on x86_64-linux and ppc64le-linux.
Tested on top of gdb-14-branch on ppc64-linux.
Co-Authored-By: Simon Marchi <simon.marchi@polymtl.ca>
PR gdb/30547
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30547
[1] https://sourceware.org/pipermail/gdb-patches/2023-October/202928.html
Remove get_current_regcache, inlining the call to get_thread_regcache in
callers. When possible, pass the right thread_info object known from
the local context. Otherwise, fall back to passing `inferior_thread ()`.
This makes the reference to global context bubble up one level, a small
step towards the long term goal of reducing the number of references to
global context (or rather, moving those references as close as possible
to the top of the call tree).
No behavior change expected.
Change-Id: Ifa6980c88825d803ea586546b6b4c633c33be8d6
While looking at the regcache code, I noticed that the address space
(passed to regcache when constructing it, and available through
regcache::aspace) wasn't relevant for the regcache itself. Callers of
regcache::aspace use that method because it appears to be a convenient
way of getting the address space for a thread, if you already have the
regcache. But there is always another way to get the address space, as
the callers pretty much always know which thread they are dealing with.
The regcache code itself doesn't use the address space.
This patch removes anything related to address_space from the regcache
code, and updates callers to get it from the thread in context. This
removes a bit of unnecessary complexity from the regcache code.
The current get_thread_arch_regcache function gets an address_space for
the given thread using the target_thread_address_space function (which
calls the target_ops::thread_address_space method). This suggest that
there might have been the intention of supporting per-thread address
spaces. But digging through the history, I did not find any such case.
Maybe this method was just added because we needed a way to get an
address space from a ptid (because constructing a regcache required an
address space), and this seemed like the right way to do it, I don't
know.
The only implementations of thread_address_space and
process_stratum_target::thread_address_space and
linux_nat_target::thread_address_space, which essentially just return
the inferior's address space. And thread_address_space is only used in
the current get_thread_arch_regcache, which gets removed. So, I think
that the thread_address_space target method can be removed, and we can
assume that it's fine to use the inferior's address space everywhere.
Callers of regcache::aspace are updated to get the address space from
the relevant inferior, either using some context they already know
about, or in last resort using the current global context.
So, to summarize:
- remove everything in regcache related to address spaces
- in particular, remove get_thread_arch_regcache, and rename
get_thread_arch_aspace_regcache to get_thread_arch_regcache
- remove target_ops::thread_address_space, and
target_thread_address_space
- adjust all users of regcache::aspace to get the address space another
way
Change-Id: I04fd41b22c83fe486522af7851c75bcfb31c88c7
Remove this typedef. I think that hiding the real type (std::vector)
behind a typedef just hinders readability.
Change-Id: I80949da3392f60a2826c56c268e0ec6f503ad79f
Approved-By: Pedro Alves <pedro@palves.net>
Reviewed-By: Reviewed-By: Lancelot Six <lancelot.six@amd.com>
This function is just a wrapper around the current inferior's gdbarch.
I find that having that wrapper just obscures where the arch is coming
from, and that it's often used as "I don't know which arch to use so
I'll use this magical target_gdbarch function that gets me an arch" when
the arch should in fact come from something in the context (a thread,
objfile, symbol, etc). I think that removing it and inlining
`current_inferior ()->arch ()` everywhere will make it a bit clearer
where that arch comes from and will trigger people into reflecting
whether this is the right place to get the arch or not.
Change-Id: I79f14b4e4934c88f91ca3a3155f5fc3ea2fadf6b
Reviewed-By: John Baldwin <jhb@FreeBSD.org>
Approved-By: Andrew Burgess <aburgess@redhat.com>
I see these failures, when running with the native-gdbserver of
native-extended-gdbserver boards:
Running /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.reverse/finish-reverse-next.exp ...
FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 LEP from function body
FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b = 5, from function body
FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 GEP call from function body
FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b = 50 from function body
Let's use this simpler program to illustrate the problem:
int main()
{
int a = 362;
a = a * 17;
return a;
}
It compiles down to:
int a = 362;
401689: c7 45 fc 6a 01 00 00 movl $0x16a,-0x4(%rbp)
a = a * 17;
401690: 8b 55 fc mov -0x4(%rbp),%edx
401693: 89 d0 mov %edx,%eax
401695: c1 e0 04 shl $0x4,%eax
401698: 01 d0 add %edx,%eax
40169a: 89 45 fc mov %eax,-0x4(%rbp)
return a;
40169d: 8b 45 fc mov -0x4(%rbp),%eax
When single stepping these lines, debugging locally, while recording,
these are the recorded instructions (basically one for each instruction
shown above):
(gdb) maintenance print record-instruction 0
4 bytes of memory at address 0x00007fffffffdc5c changed from: 6a 01 00 00
Register rip changed: (void (*)()) 0x40169a <main+21>
(gdb) maintenance print record-instruction -1
Register rax changed: 5792
Register eflags changed: [ PF AF IF ]
Register rip changed: (void (*)()) 0x401698 <main+19>
(gdb) maintenance print record-instruction -2
Register rax changed: 362
Register eflags changed: [ PF ZF IF ]
Register rip changed: (void (*)()) 0x401695 <main+16>
(gdb) maintenance print record-instruction -3
Register rax changed: 4200069
Register rip changed: (void (*)()) 0x401693 <main+14>
(gdb) maintenance print record-instruction -4
Register rdx changed: 140737488346696
Register rip changed: (void (*)()) 0x401690 <main+11>
(gdb) maintenance print record-instruction -5
4 bytes of memory at address 0x00007fffffffdc5c changed from: 00 00 00 00
Register rip changed: (void (*)()) 0x401689 <main+4>
(gdb) maintenance print record-instruction -6
Not enough recorded history
But when debugging remotely:
(gdb) maintenance print record-instruction 0
Register rdx changed: 140737488346728
Register rip changed: (void (*)()) 0x401690 <main+11>
(gdb) maintenance print record-instruction -1
4 bytes of memory at address 0x00007fffffffdc7c changed from: 00 00 00 00
Register rip changed: (void (*)()) 0x401689 <main+4>
(gdb) maintenance print record-instruction -2
Not enough recorded history
In this list, we only have entries for the beginning of each line. This
is because of the remote target's support for range stepping. The
record-full layer can only record instructions when the underlying
process target reports a stop. With range stepping, the remote target
single-steps multiple instructions at a time, so the record-full target
doesn't get to see and record them all.
Fix this by making the record-full layer disable range-stepping
before handing the resume request to the beneath layer, forcing the
remote target to report stops for each instruction.
Change-Id: Ia95ea62720bbcd0b6536a904360ffbf839eb823d
As described in the previous commit for this series, I became
concerned that there might be instances in which a QUIT (due to either
a SIGINT or SIGTERM) might not cause execution to return to the top
level. In some (though very few) instances, it is okay to not
propagate the exception for a Ctrl-C / SIGINT, but I don't think that
it is ever okay to swallow the exception caused by a SIGTERM.
Allowing that to happen would definitely be a deviation from the
current behavior in which GDB exits upon receipt of a SIGTERM.
I looked at all cases where an exception handler catches a
gdb_exception. Handlers which did NOT need modification were those
which satisifed one or more of the following conditions:
1) There is no call path to maybe_quit() in the try block. I used a
static analysis tool to help make this determination. In
instances where the tool didn't provide an answer of "yes, this
call path can result in maybe_quit() being called", I reviewed it
by hand.
2) The catch block contains a throw for conditions that it
doesn't want to handle; these "not handled" conditions
must include the quit exception and the new "forced quit" exception.
3) There was (also) a catch for gdb_exception_quit.
Any try/catch blocks not meeting the above conditions could
potentially swallow a QUIT exception.
My first thought was to add catch blocks for gdb_exception_quit and
then rethrow the exception. But Pedro pointed out that this can be
handled without adding additional code by simply catching
gdb_exception_error instead. That's what this patch series does.
There are some oddball cases which needed to be handled differently,
plus the extension languages, but those are handled in later patches.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=26761
Tested-by: Tom de Vries <tdevries@suse.de>
Approved-by: Pedro Alves <pedro@palves.net>
While chasing some reverse debugging bugs, I found myself wondering what
was recorded by GDB to undo and redo a certain instruction. This commit
implements a simple way of printing that information.
If there isn't enough history to print the desired instruction (such as
when the user hasn't started recording yet or when they request 2
instructions back but only 1 was recorded), GDB warns the user like so:
(gdb) maint print record-instruction
Not enough recorded history
If there is enough, GDB prints the instruction like so:
(gdb) maint print record-instruction
4 bytes of memory at address 0x00007fffffffd5dc changed from: 01 00 00 00
Register eflags changed: [ IF ]
Register rip changed: (void (*)()) 0x401115 <main+15>
Approved-by: Eli Zaretskii <eliz@gnu.org>
Reviewed-by: Alexandra Hajkova <ahajkova@redhat.com>
Reviewed-by: Lancelot Six <lsix@lancelotsix.com>
Approved-by: Tom Tromey <tom@tromey.com>
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.
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.
Now that target_resume always enables async mode after target::resume
returns, these calls are redundant.
The other place that target resume methods are invoked outside of
target_resume are as the beneath target in record_full_wait_1. In
this case, async mode should already be enabled when supported by the
target before the resume method is invoked due to the following:
In general, targets which support async mode run as async until
::wait returns TARGET_WAITKIND_NO_RESUMED to indicate that there are
no unwaited for children (either they have exited or are stopped).
When that occurs, the loop in wait_one disables async mode. Later
if a stopped child is resumed, async mode is re-enabled in
do_target_resume before waiting for the next event.
In the case of record_full_wait_1, this function is invoked from the
::wait target method when fetching an event. If the underlying
target supports async mode, then an earlier call to do_target_resume
to resume the child reporting an event in the loop in
record_full_wait_1 would have already enabled async mode before
::wait was invoked. In addition, nothing in the code executed in
the loop in record_full_wait_1 disables async mode. Async mode is
only disabled higher in the call stack in wait_one after ::wait
returns.
It is also true that async mode can be disabled by an
INF_EXEC_COMPLETE event passed to inferior_event_handle, but all of
the places that invoke that are in the gdb core which is "above" a
target ::wait method.
Note that there is an earlier call to enable async mode in
linux_nat_target::resume. That call also marks the async event pipe
to report an existing event after enabling async mode, so it needs to
stay.
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.
While working on another patch relating to how GDB manages threads
executing and resumed state, I spotted the following code in
record-btrace.c:
executing = tp->executing ();
set_executing (proc_target, inferior_ptid, false);
id = null_frame_id;
try
{
id = get_frame_id (get_current_frame ());
}
catch (const gdb_exception &except)
{
/* Restore the previous execution state. */
set_executing (proc_target, inferior_ptid, executing);
throw;
}
/* Restore the previous execution state. */
set_executing (proc_target, inferior_ptid, executing);
return id;
I notice that we only catch the exception so we can call
set_executing, and this is the same call to set_executing that we need
to perform in the non-exception return path.
This would be much cleaner if we could use SCOPE_EXIT to avoid the
try/catch, so lets do that.
While cleaning this up, I also applied a similar patch to
record-full.c, though there's no try/catch in that case, but using
SCOPE_EXIT makes the code safe if, in the future, we do start throwing
exceptions.
There should be no user visible changes after this commit.
There's a common pattern to call add_basic_prefix_cmd and
add_show_prefix_cmd to add matching set and show commands. Add the
add_setshow_prefix_cmd function to factor that out and use it at a few
places.
Change-Id: I6e9e90a30e9efb7b255bf839cac27b85d7069cfd
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
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
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
Remove it, change users (well, a single one) to use all_bp_locations.
This requires moving all_bp_locations to breakpoint.h to expose it.
gdb/ChangeLog:
* breakpoint.h (iterate_over_bp_locations): Remove. Update
users to use all_bp_locations.
(all_bp_locations): New.
* breakpoint.c (all_bp_locations): Make non-static.
(iterate_over_bp_locations): Remove.
Change-Id: Iaf1f716d6c2c5b2975579b3dc113a86f5d0975be
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
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.
Use a function_view instead of function pointer + data. Actually,
nothing uses the data anyway, but that makes iterate_over_bp_locations
more like iterate_over_breakpoints.
gdb/ChangeLog:
* breakpoint.c (iterate_over_bp_locations): Change callback to
function view, remove data parameter.
* breakpoint.h (iterate_over_bp_locations): Likewise.
* record-full.c (record_full_sync_record_breakpoints): Remove
data parameter.
Change-Id: I66cdc94a505f67bc640bcc66865fb535ee939a57
The rationale for this patch comes from the ROCm port [1], the goal
being to reduce the number of back and forths between GDB and the
target when doing successive operations. I'll start with explaining
the rationale and then go over the implementation. In the ROCm / GPU
world, the term "wave" is somewhat equivalent to a "thread" in GDB.
So if you read if from a GPU stand point, just s/thread/wave/.
ROCdbgapi, the library used by GDB [2] to communicate with the GPU
target, gives the illusion that it's possible for the debugger to
control (start and stop) individual threads. But in reality, this is
not how it works. Under the hood, all threads of a queue are
controlled as a group. To stop one thread in a group of running ones,
the state of all threads is retrieved from the GPU, all threads are
destroyed, and all threads but the one we want to stop are re-created
from the saved state. The net result, from the point of view of GDB,
is that the library stopped one thread. The same thing goes if we
want to resume one thread while others are running: the state of all
running threads is retrieved from the GPU, they are all destroyed, and
they are all re-created, including the thread we want to resume.
This leads to some inefficiencies when combined with how GDB works,
here are two examples:
- Stopping all threads: because the target operates in non-stop mode,
when the user interface mode is all-stop, GDB must stop all threads
individually when presenting a stop. Let's suppose we have 1000
threads and the user does ^C. GDB asks the target to stop one
thread. Behind the scenes, the library retrieves 1000 thread
states and restores the 999 others still running ones. GDB asks
the target to stop another one. The target retrieves 999 thread
states and restores the 998 remaining ones. That means that to
stop 1000 threads, we did 1000 back and forths with the GPU. It
would have been much better to just retrieve the states once and
stop there.
- Resuming with pending events: suppose the 1000 threads hit a
breakpoint at the same time. The breakpoint is conditional and
evaluates to true for the first thread, to false for all others.
GDB pulls one event (for the first thread) from the target, decides
that it should present a stop, so stops all threads using
stop_all_threads. All these other threads have a breakpoint event
to report, which is saved in `thread_info::suspend::waitstatus` for
later. When the user does "continue", GDB resumes that one thread
that did hit the breakpoint. It then processes the pending events
one by one as if they just arrived. It picks one, evaluates the
condition to false, and resumes the thread. It picks another one,
evaluates the condition to false, and resumes the thread. And so
on. In between each resumption, there is a full state retrieval
and re-creation. It would be much nicer if we could wait a little
bit before sending those threads on the GPU, until it processed all
those pending events.
To address this kind of performance issue, ROCdbgapi has a concept
called "forward progress required", which is a boolean state that
allows its user (i.e. GDB) to say "I'm doing a bunch of operations,
you can hold off putting the threads on the GPU until I'm done" (the
"forward progress not required" state). Turning forward progress back
on indicates to the library that all threads that are supposed to be
running should now be really running on the GPU.
It turns out that GDB has a similar concept, though not as general,
commit_resume. One difference is that commit_resume is not stateful:
the target can't look up "does the core need me to schedule resumed
threads for execution right now". It is also specifically linked to
the resume method, it is not used in other contexts. The target
accumulates resumption requests through target_ops::resume calls, and
then commits those resumptions when target_ops::commit_resume is
called. The target has no way to check if it's ok to leave resumed
threads stopped in other target methods.
To bridge the gap, this patch generalizes the commit_resume concept in
GDB to match the forward progress concept of ROCdbgapi. The current
name (commit_resume) can be interpreted as "commit the previous resume
calls". I renamed the concept to "commit_resumed", as in "commit the
threads that are resumed".
In the new version, we have two things:
- the commit_resumed_state field in process_stratum_target: indicates
whether GDB requires target stacks using this target to have
resumed threads committed to the execution target/device. If
false, an execution target is allowed to leave resumed threads
un-committed at the end of whatever method it is executing.
- the commit_resumed target method: called when commit_resumed_state
transitions from false to true. While commit_resumed_state was
false, the target may have left some resumed threads un-committed.
This method being called tells it that it should commit them back
to the execution device.
Let's take the "Stopping all threads" scenario from above and see how
it would work with the ROCm target with this change. Before stopping
all threads, GDB would set the target's commit_resumed_state field to
false. It would then ask the target to stop the first thread. The
target would retrieve all threads' state from the GPU and mark that
one as stopped. Since commit_resumed_state is false, it leaves all
the other threads (still resumed) stopped. GDB would then proceed to
call target_stop for all the other threads. Since resumed threads are
not committed, this doesn't do any back and forth with the GPU.
To simplify the implementation of targets, this patch makes it so that
when calling certain target methods, the contract between the core and
the targets guarantees that commit_resumed_state is false. This way,
the target doesn't need two paths, one for commit_resumed_state ==
true and one for commit_resumed_state == false. It can just assert
that commit_resumed_state is false and work with that assumption.
This also helps catch places where we forgot to disable
commit_resumed_state before calling the method, which represents a
probable optimization opportunity. The commit adds assertions in the
target method wrappers (target_resume and friends) to have some
confidence that this contract between the core and the targets is
respected.
The scoped_disable_commit_resumed type is used to disable the commit
resumed state of all process targets on construction, and selectively
re-enable it on destruction (see below for criteria). Note that it
only sets the process_stratum_target::commit_resumed_state flag. A
subsequent call to maybe_call_commit_resumed_all_targets is necessary
to call the commit_resumed method on all target stacks with process
targets that got their commit_resumed_state flag turned back on. This
separation is because we don't want to call the commit_resumed methods
in scoped_disable_commit_resumed's destructor, as they may throw.
On destruction, commit-resumed is not re-enabled for a given target
if:
1. this target has no threads resumed, or
2. this target has at least one resumed thread with a pending status
known to the core (saved in thread_info::suspend::waitstatus).
The first point is not technically necessary, because a proper
commit_resumed implementation would be a no-op if the target has no
resumed threads. But since we have a flag do to a quick check, it
shouldn't hurt.
The second point is more important: together with the
scoped_disable_commit_resumed instance added in fetch_inferior_event,
it makes it so the "Resuming with pending events" described above is
handled efficiently. Here's what happens in that case:
1. The user types "continue".
2. Upon destruction, the scoped_disable_commit_resumed in the
`proceed` function does not enable commit-resumed, as it sees some
threads have pending statuses.
3. fetch_inferior_event is called to handle another event, the
breakpoint hit evaluates to false, and that thread is resumed.
Because there are still more threads with pending statuses, the
destructor of scoped_disable_commit_resumed in
fetch_inferior_event still doesn't enable commit-resumed.
4. Rinse and repeat step 3, until the last pending status is handled
by fetch_inferior_event. In that case,
scoped_disable_commit_resumed's destructor sees there are no more
threads with pending statues, so it asks the target to commit
resumed threads.
This allows us to avoid all unnecessary back and forths, there is a
single commit_resumed call once all pending statuses are processed.
This change required remote_target::remote_stop_ns to learn how to
handle stopping threads that were resumed but pending vCont. The
simplest example where that happens is when using the remote target in
all-stop, but with "maint set target-non-stop on", to force it to
operate in non-stop mode under the hood. If two threads hit a
breakpoint at the same time, GDB will receive two stop replies. It
will present the stop for one thread and save the other one in
thread_info::suspend::waitstatus.
Before this patch, when doing "continue", GDB first resumes the thread
without a pending status:
Sending packet: $vCont;c:p172651.172676#f3
It then consumes the pending status in the next fetch_inferior_event
call:
[infrun] do_target_wait_1: Using pending wait status status->kind = stopped, signal = GDB_SIGNAL_TRAP for Thread 1517137.1517137.
[infrun] target_wait (-1.0.0, status) =
[infrun] 1517137.1517137.0 [Thread 1517137.1517137],
[infrun] status->kind = stopped, signal = GDB_SIGNAL_TRAP
It then realizes it needs to stop all threads to present the stop, so
stops the thread it just resumed:
[infrun] stop_all_threads: Thread 1517137.1517137 not executing
[infrun] stop_all_threads: Thread 1517137.1517174 executing, need stop
remote_stop called
Sending packet: $vCont;t:p172651.172676#04
This is an unnecessary resume/stop. With this patch, we don't commit
resumed threads after proceeding, because of the pending status:
[infrun] maybe_commit_resumed_all_process_targets: not requesting commit-resumed for target extended-remote, a thread has a pending waitstatus
When GDB handles the pending status and stop_all_threads runs, we stop a
resumed but pending vCont thread:
remote_stop_ns: Enqueueing phony stop reply for thread pending vCont-resume (1520940, 1520976, 0)
That thread was never actually resumed on the remote stub / gdbserver,
so we shouldn't send a packet to the remote side asking to stop the
thread.
Note that there are paths that resume the target and then do a
synchronous blocking wait, in sort of nested event loop, via
wait_sync_command_done. For example, inferior function calls, or any
run control command issued from a breakpoint command list. We handle
that making wait_sync_command_one a "sync" point -- force forward
progress, or IOW, force-enable commit-resumed state.
gdb/ChangeLog:
yyyy-mm-dd Simon Marchi <simon.marchi@efficios.com>
Pedro Alves <pedro@palves.net>
* infcmd.c (run_command_1, attach_command, detach_command)
(interrupt_target_1): Use scoped_disable_commit_resumed.
* infrun.c (do_target_resume): Remove
target_commit_resume call.
(commit_resume_all_targets): Remove.
(maybe_set_commit_resumed_all_targets): New.
(maybe_call_commit_resumed_all_targets): New.
(enable_commit_resumed): New.
(scoped_disable_commit_resumed::scoped_disable_commit_resumed)
(scoped_disable_commit_resumed::~scoped_disable_commit_resumed)
(scoped_disable_commit_resumed::reset)
(scoped_disable_commit_resumed::reset_and_commit)
(scoped_enable_commit_resumed::scoped_enable_commit_resumed)
(scoped_enable_commit_resumed::~scoped_enable_commit_resumed):
New.
(proceed): Use scoped_disable_commit_resumed and
maybe_call_commit_resumed_all_targets.
(fetch_inferior_event): Use scoped_disable_commit_resumed.
* infrun.h (struct scoped_disable_commit_resumed): New.
(maybe_call_commit_resumed_all_process_targets): New.
(struct scoped_enable_commit_resumed): New.
* mi/mi-main.c (exec_continue): Use scoped_disable_commit_resumed.
* process-stratum-target.h (class process_stratum_target):
<commit_resumed_state>: New.
* record-full.c (record_full_wait_1): Change commit_resumed_state
around calling commit_resumed.
* remote.c (class remote_target) <commit_resume>: Rename to...
<commit_resumed>: ... this.
(struct stop_reply): Move up.
(remote_target::commit_resume): Rename to...
(remote_target::commit_resumed): ... this. Check if there is any
thread pending vCont resume.
(remote_target::remote_stop_ns): Generate stop replies for resumed
but pending vCont threads.
(remote_target::wait_ns): Add gdb_assert.
* target-delegates.c: Regenerate.
* target.c (target_wait, target_resume): Assert that the current
process_stratum target isn't in commit-resumed state.
(defer_target_commit_resume): Remove.
(target_commit_resume): Remove.
(target_commit_resumed): New.
(make_scoped_defer_target_commit_resume): Remove.
(target_stop): Assert that the current process_stratum target
isn't in commit-resumed state.
* target.h (struct target_ops) <commit_resume>: Rename to ...
<commit_resumed>: ... this.
(target_commit_resume): Remove.
(target_commit_resumed): New.
(make_scoped_defer_target_commit_resume): Remove.
* top.c (wait_sync_command_done): Use
scoped_enable_commit_resumed.
[1] https://github.com/ROCm-Developer-Tools/ROCgdb/
[2] https://github.com/ROCm-Developer-Tools/ROCdbgapi
Change-Id: I836135531a29214b21695736deb0a81acf8cf566
Same as the previous patch, but for the push_target functions.
The implementation of the move variant is moved to a new overload of
inferior::push_target.
gdb/ChangeLog:
* target.h (push_target): Remove, update callers to use
inferior::push_target.
* target.c (push_target): Remove.
* inferior.h (class inferior) <push_target>: New overload.
Change-Id: I5a95496666278b8f3965e5e8aecb76f54a97c185
unpush_target unpushes the passed-in target from the current inferior's
target stack. Calling it is therefore an implicit dependency on the
current global inferior. Remove that function and make the callers use
the inferior::unpush_target method directly. This sometimes allows
using the inferior from the context rather than the global current
inferior.
target_unpusher::operator() now needs to be implemented in target.c,
otherwise target.h and inferior.h both need to include each other, and
that wouldn't work.
gdb/ChangeLog:
* target.h (unpush_target): Remove, update all callers
to use `inferior::unpush_target` instead.
(struct target_unpusher) <operator()>: Just declare.
* target.c (unpush_target): Remove.
(target_unpusher::operator()): New.
Change-Id: Ia5172dfb3f373e0a75b991885b50322ca2142a8c
For the same reason explained in the previous patch (which was for the
record-btrace target), move clearing of the async event handler of the
record-full target to the wait method.
I'm not sure if/where that target needs to re-set its async event
handler in the wait method. Since it only supports a single thread,
there probably can't be multiple events to report at the same time.
gdb/ChangeLog:
* record-full.c (record_full_async_inferior_event_handler):
Don't clear async event handler.
(record_full_base_target::wait): Clear async event handler at
beginning.
Change-Id: I146fbdb53d99e3a32766ac7cd337ac5ed7fd9adf
The `ready` flag of async event handlers is cleared by the async event
handler system right before invoking the associated callback, in
check_async_event_handlers.
This is not ideal with how the infrun subsystem consumes events: all
targets' async event handler callbacks essentially just invoke
`inferior_event_handler`, which eventually calls `fetch_inferior_event`
and `do_target_wait`. `do_target_wait` picks an inferior at random,
and thus a target at random (it could be the target whose `ready` flag
was cleared, or not), and pulls one event from it.
So it's possible that:
- the async event handler for a target A is called
- we end up consuming an event for target B
- all threads of target B are stopped, target_async(0) is called on it,
so its async event handler is cleared (e.g.
record_btrace_target::async)
As a result, target A still has events to report while its async event
handler is left unmarked, so these events are not consumed. To counter
this, at the end of their async event handler callbacks, targets check
if they still have something to report and re-mark their async event
handler (e.g. remote_async_inferior_event_handler).
The linux_nat target does not suffer from this because it doesn't use an
async event handler at the moment. It only uses a pipe registered with
the event loop. It is written to in the SIGCHLD handler (and in other
spots that want to get target wait method called) and read from in
the target's wait method. So if linux_nat happened to be target A in
the example above, the pipe would just stay readable, and the event loop
would wake up again, until linux_nat's wait method is finally called and
consumes the contents of the pipe.
I think it would be nicer if targets using async_event_handler worked in
a similar way, where the flag would stay set until the target's wait
method is actually called. As a first step towards that, this patch
moves the responsibility of clearing the ready flags of async event
handlers to the invoked callback.
All async event handler callbacks are modified to clear their ready flag
before doing anything else. So in practice, nothing changes with this
patch. It's only the responsibility of clearing the flag that is
shifted toward the callee.
gdb/ChangeLog:
* async-event.h (async_event_handler_func): Add documentation.
* async-event.c (check_async_event_handlers): Don't clear
async_event_handler ready flag.
* infrun.c (infrun_async_inferior_event_handler): Clear ready
flag.
* record-btrace.c (record_btrace_handle_async_inferior_event):
Likewise.
* record-full.c (record_full_async_inferior_event_handler):
Likewise.
* remote-notif.c (remote_async_get_pending_events_handler):
Likewise.
* remote.c (remote_async_inferior_event_handler): Likewise.
Change-Id: I179ef8e99580eae642d332846fd13664dbddc0c1
The previous patch made the commit_resume implementations in the record
targets unnecessary, as the remote target's commit_resume implementation
won't commit-resume threads for which it didn't see a resume. This
patch removes them.
gdb/ChangeLog:
* record-btrace.c (class record_btrace_target): Remove.
(record_btrace_target::commit_resume): Remove.
* record-full.c (class record_full_target): Remove.
(record_full_target::commit_resume): Remove.
Change-Id: I3a68d3d726fb09d8b7165b4edefc330d27803b27
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.
Because target_section_table only holds a vector, and because it is
used in an "open" way, this patch makes it just be an alias for the
std::vector specialization. This makes the code less wordy. If we do
ever want to add more specialized behavior to this type, it's simple
enough to convert it back to a struct with the few needed methods
implied by this change.
gdb/ChangeLog
2020-10-12 Tom Tromey <tom@tromey.com>
* target.h (struct target_ops) <get_section_table>: Update.
(target_get_section_table): Update.
* target.c (target_get_section_table, target_section_by_addr)
(memory_xfer_partial_1): Update.
* target-section.h (target_section_table): Now an alias.
* target-delegates.c: Rebuild.
* target-debug.h (target_debug_print_target_section_table_p):
Rename from target_debug_print_struct_target_section_table_p.
* symfile.c (build_section_addr_info_from_section_table): Update.
* solib.c (solib_map_sections, solib_contains_address_p): Update.
* solib-svr4.c (scan_dyntag): Update.
* solib-dsbt.c (scan_dyntag): Update.
* remote.c (remote_target::remote_xfer_live_readonly_partial):
Update.
* record-full.c (record_full_core_target::xfer_partial): Update.
* progspace.h (struct program_space) <target_sections>: Update.
* exec.h (print_section_info): Update.
* exec.c (exec_target::close, build_section_table)
(add_target_sections, add_target_sections_of_objfile)
(remove_target_sections, exec_on_vfork)
(section_table_available_memory)
(section_table_xfer_memory_partial)
(exec_target::get_section_table, exec_target::xfer_partial)
(print_section_info, set_section_command)
(exec_set_section_address, exec_target::has_memory): Update.
* corelow.c (core_target::build_file_mappings)
(core_target::xfer_partial, core_target::info_proc_mappings)
(core_target::info_proc_mappings): Update.
* bfd-target.c (class target_bfd): Update
I noticed that build_section_table cannot fail. This patch changes it
to return a target_section_table and then removes the dead code.
gdb/ChangeLog
2020-10-12 Tom Tromey <tom@tromey.com>
* solib.c (solib_map_sections): Update.
* record-full.c (record_full_core_open_1): Update.
* exec.h (build_section_table): Return a target_section_table.
* exec.c (exec_file_attach): Update.
(build_section_table): Return a target_section_table.
* corelow.c (core_target::core_target): Update.
* bfd-target.c (target_bfd::target_bfd): Update.
This changes target_section_table to wrap a std::vector. This
simplifies some code, and also enables the simplifications coming in
the subsequent patches.
Note that for solib, I chose to have it use a pointer to a
target_section_table. This is more convoluted than would be ideal,
but I didn't want to convert solib to new/delete as a prerequisite for
this series.
gdb/ChangeLog
2020-10-12 Tom Tromey <tom@tromey.com>
* target.c (target_section_by_addr, memory_xfer_partial_1):
Update.
* target-section.h (struct target_section_table): Use
std::vector.
* symfile.h (build_section_addr_info_from_section_table): Take a
target_section_table.
* symfile.c (build_section_addr_info_from_section_table): Take a
target_section_table.
* solist.h (struct so_list) <sections>: Change type.
<sections_end>: Remove.
* solib.c (solib_map_sections, clear_so, solib_read_symbols)
(solib_contains_address_p): Update.
* solib-svr4.c (scan_dyntag): Update.
* solib-dsbt.c (scan_dyntag): Update.
* remote.c (remote_target::remote_xfer_live_readonly_partial):
Update.
* record-full.c (record_full_core_start, record_full_core_end):
Remove.
(record_full_core_sections): New global.
(record_full_core_open_1, record_full_core_target::xfer_partial):
Update.
* exec.h (build_section_table, section_table_xfer_memory_partial)
(add_target_sections): Take a target_section_table.
* exec.c (exec_file_attach, clear_section_table): Update.
(resize_section_table): Remove.
(build_section_table, add_target_sections): Take a
target_section_table.
(add_target_sections_of_objfile, remove_target_sections)
(exec_on_vfork): Update.
(section_table_available_memory): Take a target_section_table.
(section_table_read_available_memory): Update.
(section_table_xfer_memory_partial): Take a target_section_table.
(print_section_info, set_section_command)
(exec_set_section_address, exec_target::has_memory): Update.
* corelow.c (class core_target) <m_core_section_table,
m_core_file_mappings>: Remove braces.
<~core_target>: Remove.
(core_target::core_target): Update.
(core_target::~core_target): Remove.
(core_target::build_file_mappings)
(core_target::xfer_memory_via_mappings)
(core_target::xfer_partial, core_target::info_proc_mappings):
Update.
* bfd-target.c (target_bfd::xfer_partial): Update.
(target_bfd::target_bfd): Update.
(target_bfd::~target_bfd): Remove.
Assign names to async event/signal handlers. They will be used in debug
messages when file handlers are invoked.
Unlike in the previous patch, the names are not copied in the structure,
since we don't need to (all names are string literals for the moment).
gdb/ChangeLog:
* async-event.h (create_async_signal_handler): Add name
parameter.
(create_async_event_handler): Likewise.
* async-event.c (struct async_signal_handler) <name>: New field.
(struct async_event_handler) <name>: New field.
(create_async_signal_handler): Assign name.
(create_async_event_handler): Assign name.
* event-top.c (async_init_signals): Pass name when creating
handler.
* infrun.c (_initialize_infrun): Likewise.
* record-btrace.c (record_btrace_push_target): Likewise.
* record-full.c (record_full_open): Likewise.
* remote-notif.c (remote_notif_state_allocate): Likewise.
* remote.c (remote_target::open_1): Likewise.
* tui/tui-win.c (tui_initialize_win): Likewise.
Change-Id: Icd9d9f775542ae5fc2cd148c12f481e7885936d5
I noticed that fetch_inferior_event receives the client_data parameter
from its caller, inferior_event_handler, but doesn't actually need it.
This patch removes it. In turn, inferior_event_handler doesn't use its
parameter, so remove it too.
The `data` argument used when registering
remote_async_inferior_event_handler is changed to NULL, to avoid
confusion. It could make people think that the value passed is used
somewhere, when in fact it's not.
gdb/ChangeLog:
* inf-loop.c (inferior_event_handler): Remove client_data param.
* inf-loop.h (inferior_event_handler): Likewise.
* infcmd.c (step_1): Adjust.
* infrun.c (proceed): Adjust.
(fetch_inferior_event): Remove client_data param.
(infrun_async_inferior_event_handler): Adjust.
* infrun.h (fetch_inferior_event): Remove `void *` param.
* linux-nat.c (handle_target_event): Adjust.
* record-btrace.c (record_btrace_handle_async_inferior_event):
Adjust.
* record-full.c (record_full_async_inferior_event_handler):
Adjust.
* remote.c (remote_async_inferior_event_handler): Adjust.
Change-Id: I3c2aa1eb0ea3e0985df096660d2dcd794674f2ea
Currently there are many prefix commands that do nothing but call
either help_list or cmd_show_list. I happened to notice that one such
call, for "set print type", used the wrong command list parameter,
causing incorrect output.
Rather than fix this bug in isolation, I decided to eliminate this
possibility by adding two new ways to add prefix commands, which
simply route the call to help_list or cmd_show_list, as appropriate.
This makes it impossible for a mismatch to occur.
In some cases, a bit of output was removed; however, I don't think
this output in general was very useful. It seemed redundant with
what's already printed by help_list. A representative example is this
hunk, removed from ada-lang.c:
- printf_unfiltered (_(\
-"\"set ada\" must be followed by the name of a setting.\n"));
This simplified the CLI style set/show commands quite a bit, and
allowed the deletion of a macro.
This also cleans up some unusual code in windows-tdep.c.
Tested on x86-64 Fedora 30. Note that I have no way to build the
go32-nat.c change.
gdb/ChangeLog
2020-04-17 Tom Tromey <tromey@adacore.com>
* auto-load.c (show_auto_load_cmd): Remove.
(auto_load_show_cmdlist_get): Use add_show_prefix_cmd.
* arc-tdep.c (_initialize_arc_tdep): Use add_show_prefix_cmd.
(maintenance_print_arc_command): Remove.
* tui/tui-win.c (tui_command): Remove.
(tui_get_cmd_list): Use add_basic_prefix_cmd.
* tui/tui-layout.c (tui_layout_command): Remove.
(_initialize_tui_layout): Use add_basic_prefix_cmd.
* python/python.c (user_set_python, user_show_python): Remove.
(_initialize_python): Use add_basic_prefix_cmd,
add_show_prefix_cmd.
* guile/guile.c (set_guile_command, show_guile_command): Remove.
(install_gdb_commands): Use add_basic_prefix_cmd,
add_show_prefix_cmd.
(info_guile_command): Remove.
* dwarf2/read.c (set_dwarf_cmd, show_dwarf_cmd): Remove.
(_initialize_dwarf2_read): Use add_basic_prefix_cmd,
add_show_prefix_cmd.
* cli/cli-style.h (class cli_style_option) <add_setshow_commands>:
Remove do_set and do_show parameters.
* cli/cli-style.c (set_style, show_style): Remove.
(_initialize_cli_style): Use add_basic_prefix_cmd,
add_show_prefix_cmd.
(cli_style_option::add_setshow_commands): Remove do_set and
do_show parameters.
(cli_style_option::add_setshow_commands): Use
add_basic_prefix_cmd, add_show_prefix_cmd.
(STYLE_ADD_SETSHOW_COMMANDS): Remove macro.
(set_style_name): Remove.
* cli/cli-dump.c (dump_command, append_command): Remove.
(srec_dump_command, ihex_dump_command, verilog_dump_command)
(tekhex_dump_command, binary_dump_command)
(binary_append_command): Remove.
(_initialize_cli_dump): Use add_basic_prefix_cmd.
* windows-tdep.c (w32_prefix_command_valid): Remove global.
(init_w32_command_list): Remove; move into ...
(_initialize_windows_tdep): ... here. Use add_basic_prefix_cmd.
* valprint.c (set_print, show_print, set_print_raw)
(show_print_raw): Remove.
(_initialize_valprint): Use add_basic_prefix_cmd,
add_show_prefix_cmd.
* typeprint.c (set_print_type, show_print_type): Remove.
(_initialize_typeprint): Use add_basic_prefix_cmd,
add_show_prefix_cmd.
* record.c (set_record_command, show_record_command): Remove.
(_initialize_record): Use add_basic_prefix_cmd,
add_show_prefix_cmd.
* cli/cli-cmds.c (_initialize_cli_cmds): Use add_basic_prefix_cmd,
add_show_prefix_cmd.
(info_command, show_command, set_debug, show_debug): Remove.
* top.h (set_history, show_history): Don't declare.
* top.c (set_history, show_history): Remove.
* target-descriptions.c (set_tdesc_cmd, show_tdesc_cmd)
(unset_tdesc_cmd): Remove.
(_initialize_target_descriptions): Use add_basic_prefix_cmd,
add_show_prefix_cmd.
* symtab.c (info_module_command): Remove.
(_initialize_symtab): Use add_basic_prefix_cmd.
* symfile.c (overlay_command): Remove.
(_initialize_symfile): Use add_basic_prefix_cmd.
* sparc64-tdep.c (info_adi_command): Remove.
(_initialize_sparc64_adi_tdep): Use add_basic_prefix_cmd.
* sh-tdep.c (show_sh_command, set_sh_command): Remove.
(_initialize_sh_tdep): Use add_basic_prefix_cmd,
add_show_prefix_cmd.
* serial.c (serial_set_cmd, serial_show_cmd): Remove.
(_initialize_serial): Use add_basic_prefix_cmd,
add_show_prefix_cmd.
* ser-tcp.c (set_tcp_cmd, show_tcp_cmd): Remove.
(_initialize_ser_tcp): Use add_basic_prefix_cmd,
add_show_prefix_cmd.
* rs6000-tdep.c (set_powerpc_command, show_powerpc_command)
(_initialize_rs6000_tdep): Use add_basic_prefix_cmd,
add_show_prefix_cmd.
* riscv-tdep.c (show_riscv_command, set_riscv_command)
(show_debug_riscv_command, set_debug_riscv_command): Remove.
(_initialize_riscv_tdep): Use add_basic_prefix_cmd,
add_show_prefix_cmd.
* remote.c (remote_command, set_remote_cmd): Remove.
(_initialize_remote): Use add_basic_prefix_cmd.
* record-full.c (set_record_full_command)
(show_record_full_command): Remove.
(_initialize_record_full): Use add_basic_prefix_cmd,
add_show_prefix_cmd.
* record-btrace.c (cmd_set_record_btrace)
(cmd_show_record_btrace, cmd_set_record_btrace_bts)
(cmd_show_record_btrace_bts, cmd_set_record_btrace_pt)
(cmd_show_record_btrace_pt): Remove.
(_initialize_record_btrace): Use add_basic_prefix_cmd,
add_show_prefix_cmd.
* ravenscar-thread.c (set_ravenscar_command)
(show_ravenscar_command): Remove.
(_initialize_ravenscar): Use add_basic_prefix_cmd,
add_show_prefix_cmd.
* mips-tdep.c (show_mips_command, set_mips_command)
(_initialize_mips_tdep): Use add_basic_prefix_cmd,
add_show_prefix_cmd.
* maint.c (maintenance_command, maintenance_info_command)
(maintenance_check_command, maintenance_print_command)
(maintenance_set_cmd, maintenance_show_cmd): Remove.
(_initialize_maint_cmds): Use add_basic_prefix_cmd,
add_show_prefix_cmd.
(show_per_command_cmd): Remove.
* maint-test-settings.c (maintenance_set_test_settings_cmd):
Remove.
(maintenance_show_test_settings_cmd): Remove.
(_initialize_maint_test_settings): Use add_basic_prefix_cmd,
add_show_prefix_cmd.
* maint-test-options.c (maintenance_test_options_command):
Remove.
(_initialize_maint_test_options): Use add_basic_prefix_cmd.
* macrocmd.c (macro_command): Remove
(_initialize_macrocmd): Use add_basic_prefix_cmd.
* language.c (set_check, show_check): Remove.
(_initialize_language): Use add_basic_prefix_cmd,
add_show_prefix_cmd.
* infcmd.c (unset_command): Remove.
(_initialize_infcmd): Use add_basic_prefix_cmd.
* i386-tdep.c (set_mpx_cmd, show_mpx_cmd): Remove.
(_initialize_i386_tdep): Use add_basic_prefix_cmd,
add_show_prefix_cmd.
* go32-nat.c (go32_info_dos_command): Remove.
(_initialize_go32_nat): Use add_basic_prefix_cmd.
* cli/cli-decode.c (do_prefix_cmd, add_basic_prefix_cmd)
(do_show_prefix_cmd, add_show_prefix_cmd): New functions.
* frame.c (set_backtrace_cmd, show_backtrace_cmd): Remove.
(_initialize_frame): Use add_basic_prefix_cmd,
add_show_prefix_cmd.
* dcache.c (set_dcache_command, show_dcache_command): Remove.
(_initialize_dcache): Use add_basic_prefix_cmd,
add_show_prefix_cmd.
* cp-support.c (maint_cplus_command): Remove.
(_initialize_cp_support): Use add_basic_prefix_cmd.
* btrace.c (maint_btrace_cmd, maint_btrace_set_cmd)
(maint_btrace_show_cmd, maint_btrace_pt_set_cmd)
(maint_btrace_pt_show_cmd, _initialize_btrace): Use
add_basic_prefix_cmd, add_show_prefix_cmd.
* breakpoint.c (save_command): Remove.
(_initialize_breakpoint): Use add_basic_prefix_cmd.
* arm-tdep.c (set_arm_command, show_arm_command): Remove.
(_initialize_arm_tdep): Use add_basic_prefix_cmd,
add_show_prefix_cmd.
* ada-lang.c (maint_set_ada_cmd, maint_show_ada_cmd)
(set_ada_command, show_ada_command): Remove.
(_initialize_ada_language): Use add_basic_prefix_cmd,
add_show_prefix_cmd.
* command.h (add_basic_prefix_cmd, add_show_prefix_cmd): Declare.
gdb/testsuite/ChangeLog
2020-04-17 Tom Tromey <tromey@adacore.com>
* gdb.cp/maint.exp (test_help): Simplify multiple_help_body.
Update tests.
* gdb.btrace/cpu.exp: Update tests.
* gdb.base/maint.exp: Update tests.
* gdb.base/default.exp: Update tests.
* gdb.base/completion.exp: Update tests.
This patch splits out some gdb-specific code from event-loop, into new
files async-event.[ch]. Strictly speaking this code could perhaps be
put into gdbsupport/, but because gdbserver does not currently use it,
it seemed better, for size reasons, to split it out.
gdb/ChangeLog
2020-04-13 Tom Tromey <tom@tromey.com>
* tui/tui-win.c: Include async-event.h.
* remote.c: Include async-event.h.
* remote-notif.c: Include async-event.h.
* record-full.c: Include async-event.h.
* record-btrace.c: Include async-event.h.
* infrun.c: Include async-event.h.
* event-top.c: Include async-event.h.
* event-loop.h: Move some declarations to async-event.h.
* event-loop.c: Don't include ser-event.h or top.h. Move some
code to async-event.c.
* async-event.h: New file.
* async-event.c: New file.
* Makefile.in (COMMON_SFILES): Add async-event.c.
(HFILES_NO_SRCDIR): Add async-event.h.
