| Commit message (Collapse) | Author | Age | Files | Lines |
|---|
| |
|
|
|
|
|
|
| |
This replaces ALL_OBJFILE_OSECTIONS with an iterator so that for-each
can be used.
|
| |
|
|
|
|
|
|
|
|
| |
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>
|
| |
|
|
|
|
|
|
|
| |
This unifies arch_float_type and init_float_type by using a type
allocator.
Reviewed-By: Simon Marchi <simon.marchi@efficios.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>
|
| |
|
|
|
|
| |
Replace spaces with tabs in a bunch of places.
Change-Id: If0f87180f1d13028dc178e5a8af7882a067868b0
|
| |
|
|
|
|
|
|
| |
Now that gdb_indent.sh has been removed, I think it makes sense to
also remove the directives intended for GNU indent.
|
| |
|
|
|
|
|
|
|
|
|
|
|
| |
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 changes allocate_value to be a static "constructor" of value.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
| |
|
|
|
|
|
|
|
|
| |
This changes value_type to be a method of value. Much of this patch
was written by script.
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>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
It's currently not clear how the ownership of gdbarch_tdep objects
works. In fact, nothing ever takes ownership of it. This is mostly
fine because we never free gdbarch objects, and thus we never free
gdbarch_tdep objects. There is an exception to that however: when
initialization fails, we do free the gdbarch object that is not going to
be used, and we free the tdep too. Currently, i386 and s390 do it.
To make things clearer, change gdbarch_alloc so that it takes ownership
of the tdep. The tdep is thus automatically freed if the gdbarch is
freed.
Change all gdbarch initialization functions to pass a new gdbarch_tdep
object to gdbarch_alloc and then retrieve a non-owning reference from
the gdbarch object.
Before this patch, the xtensa architecture had a single global instance
of xtensa_gdbarch_tdep. Since we need to pass a dynamically allocated
gdbarch_tdep_base instance to gdbarch_alloc, remove this global
instance, and dynamically allocate one as needed, like we do for all
other architectures. Make the `rmap` array externally visible and
rename it to the less collision-prone `xtensa_rmap` name.
Change-Id: Id3d70493ef80ce4bdff701c57636f4c79ed8aea2
Approved-By: Andrew Burgess <aburgess@redhat.com>
|
| |
|
|
|
|
|
|
| |
This patch updates the gdbarch_return_value_as_value implementations
to work correctly with variably-sized return types.
|
| |
|
|
|
|
|
|
|
|
| |
This converts a few selected architectures to use
gdbarch_return_value_as_value rather than gdbarch_return_value. The
architectures are just the ones that I am able to test. This patch
should not introduce any behavior changes.
|
| |
|
|
|
|
|
| |
This commit is the result of running the gdb/copyright.py script,
which automated the update of the copyright year range for all
source files managed by the GDB project to be updated to include
year 2023.
|
| |
|
|
|
|
|
| |
These headers define the register numbers for each port to implement
the sim_fetch_register & sim_store_register interfaces. While gdb
uses these, the APIs are part of the sim, not gdb. Move the headers
out of the gdb/ include namespace and into sim/ instead.
|
| |
|
|
|
|
|
|
|
|
| |
For v8m, the EXC_RETURN pattern, without security extension, consists of
FType, Mode and SPSEL. These are the same bits that are used in v7m.
This patch extends the list of patterns to include also the FType bit
and not just Mode and SPSEL bits for v8m targets without security
extension.
Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
|
| |
|
|
|
|
|
|
|
| |
As part of the rebase of the patch, I managed to loose the local
changes I had for the comments from Tomas in
https://sourceware.org/pipermail/gdb-patches/2022-November/193413.html
This patch corrects the obvious two typos.
Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
|
| |
|
|
|
|
|
|
|
| |
For targets with secext, msp and psp can be seen as an alias for one
of msp_s, msp_ns, psp_s or psp_ns.
Without this patch, sp might be secure, but msp or psp is non-secure
(this state can not happen in the hardware).
Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
|
| |
|
|
|
|
|
|
| |
For targets with secext, msp and psp can be seen as an alias for one
of msp_s, msp_ns, psp_s or psp_ns. When switching active sp, the
corresponding msp/psp needs to be switched too.
Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Arm v8-M Architecture Reference Manual,
D1.2.95 EXC_RETURN, Exception Return Payload
describes ES bit:
"ES, bit [0]
Exception Secure. The security domain the exception was taken to.
The possible values of this bit are:
0 Non-secure.
1 Secure"
arm-tdep.c:3443, arm_m_exception_cache () function tests this bit:
exception_domain_is_secure = (bit (lr, 0) == 0);
The test is negated!
Later on line 3553, the condition evaluates if an additional state
context is stacked:
/* With the Security extension, the hardware saves R4..R11 too. */
if (tdep->have_sec_ext && secure_stack_used
&& (!default_callee_register_stacking || exception_domain_is_secure))
RM, B3.19 Exception entry, context stacking
reads:
RPLHM "In a PE with the Security Extension, on taking an exception,
the PE hardware:
...
2. If exception entry requires a transition from Secure state to
Non-secure state, the PE hardware extends the stack frame and also
saves additional state context."
So we should test for !exception_domain_is_secure instead of non-negated
value!
These two bugs compensate each other so unstacking works correctly.
But another test of exception_domain_is_secure (negated due to the
first bug) prevents arm_unwind_secure_frames to work as expected:
/* Unwinding from non-secure to secure can trip security
measures. In order to avoid the debugger being
intrusive, rely on the user to configure the requested
mode. */
if (secure_stack_used && !exception_domain_is_secure
&& !arm_unwind_secure_frames)
Test with GNU gdb (GDB) 13.0.50.20221016-git.
Stopped in a non-secure handler:
(gdb) set arm unwind-secure-frames 0
(gdb) bt
#0 HAL_SYSTICK_Callback () at C:/dvl/stm32l5trustzone/GPIO_IOToggle_TrustZone/NonSecure/Src/nsmain.c:490
#1 0x0804081c in SysTick_Handler ()
at C:/dvl/stm32l5trustzone/GPIO_IOToggle_TrustZone/NonSecure/Src/nsstm32l5xx_it.c:134
#2 <signal handler called>
#3 HAL_GPIO_ReadPin (GPIOx=0x52020800, GPIO_Pin=8192)
at C:/dvl/stm32l5trustzone/GPIO_IOToggle_TrustZone/Drivers/STM32L5xx_HAL_Driver/Src/stm32l5xx_hal_gpio.c:386
#4 0x0c000338 in SECURE_Mode () at C:/dvl/stm32l5trustzone/GPIO_IOToggle_TrustZone/Secure/Src/main.c:86
#5 0x080403f2 in main () at C:/dvl/stm32l5trustzone/GPIO_IOToggle_TrustZone/NonSecure/Src/nsmain.c:278
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
The frames #3 and #4 are secure. backtrace should stop before #3.
Stopped in a secure handler:
(gdb) bt
#0 HAL_SYSTICK_Callback () at C:/dvl/stm32l5trustzone/GPIO_IOToggle_TrustZone/Secure/Src/main.c:425
#1 0x0c000b6a in SysTick_Handler ()
at C:/dvl/stm32l5trustzone/GPIO_IOToggle_TrustZone/Secure/Src/stm32l5xx_it.c:234
warning: Non-secure to secure stack unwinding disabled.
#2 <signal handler called>
The exception from secure to secure erroneously stops unwinding. It should
continue as far as the security unlimited backtrace:
(gdb) set arm unwind-secure-frames 1
(gdb) si <-- used to rebuild frame cache after change of unwind-secure-frames
0x0c0008e6 425 if (SecureTimingDelay != 0U)
(gdb) bt
#0 0x0c0008e6 in HAL_SYSTICK_Callback () at C:/dvl/stm32l5trustzone/GPIO_IOToggle_TrustZone/Secure/Src/main.c:425
#1 0x0c000b6a in SysTick_Handler ()
at C:/dvl/stm32l5trustzone/GPIO_IOToggle_TrustZone/Secure/Src/stm32l5xx_it.c:234
#2 <signal handler called>
#3 0x0c000328 in SECURE_Mode () at C:/dvl/stm32l5trustzone/GPIO_IOToggle_TrustZone/Secure/Src/main.c:88
#4 0x080403f2 in main () at C:/dvl/stm32l5trustzone/GPIO_IOToggle_TrustZone/NonSecure/Src/nsmain.c:278
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
Set exception_domain_is_secure to the value expected by its name.
Fix exception_domain_is_secure usage in the additional state context
stacking condition.
Signed-off-by: Tomas Vanek <vanekt@fbl.cz>
|
| |
|
|
|
|
|
|
|
|
| |
Arm v8-M Architecture Reference Manual,
D1.2.141 IPSR, Interrupt Program Status Register reads
"Exception, bits [8:0]"
9 bits, not 8! It is uncommon but true!
Signed-off-by: Tomas Vanek <vanekt@fbl.cz>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
In the lockup state the PC value of the the outer frame is irreversibly
lost. The other registers are intact so LR likely contains
PC of some frame next to the outer one, but we cannot analyze
the nearest outer frame without knowing its PC
therefore we do not know SP fixup for this frame.
The frame unwinder possibly gets mad due to the wrong SP value.
To prevent problems terminate unwinding if PC contains the magic
value of the lockup state.
Example session wihtout this change,
Cortex-M33 CPU in lockup, gdb 13.0.50.20221016-git:
----------------
(gdb) c
Continuing.
Program received signal SIGINT, Interrupt.
0xeffffffe in ?? ()
(gdb) bt
#0 0xeffffffe in ?? ()
#1 0x0c000a9c in HardFault_Handler ()
at C:/dvl/stm32l5trustzone/GPIO_IOToggle_TrustZone/Secure/Src/stm32l5xx_it.c:99
#2 0x2002ffd8 in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
(gdb)
----------------
The frame #1 is at correct PC taken from LR, #2 is a total nonsense.
With the change:
----------------
(gdb) c
Continuing.
Program received signal SIGINT, Interrupt.
warning: ARM M in lockup state, stack unwinding terminated.
<signal handler called>
(gdb) bt
#0 <signal handler called>
(gdb)
----------------
There is a visible drawback of emitting a warning in a cache buildnig routine
as introduced in Torbjörn SVENSSON's
[PATCH v4] gdb/arm: Stop unwinding on error, but do not assert
The warning is printed just once and not repeated on each backtrace command.
Signed-off-by: Tomas Vanek <vanekt@fbl.cz>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Currently, every internal_error call must be passed __FILE__/__LINE__
explicitly, like:
internal_error (__FILE__, __LINE__, "foo %d", var);
The need to pass in explicit __FILE__/__LINE__ is there probably
because the function predates widespread and portable variadic macros
availability. We can use variadic macros nowadays, and in fact, we
already use them in several places, including the related
gdb_assert_not_reached.
So this patch renames the internal_error function to something else,
and then reimplements internal_error as a variadic macro that expands
__FILE__/__LINE__ itself.
The result is that we now should call internal_error like so:
internal_error ("foo %d", var);
Likewise for internal_warning.
The patch adjusts all calls sites. 99% of the adjustments were done
with a perl/sed script.
The non-mechanical changes are in gdbsupport/errors.h,
gdbsupport/gdb_assert.h, and gdb/gdbarch.py.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Change-Id: Ia6f372c11550ca876829e8fd85048f4502bdcf06
|
| |
|
|
|
|
|
|
| |
Setting SP of the next frame to the same address as the current frame
is an ugly way to stop the unwinding. A cleaner way is to rely on
the frame_unwind_stop_reason function to return UNWIND_OUTERMOST.
Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
When it's impossible to read the FPCCR and XPSR, the unwinding is
unpredictable as the it's not possible to determine the correct
frame size or padding.
The only sane thing to do in this condition is to stop the unwinding.
Example session without this patch:
(gdb) bt
#0 SVC_Handler () at .../GPIO/GPIO_EXTI/Src/stm32f4xx_it.c:112
.../gdb/arm-tdep.c:3594: internal-error: arm_m_exception_cache: Assertion `safe_read_memory_unsigned_integer (FPCCR, ARM_INT_REGISTER_SIZE, byte_order, &fpccr)' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
----- Backtrace -----
0x5583bfb2a157 gdb_internal_backtrace_1
...
---------------------
This is a bug, please report it. For instructions, see:
<https://www.gnu.org/software/gdb/bugs/>.
Aborted (core dumped)
Example session with this patch:
(gdb) bt
#0 SVC_Handler () at .../GPIO/GPIO_EXTI/Src/stm32f4xx_it.c:112
warning: Could not fetch required FPCCR content. Further unwind is impossible.
#1 <signal handler called>
(gdb)
Reviewed-by: Pedro Alves <pedro@palves.net>
Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
This changes GDB to use frame_info_ptr instead of frame_info *
The substitution was done with multiple sequential `sed` commands:
sed 's/^struct frame_info;/class frame_info_ptr;/'
sed 's/struct frame_info \*/frame_info_ptr /g' - which left some
issues in a few files, that were manually fixed.
sed 's/\<frame_info \*/frame_info_ptr /g'
sed 's/frame_info_ptr $/frame_info_ptr/g' - used to remove whitespace
problems.
The changed files were then manually checked and some 'sed' changes
undone, some constructors and some gets were added, according to what
made sense, and what Tromey originally did
Co-Authored-By: Bruno Larsen <blarsen@redhat.com>
Approved-by: Tom Tomey <tom@tromey.com>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Read LSPEN, ASPEN and LSPACT bits from FPCCR and use them together
with FPCAR to identify if lazy FPU state preservation is active for
the current frame. See "Lazy context save of FP state", in B1.5.7,
also ARM AN298, supported by Cortex-M4F architecture for details on
lazy FPU register stacking. The same conditions are valid for other
Cortex-M cores with FPU.
This patch has been verified on a STM32F4-Discovery board by:
a) writing a non-zero value (lets use 0x1122334455667788 as an
example) to all the D-registers in the main function
b) configured the SysTick to fire
c) in the SysTick_Handler, write some other value (lets use
0x0022446688aaccee as an example) to one of the D-registers (D0 as
an example) and then do "SVC #0"
d) in the SVC_Handler, write some other value (lets use
0x0099aabbccddeeff) to one of the D-registers (D0 as an example)
In GDB, suspend the execution in the SVC_Handler function and compare
the value of the D-registers for the SVC_handler frame and the
SysTick_Handler frame. With the patch, the value of the modified
D-register (D0) should be the new value (0x009..eff) on the
SVC_Handler frame, and the intermediate value (0x002..cee) for the
SysTick_Handler frame. Now compare the D-register value for the
SysTick_Handler frame and the main frame. The main frame should
have the initial value (0x112..788).
Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
Signed-off-by: Yvan ROUX <yvan.roux@foss.st.com>
|
| |
|
|
|
|
| |
Remove the macro, replace all uses with calls to type::length.
Change-Id: Ib9bdc954576860b21190886534c99103d6a47afb
|
| |
|
|
|
|
| |
Remove the macro, replace all uses by calls to type::target_type.
Change-Id: Ie51d3e1e22f94130176d6abd723255282bb6d1ed
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
v2:
- Add 32-bit Arm instruction selftest
- Refactored abstract memory reader into abstract instruction reader
- Adjusted code to use templated type and to use host endianness as
opposed to target endianness.
The arm record tests handle 16-bit and 32-bit thumb instructions, but the
code is laid out in a way that handles the 32-bit thumb instructions as
two 16-bit parts.
This is fine, but it is prone to host-endianness issues given how the two
16-bit parts are stored and how they are accessed later on. Arm is
little-endian by default, so running this test with a GDB built with
--enable-targets=all and on a big endian host will run into the following:
Running selftest arm-record.
Process record and replay target doesn't support syscall number -2036195
Process record does not support instruction 0x7f70ee1d at address 0x0.
Self test failed: self-test failed at ../../binutils-gdb/gdb/arm-tdep.c:14482
It turns out the abstract memory reader class is more generic than it needs to
be, and we can simplify the code a bit by assuming we have a simple instruction
reader that only reads up to 4 bytes, which is the length of a 32-bit
instruction.
Instead of returning a bool, we return instead the instruction that has been
read. This way we avoid having to deal with the endianness conversion, and use
the host endianness instead. The Arm selftests can be executed on non-Arm
hosts.
While at it, Tom suggested adding a 32-bit Arm instruction selftest to increase
the coverage of the selftests.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29432
Co-authored-by: Tom de Vries <tdevries@suse.de>
|
| |
|
|
|
|
|
|
|
|
|
|
|
| |
With this change, only valid contents of LR are accepted when unwinding
exception frames for m-profile targets.
If the contents of LR are anything but EXC_RETURN or FNC_RETURN, it
will cause GDB to print an error and/or abort unwinding of the frame as
it's an invalid state for the unwinder.
The FNC_RETURN pattern requires Security Extensions to be enabled.
Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
|
| |
|
|
|
|
|
|
|
|
|
|
|
| |
This changes struct objfile to use a gdb_bfd_ref_ptr. In addition to
removing some manual memory management, this fixes a use-after-free
that was introduced by the registry rewrite series. The issue there
was that, in some cases, registry shutdown could refer to memory that
had already been freed. This help fix the bug by delaying the
destruction of the BFD reference (and thus the per-bfd object) until
after the registry has been shut down.
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
This rewrites registry.h, removing all the macros and replacing it
with relatively ordinary template classes. The result is less code
than the previous setup. It replaces large macros with a relatively
straightforward C++ class, and now manages its own cleanup.
The existing type-safe "key" class is replaced with the equivalent
template class. This approach ended up requiring relatively few
changes to the users of the registry code in gdb -- code using the key
system just required a small change to the key's declaration.
All existing users of the old C-like API are now converted to use the
type-safe API. This mostly involved changing explicit deletion
functions to be an operator() in a deleter class.
The old "save/free" two-phase process is removed, and replaced with a
single "free" phase. No existing code used both phases.
The old "free" callbacks took a parameter for the enclosing container
object. However, this wasn't truly needed and is removed here as
well.
|
| |
|
|
|
|
|
|
|
|
|
|
|
| |
For Arm Cortex-M33 with security extensions, there are 4 different
stack pointers (msp_s, msp_ns, psp_s, psp_ns), without security
extensions and for other Cortex-M targets, there are 2 different
stack pointers (msp and psp).
With this patch, sp will always be in sync with one of the real stack
pointers on Arm targets that contain more than one stack pointer.
Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
Signed-off-by: Yvan Roux <yvan.roux@foss.st.com>
|
| |
|
|
|
|
|
| |
As the register numbers for the alternative Arm SP registers are not
constant, it's not possible to use switch statement to define the
rules. In order to not have a mix, replace the few existing
switch statements with regular if-else if statements
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
I built GDB for all targets on a x86-64/GNU-Linux system, and
then (accidentally) passed GDB a RISC-V binary, and asked GDB to "run"
the binary on the native target. I got this error:
(gdb) show architecture
The target architecture is set to "auto" (currently "i386").
(gdb) file /tmp/hello.rv32.exe
Reading symbols from /tmp/hello.rv32.exe...
(gdb) show architecture
The target architecture is set to "auto" (currently "riscv:rv32").
(gdb) run
Starting program: /tmp/hello.rv32.exe
../../src/gdb/i387-tdep.c:596: internal-error: i387_supply_fxsave: Assertion `tdep->st0_regnum >= I386_ST0_REGNUM' failed.
What's going on here is this; initially the architecture is i386, this
is based on the default architecture, which is set based on the native
target. After loading the RISC-V executable the architecture of the
current inferior is updated based on the architecture of the
executable.
When we "run", GDB does a fork & exec, with the inferior being
controlled through ptrace. GDB sees an initial stop from the inferior
as soon as the inferior comes to life. In response to this stop GDB
ends up calling save_stop_reason (linux-nat.c), which ends up trying
to read register from the inferior, to do this we end up calling
target_ops::fetch_registers, which, for the x86-64 native target,
calls amd64_linux_nat_target::fetch_registers.
After this I eventually end up in i387_supply_fxsave, different x86
based targets will end in different functions to fetch registers, but
it doesn't really matter which function we end up in, the problem is
this line, which is repeated in many places:
i386_gdbarch_tdep *tdep = (i386_gdbarch_tdep *) gdbarch_tdep (arch);
The problem here is that the ARCH in this line comes from the current
inferior, which, as we discussed above, will be a RISC-V gdbarch, the
tdep field will actually be of type riscv_gdbarch_tdep, not
i386_gdbarch_tdep. After this cast we are relying on undefined
behaviour, in my case I happen to trigger an assert, but this might
not always be the case.
The thing I tried that exposed this problem was of course, trying to
start an executable of the wrong architecture on a native target. I
don't think that the correct solution for this problem is to detect,
at the point of cast, that the gdbarch_tdep object is of the wrong
type, but, I did wonder, is there a way that we could protect
ourselves from incorrectly casting the gdbarch_tdep object?
I think that there is something we can do here, and this commit is the
first step in that direction, though no actual check is added by this
commit.
This commit can be split into two parts:
(1) In gdbarch.h and arch-utils.c. In these files I have modified
gdbarch_tdep (the function) so that it now takes a template argument,
like this:
template<typename TDepType>
static inline TDepType *
gdbarch_tdep (struct gdbarch *gdbarch)
{
struct gdbarch_tdep *tdep = gdbarch_tdep_1 (gdbarch);
return static_cast<TDepType *> (tdep);
}
After this change we are no better protected, but the cast is now
done within the gdbarch_tdep function rather than at the call sites,
this leads to the second, much larger change in this commit,
(2) Everywhere gdbarch_tdep is called, we make changes like this:
- i386_gdbarch_tdep *tdep = (i386_gdbarch_tdep *) gdbarch_tdep (arch);
+ i386_gdbarch_tdep *tdep = gdbarch_tdep<i386_gdbarch_tdep> (arch);
There should be no functional change after this commit.
In the next commit I will build on this change to add an assertion in
gdbarch_tdep that checks we are casting to the correct type.
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
The three targets that implement gdbarch_adjust_breakpoint_address are
arm, frv, and mips. In each of these targets the adjust breakpoint
address function does some combination of reading the symbol table, or
reading memory at the location the breakpoint could be placed.
The problem is that performing these actions requires that the current
inferior and program space be the one in which the breakpoint will be
placed, and this is not currently always the case.
Consider a GDB session with multiple inferiors. One inferior might be
a native target while another could be a remote target of a completely
different architecture. Alternatively, if we consider ARM and
AArch64, one native inferior might be AArch64, while a second native
inferior could be ARM.
In these cases it is possible, and valid, for a user to have one
inferior selected, and place a breakpoint in the other inferior by
placing a breakpoint on a particular symbol.
If this happens, then currently, when
gdbarch_adjust_breakpoint_address is called, the wrong inferior (and
program space) will be selected, and memory reads, and symbol look
ups, will not return the expected results, this could lead to
breakpoints being placed in the wrong location.
There are currently two places where gdbarch_adjust_breakpoint_address
is called:
1. In infrun.c, in the function handle_step_into_function. In this
case, I believe that the correct inferior and program space will
already be selected as this is called as part of the stop event
handling, so I don't think we need to worry about this case, and
2. In breakpoint.c, in the function adjust_breakpoint_address, which
is itself called from code_breakpoint::add_location and
watch_command_1.
The watch_command_1 case I don't think we need to worry about, this
is for when a local watch expression is created, which can only be
in the currently selected inferior, so this case should be fine.
The code_breakpoint::add_location case is the one that needs fixing,
this is what allows a breakpoint to be created between inferiors.
To fix the code_breakpoint::add_location case, I propose that we pass
the "correct" program_space (i.e. the program space in which the
breakpoint will be created) to the adjust_breakpoint_address function.
Then in adjust_breakpoint_address we can make use of
switch_to_program_space_and_thread to switch program_space and
inferior before calling gdbarch_adjust_breakpoint_address.
I discovered this issue while working on a later patch in this
series. This later patch will detect when we cast the result of
gdbarch_tdep to the wrong type.
With this later patch in place I ran gdb.multi/multi-arch.exp on an
AArch64 target. In this situation, two inferiors are created, an
AArch64 inferior, and an ARM inferior. The test selected the AArch64
inferior and tries to create a breakpoint in the ARM inferior.
As a result of this we end up in arm_adjust_breakpoint_address, which
calls arm_pc_is_thumb. Before this commit the AArch64 inferior would
be current. As a result, all of the checks in arm_pc_is_thumb would
fail (they rely on reading symbols from the current program space),
and so, at the end of arm_pc_is_thumb we would call
arm_frame_is_thumb. However, remember, at this point the current
inferior is the AArch64 inferior, so the current frame is an AArch64
frame.
In arm_frame_is_thumb we call arm_psr_thumb_bit, which calls
gdbarch_tdep and casts the result to arm_gdbarch_tdep. This is wrong,
the tdep field is of type aarch64_gdbarch_tdep. After this we have
undefined behaviour.
With this patch in place, we will have switched to a thread in the ARM
program space before calling arm_adjust_breakpoint_address. As a
result, we now succeed in looking up the required symbols in
arm_pc_is_thumb, and so we never call arm_frame_is_thumb.
However, in the worst case scenario, if we did end up calling
arm_frame_is_thumb, as the current inferior should now be the ARM
inferior, the current frame should be an ARM frame, so we still should
not hit undefined behaviour.
I have added an assert to arm_frame_is_thumb.
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
This is a small refactor to resolve an issue before it becomes a
problem in a later commit.
Move the fetching of an arm_gdbarch_tdep into a more inner scope
within two functions in arm-tdep.c.
The problem with the current code is that the functions in question
are used as the callbacks for two set/show parameters. These set/show
parameters are available no matter the current architecture, but are
really about controlling an ARM architecture specific setting. And
so, if I build GDB for all targets on an x86-64/GNU-Linux system, I
can still do this:
(gdb) show arm fpu
(gdb) show arm abi
After these calls we end up in show_fp_model and arm_show_abi
respectively, where we unconditionally do this:
arm_gdbarch_tdep *tdep
= (arm_gdbarch_tdep *) gdbarch_tdep (target_gdbarch ());
However, the gdbarch_tdep() result will only be a arm_gdbarch_tdep if
the current architecture is ARM, otherwise the result will actually be
of some other type.
This isn't actually a problem, as in both cases the use of tdep is
guarded by a later check that the gdbarch architecture is
bfd_arch_arm.
This commit just moves the call to gdbarch_tdep() after the
architecture check.
In a later commit gdbarch_tdep() will be able to spot when we are
casting the result to the wrong type, and this function will trigger
assertion failures if things are not fixed.
There should be not user visible changes after this commit.
|
| |
|
|
|
|
|
|
|
|
| |
All usages of this helper are really made to check if the register is
one of the alternative SP registers (MSP/MSP_S/MSP_NS/PSP/PSP_S/PSP_NS)
with the ARM_SP_REGNUM case being handled separately.
Signed-off-by: Luis Machado <luis.machado@arm.com>
Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
Signed-off-by: Yvan Roux <yvan.roux@foss.st.com>
|
| |
|
|
|
|
|
|
|
| |
The FPCCR.TS bit is used to identify if FPU registers are considered
non-secure or secure. If they are secure, then callee saved registers
(S16 to S31) are stacked on exception entry or otherwise skipped.
Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
Signed-off-by: Yvan Roux <yvan.roux@foss.st.com>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Without this changeset, the unwinding doesn't take into account
Non-Secure to Secure stack unwinding enablement status and
doesn't choose the proper SP to do the unwinding.
This patch only unwinds the stack when Non-Secure to Secure
unwinding is enabled, previous SP is set w/r to the current mode
(Handler -> msp_s, Thread -> psp_s) and then the Secure stack is
unwound. Ensure thumb bit is set in PSR when needed. Also, drop
thumb bit from PC if set.
Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
Signed-off-by: Yvan ROUX <yvan.roux@foss.st.com>
|
| |
|
|
|
|
|
|
|
|
| |
For Arm Cortex-M33 with security extensions, there are 4 different
stacks pointers (msp_s, msp_ns, psp_s, psp_ns).
When plain "sp" is updated during unwinding of the stack, the active
stack pointer of the 4 stack pointers needs to be kept in sync.
Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
Signed-off-by: Yvan Roux <yvan.roux@foss.st.com>
|
| |
|
|
|
|
|
|
|
| |
For Cortex-M targets, SP register is never detached from msp or
psp, it always has the same value as one of them. Let GDB treat
ARM_SP_REGNUM as an alias similar to what is done in hardware.
Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
Signed-off-by: Yvan Roux <yvan.roux@foss.st.com>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
For Arm Cortex-M33 with security extensions, there are 4 different
stack pointers (msp_s, msp_ns, psp_s, psp_ns). To be compatible
with earlier Cortex-M derivates, the msp and psp registers are
aliases for one of the 4 real stack pointer registers.
These are the combinations that exist:
sp -> msp -> msp_s
sp -> msp -> msp_ns
sp -> psp -> psp_s
sp -> psp -> psp_ns
This means that when the GDB client is to show the value of "msp",
the value should always be equal to either "msp_s" or "msp_ns".
Same goes for "psp".
To add a bit more context; GDB does not really use the register msp
(or psp) internally, but they are part of the set of registers which
are provided by the target.xml file. As a result, they will be part
of the set of registers printed by the "info r" command.
Without this particular patch, GDB will hit the assert in the bottom
of arm_cache_get_sp_register function.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29121
Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
Signed-off-by: Yvan Roux <yvan.roux@foss.st.com>
|
| |
|
|
|
|
|
|
|
|
|
| |
For Arm Cortex-M33 with security extensions, there are 4 different
stack pointers (msp_s, msp_ns, psp_s, psp_ns). In order to
identify the active one, compare the values of the different
stacks. The value of the initial sp register needs to be fetched to
perform this comparison.
Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
Signed-off-by: Yvan Roux <yvan.roux@foss.st.com>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
The motivation for this change is an upcoming Python disassembler API
that I would like to add. As part of that change I need to create a
new disassembler like class that contains a disassemble_info and a
gdbarch. The management of these two objects is identical to how we
manage these objects within gdb_disassembler, so it might be tempting
for my new class to inherit from gdb_disassembler.
The problem however, is that gdb_disassembler has a tight connection
between its constructor, and its print_insn method. In the
constructor the ui_file* that is passed in is replaced with a member
variable string_file*, and then in print_insn, the contents of the
member variable string_file are printed to the original ui_file*.
What this means is that the gdb_disassembler class has a tight
coupling between its constructor and print_insn; the class just isn't
intended to be used in a situation where print_insn is not going to be
called, which is how my (upcoming) sub-class would need to operate.
My solution then, is to separate out the management of the
disassemble_info and gdbarch into a new gdb_disassemble_info class,
and make this class a parent of gdb_disassembler.
In arm-tdep.c and mips-tdep.c, where we used to cast the
disassemble_info->application_data to a gdb_disassembler, we can now
cast to a gdb_disassemble_info as we only need to access the gdbarch
information.
Now, my new Python disassembler sub-class will still want to print
things to an output stream, and so we will want access to the
dis_asm_fprintf functionality for printing.
However, rather than move this printing code into the
gdb_disassemble_info base class, I have added yet another level of
hierarchy, a gdb_printing_disassembler, thus the class structure is
now:
struct gdb_disassemble_info {};
struct gdb_printing_disassembler : public gdb_disassemble_info {};
struct gdb_disassembler : public gdb_printing_disassembler {};
In a later commit my new Python disassembler will inherit from
gdb_printing_disassembler.
The reason for adding the additional layer to the class hierarchy is
that in yet another commit I intend to rewrite the function
gdb_buffered_insn_length, and to do this I will be creating yet more
disassembler like classes, however, these will not print anything,
thus I will add a gdb_non_printing_disassembler class that also
inherits from gdb_disassemble_info. Knowing that that change is
coming, I've gone with the above class hierarchy now.
There should be no user visible changes after this commit.
|
