I spent a lot of time reading infrun debug logs recently, and I think
they could be made much more readable by being indented, to clearly see
what operation is done as part of what other operation. In the current
format, there are no visual cues to tell where things start and end,
it's just a big flat list. It's also difficult to understand what
caused a given operation (e.g. a call to resume_1) to be done.
To help with this, I propose to add the new scoped_debug_start_end
structure, along with a bunch of macros to make it convenient to use.
The idea of scoped_debug_start_end is simply to print a start and end
message at construction and destruction. It also increments/decrements
a depth counter in order to make debug statements printed during this
range use some indentation. Some care is taken to handle the fact that
debug can be turned on or off in the middle of such a range. For
example, a "set debug foo 1" command in a breakpoint command, or a
superior GDB manually changing the debug_foo variable.
Two macros are added in gdbsupport/common-debug.h, which are helpers to
define module-specific macros:
- scoped_debug_start_end: takes a message that is printed both at
construction / destruction, with "start: " and "end: " prefixes.
- scoped_debug_enter_exit: prints hard-coded "enter" and "exit"
messages, to denote the entry and exit of a function.
I added some examples in the infrun module to give an idea of how it can
be used and what the result looks like. The macros are in capital
letters (INFRUN_SCOPED_DEBUG_START_END and
INFRUN_SCOPED_DEBUG_ENTER_EXIT) to mimic the existing SCOPE_EXIT, but
that can be changed if you prefer something else.
Here's an excerpt of the debug
statements printed when doing "continue", where a displaced step is
started:
[infrun] proceed: enter
[infrun] proceed: addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT
[infrun] global_thread_step_over_chain_enqueue: enqueueing thread Thread 0x7ffff75a5640 (LWP 2289301) in global step over chain
[infrun] start_step_over: enter
[infrun] start_step_over: stealing global queue of threads to step, length = 1
[infrun] start_step_over: resuming [Thread 0x7ffff75a5640 (LWP 2289301)] for step-over
[infrun] resume_1: step=1, signal=GDB_SIGNAL_0, trap_expected=1, current thread [Thread 0x7ffff75a5640 (LWP 2289301)] at 0x5555555551bd
[displaced] displaced_step_prepare_throw: displaced-stepping Thread 0x7ffff75a5640 (LWP 2289301) now
[displaced] prepare: selected buffer at 0x5555555550c2
[displaced] prepare: saved 0x5555555550c2: 1e fa 31 ed 49 89 d1 5e 48 89 e2 48 83 e4 f0 50
[displaced] amd64_displaced_step_copy_insn: copy 0x5555555551bd->0x5555555550c2: c7 45 fc 00 00 00 00 eb 13 8b 05 d4 2e 00 00 83
[displaced] displaced_step_prepare_throw: prepared successfully thread=Thread 0x7ffff75a5640 (LWP 2289301), original_pc=0x5555555551bd, displaced_pc=0x5555555550c2
[displaced] resume_1: run 0x5555555550c2: c7 45 fc 00
[infrun] infrun_async: enable=1
[infrun] prepare_to_wait: prepare_to_wait
[infrun] start_step_over: [Thread 0x7ffff75a5640 (LWP 2289301)] was resumed.
[infrun] operator(): step-over queue now empty
[infrun] start_step_over: exit
[infrun] proceed: start: resuming threads, all-stop-on-top-of-non-stop
[infrun] proceed: resuming Thread 0x7ffff7da7740 (LWP 2289296)
[infrun] resume_1: step=0, signal=GDB_SIGNAL_0, trap_expected=0, current thread [Thread 0x7ffff7da7740 (LWP 2289296)] at 0x7ffff7f7d9b7
[infrun] prepare_to_wait: prepare_to_wait
[infrun] proceed: resuming Thread 0x7ffff7da6640 (LWP 2289300)
[infrun] resume_1: thread Thread 0x7ffff7da6640 (LWP 2289300) has pending wait status status->kind = stopped, signal = GDB_SIGNAL_TRAP (currently_stepping=0).
[infrun] prepare_to_wait: prepare_to_wait
[infrun] proceed: [Thread 0x7ffff75a5640 (LWP 2289301)] resumed
[infrun] proceed: resuming Thread 0x7ffff6da4640 (LWP 2289302)
[infrun] resume_1: thread Thread 0x7ffff6da4640 (LWP 2289302) has pending wait status status->kind = stopped, signal = GDB_SIGNAL_TRAP (currently_stepping=0).
[infrun] prepare_to_wait: prepare_to_wait
[infrun] proceed: end: resuming threads, all-stop-on-top-of-non-stop
[infrun] proceed: exit
We can easily see where the call to `proceed` starts and end. We can
also see why there are a bunch of resume_1 calls, it's because we are
resuming threads, emulating all-stop on top of a non-stop target.
We also see that debug statements nest well with other modules that have
been migrated to use the "new" debug statement helpers (because they all
use debug_prefixed_vprintf in the end. I think this is desirable, for
example we could see the debug statements about reading the DWARF info
of a library nested under the debug statements about loading that
library.
Of course, modules that haven't been migrated to use the "new" helpers
will still print without indentations. This will be one good reason to
migrate them.
I think the runtime cost (when debug statements are disabled) of this is
reasonable, given the improvement in readability. There is the cost of
the conditionals (like standard debug statements), one more condition
(if (m_must_decrement_print_depth)) and the cost of constructing a stack
object, which means copying a fews pointers.
Adding the print in fetch_inferior_event breaks some tests that use "set
debug infrun", because it prints a debug statement after the prompt. I
adapted these tests to cope with it, by using the "-prompt" switch of
gdb_test_multiple to as if this debug statement is part of the expected
prompt. It's unfortunate that we have to do this, but I think the debug
print is useful, and I don't want a few tests to get in the way of
adding good debug output.
gdbsupport/ChangeLog:
* common-debug.h (debug_print_depth): New.
(struct scoped_debug_start_end): New.
(scoped_debug_start_end): New.
(scoped_debug_enter_exit): New.
* common-debug.cc (debug_prefixed_vprintf): Print indentation.
gdb/ChangeLog:
* debug.c (debug_print_depth): New.
* infrun.h (INFRUN_SCOPED_DEBUG_START_END): New.
(INFRUN_SCOPED_DEBUG_ENTER_EXIT): New.
* infrun.c (start_step_over): Use
INFRUN_SCOPED_DEBUG_ENTER_EXIT.
(proceed): Use INFRUN_SCOPED_DEBUG_ENTER_EXIT and
INFRUN_SCOPED_DEBUG_START_END.
(fetch_inferior_event): Use INFRUN_SCOPED_DEBUG_ENTER_EXIT.
gdbserver/ChangeLog:
* debug.cc (debug_print_depth): New.
gdb/testsuite/ChangeLog:
* gdb.base/ui-redirect.exp: Expect infrun debug print after
prompt.
* gdb.threads/ia64-sigill.exp: Likewise.
* gdb.threads/watchthreads-reorder.exp: Likewise.
Change-Id: I7c3805e6487807aa63a1bae318876a0c69dce949
The code in print_target_wait_results uses a single call to debug_printf
in order to make sure a single timestamp is emitted, despite printing
multiple lines. The result is:
941502.043284 [infrun] target_wait (-1.0.0, status) =
[infrun] 649832.649832.0 [process 649832],
[infrun] status->kind = stopped, signal = GDB_SIGNAL_TRAP
I find this decision a bit counter productive, because it messes up the
alignment of the three lines. We don't care that three (slightly
different) timestamps are printed.
I suggest to change this function to use infrun_debug_printf, with this
result:
941601.425771 [infrun] print_target_wait_results: target_wait (-1.0.0 [process -1], status) =
941601.425824 [infrun] print_target_wait_results: 651481.651481.0 [process 651481],
941601.425867 [infrun] print_target_wait_results: status->kind = stopped, signal = GDB_SIGNAL_TRAP
Note that the current code only prints the waiton_ptid as a string
between square brackets if pid != -1. I don't think this complexity is
needed in a debug print. I made it so it's always printed, which I
think results in a much simpler function.
gdb/ChangeLog:
* infrun.c (print_target_wait_results): Use infrun_debug_printf.
Change-Id: I817bd10286b8e641a6c751ac3a1bd1ddf9b18ce0
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.
gdb/ChangeLog:
* infrun.h (debug_infrun): Make a bool.
* infrun.c (debug_infrun): Make a bool.
(_initialize_infrun): Use add_setshow_boolean_cmd to define "set
debug infrun".
Change-Id: If934106a6d3f879b93d265855eb705b1d606339a
This changes stop_context to use a thread_info_ref, removing some
manual reference counting.
gdb/ChangeLog
2020-12-11 Tom Tromey <tom@tromey.com>
* infrun.c (struct stop_context) <thread>: Now a thread_info_ref.
(stop_context::stop_context): Update.
(stop_context::~stop_context): Remove.
Today, GDB only allows a single displaced stepping operation to happen
per inferior at a time. There is a single displaced stepping buffer per
inferior, whose address is fixed (obtained with
gdbarch_displaced_step_location), managed by infrun.c.
In the case of the AMD ROCm target [1] (in the context of which this
work has been done), it is typical to have thousands of threads (or
waves, in SMT terminology) executing the same code, hitting the same
breakpoint (possibly conditional) and needing to to displaced step it at
the same time. The limitation of only one displaced step executing at a
any given time becomes a real bottleneck.
To fix this bottleneck, we want to make it possible for threads of a
same inferior to execute multiple displaced steps in parallel. This
patch builds the foundation for that.
In essence, this patch moves the task of preparing a displaced step and
cleaning up after to gdbarch functions. This allows using different
schemes for allocating and managing displaced stepping buffers for
different platforms. The gdbarch decides how to assign a buffer to a
thread that needs to execute a displaced step.
On the ROCm target, we are able to allocate one displaced stepping
buffer per thread, so a thread will never have to wait to execute a
displaced step.
On Linux, the entry point of the executable if used as the displaced
stepping buffer, since we assume that this code won't get used after
startup. From what I saw (I checked with a binary generated against
glibc and musl), on AMD64 we have enough space there to fit two
displaced stepping buffers. A subsequent patch makes AMD64/Linux use
two buffers.
In addition to having multiple displaced stepping buffers, there is also
the idea of sharing displaced stepping buffers between threads. Two
threads doing displaced steps for the same PC could use the same buffer
at the same time. Two threads stepping over the same instruction (same
opcode) at two different PCs may also be able to share a displaced
stepping buffer. This is an idea for future patches, but the
architecture built by this patch is made to allow this.
Now, the implementation details. The main part of this patch is moving
the responsibility of preparing and finishing a displaced step to the
gdbarch. Before this patch, preparing a displaced step is driven by the
displaced_step_prepare_throw function. It does some calls to the
gdbarch to do some low-level operations, but the high-level logic is
there. The steps are roughly:
- Ask the gdbarch for the displaced step buffer location
- Save the existing bytes in the displaced step buffer
- Ask the gdbarch to copy the instruction into the displaced step buffer
- Set the pc of the thread to the beginning of the displaced step buffer
Similarly, the "fixup" phase, executed after the instruction was
successfully single-stepped, is driven by the infrun code (function
displaced_step_finish). The steps are roughly:
- Restore the original bytes in the displaced stepping buffer
- Ask the gdbarch to fixup the instruction result (adjust the target's
registers or memory to do as if the instruction had been executed in
its original location)
The displaced_step_inferior_state::step_thread field indicates which
thread (if any) is currently using the displaced stepping buffer, so it
is used by displaced_step_prepare_throw to check if the displaced
stepping buffer is free to use or not.
This patch defers the whole task of preparing and cleaning up after a
displaced step to the gdbarch. Two new main gdbarch methods are added,
with the following semantics:
- gdbarch_displaced_step_prepare: Prepare for the given thread to
execute a displaced step of the instruction located at its current PC.
Upon return, everything should be ready for GDB to resume the thread
(with either a single step or continue, as indicated by
gdbarch_displaced_step_hw_singlestep) to make it displaced step the
instruction.
- gdbarch_displaced_step_finish: Called when the thread stopped after
having started a displaced step. Verify if the instruction was
executed, if so apply any fixup required to compensate for the fact
that the instruction was executed at a different place than its
original pc. Release any resources that were allocated for this
displaced step. Upon return, everything should be ready for GDB to
resume the thread in its "normal" code path.
The displaced_step_prepare_throw function now pretty much just offloads
to gdbarch_displaced_step_prepare and the displaced_step_finish function
offloads to gdbarch_displaced_step_finish.
The gdbarch_displaced_step_location method is now unnecessary, so is
removed. Indeed, the core of GDB doesn't know how many displaced step
buffers there are nor where they are.
To keep the existing behavior for existing architectures, the logic that
was previously implemented in infrun.c for preparing and finishing a
displaced step is moved to displaced-stepping.c, to the
displaced_step_buffer class. Architectures are modified to implement
the new gdbarch methods using this class. The behavior is not expected
to change.
The other important change (which arises from the above) is that the
core of GDB no longer prevents concurrent displaced steps. Before this
patch, start_step_over walks the global step over chain and tries to
initiate a step over (whether it is in-line or displaced). It follows
these rules:
- if an in-line step is in progress (in any inferior), don't start any
other step over
- if a displaced step is in progress for an inferior, don't start
another displaced step for that inferior
After starting a displaced step for a given inferior, it won't start
another displaced step for that inferior.
In the new code, start_step_over simply tries to initiate step overs for
all the threads in the list. But because threads may be added back to
the global list as it iterates the global list, trying to initiate step
overs, start_step_over now starts by stealing the global queue into a
local queue and iterates on the local queue. In the typical case, each
thread will either:
- have initiated a displaced step and be resumed
- have been added back by the global step over queue by
displaced_step_prepare_throw, because the gdbarch will have returned
that there aren't enough resources (i.e. buffers) to initiate a
displaced step for that thread
Lastly, if start_step_over initiates an in-line step, it stops
iterating, and moves back whatever remaining threads it had in its local
step over queue to the global step over queue.
Two other gdbarch methods are added, to handle some slightly annoying
corner cases. They feel awkwardly specific to these cases, but I don't
see any way around them:
- gdbarch_displaced_step_copy_insn_closure_by_addr: in
arm_pc_is_thumb, arm-tdep.c wants to get the closure for a given
buffer address.
- gdbarch_displaced_step_restore_all_in_ptid: when a process forks
(at least on Linux), the address space is copied. If some displaced
step buffers were in use at the time of the fork, we need to restore
the original bytes in the child's address space.
These two adjustments are also made in infrun.c:
- prepare_for_detach: there may be multiple threads doing displaced
steps when we detach, so wait until all of them are done
- handle_inferior_event: when we handle a fork event for a given
thread, it's possible that other threads are doing a displaced step at
the same time. Make sure to restore the displaced step buffer
contents in the child for them.
[1] https://github.com/ROCm-Developer-Tools/ROCgdb
gdb/ChangeLog:
* displaced-stepping.h (struct
displaced_step_copy_insn_closure): Adjust comments.
(struct displaced_step_inferior_state) <step_thread,
step_gdbarch, step_closure, step_original, step_copy,
step_saved_copy>: Remove fields.
(struct displaced_step_thread_state): New.
(struct displaced_step_buffer): New.
* displaced-stepping.c (displaced_step_buffer::prepare): New.
(write_memory_ptid): Move from infrun.c.
(displaced_step_instruction_executed_successfully): New,
factored out of displaced_step_finish.
(displaced_step_buffer::finish): New.
(displaced_step_buffer::copy_insn_closure_by_addr): New.
(displaced_step_buffer::restore_in_ptid): New.
* gdbarch.sh (displaced_step_location): Remove.
(displaced_step_prepare, displaced_step_finish,
displaced_step_copy_insn_closure_by_addr,
displaced_step_restore_all_in_ptid): New.
* gdbarch.c: Re-generate.
* gdbarch.h: Re-generate.
* gdbthread.h (class thread_info) <displaced_step_state>: New
field.
(thread_step_over_chain_remove): New declaration.
(thread_step_over_chain_next): New declaration.
(thread_step_over_chain_length): New declaration.
* thread.c (thread_step_over_chain_remove): Make non-static.
(thread_step_over_chain_next): New.
(global_thread_step_over_chain_next): Use
thread_step_over_chain_next.
(thread_step_over_chain_length): New.
(global_thread_step_over_chain_enqueue): Add debug print.
(global_thread_step_over_chain_remove): Add debug print.
* infrun.h (get_displaced_step_copy_insn_closure_by_addr):
Remove.
* infrun.c (get_displaced_stepping_state): New.
(displaced_step_in_progress_any_inferior): Remove.
(displaced_step_in_progress_thread): Adjust.
(displaced_step_in_progress): Adjust.
(displaced_step_in_progress_any_thread): New.
(get_displaced_step_copy_insn_closure_by_addr): Remove.
(gdbarch_supports_displaced_stepping): Use
gdbarch_displaced_step_prepare_p.
(displaced_step_reset): Change parameter from inferior to
thread.
(displaced_step_prepare_throw): Implement using
gdbarch_displaced_step_prepare.
(write_memory_ptid): Move to displaced-step.c.
(displaced_step_restore): Remove.
(displaced_step_finish): Implement using
gdbarch_displaced_step_finish.
(start_step_over): Allow starting more than one displaced step.
(prepare_for_detach): Handle possibly multiple threads doing
displaced steps.
(handle_inferior_event): Handle possibility that fork event
happens while another thread displaced steps.
* linux-tdep.h (linux_displaced_step_prepare): New.
(linux_displaced_step_finish): New.
(linux_displaced_step_copy_insn_closure_by_addr): New.
(linux_displaced_step_restore_all_in_ptid): New.
(linux_init_abi): Add supports_displaced_step parameter.
* linux-tdep.c (struct linux_info) <disp_step_buf>: New field.
(linux_displaced_step_prepare): New.
(linux_displaced_step_finish): New.
(linux_displaced_step_copy_insn_closure_by_addr): New.
(linux_displaced_step_restore_all_in_ptid): New.
(linux_init_abi): Add supports_displaced_step parameter,
register displaced step methods if true.
(_initialize_linux_tdep): Register inferior_execd observer.
* amd64-linux-tdep.c (amd64_linux_init_abi_common): Add
supports_displaced_step parameter, adjust call to
linux_init_abi. Remove call to
set_gdbarch_displaced_step_location.
(amd64_linux_init_abi): Adjust call to
amd64_linux_init_abi_common.
(amd64_x32_linux_init_abi): Likewise.
* aarch64-linux-tdep.c (aarch64_linux_init_abi): Adjust call to
linux_init_abi. Remove call to
set_gdbarch_displaced_step_location.
* arm-linux-tdep.c (arm_linux_init_abi): Likewise.
* i386-linux-tdep.c (i386_linux_init_abi): Likewise.
* alpha-linux-tdep.c (alpha_linux_init_abi): Adjust call to
linux_init_abi.
* arc-linux-tdep.c (arc_linux_init_osabi): Likewise.
* bfin-linux-tdep.c (bfin_linux_init_abi): Likewise.
* cris-linux-tdep.c (cris_linux_init_abi): Likewise.
* csky-linux-tdep.c (csky_linux_init_abi): Likewise.
* frv-linux-tdep.c (frv_linux_init_abi): Likewise.
* hppa-linux-tdep.c (hppa_linux_init_abi): Likewise.
* ia64-linux-tdep.c (ia64_linux_init_abi): Likewise.
* m32r-linux-tdep.c (m32r_linux_init_abi): Likewise.
* m68k-linux-tdep.c (m68k_linux_init_abi): Likewise.
* microblaze-linux-tdep.c (microblaze_linux_init_abi): Likewise.
* mips-linux-tdep.c (mips_linux_init_abi): Likewise.
* mn10300-linux-tdep.c (am33_linux_init_osabi): Likewise.
* nios2-linux-tdep.c (nios2_linux_init_abi): Likewise.
* or1k-linux-tdep.c (or1k_linux_init_abi): Likewise.
* riscv-linux-tdep.c (riscv_linux_init_abi): Likewise.
* s390-linux-tdep.c (s390_linux_init_abi_any): Likewise.
* sh-linux-tdep.c (sh_linux_init_abi): Likewise.
* sparc-linux-tdep.c (sparc32_linux_init_abi): Likewise.
* sparc64-linux-tdep.c (sparc64_linux_init_abi): Likewise.
* tic6x-linux-tdep.c (tic6x_uclinux_init_abi): Likewise.
* tilegx-linux-tdep.c (tilegx_linux_init_abi): Likewise.
* xtensa-linux-tdep.c (xtensa_linux_init_abi): Likewise.
* ppc-linux-tdep.c (ppc_linux_init_abi): Adjust call to
linux_init_abi. Remove call to
set_gdbarch_displaced_step_location.
* arm-tdep.c (arm_pc_is_thumb): Call
gdbarch_displaced_step_copy_insn_closure_by_addr instead of
get_displaced_step_copy_insn_closure_by_addr.
* rs6000-aix-tdep.c (rs6000_aix_init_osabi): Adjust calls to
clear gdbarch methods.
* rs6000-tdep.c (struct ppc_inferior_data): New structure.
(get_ppc_per_inferior): New function.
(ppc_displaced_step_prepare): New function.
(ppc_displaced_step_finish): New function.
(ppc_displaced_step_restore_all_in_ptid): New function.
(rs6000_gdbarch_init): Register new gdbarch methods.
* s390-tdep.c (s390_gdbarch_init): Don't call
set_gdbarch_displaced_step_location, set new gdbarch methods.
gdb/testsuite/ChangeLog:
* gdb.arch/amd64-disp-step-avx.exp: Adjust pattern.
* gdb.threads/forking-threads-plus-breakpoint.exp: Likewise.
* gdb.threads/non-stop-fair-events.exp: Likewise.
Change-Id: I387cd235a442d0620ec43608fd3dc0097fcbf8c8
This is a preparatory patch to reduce the size of the diff of the
upcoming main patch. It introduces enum types for the return values of
displaced step "prepare" and "finish" operations. I find that this
expresses better the intention of the code, rather than returning
arbitrary integer values (-1, 0 and 1) which are difficult to remember.
That makes the code easier to read.
I put the new enum types in a new displaced-stepping.h file, because I
introduce that file in a later patch anyway. Putting it there avoids
having to move it later.
There is one change in behavior for displaced_step_finish: it currently
returns 0 if the thread wasn't doing a displaced step and 1 if the
thread was doing a displaced step which was executed successfully. It
turns out that this distinction is not needed by any caller, so I've
merged these two cases into "_OK", rather than adding an extra
enumerator.
gdb/ChangeLog:
* infrun.c (displaced_step_prepare_throw): Change return type to
displaced_step_prepare_status.
(displaced_step_prepare): Likewise.
(displaced_step_finish): Change return type to
displaced_step_finish_status.
(resume_1): Adjust.
(stop_all_threads): Adjust.
* displaced-stepping.h: New file.
Change-Id: I5c8fe07212cd398d5b486b5936d9d0807acd3788
This is a preparatory patch to reduce a little bit the diff size of the
main patch later in this series. It renames the displaced_step_fixup
function in infrun.c to displaced_step_finish.
The rationale is to better differentiate the low and high level
operations.
We first have the low level operation of writing an instruction to a
displaced buffer, called "copy_insn". The mirror low level operation to
fix up the state after having executed the instruction is "fixup". The
high level operation of preparing a thread for a displaced step (which
includes doing the "copy_insn" and some more bookkeeping) is called
"prepare" (as in displaced_step_prepare). The mirror high level
operation to cleaning up after a displaced step (which includes doing
the "fixup" and some more bookkeeping) is currently also called "fixup"
(as in displaced_step_fixup), just like the low level operation.
I think that choosing a different name for the low and high level
cleanup operation makes it clearer, hence "finish".
gdb/ChangeLog:
* infrun.c (displaced_step_fixup): Rename to...
(displaced_step_finish): ... this, update all callers.
Change-Id: Id32f48c1e2091d09854c77fcedcc14d2519957a2
Rename step_over_queue_head to global_thread_step_over_chain_head, to
make it more obvious when reading code that we are touching the global
queue. Rename all functions that operate on it to have "global" in
their name, to make it clear on which chain they operate on. Also, in a
subsequent patch, we'll need both global and non-global versions of
these functions, so it will be easier to do the distinction if they are
named properly.
Normalize the naming to use "chain" everywhere instead of sometimes
"queue", sometimes "chain".
I also reworded a few comments in gdbthread.h. They implied that the
step over chain is per-inferior, when in reality there is only one
global chain, not one per inferior, as far as I understand.
gdb/ChangeLog:
* gdbthread.h (thread_step_over_chain_enqueue): Rename to...
(global_thread_step_over_chain_enqueue): ... this. Update all
users.
(thread_step_over_chain_remove): Rename to...
(global_thread_step_over_chain_remove): ... this. Update all
users.
(thread_step_over_chain_next): Rename to...
(global_thread_step_over_chain_next): ... this. Update all
users.
* infrun.h (step_over_queue_head): Rename to...
(global_thread_step_over_chain_head): ... this. Update all
users.
* infrun.c (step_over_queue_head): Rename to...
(global_thread_step_over_chain_head): ... this. Update all
users.
* thread.c (step_over_chain_remove): Rename to...
(thread_step_over_chain_remove): ... this. Update all users.
(thread_step_over_chain_next): Rename to...
(global_thread_step_over_chain_next): ... this. Update all
users.
(thread_step_over_chain_enqueue): Rename to...
(global_thread_step_over_chain_enqueue): ... this. Update all
users.
(thread_step_over_chain_remove): Rename to...
(global_thread_step_over_chain_remove): ... this. Update all
users.
Change-Id: Iabbf57d83c01321ca199d83fadb57f5b04e4d6d9
Remove function get_displaced_stepping_state. When it was introduced,
inferiors' displaced stepping state was kept in a linked list in
infrun.c, so it was handy. Nowadays, the state is kept inside struct
inferior directly, so we can just access it directly instead.
gdb/ChangeLog:
* infrun.c (get_displaced_stepping_state): Remove, change
callers to access the field directly.
Change-Id: I9a733e32e29c7ebf856ab0befe1076bbb8c7af69
In handle_inferior_event, where we handle forks, we make sure to restore
the bytes of the displaced stepping buffer in the child's address
space. However, we only do it when the forking thread was the one
doing a displaced step. It could happen that a thread forks while
another one is doing a displaced step. In this case, we also need to
restore the bytes in the child.
Move the byte-restoring code outside of the condition that checks
whether the event thread was displaced stepping.
gdb/ChangeLog:
* infrun.c (handle_inferior_event): Restore displaced step
buffer bytes in child process when handling fork, even if fork
happened in another thread than the displaced-stepping one.
Change-Id: Ibb0daaeb123aba03f4fb4b4d820754eb2436bc69
When a process does an exec, all its program space is replaced with the
newly loaded executable. All non-main threads disappear and the main
thread starts executing at the entry point of the new executable.
Things can go wrong if a displaced step operation is in progress while
we process the exec event.
If the main thread is the one executing the displaced step: when that
thread (now executing in the new executable) stops somewhere (say, at a
breakpoint), displaced_step_fixup will run and clear up the state. We
will execute the "fixup" phase for the instruction we single-stepped in
the old program space. We are now in a completely different context,
so doing the fixup may corrupt the state.
If it is a non-main thread that is doing the displaced step: while
handling the exec event, GDB deletes the thread_info representing that
thread (since the thread doesn't exist in the inferior after the exec).
But inferior::displaced_step_state::step_thread will still point to it.
When handling events later, this condition, in displaced_step_fixup,
will likely never be true:
/* Was this event for the thread we displaced? */
if (displaced->step_thread != event_thread)
return 0;
... since displaced->step_thread points to a deleted thread (unless that
storage gets re-used for a new thread_info, but that wouldn't be good
either). This effectively makes the displaced stepping buffer occupied
for ever. When a thread in the new program space will want to do a
displaced step, it will wait for ever.
I think we simply need to reset the displaced stepping state of the
inferior on exec. Everything execution-related that existed before the
exec is now gone.
Similarly, if a thread does an in-line step over an exec syscall
instruction, nothing clears the in-line step over info when the event is
handled. So it the in-line step over info stays there indefinitely, and
things hang because we can never start another step over. To fix this,
I added a call to clear_step_over_info in infrun_inferior_execd.
Add a test with a program with two threads that does an exec. The test
includes the following axes:
- whether it's the leader thread or the other thread that does the exec.
- whether the exec'r and exec'd program have different text segment
addresses. This is to hopefully catch cases where the displaced
stepping info doesn't get reset, and GDB later tries to restore bytes
of the old address space in the new address space. If the mapped
addresses are different, we should get some memory error. This
happens without the patch applied:
$ ./gdb -q -nx --data-directory=data-directory testsuite/outputs/gdb.threads/step-over-exec/step-over-exec-execr-thread-leader-diff-text-segs-true -ex "b main" -ex r -ex "b my_execve_syscall if 0" -ex "set displaced-stepping on"
...
Breakpoint 1, main (argc=1, argv=0x7fffffffde38) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.threads/step-over-exec.c:69
69 argv0 = argv[0];
Breakpoint 2 at 0x60133a: file /home/simark/src/binutils-gdb/gdb/testsuite/lib/my-syscalls.S, line 34.
(gdb) c
Continuing.
[New Thread 0x7ffff7c62640 (LWP 1455423)]
Leader going in exec.
Exec-ing /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-over-exec/step-over-exec-execr-thread-leader-diff-text-segs-true-execd
[Thread 0x7ffff7c62640 (LWP 1455423) exited]
process 1455418 is executing new program: /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-over-exec/step-over-exec-execr-thread-leader-diff-text-segs-true-execd
Error in re-setting breakpoint 2: Function "my_execve_syscall" not defined.
No unwaited-for children left.
(gdb) n
Single stepping until exit from function _start,
which has no line number information.
Cannot access memory at address 0x6010d2
(gdb)
- Whether displaced stepping is allowed or not, so that we end up
testing both displaced stepping and in-line stepping on arches that do
support displaced stepping (otherwise, it just tests in-line stepping
twice I suppose)
To be able to precisely put a breakpoint on the syscall instruction, I
added a small assembly file (lib/my-syscalls.S) that contains minimal
Linux syscall wrappers. I prefer that to the strategy used in
gdb.base/step-over-syscall.exp, which is to stepi into the glibc wrapper
until we find something that looks like a syscall instruction, I find
that more predictable.
gdb/ChangeLog:
* infrun.c (infrun_inferior_execd): New function.
(_initialize_infrun): Attach inferior_execd observer.
gdb/testsuite/ChangeLog:
* gdb.threads/step-over-exec.exp: New.
* gdb.threads/step-over-exec.c: New.
* gdb.threads/step-over-exec-execd.c: New.
* lib/my-syscalls.S: New.
* lib/my-syscalls.h: New.
Change-Id: I1bbc8538e683f53af5b980091849086f4fec5ff9
I want to add another action (clearing displaced stepping state) that
happens when an inferior execs. I think it would be cleaner to have an
observer for this event, rather than have infrun know about each other
sub-component.
Replace the calls to solib_create_inferior_hook and
jit_inferior_created_hook in follow_exec by observers.
gdb/ChangeLog:
* observable.h (inferior_execd): Declare new observable.
* observable.c (inferior_execd): Declare new observable.
* infrun.c (follow_exec): Notify inferior_execd observer.
* jit.c (jit_inferior_created_hook): Make static.
(_initialize_jit): Register inferior_execd observer.
* jit.h (jit_inferior_created_hook): Remove declaration.
* solib.c (_initialize_solib): Register inferior_execd observer.
Change-Id: I000cce00094e23baa67df693d912646b6ae38e44
A while back I noticed that fetch_inferior_event used "int" for
should_stop, whereas it can be bool. The method it is assigned from:
should_stop = thread_fsm->should_stop (thr);
... already returns bool.
Tested by rebuilding.
gdb/ChangeLog
2020-11-14 Tom Tromey <tom@tromey.com>
* infrun.c (fetch_inferior_event): Use "bool" for should_stop.
The *_debug_print_1 functions are all very similar, the only difference
being the subsystem name. Remove them all and make the logging macros
use a new debug_prefixed_printf function directly.
gdb/ChangeLog:
* infrun.c (infrun_debug_printf_1): Remove.
(displaced_debug_printf_1): Remove.
(stop_all_threads): Use debug_prefixed_printf.
* infrun.h (infrun_debug_printf_1): Remove.
(infrun_debug_printf): Use debug_prefixed_printf.
(displaced_debug_printf_1): Remove.
(displaced_debug_printf): Use debug_prefixed_printf.
* linux-nat.c (linux_nat_debug_printf_1): Remove.
(linux_nat_debug_printf): Use debug_prefixed_printf.
gdbsupport/ChangeLog:
* common-debug.cc (debug_prefixed_printf): New.
* common-debug.h (debug_prefixed_printf): New declaration.
* event-loop.cc (event_loop_debug_printf_1): Remove.
* event-loop.h (event_loop_debug_printf_1): Remove.
(event_loop_debug_printf): Use debug_prefixed_printf.
(event_loop_ui_debug_printf): Use debug_prefixed_printf.
Change-Id: Ib323087c7257f0060121d302055c41eb64aa60c6
Move all debug prints of the "displaced" category to use a new
displaced_debug_printf macro, like what was done for infrun and others
earlier.
The debug output for one displaced step one amd64 looks like:
[displaced] displaced_step_prepare_throw: stepping process 3367044 now
[displaced] displaced_step_prepare_throw: saved 0x555555555042: 1e fa 31 ed 49 89 d1 5e 48 89 e2 48 83 e4 f0 50
[displaced] amd64_displaced_step_copy_insn: copy 0x555555555131->0x555555555042: b8 00 00 00 00 5d c3 0f 1f 84 00 00 00 00 00 f3
[displaced] displaced_step_prepare_throw: displaced pc to 0x555555555042
[displaced] resume_1: run 0x555555555042: b8 00 00 00
[displaced] displaced_step_restore: restored process 3367044 0x555555555042
[displaced] amd64_displaced_step_fixup: fixup (0x555555555131, 0x555555555042), insn = 0xb8 0x00 ...
[displaced] amd64_displaced_step_fixup: relocated %rip from 0x555555555047 to 0x555555555136
On test case needed to be updated because it relied on the specific
formatting of the message.
gdb/ChangeLog:
* infrun.h (displaced_debug_printf): New macro. Replace
displaced debug prints throughout to use it.
(displaced_debug_printf_1): New declaration.
(displaced_step_dump_bytes): Return string, remove ui_file
parameter, update all callers.
* infrun.c (displaced_debug_printf_1): New function.
(displaced_step_dump_bytes): Return string, remove ui_file
parameter
gdb/testsuite/ChangeLog:
* gdb.arch/amd64-disp-step-avx.exp: Update displaced step debug
expected output.
Change-Id: Ie78837f56431f6f98378790ba1e6051337bf6533
Having pagination enabled when handling an inferior event gives the
user an option to quit, which causes early exit in GDB's flow and may
lead to half-baked state. For instance, here is a case where we quit
in the middle of handling an inferior exit:
$ gdb ./a.out
Reading symbols from ./a.out...
(gdb) set height 2
(gdb) run
Starting program: ./a.out
--Type <RET> for more, q to quit, c to continue without paging--q
Quit
Couldn't get registers: No such process.
(gdb) set height unlimited
Couldn't get registers: No such process.
(gdb) info threads
Id Target Id Frame
* 1 process 27098 Couldn't get registers: No such process.
Couldn't get registers: No such process.
(gdb)
Or suppose having a multi-threaded program like below:
static void *
fun (void *dummy)
{
int a = 1; /* break-here */
return NULL;
}
int
main (void)
{
pthread_t thread;
pthread_create (&thread, NULL, fun, NULL);
pthread_join (thread, NULL);
return 0;
}
If we define a breakpoint at line "break-here", we expect only Thread
2 to hit it.
$ gdb ./a.out
Reading symbols from ./a.out...
(gdb) break 7
Breakpoint 1 at 0x1182: file mt.c, line 7.
(gdb) set height 2
(gdb) run
Starting program: ./a.out
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
[New Thread 0x7ffff77c4700 (LWP 23048)]
--Type <RET> for more, q to quit, c to continue without paging--q
Quit
(gdb) set height unlimited
(gdb) info thread
Id Target Id Frame
* 1 Thread 0x7ffff7fe3740 (LWP 23044) "a.out" 0x00007ffff7bbed2d in ...
2 Thread 0x7ffff77c4700 (LWP 23048) "a.out" fun (dummy=0x0) at mt.c:7
(gdb)
The prompt for continuation was triggered because Thread 2 hit the
breakpoint. (If we had hit 'c', we were going to see that stop event,
but we didn't.) The context did not switch to Thread 2. GDB also did
not execute several other things it would normally do in
infrun.c:normal_stop after outputting "[Switching to Thread ...]" (but
it seems harmless in this case). If we 'continue' at this state, both
threads run until termination, and we don't see the breakpoint hit
event ever.
Here is another related and more complicated scenario that leads to a
GDB crash. Create two inferiors, one sitting on top of a native
target, and the other on a remote target, so that we have a
multi-target setting, like so:
(gdb) i inferiors
Num Description Connection Executable
1 process 13786 1 (native) a.out
* 2 process 13806 2 (remote ...) target:a.out
Next, resume both inferiors to run until termination:
(gdb) set schedule-multiple on
(gdb) set height 2
(gdb) continue
Continuing.
--Type <RET> for more, q to quit, c to continue without paging--[Inferior 2 (process 13806) exited normally]
terminate called after throwing an instance of 'gdb_exception_error'
Aborted
Here, GDB first received a termination event from Inferior 1. GDB
attempted to print this event, triggering a "prompt for continue", and
GDB started polling for events, hoping to get an input from the user.
However, the exit event from Inferior 2 was received instead. So, GDB
started processing an exit event while being in the middle of
processing another exit event. It was not ready for this situation
and eventually crashed.
To address these cases, temporarily disable pagination in
fetch_inferior_event. This doesn't affect commands like 'info
threads', 'backtrace', or 'thread apply'.
Regression-tested on X86_64 Linux.
gdb/ChangeLog:
2020-10-30 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
* infrun.c (fetch_inferior_event): Temporarily disable pagination.
gdb/testsuite/ChangeLog:
2020-10-30 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
* gdb.base/paginate-after-ctrl-c-running.exp: Update with no pagination
behavior.
* gdb.base/paginate-bg-execution.exp: Ditto.
* gdb.base/paginate-inferior-exit.exp: Ditto.
* gdb.base/double-prompt-target-event-error.c: Remove.
* gdb.base/double-prompt-target-event-error.exp: Remove.
If the remote target closes while we're reading registers/memory for
restoring the selected frame in scoped_restore_current_thread's dtor,
the corresponding TARGET_CLOSE_ERROR error is swallowed by the
scoped_restore_current_thread's dtor, because letting exceptions
escape from a dtor is bad. It isn't great to lose that errors like
that, though. I've been thinking about how to avoid it, and I came up
with this patch.
The idea here is to make scoped_restore_current_thread's dtor do as
little as possible, to avoid any work that might throw in the first
place. And to do that, instead of having the dtor call
restore_selected_frame, which re-finds the previously selected frame,
just record the frame_id/level of the desired selected frame, and have
get_selected_frame find the frame the next time it is called. In
effect, this implements most of Cagney's suggestion, here:
/* On demand, create the selected frame and then return it. If the
selected frame can not be created, this function prints then throws
an error. When MESSAGE is non-NULL, use it for the error message,
otherwize use a generic error message. */
/* FIXME: cagney/2002-11-28: At present, when there is no selected
frame, this function always returns the current (inner most) frame.
It should instead, when a thread has previously had its frame
selected (but not resumed) and the frame cache invalidated, find
and then return that thread's previously selected frame. */
extern struct frame_info *get_selected_frame (const char *message);
The only thing missing to fully implement that would be to make
reinit_frame_cache just clear selected_frame instead of calling
select_frame(NULL), and the call select_frame(NULL) explicitly in the
places where we really wanted reinit_frame_cache to go back to the
current frame too. That can done separately, though, I'm not
proposing to do that in this patch.
Note that this patch renames restore_selected_frame to
lookup_selected_frame, and adds a new restore_selected_frame function
that doesn't throw, to be paired with the also-new save_selected_frame
function.
There's a restore_selected_frame function in infrun.c that I think can
be replaced by the new one in frame.c.
Also done in this patch is make the get_selected_frame's parameter be
optional, so that we don't have to pass down nullptr explicitly all
over the place.
lookup_selected_frame should really move from thread.c to frame.c, but
I didn't do that here, just to avoid churn in the patch while it
collects comments. I did make it extern and declared it in frame.h
already, preparing for the move. I will do the move as a follow up
patch if people agree with this approach.
Incidentally, this patch alone would fix the crashes fixed by the
previous patches in the series, because with this,
scoped_restore_current_thread's constructor doesn't throw either.
gdb/ChangeLog:
* blockframe.c (block_innermost_frame): Use get_selected_frame.
* frame.c
(scoped_restore_selected_frame::scoped_restore_selected_frame):
Use save_selected_frame. Save language as well.
(scoped_restore_selected_frame::~scoped_restore_selected_frame):
Use restore_selected_frame, and restore language as well.
(selected_frame_id, selected_frame_level): New.
(selected_frame): Update comments.
(save_selected_frame, restore_selected_frame): New.
(get_selected_frame): Use lookup_selected_frame.
(get_selected_frame_if_set): Delete.
(select_frame): Record selected_frame_level and selected_frame_id.
* frame.h (scoped_restore_selected_frame) <m_level, m_lang>: New
fields.
(get_selected_frame): Make 'message' parameter optional.
(get_selected_frame_if_set): Delete declaration.
(select_frame): Update comments.
(save_selected_frame, restore_selected_frame)
(lookup_selected_frame): Declare.
* gdbthread.h (scoped_restore_current_thread) <m_lang>: New field.
* infrun.c (struct infcall_control_state) <selected_frame_level>:
New field.
(save_infcall_control_state): Use save_selected_frame.
(restore_selected_frame): Delete.
(restore_infcall_control_state): Use restore_selected_frame.
* stack.c (select_frame_command_core, frame_command_core): Use
get_selected_frame.
* thread.c (restore_selected_frame): Rename to ...
(lookup_selected_frame): ... this and make extern. Select the
current frame if the frame level is -1.
(scoped_restore_current_thread::restore): Also restore the
language.
(scoped_restore_current_thread::~scoped_restore_current_thread):
Don't try/catch.
(scoped_restore_current_thread::scoped_restore_current_thread):
Save the language as well. Use save_selected_frame.
Change-Id: I73fd1cfc40d8513c28e5596383b7ecd8bcfe700f
I noticed that the closure parameter of
gdbarch_displaced_step_hw_singlestep is never used by any
implementation of the method, so this patch removes it.
gdb/ChangeLog:
* gdbarch.sh (displaced_step_hw_singlestep): Remove closure
parameter.
* aarch64-tdep.c (aarch64_displaced_step_hw_singlestep):
Likewise.
* aarch64-tdep.h (aarch64_displaced_step_hw_singlestep):
Likewise.
* arch-utils.c (default_displaced_step_hw_singlestep):
Likewise.
* arch-utils.h (default_displaced_step_hw_singlestep):
Likewise.
* rs6000-tdep.c (ppc_displaced_step_hw_singlestep):
Likewise.
* s390-tdep.c (s390_displaced_step_hw_singlestep):
Likewise.
* gdbarch.c: Re-generate.
* gdbarch.h: Re-generate.
* infrun.c (resume_1): Adjust.
Change-Id: I7354f0b22afc2692ebff0cd700a462db8f389fc1
Use the inferior parameter now available in jit_inferior_created_hook.
It is passed down to jit_inferior_init, which uses it as much as
possible instead of the current inferior or current program space.
gdb/ChangeLog:
* jit.c (jit_reader_load_command): Pass current inferior.
(jit_inferior_init): Change parameter type to inferior, use it.
(jit_inferior_created): Remove.
(jit_inferior_created_hook): Pass inferior parameter down.
(_initialize_jit): Use jit_inferior_created_hook instead of
jit_inferior_created.
* jit.h (jit_inferior_created_hook): Add inferior parameter.
* infrun.c (follow_exec): Pass inferior to
jit_inferior_created_hook.
Change-Id: If3a2114a933370dd313d5abd623136d273cdb8fa
Debugging with "maintenance set target-async off" on Linux has been
broken since 5b6d1e4fa4 ("Multi-target support").
The issue is easy to reproduce:
$ ./gdb -q --data-directory=data-directory -nx ./test
Reading symbols from ./test...
(gdb) maintenance set target-async off
(gdb) start
Temporary breakpoint 1 at 0x1151: file test.c, line 5.
Starting program: /home/simark/build/binutils-gdb/gdb/test
... and it hangs there.
The difference between pre-5b6d1e4fa4f and 5b6d1e4fa4 is that
fetch_inferior_event now calls target_wait with TARGET_WNOHANG for
non-async-capable targets, whereas it didn't before.
For non-async-capable targets, this is how it's expected to work when
resuming execution:
1. we call resume
2. the infrun async handler is marked in prepare_to_wait, to immediately
wake up the event loop when we get back to it
3. fetch_inferior_event calls the target's wait method without
TARGET_WNOHANG, effectively blocking until the target has something
to report
However, since we call the target's wait method with TARGET_WNOHANG,
this happens:
1. we call resume
2. the infrun async handler is marked in prepare_to_wait, to immediately
wake up the event loop when we get back to it
3. fetch_inferior_event calls the target's wait method with
TARGET_WNOHANG, the target has nothing to report yet
4. we go back to blocking on the event loop
5. SIGCHLD finally arrives, but the event loop is not woken up, because
we are not in async mode. Normally, we should have been stuck in
waitpid the SIGCHLD would have unblocked us.
We end up in this situation because these two necessary conditions are
met:
1. GDB uses the TARGET_WNOHANG option with a target that can't do async.
I don't think this makes sense. I mean, it's technically possible,
the doc for TARGET_WNOHANG is:
/* Return immediately if there's no event already queued. If this
options is not requested, target_wait blocks waiting for an
event. */
TARGET_WNOHANG = 1,
... which isn't in itself necessarily incompatible with synchronous
targets. It could be possible for a target to support non-blocking
polls, while not having a way to asynchronously wake up the event
loop, which is also necessary to support async. But as of today,
we don't expect GDB and sync targets to work this way.
2. The linux-nat target, even in the mode where it emulates a
synchronous target (with "maintenance set target-async off") respects
TARGET_WNOHANG. Other non-async targets, such as windows_nat_target,
simply don't check / support TARGET_WNOHANG, so their wait method is
always blocking.
Fix the first issue by avoiding using TARGET_WNOHANG on non-async
targets, in do_target_wait_1. Add an assert in target_wait to verify it
doesn't happen.
The new test gdb.base/maint-target-async-off.exp is a simple test that
just tries running to main and then to the end of the program, with
"maintenance set target-async off".
gdb/ChangeLog:
PR gdb/26642
* infrun.c (do_target_wait_1): Clear TARGET_WNOHANG if the
target can't do async.
* target.c (target_wait): Assert that we don't pass
TARGET_WNOHANG to a target that can't async.
gdb/testsuite/ChangeLog:
PR gdb/26642
* gdb.base/maint-target-async-off.c: New test.
* gdb.base/maint-target-async-off.exp: New test.
Change-Id: I69ad3a14598863d21338a8c4e78700a58ce7ad86
The following patch needs to output debug prints from gdbsupport code.
Move debug_prefixed_vprintf so that it is possible to use it from
gdbsupport.
gdb/ChangeLog:
* debug.c (debug_prefixed_vprintf): Move to gdbsupport.
* debug.h: Remove.
* infrun.c: Include gdbsupport/common-debug.h.
* linux-nat.c: Likewise.
gdbsupport/ChangeLog:
* common-debug.cc (debug_prefixed_vprintf): Move here.
* common-debug.h (debug_prefixed_vprintf): Move here.
Change-Id: I5170065fc10a7a49c0f1bba67c691decb2cf3bcb
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 non of the listeners of the inferior_created observable
used either of the arguments. Remove them. This in turn allows
removing the target parameter of post_create_inferior.
Tested only by rebuilding.
gdb/ChangeLog:
* observable.h <inferior_created>: Remove parameters. Update all
listeners.
* inferior.h (post_create_inferior): Remove target parameter.
Update all callers.
Change-Id: I8944cefdc4447ed5347dc927b75abf1e7a0e27e6
This changes the object-like macro target_have_steppable_watchpoint
into an inline function.
gdb/ChangeLog
2020-09-28 Tom Tromey <tom@tromey.com>
* infrun.c (displaced_step_fixup, thread_still_needs_step_over)
(handle_signal_stop): Update.
* procfs.c (procfs_target::insert_watchpoint): Update.
* target.h (target_have_steppable_watchpoint): Now a function.
This changes the object-like macro target_can_lock_scheduler into an
inline function.
gdb/ChangeLog
2020-09-28 Tom Tromey <tom@tromey.com>
* infrun.c (set_schedlock_func): Update.
* target.h (target_can_lock_scheduler): Now a function.
This changes target_can_execute_reverse from an object-like macro to
an inline function.
gdb/ChangeLog
2020-09-28 Tom Tromey <tom@tromey.com>
* mi/mi-main.c (exec_reverse_continue)
(mi_cmd_list_target_features): Update.
* infrun.c (set_exec_direction_func): Update.
* target.c (default_execution_direction): Update.
* reverse.c (exec_reverse_once): Update.
* target.h (target_can_execute_reverse): Now a function.
PR gdb/26598 notes that, before commit bcfe6157ca ("Use the linkage
name if it exists"), the "skip" command would match the demangled name
of a symbol, but now only matches the linkage name.
This patch fixes this regression. I looked at all calls to
function_name_is_marked_for_skip, and only one used the linkage name.
2020-09-16 Tom Tromey <tromey@adacore.com>
PR gdb/26598:
* infrun.c (fill_in_stop_func): Use find_pc_partial_function_sym.
gdb/testsuite/ChangeLog
2020-09-16 Tom Tromey <tromey@adacore.com>
PR gdb/26598:
* gdb.base/skipcxx.exp: New file.
* gdb.base/skipcxx.cc: New file.
I noticed this while testing the GDB in the context of the upcoming
GDB 10 release branching, because part of the process involves setting
development to False, which in turn changes the default for including
unittest to false as well. As a result, without this patch, we get
compilation errors in infrun.c such as:
infrun.c:9219:5: error: `scoped_mock_context' was not declared in this scope
This patch fixes it by bracketing the unitttest in namespace selftest
with an #if GDB_SELF_TEST.
gdb/ChangeLog:
* infrun.c (namespace selftests): Only define #if GDB_SELF_TEST.
Tested on x86_64-linux, with and without self-tests.
Use the available `switch_to_target_no_thread` function to switch the
target. This is a refactoring.
gdb/ChangeLog:
2020-09-07 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
* infrun.c (fetch_inferior_event): Use
`switch_to_target_no_thread` to switch the target.
To help ensure that all debug statements have the same format, introduce
the debug_prefixed_vprintf helper. Implement linux_nat_debug_printf_1
and infrun_debug_printf_1 with it.
I would eventually like to style the module and function name with some
color, to help them stick out, but I don't really know how to do that
yet, it can always be done later.
gdb/ChangeLog:
* debug.h: New file.
* debug.c (debug_prefixed_vprintf): New function.
* infrun.c (infrun_debug_printf_1): Use debug_prefixed_vprintf.
* linux-nat.c (linux_nat_debug_printf_1): Likewise.
Change-Id: Iccc290a2dc6b5fffcbe1c2866ed8d804ad380764
Introduce this macro to print debug statements in the infrun.c file,
same idea as what was done in 9327494e0e ("gdb: add
linux_nat_debug_printf macro").
Although in this case, there are places outside infrun.c that print
debug statements if debug_infrun is set. So the macro has to be
declared in the header file, so that it can be used in these other
files.
Note one special case. In stop_all_threads, I've used an explicit
if (debug_infrun)
infrun_debug_printf_1 ("stop_all_threads", "done");
for the message in the SCOPE_EXIT. Otherwise, the message appears like
this:
[infrun] operator(): done
Until we find a better solution for extracting a meaningful function
name for lambda functions, I think it's fine to handle these special
cases manually, they are quite rare.
Some tests need to be updated, because they rely on some infrun debug
statements.
gdb/ChangeLog:
* infrun.h (infrun_debug_printf_1): New function declaration.
(infrun_debug_printf): New macro.
* infrun.c (infrun_debug_printf_1): Use infrun_debug_printf
throughout.
(infrun_debug_printf): New function.
* breakpoint.c (should_be_inserted): Use infrun_debug_printf.
(handle_jit_event): Likewise.
gdb/testsuite/ChangeLog:
* gdb.base/gdb-sigterm.exp (do_test): Update expected regexp.
* gdb.threads/signal-while-stepping-over-bp-other-thread.exp:
Likewise.
* gdb.threads/stepi-random-signal.exp: Likewise.
Change-Id: I66433c8a9caa64c8525ab57c593022b9d1956d5c
I noticed what I think is a potential bug. I did not observe it nor was
I able to reproduce it using actual debugging. It's quite unlikely,
because it involves multi-target and ptid clashes. I added selftests
that demonstrate it though.
The thread_ptid_changed observer says that thread with OLD_PTID now has
NEW_PTID. Now, if for some reason we happen to have two targets
defining a thread with OLD_PTID, the observers don't know which thread
this is about.
regcache::regcache_thread_ptid_changed changes all regcaches with
OLD_PTID. If there is a regcache for a thread with ptid OLD_PTID, but
that belongs to a different target, this regcache will be erroneously
changed.
Similarly, infrun_thread_ptid_changed updates inferior_ptid if
inferior_ptid matches OLD_PTID. But if inferior_ptid currently refers
not to the thread is being changed, but to a thread with the same ptid
belonging to a different target, then inferior_ptid will erroneously be
changed.
This patch adds a `process_stratum_target *` parameter to the
`thread_ptid_changed` observable and makes the two observers use it.
Tests for both are added, which would fail if the corresponding fix
wasn't done.
gdb/ChangeLog:
* observable.h (thread_ptid_changed): Add parameter
`process_stratum_target *`.
* infrun.c (infrun_thread_ptid_changed): Add parameter
`process_stratum_target *` and use it.
(selftests): New namespace.
(infrun_thread_ptid_changed): New function.
(_initialize_infrun): Register selftest.
* regcache.c (regcache_thread_ptid_changed): Add parameter
`process_stratum_target *` and use it.
(regcache_thread_ptid_changed): New function.
(_initialize_regcache): Register selftest.
* thread.c (thread_change_ptid): Pass target to
thread_ptid_changed observable.
Change-Id: I0599e61224b6d154a7b55088a894cb88298c3c71
This is a more general version of the existing handle_segmentation_fault
hook that is able to report information for an arbitrary signal, not
just SIGSEGV.
gdb/ChangeLog:
* gdbarch.c: Regenerate.
* gdbarch.h: Regenerate.
* gdbarch.sh (report_signal_info): New method.
* infrun.c (print_signal_received_reason): Invoke gdbarch
report_signal_info hook if present.
When interrupting a program in non-stop, the program gets interrupted
correctly, but GDB busy loops (the event loop is always woken up).
Here is how to reproduce it:
1. Start GDB: ./gdb -nx --data-directory=data-directory -ex "set non-stop 1" --args /bin/sleep 60
2. Run the program with "run"
3. Interrupt with ^C.
4. Look into htop, see GDB taking 100% CPU
Debugging `handle_file_event`, we see that the event source that wakes
up the event loop is the linux-nat one:
(top-gdb) p file_ptr.proc
$5 = (handler_func *) 0xb9cccd <handle_target_event(int, gdb_client_data)>
^^^^^^^^^^^^^^^^^^^
|
\-- the linux-nat callback
Debugging fetch_inferior_event and do_target_wait, we see that we
don't actually call `wait` on the linux-nat target, because
inferior_matches returns false:
auto inferior_matches = [&wait_ptid] (inferior *inf)
{
return (inf->process_target () != NULL
&& (threads_are_executing (inf->process_target ())
|| threads_are_resumed_pending_p (inf))
&& ptid_t (inf->pid).matches (wait_ptid));
};
because `threads_are_executing` is false.
What happens is:
1. User types ctrl-c, that writes in the linux-nat pipe, waking up
the event source.
2. linux-nat's wait gets called, the SIGINT event is returned, but
before returning, it marks the pipe again, in order for wait to
get called again:
/* If we requested any event, and something came out, assume there
may be more. If we requested a specific lwp or process, also
assume there may be more. */
if (target_is_async_p ()
&& ((ourstatus->kind != TARGET_WAITKIND_IGNORE
&& ourstatus->kind != TARGET_WAITKIND_NO_RESUMED)
|| ptid != minus_one_ptid))
async_file_mark ();
3. The SIGINT event is handled, the program is stopped, the stop
notification is printed.
4. The event loop is woken up again because of the `async_file_mark`
of step 2.
5. Because `inferior_matches` returns false, we never call
linux-nat's wait, so the pipe stays readable.
6. Goto 4.
Pedro says:
This commit fixes it by letting do_target_wait call target_wait even
if threads_are_executing is false. This will normally result in the
target returning TARGET_WAITKIND_NO_RESUMED, and _not_ marking its
event source again. This results in infrun only calling into the
target only once (i.e., breaking the busy loop).
Note that the busy loop bug didn't trigger in all-stop mode because
all-stop handles this by unregistering the target from the event loop
as soon as it was all stopped -- see
inf-loop.c:inferior_event_handler's INF_EXEC_COMPLETE handling. If we
remove that non-stop check from inferior_event_handler, and replace
the target_has_execution check for threads_are_executing instead, it
also fixes the issue for non-stop. I considered that as the final
solution, but decided that the solution proposed here instead is just
simpler and more future-proof design. With the
TARGET_WAITKIND_NO_RESUMED handling fixes done in the previous
patches, I think it should be possible to always keep the target
registered in the event loop, meaning we could eliminate the
target_async(0) call from inferior_event_handler as well as most of
the target_async(1) calls in the target backends. That would allow in
the future e.g., the remote target reporting asynchronous
notifications even if all threads are stopped. I haven't attempted
that, though.
gdb/ChangeLog:
yyyy-mm-dd Simon Marchi <simon.marchi@polymtl.ca>
Pedro Alves <pedro@palves.net>
PR gdb/26199
* infrun.c (threads_are_resumed_pending_p): Delete.
(do_target_wait): Remove threads_are_executing and
threads_are_resumed_pending_p checks from the inferior_matches
lambda. Update comments.
Let's consider the same use case as in the previous commit:
Say you have two inferiors 1 and 2, each connected to a different
target, A and B.
Now say you set inferior 2 running, with "continue &".
Now you select a thread of inferior 1, say thread 1.2, and continue in
the foreground. All other threads of inferior 1 are left stopped.
Thread 1.2 exits, and thus target A has no other resumed thread, so it
reports TARGET_WAITKIND_NO_RESUMED.
At this point, because the threads of inferior 2 are still executing
the TARGET_WAITKIND_NO_RESUMED event is ignored.
Now, the user types Ctrl-C. Because GDB had previously put inferior 1
in the foreground, the kernel sends the SIGINT to that inferior.
However, no thread in that inferior is executing right now, so ptrace
never intercepts the SIGINT -- it is never dequeued by any thread.
The result is that GDB's CLI is stuck. There's no way to get back the
prompt (unless inferior 2 happens to report some event).
The fix in this commit is to make handle_no_resumed give the terminal
to some other inferior that still has threads executing so that a
subsequent Ctrl-C reaches that target first (and then GDB intercepts
the SIGINT). This is a bit hacky, but seems like the best we can do
with the current design.
I think that putting all native inferiors in their own session would
help fixing this in a clean way, since with that a Ctrl-C on GDB's
terminal will _always_ reach GDB first, and then GDB can decide how to
pause the inferior. But that's a much larger change.
The testcase added by the following patch needs this fix.
gdb/ChangeLog:
PR gdb/26199
* infrun.c (handle_no_resumed): Transfer terminal to inferior with
executing threads.
handle_no_resumed is currently not considering multiple targets.
Say you have two inferiors 1 and 2, each connected to a different
target, A and B.
Now say you set inferior 2 running, with "continue &".
Now you select a thread of inferior 1, say thread 1.2, and continue in
the foreground. All other threads of inferior 1 are left stopped.
Thread 1.2 exits, and thus target A has no other resumed thread, so it
reports TARGET_WAITKIND_NO_RESUMED.
At this point, if both inferiors were running in the same target,
handle_no_resumed would realize that threads of inferior 2 are still
executing, so the TARGET_WAITKIND_NO_RESUMED event should be ignored.
But because handle_no_resumed only walks the threads of the current
target, it misses noticing that threads of inferior 2 are still
executing. The fix is just to walk over all threads of all targets.
A testcase covering the use case above will be added in a following
patch. It can't be added yet because it depends on yet another fix to
handle_no_resumed not included here.
gdb/ChangeLog:
PR gdb/26199
* infrun.c (handle_no_resumed): Handle multiple targets.
If we hit the synchronous execution command case described by
handle_no_resumed, and handle_no_resumed determines that the event
should be ignored, because it found a thread that is executing, we end
up in prepare_to_wait.
There, if the current target is not registered in the event loop right
now, we call mark_infrun_async_event_handler. With that event handler
marked, the event loop calls again into fetch_inferior_event, which
calls target_wait, which returns TARGET_WAITKIND_NO_RESUMED, and we
end up in handle_no_resumed, again ignoring the event and marking
infrun_async_event_handler. The result is that GDB is now always
keeping the CPU 100% busy in this loop, even though it continues to be
able to react to input and to real target events, because we still go
through the event-loop.
The problem is that marking of the infrun_async_event_handler in
prepare_to_wait. That is there to handle targets that don't support
asynchronous execution. So the correct predicate is whether async
execution is supported, not whether the target is async right now.
gdb/ChangeLog:
PR gdb/26199
* infrun.c (prepare_to_wait): Check target_can_async_p instead of
target_is_async_p.
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
