A recent patch from Andrew pointed out that gdbarch_inner_than returns
'int', while it should really return 'bool'.
Approved-By: Pedro Alves <pedro@palves.net>
This commit introduces a new target hook, target_is_address_tagged,
which is used instead of the gdbarch_tagged_address_p gdbarch hook in
the upper layer (printcmd.c).
This change enables easy specialization of memory tagging address
check per target in the future. As target_is_address_tagged continues
to utilize the gdbarch_tagged_address_p hook, there is no change in
behavior for all the targets that use the new target hook (i.e., the
remote.c, aarch64-linux-nat.c, and corelow.c targets).
Just the gdbarch_tagged_address_p signature is changed for convenience,
since target_is_address_tagged takes the address to be checked as a
CORE_ADDR type.
Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
Approved-By: Luis Machado <luis.machado@arm.com>
Tested-By: Luis Machado <luis.machado@arm.com>
I noticed that the disassembler_options code uses manual memory
management. It seemed simpler to replace this with std::string.
Approved-By: John Baldwin <jhb@FreeBSD.org>
We currently pass frames to function by value, as `frame_info_ptr`.
This is somewhat expensive:
- the size of `frame_info_ptr` is 64 bytes, which is a bit big to pass
by value
- the constructors and destructor link/unlink the object in the global
`frame_info_ptr::frame_list` list. This is an `intrusive_list`, so
it's not so bad: it's just assigning a few points, there's no memory
allocation as if it was `std::list`, but still it's useless to do
that over and over.
As suggested by Tom Tromey, change many function signatures to accept
`const frame_info_ptr &` instead of `frame_info_ptr`.
Some functions reassign their `frame_info_ptr` parameter, like:
void
the_func (frame_info_ptr frame)
{
for (; frame != nullptr; frame = get_prev_frame (frame))
{
...
}
}
I wondered what to do about them, do I leave them as-is or change them
(and need to introduce a separate local variable that can be
re-assigned). I opted for the later for consistency. It might not be
clear why some functions take `const frame_info_ptr &` while others take
`frame_info_ptr`. Also, if a function took a `frame_info_ptr` because
it did re-assign its parameter, I doubt that we would think to change it
to `const frame_info_ptr &` should the implementation change such that
it doesn't need to take `frame_info_ptr` anymore. It seems better to
have a simple rule and apply it everywhere.
Change-Id: I59d10addef687d157f82ccf4d54f5dde9a963fd0
Approved-By: Andrew Burgess <aburgess@redhat.com>
I don't like the name `target_so_ops`, because:
- The name `target` is so overloaded, and in this case it's not even
related to target_ops or anything else called "target".
- We do have an implementation that actually fetches solibs from the
target (solib_target_so_op in solib-target.c), so it's confusing for
the "base class" to be called target_something as well.
Rename to solib_ops.
Change-Id: I46a983d44e81400470e22deb09aaf26ad8a3587f
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.
Pass a frame_info_ptr rather than a frame_id. This avoids having to do
a frame lookup on the callee side, when we can just pass the frame down
directly.
I think this fixes a bug in rs6000-tdep.c where the id of the wrong
frame was set to `VALUE_NEXT_FRAME_ID (v)`.
Change-Id: I77039bc87ea8fc5262f16d0e1446515efa21c565
Add a new variant of gdbarch_pseudo_register_write that takes a
frame_info in order to write raw registers. Use this new method when
available:
- in put_frame_register, when trying to write a pseudo register to a given frame
- in regcache::cooked_write
No implementation is migrated to use this new method (that will come in
subsequent patches), so no behavior change is expected here.
The objective is to fix writing pseudo registers to non-current
frames. See previous commit "gdb: read pseudo register through
frame" for a more detailed explanation.
Change-Id: Ie7fe364a15a4d86c2ecb09de2b4baa08c45555ac
Reviewed-By: John Baldwin <jhb@FreeBSD.org>
The next patch introduces a new variant of gdbarch_pseudo_register_write
that takes a frame instead of a regcache for implementations to write
raw registers. Rename to old one to make it clear it's deprecated.
Change-Id: If8872c89c6f8a1edfcab983eb064248fd5ff3115
Reviewed-By: John Baldwin <jhb@FreeBSD.org>
Change gdbarch_pseudo_register_read_value to take a frame instead of a
regcache. The frame (and formerly the regcache) is used to read raw
registers needed to make up the pseudo register value. The problem with
using the regcache is that it always provides raw register values for
the current frame (frame 0).
Let's say the user wants to read the ebx register on amd64. ebx is a pseudo
register, obtained by reading the bottom half (bottom 4 bytes) of the
rbx register, which is a raw register. If the currently selected frame
is frame 0, it works fine:
(gdb) frame 0
#0 break_here_asm () at /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.arch/amd64-pseudo-unwind-asm.S:36
36 in /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.arch/amd64-pseudo-unwind-asm.S
(gdb) p/x $ebx
$1 = 0x24252627
(gdb) p/x $rbx
$2 = 0x2021222324252627
But if the user is looking at another frame, and the raw register behind
the pseudo register has been saved at some point in the call stack, then
we get a wrong answer:
(gdb) frame 1
#1 0x000055555555517d in caller () at /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.arch/amd64-pseudo-unwind-asm.S:56
56 in /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.arch/amd64-pseudo-unwind-asm.S
(gdb) p/x $ebx
$3 = 0x24252627
(gdb) p/x $rbx
$4 = 0x1011121314151617
Here, the value of ebx was computed using the value of rbx in frame 0
(through the regcache), it should have been computed using the value of
rbx in frame 1.
In other to make this work properly, make the following changes:
- Make dwarf2_frame_prev_register return nullptr if it doesn't know how
to unwind a register and that register is a pseudo register.
Previously, it returned `frame_unwind_got_register`, meaning, in our
example, "the value of ebx in frame 1 is the same as the value of ebx
in frame 0", which is obviously false. Return nullptr as a way to
say "I don't know".
- In frame_unwind_register_value, when prev_register (for instance
dwarf2_frame_prev_register) returns nullptr, and we are trying to
read a pseudo register, try to get the register value through
gdbarch_pseudo_register_read_value or gdbarch_pseudo_register_read.
If using gdbarch_pseudo_register_read, the behavior is known to be
broken. Implementations should be migrated to use
gdbarch_pseudo_register_read_value to fix that.
- Change gdbarch_pseudo_register_read_value to take a frame_info
instead of a regcache, update implementations (aarch64, amd64, i386).
In i386-tdep.c, I made a copy of i386_mmx_regnum_to_fp_regnum that
uses a frame instead of a regcache. The version using the regcache
is still used by i386_pseudo_register_write. It will get removed in
a subsequent patch.
- Add some helpers in value.{c,h} to implement the common cases of
pseudo registers: taking part of a raw register and concatenating
multiple raw registers.
- Update readable_regcache::{cooked_read,cooked_read_value} to pass the
current frame to gdbarch_pseudo_register_read_value. Passing the
current frame will give the same behavior as before: for frame 0, raw
registers will be read from the current thread's regcache.
Notes:
- I do not plan on changing gdbarch_pseudo_register_read to receive a
frame instead of a regcache. That method is considered deprecated.
Instead, we should be working on migrating implementations to use
gdbarch_pseudo_register_read_value instead.
- In frame_unwind_register_value, we still ask the unwinder to try to
unwind pseudo register values. It's apparently possible for the
debug info to provide information about [1] pseudo registers, so we
want to try that first, before falling back to computing them
ourselves.
[1] https://inbox.sourceware.org/gdb-patches/20180528174715.A954AD804AD@oc3748833570.ibm.com/
Change-Id: Id6ef1c64e19090a183dec050e4034d8c2394e7ca
Reviewed-by: John Baldwin <jhb@FreeBSD.org>
GDB doesn't handle correctly the case where a thread steps over a
breakpoint (using either in-line or displaced stepping), and the
executed instruction causes the thread to exit.
Using the test program included later in the series, this is what it
looks like with displaced-stepping, on x86-64 Linux, where we have two
displaced-step buffers:
$ ./gdb -q -nx --data-directory=data-directory build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-over-thread-exit/step-over-thread-exit -ex "b my_exit_syscall" -ex r
Reading symbols from build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-over-thread-exit/step-over-thread-exit...
Breakpoint 1 at 0x123c: file src/binutils-gdb/gdb/testsuite/lib/my-syscalls.S, line 68.
Starting program: build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-over-thread-exit/step-over-thread-exit
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/usr/lib/../lib/libthread_db.so.1".
[New Thread 0x7ffff7c5f640 (LWP 2915510)]
[Switching to Thread 0x7ffff7c5f640 (LWP 2915510)]
Thread 2 "step-over-threa" hit Breakpoint 1, my_exit_syscall () at src/binutils-gdb/gdb/testsuite/lib/my-syscalls.S:68
68 syscall
(gdb) c
Continuing.
[New Thread 0x7ffff7c5f640 (LWP 2915524)]
[Thread 0x7ffff7c5f640 (LWP 2915510) exited]
[Switching to Thread 0x7ffff7c5f640 (LWP 2915524)]
Thread 3 "step-over-threa" hit Breakpoint 1, my_exit_syscall () at src/binutils-gdb/gdb/testsuite/lib/my-syscalls.S:68
68 syscall
(gdb) c
Continuing.
[New Thread 0x7ffff7c5f640 (LWP 2915616)]
[Thread 0x7ffff7c5f640 (LWP 2915524) exited]
[Switching to Thread 0x7ffff7c5f640 (LWP 2915616)]
Thread 4 "step-over-threa" hit Breakpoint 1, my_exit_syscall () at src/binutils-gdb/gdb/testsuite/lib/my-syscalls.S:68
68 syscall
(gdb) c
Continuing.
... hangs ...
The first two times we do "continue", we displaced-step the syscall
instruction that causes the thread to exit. When the thread exits,
the main thread, waiting on pthread_join, is unblocked. It spawns a
new thread, which hits the breakpoint on the syscall again. However,
infrun was never notified that the displaced-stepping threads are done
using the displaced-step buffer, so now both buffers are marked as
used. So when we do the third continue, there are no buffers
available to displaced-step the syscall, so the thread waits forever
for its turn.
When trying the same but with in-line step over (displaced-stepping
disabled):
$ ./gdb -q -nx --data-directory=data-directory \
build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-over-thread-exit/step-over-thread-exit \
-ex "b my_exit_syscall" -ex "set displaced-stepping off" -ex r
Reading symbols from build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-over-thread-exit/step-over-thread-exit...
Breakpoint 1 at 0x123c: file src/binutils-gdb/gdb/testsuite/lib/my-syscalls.S, line 68.
Starting program: build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-over-thread-exit/step-over-thread-exit
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/usr/lib/../lib/libthread_db.so.1".
[New Thread 0x7ffff7c5f640 (LWP 2928290)]
[Switching to Thread 0x7ffff7c5f640 (LWP 2928290)]
Thread 2 "step-over-threa" hit Breakpoint 1, my_exit_syscall () at src/binutils-gdb/gdb/testsuite/lib/my-syscalls.S:68
68 syscall
(gdb) c
Continuing.
[Thread 0x7ffff7c5f640 (LWP 2928290) exited]
No unwaited-for children left.
(gdb) i th
Id Target Id Frame
1 Thread 0x7ffff7c60740 (LWP 2928285) "step-over-threa" 0x00007ffff7f7c9b7 in __pthread_clockjoin_ex () from /usr/lib/libpthread.so.0
The current thread <Thread ID 2> has terminated. See `help thread'.
(gdb) thread 1
[Switching to thread 1 (Thread 0x7ffff7c60740 (LWP 2928285))]
#0 0x00007ffff7f7c9b7 in __pthread_clockjoin_ex () from /usr/lib/libpthread.so.0
(gdb) c
Continuing.
^C^C
... hangs ...
The "continue" causes an in-line step to occur, meaning the main
thread is stopped while we step the syscall. The stepped thread exits
when executing the syscall, the linux-nat target notices there are no
more resumed threads to be waited for, so returns
TARGET_WAITKIND_NO_RESUMED, which causes the prompt to return. But
infrun never clears the in-line step over info. So if we try
continuing the main thread, GDB doesn't resume it, because it thinks
there's an in-line step in progress that we need to wait for to
finish, and we are stuck there.
To fix this, infrun needs to be informed when a thread doing a
displaced or in-line step over exits. We can do that with the new
target_set_thread_options mechanism which is optimal for only enabling
exit events of the thread that needs it; or, if that is not supported,
by using target_thread_events, which enables thread exit events for
all threads. This is done by this commit.
This patch then modifies handle_inferior_event in infrun.c to clean up
any step-over the exiting thread might have been doing at the time of
the exit. The cases to consider are:
- the exiting thread was doing an in-line step-over with an all-stop
target
- the exiting thread was doing an in-line step-over with a non-stop
target
- the exiting thread was doing a displaced step-over with a non-stop
target
The displaced-stepping buffer implementation in displaced-stepping.c
is modified to account for the fact that it's possible that we
"finish" a displaced step after a thread exit event. The buffer that
the exiting thread was using is marked as available again and the
original instructions under the scratch pad are restored. However, it
skips applying the fixup, which wouldn't make sense since the thread
does not exist anymore.
Another case that needs handling is if a displaced-stepping thread
exits, and the event is reported while we are in stop_all_threads. We
should call displaced_step_finish in the handle_one function, in that
case. It was already called in other code paths, just not the "thread
exited" path.
This commit doesn't make infrun ask the target to report the
TARGET_WAITKIND_THREAD_EXITED events yet, that'll be done later in the
series.
Note that "stop_print_frame = false;" line is moved to normal_stop,
because TARGET_WAITKIND_THREAD_EXITED can also end up with the event
transmorphed into TARGET_WAITKIND_NO_RESUMED. Moving it to
normal_stop keeps it centralized.
Co-authored-by: Simon Marchi <simon.marchi@efficios.com>
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27338
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
Change-Id: I745c6955d7ef90beb83bcf0ff1d1ac8b9b6285a5
Due to the nature of the AArch64 SVE/SME extensions in GDB, each thread
can potentially have distinct target descriptions/gdbarches.
When loading a gcore-generated core file, at the moment GDB gives priority
to the target description dumped to NT_GDB_TDESC. Though technically correct
for most targets, it doesn't work correctly for AArch64 with SVE or SME
support.
The correct approach for AArch64/Linux is to either have per-thread target
description notes in the corefiles or to rely on the
gdbarch_core_read_description hook, so it can figure out the proper target
description for a given thread based on the various available register notes.
The former, although more correct, doesn't address the case of existing gdb's
that only output a single target description note.
This patch goes for the latter, and adds a new gdbarch hook to conditionalize
the use of the corefile target description note. The hook is called
use_target_description_from_corefile_notes.
The hook defaults to returning true, meaning targets will use the corefile
target description note. AArch64 Linux overrides the hook to return false
when it detects any of the SVE or SME register notes in the corefile.
Otherwise it should be fine for AArch64 Linux to use the corefile target
description note.
When we support per-thread target description notes, then we can augment
the AArch64 Linux hook to rely on those notes.
Regression-tested on aarch64-linux Ubuntu 22.04/20.04.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
Add gdbarch_core_read_x86_xsave_layout to fetch the x86 XSAVE layout
structure from a core file.
Current OS's do not export the offsets of XSAVE state components in
core dumps, so provide an i387_guess_xsave_layout helper function to
set offsets based on known combinations of XCR0 masks and total state
sizes. Eventually when core dumps do contain this information this
function should only be used as a fall back for older core dumps.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Fix grammar in some comments and docs:
- machines that doesn't -> machines that don't
- its a -> it's a
- its the -> it's the
- if does its not -> if it does it's not
- one more instructions if doesn't match ->
one more instruction if it doesn't match
- it's own -> its own
- it's first -> its first
- it's pointer -> its pointer
I also came across "it's performance" in gdb/stubs/*-stub.c in the HP public
domain notice, I've left that alone.
Tested on x86_64-linux.
On PPC64, with the test case included in an earlier patch, we found
that "finish" would still not correctly find the return value via
entry values.
The issue is simple. The compiler emits:
0x00000000100032b8 <+28>: bl 0x1000320c <pck__create_large>
0x00000000100032bc <+32>: nop
0x00000000100032c0 <+36>: li r9,42
... but the DWARF says:
<162a> DW_AT_call_return_pc: 0x100032c0
That is, the declared return PC is one instruction past the actual
return PC.
This patch adds a new arch hook to handle this scenario, and
implements it for PPC64. Some care is taken so that GDB will continue
to work if this compiler bug is fixed. A GCC patch is here:
https://gcc.gnu.org/pipermail/gcc-patches/2023-March/613336.html
No check for 'nop' is done, as subsequent discussion revealed that the
linker might replace this with another instruction.
PowerPC supports two 128-bit floating point formats, the IBM long double
and IEEE 128-bit float. The issue is the DWARF information does not
distinguish between the two. There have been proposals of how to extend
the DWARF information as discussed in
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104194
but has not been fully implemented.
GCC introduced the _Float128 internal type as a work around for the issue.
The workaround is not transparent to GDB. The internal _Float128 type
name is printed rather then the user specified long double type. This
patch adds a new gdbarch method to allow PowerPC to detect the GCC
workaround. The workaround checks for "_Float128" name when reading the
base typedef from the die_info. If the workaround is detected, the type
and format fields from the _Float128 typedef are copied to the long
double typedef. The same is done for the complex long double typedef.
This patch fixes 74 regression test failures in
gdb.base/whatis-ptype-typedefs.exp on PowerPC with IEEE float 128 as the
default on GCC. It fixes one regression test failure in
gdb.base/complex-parts.exp.
The patch has been tested on Power 10 where GCC defaults to IEEE Float
128-bit and on Power 10 where GCC defaults to the IBM 128-bit float. The
patch as also been tested on X86-64 with no new regression failures.
The following commit changed gdbarch_components.py but failed to
format it with black:
commit cf141dd8cc
Date: Wed Feb 22 12:15:34 2023 +0000
gdb: fix reg corruption from displaced stepping on amd64
This commit just runs black on the file and commits the result.
The change is just the addition of an extra "," -- there will be no
change to the generated source files after this commit.
There will be no user visible changes after this commit.
This commit aims to address a problem that exists with the current
approach to displaced stepping, and was identified in PR gdb/22921.
Displaced stepping is currently supported on AArch64, ARM, amd64,
i386, rs6000 (ppc), and s390. Of these, I believe there is a problem
with the current approach which will impact amd64 and ARM, and can
lead to random register corruption when the inferior makes use of
asynchronous signals and GDB is using displaced stepping.
The problem can be found in displaced_step_buffers::finish in
displaced-stepping.c, and is this; after GDB tries to perform a
displaced step, and the inferior stops, GDB classifies the stop into
one of two states, either the displaced step succeeded, or the
displaced step failed.
If the displaced step succeeded then gdbarch_displaced_step_fixup is
called, which has the job of fixing up the state of the current
inferior as if the step had not been performed in a displaced manner.
This all seems just fine.
However, if the displaced step is considered to have not completed
then GDB doesn't call gdbarch_displaced_step_fixup, instead GDB
remains in displaced_step_buffers::finish and just performs a minimal
fixup which involves adjusting the program counter back to its
original value.
The problem here is that for amd64 and ARM setting up for a displaced
step can involve changing the values in some temporary registers. If
the displaced step succeeds then this is fine; after the step the
temporary registers are restored to their original values in the
architecture specific code.
But if the displaced step does not succeed then the temporary
registers are never restored, and they retain their modified values.
In this context a temporary register is simply any register that is
not otherwise used by the instruction being stepped that the
architecture specific code considers safe to borrow for the lifetime
of the instruction being stepped.
In the bug PR gdb/22921, the amd64 instruction being stepped is
an rip-relative instruction like this:
jmp *0x2fe2(%rip)
When we displaced step this instruction we borrow a register, and
modify the instruction to something like:
jmp *0x2fe2(%rcx)
with %rcx having its value adjusted to contain the original %rip
value.
Now if the displaced step does not succeed, then %rcx will be left
with a corrupted value. Obviously corrupting any register is bad; in
the bug report this problem was spotted because %rcx is used as a
function argument register.
And finally, why might a displaced step not succeed? Asynchronous
signals provides one reason. GDB sets up for the displaced step and,
at that precise moment, the OS delivers a signal (SIGALRM in the bug
report), the signal stops the inferior at the address of the displaced
instruction. GDB cancels the displaced instruction, handles the
signal, and then tries again with the displaced step. But it is that
first cancellation of the displaced step that causes the problem; in
that case GDB (correctly) sees the displaced step as having not
completed, and so does not perform the architecture specific fixup,
leaving the register corrupted.
The reason why I think AArch64, rs600, i386, and s390 are not effected
by this problem is that I don't believe these architectures make use
of any temporary registers, so when a displaced step is not completed
successfully, the minimal fix up is sufficient.
On amd64 we use at most one temporary register.
On ARM, looking at arm_displaced_step_copy_insn_closure, we could
modify up to 16 temporary registers, and the instruction being
displaced stepped could be expanded to multiple replacement
instructions, which increases the chances of this bug triggering.
This commit only aims to address the issue on amd64 for now, though I
believe that the approach I'm proposing here might be applicable for
ARM too.
What I propose is that we always call gdbarch_displaced_step_fixup.
We will now pass an extra argument to gdbarch_displaced_step_fixup,
this a boolean that indicates whether GDB thinks the displaced step
completed successfully or not.
When this flag is false this indicates that the displaced step halted
for some "other" reason. On ARM GDB can potentially read the
inferior's program counter in order figure out how far through the
sequence of replacement instructions we got, and from that GDB can
figure out what fixup needs to be performed.
On targets like amd64 the problem is slightly easier as displaced
stepping only uses a single replacement instruction. If the displaced
step didn't complete the GDB knows that the single instruction didn't
execute.
The point is that by always calling gdbarch_displaced_step_fixup, each
architecture can now ensure that the inferior state is fixed up
correctly in all cases, not just the success case.
On amd64 this ensures that we always restore the temporary register
value, and so bug PR gdb/22921 is resolved.
In order to move all architectures to this new API, I have moved the
minimal roll-back version of the code inside the architecture specific
fixup functions for AArch64, rs600, s390, and ARM. For all of these
except ARM I think this is good enough, as no temporaries are used all
that's needed is the program counter restore anyway.
For ARM the minimal code is no worse than what we had before, though I
do consider this architecture's displaced-stepping broken.
I've updated the gdb.arch/amd64-disp-step.exp test to cover the
'jmpq*' instruction that was causing problems in the original bug, and
also added support for testing the displaced step in the presence of
asynchronous signal delivery.
I've also added two new tests (for amd64 and i386) that check that GDB
can correctly handle displaced stepping over a single instruction that
branches to itself. I added these tests after a first version of this
patch relied too much on checking the program-counter value in order
to see if the displaced instruction had executed. This works fine in
almost all cases, but when an instruction branches to itself a pure
program counter check is not sufficient. The new tests expose this
problem.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=22921
Approved-By: Pedro Alves <pedro@palves.net>
This commit tweaks displaced_step_finish & friends to pass down a
target_waitstatus instead of a gdb_signal. This is needed because a
patch later in the step-over-{thread-exit,clone] series will want to
make displaced_step_buffers::finish handle
TARGET_WAITKIND_THREAD_EXITED. It also helps with the
TARGET_WAITKIND_THREAD_CLONED patch later in that same series.
It's also a bit more logical this way, as we don't have to pass down
signals when the thread didn't actually stop for a signal. So we can
also think of it as a clean up.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27338
Change-Id: I4c5d338647b028071bc498c4e47063795a2db4c0
Approved-By: Andrew Burgess <aburgess@redhat.com>
The comment on the gdbarch_displaced_step_fixup gdbarch method
indicates that this method is optional and that GDB will perform some
default if this method is not supplied. As such we define a predicate
gdbarch_displaced_step_fixup_p.
It may have been true at one point that the fixup method was optional,
but it is no longer true. If this method is not defined and GDB tries
to complete a displaced step, then GDB is going to crash.
Additionally the gdbarch_displaced_step_fixup_p predicate is not used
anywhere in GDB.
In this commit I have removed the gdbarch_displaced_step_fixup_p
predicate, and I have updated the validation check for the
gdbarch_displaced_step_fixup method; if the
gdbarch_displaced_step_copy_insn method is defined then the fixup
method must also be defined.
I believe I've manually checked all the current places where
gdbarch_displaced_step_copy_insn is defined and they all also define
the fixup method, so this change should cause no problems for anyone.
There should be no user visible changes after this commit.
Approved-By: Pedro Alves <pedro@palves.net>
The gdbarch::max_insn_length field is used mostly to support displaced
stepping; it controls the size of the buffers allocated for the
displaced-step instruction, and is also used when first copying the
instruction, and later, when fixing up the instruction, in order to
read in and parse the instruction being stepped.
However, it has started to be used in other places in GDB, for
example, it's used in the Python disassembler API, and it is used on
amd64 as part of branch-tracing instruction classification.
The problem is that the value assigned to max_insn_length is not
always the maximum instruction length, but sometimes is a multiple of
that length, as required to support displaced stepping, see rs600,
ARM, and AArch64 for examples of this.
It seems to me that we are overloading the meaning of the
max_insn_length field, and I think that could potentially lead to
confusion.
I propose that we add a new gdbarch field,
gdbarch::displaced_step_buffer_length, this new field will do
exactly what it says on the tin; represent the required displaced step
buffer size. The max_insn_length field can then do exactly what it
claims to do; represent the maximum length of a single instruction.
As some architectures (e.g. i386, and amd64) only require their
displaced step buffers to be a single instruction in size, I propose
that the default for displaced_step_buffer_length will be the
value of max_insn_length. Architectures than need more buffer space
can then override this default as needed.
I've updated all architectures to setup the new field if appropriate,
and I've audited all calls to gdbarch_max_insn_length and switched to
gdbarch_displaced_step_buffer_length where appropriate.
There should be no user visible changes after this commit.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
This commit switches the default value for the 'invalid' field from
False to True. All components that previous set the invalid field to
True explicitly have had the field removed.
I think that True is a good choice for the default, this means that we
now get the validity checks by default, and if anyone adds a new
Component they need to make a choice to add an 'invalid=False' line
and disable the validation.
The flip side of this is that 'invalid=False' seems to be far more
common than 'invalid=True'. But I don't see a huge problem with this,
we shouldn't be aiming to reduce our typing, rather we should choose
based on which is least likely to introduce bugs. I think assuming
that we should do a validity check will achieve that.
Some additional components need to have an 'invalid=False' line added
to their definition, these are components that have a predefault
value, which is sufficient; the tdep code doesn't need to replace this
value if it doesn't want to.
Without adding the 'invalid=False' these components would be
considered to be invalid if they have not changed from their
predefault value -- but the predefault is fine.
There's no change in the generated code after this commit, so there
will be no user visible changes after this commit.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
I noticed that there are a bunch of 'predefault="0"' lines in
gdbarch_components.py, and that some (just some, not all) of these are
not needed.
The gdbarch is already zero initialized, but these lines seem to
exists so that we can know when to compare against "0" and when to
compare against "NULL". At least, this seems to be useful in some
places in the generated code.
Specifically, if we remove the predefault="0" line from the
max_insn_length component then we end up generating a line like:
gdb_assert (gdbarch->max_insn_length != NULL);
which doesn't compile as we compare a ULONGEST to NULL.
In this commit I remove all the predefault="0" lines that I claim are
obviously not needed. These are lines for components that are not
Values (i.e. the component holds a function pointer anyway), or for
Value components that hold a pointer type, in which case using NULL is
fine.
The only changes after this commit are some fields that have nullptr
as their initial value, and gcore_bfd_target now compares to NULL not
0 in gdbarch_gcore_bfd_target_p, which, given the field is of type
'const char *', seems like an improvement.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
For some reason the following value components of gdbarch:
bfloat16_format
half_format
float_format
double_format
long_double_format
so_ops
All use a postdefault but no predefault to set the default value for
the component.
As the postdefault values for these components are all constant
pointers that don't depend on other fields within the gdbarch, then I
don't see any reason why we couldn't use a predefault instead.
So lets do that.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
This commit ensures that the 'invalid' property of all components is
either True, False, or a string.
Additionally, this commit allows a component to have both a predicate
and for the 'invalid' property to be a string.
Removing the option for 'invalid' to be None allows us to simplify the
algorithms in gdbarch.py a little.
Allowing a component to have both a predicate and an 'invalid' string
means that we can validate the value that a tdep sets into a field,
but also allow a predicate to ensure that the field has changed from
the default.
This functionality isn't going to be used in this series, but I have
tested it locally and believe that it would work, and this might make
it easier for others to add new components in the future.
In gdbarch_types.py, I've updated the type annotations to show that
the 'invalid' field should not be None, and I've changed the default
for this field from None to False.
The change to using False as the default is temporary. Later in this
series I'm going to change the default to True, but we need more fixes
before that can be done.
Additionally, in gdbarch_types.py I've removed an assert from
Component.get_predicate. This assert ensured that we didn't have the
predicate field set to True and the invalid field set to a string.
However, no component currently uses this configuration, and after
this commit, that combination is now supported, so the assert can be
removed.
As a consequence of the gdbarch_types.py changes we see some
additional comments generated in gdbarch.c about verification being
skipped due to the invalid field being False. This comment is inline
with plenty of other getters that also have a similar comment. Plenty
of the getters do have validation, so I think it is reasonable to have
a comment noting that the validation has been skipped for a specific
reason, rather than due to some bug.
In gdbarch_components.py I've had to add 'invalid=True' for two
components: gcore_bfd_target and max_insn_length, without this the
validation in the gdbarch getter would disappear.
And in gdbarch.py I've reworked the logic for generating the
verify_gdbarch function, and for generating the getter functions.
The logic for generating the getter functions is still not ideal, I
ended up having to add this additional logic block:
elif c.postdefault is not None and c.predefault is not None:
print(" /* Check variable changed from pre-default. */", file=f)
print(f" gdb_assert (gdbarch->{c.name} != {c.predefault});", file=f)
which was needed to ensure we continued to generate the same code as
before, without this the fact that invalid is now False when it would
previously have been None, meant that we dropped the gdb_assert in
favour of a comment like:
print(f" /* Skip verify of {c.name}, invalid_p == 0 */", file=f)
which is clearly not a good change. We could potentially look at
improving this in a later commit, but I don't plan to do that in this
series.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Restructure how gdbarch.py generates the verify_gdbarch function.
Previously the postdefault handling was bundled together with the
validation. This means that a field can't have both a postdefault,
and set its invalid attribute to a string.
This doesn't seem reasonable to me, I see no reason why a field can't
have both a postdefault (used when the tdep doesn't set the field),
and an invalid expression, which can be used to validate the value
that a tdep might set.
In this commit I restructure the verify_gdbarch generation code to
allow the above, there is no change in the actual generated code in
this commit, that will come in later commit.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Following on from the previous commit, this commit removes yet more
'invalid=True' lines from gdbarch_components.py where the invalid
setting has no effect.
Due to the algorithm used in gdbarch.py for generated verify_gdbarch,
if a component has a postdefault value then no invalid check will ever
be generated for the component, as such setting 'invalid=True' on the
component is pointless. This commit removes the setting of invalid.
There is no change in the generated code after this commit.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Due to the algorithm used to generate verify_gdbarch in gdbarch.py, if
a component has a predicate, then a validation check will never be
generated.
There are a bunch of components that are declared with both a
predicate AND have 'invalid=True' set. The 'invalid=True' has no
effect.
In this commit I clean things up by removing all these additional
'invalid=True' lines. There's no change in any of the generated files
after this commit.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Add type annotations to gdbarch*.py to fix all errors shown by pyright.
There is one change in copyright.py too, to fix this one:
/home/simark/src/binutils-gdb/gdb/gdbarch.py
/home/simark/src/binutils-gdb/gdb/gdbarch.py:206:13 - error: Type of "copyright" is partially unknown
Type of "copyright" is "(tool: Unknown, description: Unknown) -> str" (reportUnknownMemberType)
Change-Id: Ia109b53e267f6e2f5bd79a1288d0d5c9508c9ac4
Reviewed-By: Tom Tromey <tom@tromey.com>
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
Editing gdbarch-components.py is not an experience in an editor that is
minimally smart about Python. Because gdbarch-components.py is read and
exec'd by gdbarch.py, it doesn't import the Info / Method / Function /
Value types. And because these types are defined in gdbarch.py, it
can't import them, as that would make a cyclic dependency.
Solve this by introducing a third file, gdbarch_types.py, to define
these types. Make gdbarch.py and gdbarch-components.py import it.
Also, replace the read & exec of gdbarch-components.py by a regular
import. For this to work though, gdbarch-components.py needs to be
renamed to gdbarch_components.py.
Change-Id: Ibe994d56ef9efcc0698b3ca9670d4d9bf8bbb853
Reviewed-By: Tom Tromey <tom@tromey.com>
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
