When running test-case gdb.reverse/recursion.exp on arm-linux with target
board unix/-mthumb, I run into:
...
(gdb) PASS: gdb.reverse/recursion.exp: Skipping recursion from inside
reverse-next^M
bar (x=4195569) at /home/linux/gdb/src/gdb/testsuite/gdb.reverse/recursion.c:34^M
34 int r = foo (x);^M
(gdb) FAIL: gdb.reverse/recursion.exp: print frame when stepping out
...
The problem is the recording of the T1 push instruction [1,2], specifically:
...
000004d8 <foo>:
4d8: b580 push {r7, lr}
...
The current code fails to add a memory record for the memory written with the
value of the lr register.
Fix this by adding the missing memory record.
Tested on arm-linux.
Reviewed-By: Guinevere Larsen <guinevere@redhat.com>
Approved-By: Luis Machado <luis.machado@arm.com>
[1] https://developer.arm.com/documentation/ddi0406/c/Application-Level-Architecture/Instruction-Details/Encoding-of-lists-of-ARM-core-registers
[2] https://developer.arm.com/documentation/ddi0597/2024-09/T32-Instructions-by-Encoding/16-bit?lang=en#pushpop16
Make the current inferior reference bubble up one level. I think this
makes it clearer what gdbarch_update_p, which is update the passed
inferior's architecture (although the function name could probably be
better).
When gdbarch_find_by_info, it is possible for the new architecture's
init callback to be called. I have not audited all of them (there are
just too many), it's possible that some of them do care about the
current inferior, for some reason (for instance, if one of them makes a
target call). If so, they should be changed too.
Change-Id: I89f012188d7fdca395a830f4b013743565f26847
>From what I can see, lookup_minimal_symbol doesn't have any dependencies
on the global current state other than the single reference to
current_program_space. Add a program_space parameter and make that
current_program_space reference bubble up one level.
Change-Id: I759415e2f9c74c9627a2fe05bd44eb4147eee6fe
Reviewed-by: Keith Seitz <keiths@redhat.com>
Approved-By: Andrew Burgess <aburgess@redhat.com>
Most calls to lookup_minimal_symbol don't pass a value for sfile and
objf. Make these parameters optional (have a default value of
nullptr). And since passing a value to `objf` is much more common than
passing a value to `sfile`, swap the order so `objf` comes first, to
avoid having to pass a nullptr value to `sfile` when wanting to pass a
value to `objf`.
Change-Id: I8e9cc6b942e593bec640f9dfd30f62786b0f5a27
Reviewed-by: Keith Seitz <keiths@redhat.com>
Approved-By: Andrew Burgess <aburgess@redhat.com>
This is a simple find / replace from "struct bound_minimal_symbol" to
"bound_minimal_symbol", to make things shorter and more consisten
througout. In some cases, move variable declarations where first used.
Change-Id: Ica4af11c4ac528aa842bfa49a7afe8fe77a66849
Reviewed-by: Keith Seitz <keiths@redhat.com>
Approved-By: Andrew Burgess <aburgess@redhat.com>
Most files including gdbcmd.h currently rely on it to access things
actually declared in cli/cli-cmds.h (setlist, showlist, etc). To make
things easy, replace all includes of gdbcmd.h with includes of
cli/cli-cmds.h. This might lead to some unused includes of
cli/cli-cmds.h, but it's harmless, and much faster than going through
the 170 or so files by hand.
Change-Id: I11f884d4d616c12c05f395c98bbc2892950fb00f
Approved-By: Tom Tromey <tom@tromey.com>
Move the declarations out of defs.h, and the implementations out of
findvar.c.
I opted for a new file, because this functionality of converting
integers to bytes and vice-versa seems a bit to generic to live in
findvar.c.
Change-Id: I524858fca33901ee2150c582bac16042148d2251
Approved-By: John Baldwin <jhb@FreeBSD.org>
Now that defs.h, server.h and common-defs.h are included via the
`-include` option, it is no longer necessary for source files to include
them. Remove all the inclusions of these files I could find. Update
the generation scripts where relevant.
Change-Id: Ia026cff269c1b7ae7386dd3619bc9bb6a5332837
Approved-By: Pedro Alves <pedro@palves.net>
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>
When running test-case gdb.reverse/func-map-to-same-line.exp on arm-linux with
target board unix/-mthumb, we run into:
...
(gdb) reverse-step
func2 () at func-map-to-same-line.c:26
26 {
(gdb) FAIL: gdb.reverse/func-map-to-same-line.exp: \
column_info_flag=column-info: step-test: reverse-step into func2
...
The FAIL is caused by incorrect recording of this insn:
...
4f6: f85d 7b04 ldr.w r7, [sp], #4
...
The insn updates the sp, but we don't record this:
...
$ gdb -q -batch func-map-to-same-line \
-ex "b *func2+8" \
-ex run \
-ex record \
-ex "set debug record 2" \
-ex stepi
Breakpoint 1 at 0x4f6: file func-map-to-same-line.c, line 27.
Breakpoint 1, 0xaaaaa4f6 in func2 () at func-map-to-same-line.c:27
27 } /* END FUNC2 */
Process record: arm_process_record addr = 0xaaaaa4f6
Process record: add register num = 15 to record list.
Process record: record_full_arch_list_add 0xabc6c460.
Process record: add register num = 7 to record list.
Process record: record_full_arch_list_add 0xabc3b868.
Process record: add register num = 25 to record list.
...
[ Note that sp is r13, and we see here only r15 (pc), r7, and r25 (ps). ]
The problem is that the specific insn, an LDR(immediate) T4, is not handled in
thumb2_record_ld_word.
Fix this by detecting the insn in thumb2_record_ld_word, and recording the
updated base register.
Tested on arm-linux.
Reported-By: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
Approved-By: Luis Machado <luis.machado@arm.com>
PR tdep/31278
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31278
On arm-linux the linaro CI occasionally reports:
...
(gdb) up 10
#4 0x0001b864 in pthread_join ()
(gdb) FAIL: gdb.threads/staticthreads.exp: up 10
...
while this is expected:
...
(gdb) up 10
#3 0x00010568 in main (argc=1, argv=0xfffeede4) at staticthreads.c:76
76 pthread_join (thread, NULL);
(gdb) PASS: gdb.threads/staticthreads.exp: up 10
...
Thiago investigated the problem, and using valgrind found an invalid read in
arm_exidx_fill_cache.
The problem happens as follows:
- an objfile and corresponding per_bfd are allocated
- some memory is allocated in arm_exidx_new_objfile using
objfile->objfile_obstack, for the "exception table entry cache".
- a symbol reread is triggered, and the objfile, including the
objfile_obstack, is destroyed
- a new objfile is allocated, using the same per_bfd
- again arm_exidx_new_objfile is called, but since the same per_bfd is used,
it doesn't allocate any new memory for the "exception table entry cache".
- the "exception table entry cache" is accessed by arm_exidx_fill_cache,
and we have a use-after-free.
This is a regression since commit a2726d4ff8 ("[ARM] Store exception handling
information per-bfd instead of per-objfile"), which changed the "exception
table entry cache" from per-objfile to per-bfd, but failed to update the
obstack_alloc.
Fix this by using objfile->per_bfd->storage_obstack instead of
objfile->objfile_obstack.
I couldn't reproduce the FAIL myself, but Thiago confirmed that the patch
fixes it.
Tested on arm-linux.
Approved-By: Luis Machado <luis.machado@arm.com>
PR tdep/31254
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31254
arm_epilogue_frame_this_id has a comment saying that it fall backs to using
the current PC if the function start address can't be identified, but it
actually uses only the PC to make the frame id.
This patch makes the code match the comment. Another hint that it's what
is intended is that arm_prologue_this_id, a function almost identical to
it, does that.
The problem was found by code inspection. It fixes the following testsuite
failures:
FAIL: gdb.base/unwind-on-each-insn.exp: foo: instruction 9: check frame-id matches
FAIL: gdb.reverse/solib-reverse.exp: reverse-next third shr1
FAIL: gdb.reverse/solib-reverse.exp: reverse-next second shr1
FAIL: gdb.reverse/solib-reverse.exp: reverse-next first shr1
FAIL: gdb.reverse/solib-reverse.exp: reverse-next generic
FAIL: gdb.reverse/solib-reverse.exp: reverse-step into solib function one
FAIL: gdb.reverse/solib-reverse.exp: reverse-step within solib function one
FAIL: gdb.reverse/solib-reverse.exp: reverse-step into solib function two
FAIL: gdb.reverse/solib-reverse.exp: reverse-step within solib function two
Tested on arm-linux-gnueabi-hf.
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.
Make arm use the new gdbarch_pseudo_register_write. This fixes writing
pseudo registers to non-current frames for that architecture.
Change-Id: Icb2a649ab6394817844230e9e94c3d0564d2f765
Reviewed-By: John Baldwin <jhb@FreeBSD.org>
Approved-by: Luis Machado <luis.machado@arm.com>
Make arm use the "newer" gdbarch_pseudo_register_read_value. This fixes
reading pseudo registers in non-current frames on that architecture.
Change-Id: Ic4d3d5d96957a4addfa3443c7b567dc4a31794a9
Reviewed-By: John Baldwin <jhb@FreeBSD.org>
Approved-by: Luis Machado <luis.machado@arm.com>
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>
Some functions related to the handling of registers in frames accept
"this frame", for which we want to read or write the register values,
while other functions accept "the next frame", which is the frame next
to that. The later is needed because we sometimes need to read register
values for a frame that does not exist yet (usually when trying to
unwind that frame-to-be).
value_of_register and value_of_register_lazy both take "this frame",
even if what they ultimately want internally is "the next frame". This
is annoying if you are in a spot that currently has "the next frame" and
need to call one of these functions (which happens later in this
series). You need to get the previous frame only for those functions to
get the next frame again. This is more manipulations, more chances of
mistake.
I propose to change these functions (and a few more functions in the
subsequent patches) to operate on "the next frame". Things become a bit
less awkward when all these functions agree on which frame they take.
So, in this patch, change value_of_register_lazy and value_of_register
to take "the next frame" instead of "this frame". This adds a lot of
get_next_frame_sentinel_okay, but if we convert the user registers API
to also use "the next frame" instead of "this frame", it will get simple
again.
Change-Id: Iaa24815e648fbe5ae3c214c738758890a91819cd
Reviewed-By: John Baldwin <jhb@FreeBSD.org>
Right now, gdbsupport/common-regcache.h contains two abstractons for a
regcache. An opaque type `regcache` (gdb and gdbserver both have their
own regcache that is the concrete version of this) and an abstract base
class `reg_buffer_common`, that is the base of regcaches on both sides.
These abstractions allow code to be written for both gdb and gdbserver,
for instance in the gdb/arch sub-directory.
However, having two
different abstractions is impractical. If some common code has a regcache,
and wants to use an operation defined on reg_buffer_common, it can't.
It would be better to have just one. Change all instances of `regcache
*` in gdbsupport/common-regcache.h to be `reg_buffer_common *`, then fix
fallouts.
Implementations in gdb and gdbserver now need to down-cast (using
gdb::checked_static_cast) from reg_buffer_common to their concrete
regcache type. Some of them could be avoided by changing free functions
(like regcache_register_size) to be virtual methods on
reg_buffer_common. I tried it, it seems to work, but I did not include
it in this series to avoid adding unnecessary changes.
Change-Id: Ia5503adb6b5509a0f4604bd2a68b4642cc5283fd
Reviewed-by: John Baldwin <jhb@FreeBSD.org>
This changes gdb to use the C++17 [[fallthrough]] attribute rather
than special comments.
This was mostly done by script, but I neglected a few spellings and so
also fixed it up by hand.
I suspect this fixes the bug mentioned below, by switching to a
standard approach that, presumably, clang supports.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=23159
Approved-By: John Baldwin <jhb@FreeBSD.org>
Approved-By: Luis Machado <luis.machado@arm.com>
Approved-By: Pedro Alves <pedro@palves.net>
Since GDB now requires C++17, we don't need the internally maintained
gdb::optional implementation. This patch does the following replacing:
- gdb::optional -> std::optional
- gdb::in_place -> std::in_place
- #include "gdbsupport/gdb_optional.h" -> #include <optional>
This change has mostly been done automatically. One exception is
gdbsupport/thread-pool.* which did not use the gdb:: prefix as it
already lives in the gdb namespace.
Change-Id: I19a92fa03e89637bab136c72e34fd351524f65e9
Approved-By: Tom Tromey <tom@tromey.com>
Approved-By: Pedro Alves <pedro@palves.net>
When compiling gdb with -fsanitize=address on ARM, I get a crash in test
gdb.arch/arm-disp-step.exp, reproduced easily with:
$ ./gdb -nx -q --data-directory=data-directory testsuite/outputs/gdb.arch/arm-disp-step/arm-disp-step -ex "break *test_call_end"
Reading symbols from testsuite/outputs/gdb.arch/arm-disp-step/arm-disp-step...
=================================================================
==23295==ERROR: AddressSanitizer: heap-buffer-overflow on address 0xb4a14fd1 at pc 0x01a48871 bp 0xbeab8490 sp 0xbeab8494
Since it doesn't require running the program, it can be reproduced locally on a
dev machine other than ARM, after acquiring the test binary.
The length of the allocate buffer `buf` is 1, and we try to extract an
integer of size 2 from it. The length of 1 comes from the subtraction
`bpaddr - boundary`. Normally, on ARM, all instructions are aligned on
a multiple of 2, so it's weird for this subtraction to result in 1. In
this case, boundary comes from the result of find_pc_partial_function
returning 0x549:
(gdb) p/x bpaddr
$2 = 0x54a
(gdb) p/x boundary
$3 = 0x549
(gdb) p/x bpaddr - boundary
$4 = 0x1
0x549 is the address of the test_call_subr label, 0x548, with the thumb
bit enabled. Before doing some math with the address, I think we need
to strip the thumb bit, like is done elsewhere (for instance for bpaddr
earlier in the same function).
I wonder if find_pc_partial_function should do that itself, in order to
return an address that is suitable for arithmetic. In any case, that
would be a change with a broad impact, so for now just fix the issue
locally.
After the patch:
$ ./gdb -nx -q --data-directory=data-directory testsuite/outputs/gdb.arch/arm-disp-step/arm-disp-step -ex "break *test_call_end"
Reading symbols from testsuite/outputs/gdb.arch/arm-disp-step/arm-disp-step...
Breakpoint 1 at 0x54a: file /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.arch/arm-disp-step.S, line 103.
Change-Id: I74fc458dbea0d2c1e1f5eadd90755188df089288
Approved-By: Luis Machado <luis.machado@arm.com>
On a big-endian ARM machine, the "return" command resulted in the
wrong value being returned when the function had a fixed-point return
type. This patch fixes the problem by unpacking and repacking the
fixed-point type appropriately.
Approved-By: Luis Machado <luis.machado@arm.com>
On big-endian ARM, "return"ing from a function that returned a range
type did not work. This patch strips the range type to treat the
function as though it were returning the underlying type instead.
Approved-By: Luis Machado <luis.machado@arm.com>
On a big-endian ARM system, "finish" printed the wrong value when
finishing from a function that returned a vector type. Similarly,
calls to a function also resulted in the wrong value being passed. I
think both the read- and write-functions here should ignore the
endian-ness.
I tested this using the AdaCore internal test suite; the test case
that caught this is identical to gdb.base/gnu_vector.exp.
Approved-By: Luis Machado <luis.machado@arm.com>
On ARM (I tested big-endian but it may not matter), "finish" can
sometimes print the wrong result when the return type is a range type.
Range types should really be treated as their underlying type
(normally integer, but sometimes fixed-point). This patch implements
this.
Approved-By: Luis Machado <luis.machado@arm.com>
On big-endian ARM, an inferior call with a small integer will pass the
wrong value. This patch fixes the problem. Because the code here
works using scalar values, and not just bytes, left-shifting is
unnecessary.
Approved-By: Luis Machado <luis.machado@arm.com>
This function is just a wrapper around the current inferior's gdbarch.
I find that having that wrapper just obscures where the arch is coming
from, and that it's often used as "I don't know which arch to use so
I'll use this magical target_gdbarch function that gets me an arch" when
the arch should in fact come from something in the context (a thread,
objfile, symbol, etc). I think that removing it and inlining
`current_inferior ()->arch ()` everywhere will make it a bit clearer
where that arch comes from and will trigger people into reflecting
whether this is the right place to get the arch or not.
Change-Id: I79f14b4e4934c88f91ca3a3155f5fc3ea2fadf6b
Reviewed-By: John Baldwin <jhb@FreeBSD.org>
Approved-By: Andrew Burgess <aburgess@redhat.com>
The new_objfile observer is currently used to indicate both when a new
objfile is added to program space (when passed non-nullptr) and when all
objfiles of a program space were just removed (when passed nullptr).
I think this is confusing (and Andrew apparently thinks so too [1]).
Add a new "all_objfiles_removed" observer to remove the second role from
"new_objfile".
Some existing users of new_objfile do nothing if the passed objfile is
nullptr. For them, we can simply drop the nullptr check. For others,
add a new all_objfiles_removed callback, and refactor things a bit to
keep the existing behavior as much as possible.
Some callbacks relied on current_program_space, and following
the refactoring now use either objfile->pspace or the pspace passed to
all_objfiles_removed. I think this should be relatively safe, and in
general a step in the right direction.
On the notify side, I found only one call site to change from
new_objfile to all_objfiles_removed, in clear_symtab_users. It is not
entirely clear to me that this is entirely correct. clear_symtab_users
appears to be called in spots that don't remove all objfiles
(functions finish_new_objfile, remove_symbol_file_command, reread_symbols,
do_module_cleanups). But I think that this patch at least makes the
current code clearer.
[1] a0a031bce0
Change-Id: Icb648f72862e056267f30f44dd439bd4ec766f13
Approved-By: Tom Tromey <tom@tromey.com>
I noticed a comment by an include and remembered that I think these
don't really provide much value -- sometimes they are just editorial,
and sometimes they are obsolete. I think it's better to just remove
them. Tested by rebuilding.
Approved-By: Andrew Burgess <aburgess@redhat.com>
As noted by Tom Tromey, there are some formatting issues with the ternary
operator in the aarch64/arm codebase. This patch fixes those.
Reviewed-By: Tom Tromey <tom@tromey.com>
The gdb.dwarf2/dw2-prologue-end-2.exp test was failing for both AArch64 and
Arm.
As Tom pointed out here (https://inbox.sourceware.org/gdb-patches/6663707c-4297-c2f2-a0bd-f3e84fc62aad@suse.de/),
there are issues with both the prologue skipper for AArch64 and Arm and an
incorrect assumption by the testcase.
This patch fixes both of AArch64's and Arm's prologue skippers to not skip past
the end of a function. It also incorporates a fix to the testcase so it
doesn't assume the prologue skipper will stop at the first instruction of the
functions/labels.
Regression-tested on aarch64-linux/arm-linux Ubuntu 20.04/22.04 and
x86_64-linux Ubuntu 20.04.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30506
Co-Authored-By: Tom de Vries <tdevries@suse.de>
Co-Authored-By: Luis Machado <luis.machado@arm.com>
This changes field_is_static to be a method on struct field, and
updates all the callers. Most of this patch was written by script.
Regression tested on x86-64 Fedora 36.
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>
GDB expected PC should point right after the SVC instruction when the
syscall is active. But some active syscalls keep PC pointing to the SVC
instruction itself.
This leads to a broken backtrace like:
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
#0 0xb6f8681c in pthread_cond_timedwait@@GLIBC_2.4 () from /lib/arm-linux-gnueabihf/libpthread.so.0
#1 0xb6e21f80 in ?? ()
The reason is that .ARM.exidx unwinder gives up if PC does not point
right after the SVC (syscall) instruction. I did not investigate why but
some syscalls will point PC to the SVC instruction itself. This happens
for the "futex" syscall used by pthread_cond_timedwait.
That normally does not matter as ARM prologue unwinder gets called
instead of the .ARM.exidx one. Unfortunately some glibc calls have more
complicated prologue where the GDB unwinder fails to properly determine
the return address (that is in fact an orthogonal GDB bug). I expect it
is due to the "vpush" there in this case but I did not investigate it more:
Dump of assembler code for function pthread_cond_timedwait@@GLIBC_2.4:
0xb6f8757c <+0>: push {r4, r5, r6, r7, r8, r9, r10, r11, lr}
0xb6f87580 <+4>: mov r10, r2
0xb6f87584 <+8>: vpush {d8}
Regression tested on armv7l kernel 5.15.32-v7l+ (Raspbian 11).
Approved-By: Luis Machado <luis.machado@arm.com>
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 turns the remaining value_contents functions -- value_contents,
value_contents_all, value_contents_for_printing, and
value_contents_for_printing_const -- into methods of value. It also
converts the static functions require_not_optimized_out and
require_available to be private methods.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
This turns value_contents_raw, value_contents_writeable, and
value_contents_all_raw into methods on value. The remaining functions
will be changed later in the series; they were a bit trickier and so I
didn't include them in this patch.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
This patch resolves the performance issue reported in pr/29738 by
caching the values for the stack pointers for the inner frame. By
doing so, the impact can be reduced to checking the state and
returning the appropriate value.
Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
Signed-off-by: Yvan Roux <yvan.roux@foss.st.com>
This helps resolve some cyclic include problem later in the series.
The only language-related thing frame.h needs is enum language, and that
is in defs.h.
Doing so reveals that a bunch of files were relying on frame.h to
include language.h, so fix the fallouts here and there.
Change-Id: I178a7efec1953c2d088adb58483bade1f349b705
Reviewed-By: Bruno Larsen <blarsen@redhat.com>
