| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
Remove uses of VLAs, replace with gdb::byte_vector. There might be more
in files that I can't compile, but it's difficult to tell without
actually compiling on all platforms.
Many thanks to the Linaro pre-commit CI for helping find some problems
with an earlier iteration of this patch.
Change-Id: I3e5e34fcac51f3e6b732bb801c77944e010b162e
Reviewed-by: Keith Seitz <keiths@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>
|
|
I've noticed that doc strings of some commands, like "set cwd"
and "set inferior-tty", have some excess whitespace, which
makes them display with unexpected indentation, at least in a
Windows command prompt window. This patch fixes that.
* gdb/linux-nat.c (_initialize_linux_nat):
* gdb/riscv-tdep.c (riscv_insn):
* gdb/top.c (quit_force):
* gdb/infcmd.c (_initialize_infcmd): Remove excess whitespace.
|
|
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>
|
|
Per RISC-V spec, the lr/sc sequence can consist of up to 16 instructions, and we
cannot insert breakpoints in the middle of this sequence. Before this, we only
detected a specific pattern (the most common one). This patch improves this part
and now supports more complex pattern detection.
Signed-off-by: Yang Liu <liuyang22@iscas.ac.cn>
Approved-By: Andrew Burgess <aburgess@redhat.com>
Reviewed-by: Palmer Dabbelt <palmer@rivosinc.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.
|
|
gdbarch_deprecated_pseudo_register_write
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>
|
|
C++17 makes the second parameter to static_assert optional, so we can
remove gdb_static_assert now.
|
|
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>
|
|
If the length of a register name was greater than 15,
print_spaces was called with a negative number, which
prints random data from the heap instead of the requested
number of spaces.
This could happen if a target-description file was used
to specify additional long-named registers.
Fix is simple - don't ask for fewer than 1 space (since
we still want column separation).
Approved-by: Kevin Buettner <kevinb@redhat.com>
|
|
Currently riscv gdb code looks at arch byte order
when fetching instructions. This works when the
target is LE, but on BE arch it will byte swap the
instruction, while the riscv spec defines all
instructions are LE encoded regardless of
system memory endianess.
|
|
Latest libc++[1] causes transitive include to <locale> when
<mutex> or <thread> header is included. This causes
gdb to not build[2] since <locale> defines isupper/islower etc.
functions that are explicitly macroed-out in safe-ctype.h to
prevent their use.
Use the suggestion from libc++ to include <locale> internally when
building in C++ mode to avoid build errors.
Use safe-gdb-ctype.h as the include instead of "safe-ctype.h"
to keep this isolated to gdb since rest of binutils
does not seem to use much C++.
[1]: https://reviews.llvm.org/D144331
[2]: https://issuetracker.google.com/issues/277967395
|
|
I was seeing some failures in gdb.threads/omp-par-scope.exp when run
on a riscv64 target. It turns out the cause of the problem is that I
didn't have debug information installed for libgomp.so, which this
test makes use of. The test requires GDB to backtrace through a
libgomp function, and the riscv prologue unwinder was failing to
unwind this particular stack frame.
The reason for the failure to unwind was that the function prologue
includes a c.li (compressed load immediate) instruction, and the riscv
prologue scanning unwinder doesn't know what to do with this
instruction, though the unwinder does understand c.lui (compressed
load unsigned immediate).
This commit adds support for c.li. After this GDB is able to unwind
through libgomp, and I no longer see any unexpected failures in
gdb.threads/omp-par-scope.exp.
I've also included a new test in gdb.arch/ which specifically checks
for our c.li support.
|
|
This commit builds on the previous one to fix all the remaining
failures in gdb.base/unwind-on-each-insn.exp for RISC-V.
The problem we have in gdb.base/unwind-on-each-insn.exp is that, when
we are in the function epilogue, the previous frame and stack pointer
values are being restored, and so, the values that we calculated
during the function prologue are no longer suitable.
Here's an example from the function 'bar' in the mentioned test. This
was compiled for 64-bit RISC-V with compressed instruction support:
Dump of assembler code for function bar:
0x000000000001018a <+0>: add sp,sp,-32
0x000000000001018c <+2>: sd ra,24(sp)
0x000000000001018e <+4>: sd fp,16(sp)
0x0000000000010190 <+6>: add fp,sp,32
0x0000000000010192 <+8>: sd a0,-24(fp)
0x0000000000010196 <+12>: ld a0,-24(fp)
0x000000000001019a <+16>: jal 0x10178 <foo>
0x000000000001019e <+20>: nop
0x00000000000101a0 <+22>: ld ra,24(sp)
0x00000000000101a2 <+24>: ld fp,16(sp)
0x00000000000101a4 <+26>: add sp,sp,32
0x00000000000101a6 <+28>: ret
End of assembler dump.
When we are at address 0x101a4 the previous instruction has restored
the frame-pointer, as such GDB's (current) preference for using the
frame-pointer as the frame base address is clearly not going to work.
We need to switch to using the stack-pointer instead.
At address 0x101a6 the previous instruction has restored the
stack-pointer value. Currently GDB will not understand this and so
will still assume the stack has been decreased by 32 bytes in this
function.
My proposed solution is to extend GDB such that GDB will scan the
instructions at the current $pc looking for this pattern:
ld fp,16(sp)
add sp,sp,32
ret
Obviously the immediates can change, but the basic pattern indicates
that the function is in the process of restoring state before
returning. If GDB sees this pattern then GDB can use the inferior's
position within this instruction sequence to help calculate the
correct frame-id.
With this implemented then gdb.base/unwind-on-each-insn.exp now fully
passes.
Obviously what I've implemented is just a heuristic. It's not going
to work for every function. If the compiler reorders the
instructions, or merges the epilogue back into the function body then
GDB is once again going to get the frame-id wrong.
I'm OK with that, we're no worse off that we are right now in that
situation (plus we can always improve the heuristic later).
Remember, this is for debugging code without debug information,
and (in our imagined situation) with more aggressive levels of
optimisation being used. Obviously GDB is going to struggle in these
situations.
My thinking is, lets get something in place now. Then, later, if
possible, we might be able to improve the logic to cover more
situations -- if there's an interest in doing so. But I figure we
need something in place as a starting point.
After this commit gdb.base/unwind-on-each-insn.exp passes with no
failures on RV64.
|
|
Add support to the RISC-V prologue scanner for c.ldsp and c.lwsp
instructions.
This fixes some of the failures in gdb.base/unwind-on-each-insn.exp,
though there are further failures that are not fixed by this commit.
This change started as a wider fix that would address all the failures
in gdb.base/unwind-on-each-insn.exp, however, that wider fix needed
support for the two additional compressed instructions.
When I added support for those two compressed instructions I noticed
that some of the failures in gdb.base/unwind-on-each-insn.exp resolved
themselves!
Here's what's going on:
The reason for the failures is that GDB is trying to build the
frame-id during the last few instructions of the function. These are
the instructions that restore the frame and stack pointers just prior
to the return instruction itself.
By the time we reach the function epilogue the stack offset that we
calculated during the prologue scan is no longer valid, and so we
calculate the wrong frame-id.
However, in the particular case of interest here, the test function
'foo', the function is so simple and short (the empty function) that
GDB's prologue scan could, in theory, scan every instruction of the
function.
I say "could, in theory," because currently GDB stops the prologue
scan early when it hits an unknown instruction. The unknown
instruction happens to be one of the compressed instructions that I'm
adding support for in this commit.
Now that GDB understands the compressed instructions the prologue scan
really does go from the start of the function right up to the current
program counter. As such, GDB sees that the stack frame has been
allocated, and then deallocated, and so builds the correct frame-id.
Of course, most real functions are not as simple as the test function
'foo'. As such, we can't usually rely on scanning right up to the end
of the function -- there are some instructions we always need to stop
at because GDB can't reason about how they change the inferior
state (e.g. a function call). The test function 'bar' is just such an
example.
After this commit, we can now build the frame-id correctly for every
instruction in 'foo', but there are some tests still failing in 'bar'.
|
|
Convert the RISC-V specific debug settings to use the new debug
printing scheme. This updates the following settings:
set/show debug riscv breakpoints
set/show debug riscv gdbarch
set/show debug riscv infcall
set/show debug riscv unwinder
All of these settings now take a boolean rather than an integer, and
all print their output using the new debug scheme.
There should be no visible change for anyone not turning on debug.
|
|
This commit is initial support for SystemTap for RISC-V Linux. The
following two tests exercise SystemTap functionality, and are showing
many failures, which are all fixed by this commit:
gdb.cp/exceptprint.exp
gdb.base/stap-probe.exp
One thing I wasn't sure about is if the SystemTap support should be
Linux specific, or architecture specific. For aarch64, arm, ia64, and
ppc, the SystemTap support seems to libe in the ARCH-linux-tdep.c
file, while for amd64, i386, and s390 the implementation lives in
ARCH-tdep.c. I have no idea which of these is the better choice -- or
maybe both choices are correct in the right circumstances, and I'm
just not aware of how to choose between them.
Anyway, for this patch I selected riscv-tdep.c (though clearly, moving
the changes to riscv-linux-tdep.c is trivial if anyone thinks that's a
more appropriate location).
The stap-probe.exp file tests immediate, register, and register
indirect operands, all of which appear to be working fine with this
commit. The generic expression support doesn't appear to be
architecture specific, so I'd expect that to work fine too.
|
|
This turns many functions that are related to optimized-out or
availability-checking to be methods of value. The static function
value_entirely_covered_by_range_vector is also converted to be a
private method.
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 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>
|
|
A few tdep files include block.h but do not need to. This patch
removes the inclusions. I checked that this worked correctly by
examining the resulting .Po file to make sure that block.h was not
being included by some other route.
|
|
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.
|
|
This changes the uses of value_print_options to use 'true' and 'false'
rather than integers.
|
|
According to the riscv psabi, the mapping relationship between the
DWARF registers and the machine registers is as follows:
DWARF Number | Register Name | Description
0 - 31 | x0 - x31 | Integer Registers
32 - 63 | f0 - f31 | Floating-point Registers
This is not modelled quite right in riscv_dwarf_reg_to_regnum, the
DWARF register numbers 31 and 63 are not handled correctly due to a
use of '<' instead of '<='. This commit fixes this issue.
|
|
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
|
|
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>
|
|
This just fixes some indentation in riscv-tdep.c.
|
|
Because riscv_insn_length started to support instructions up to 176-bit,
we need to increase buf size to 176-bit in size.
Also, that would break an assumption in riscv_insn::decode so this commit
fixes it, noting that instructions longer than 64-bit are not fully
supported yet.
|
|
After the previous few commit, gdbarch_register_name no longer returns
nullptr. This commit audits all the calls to gdbarch_register_name
and removes any code that checks the result against nullptr.
There should be no visible change after this commit.
|
|
I noticed a test failure in gdb.base/completion.exp for RISC-V on
a native Linux target, this is the failure:
(gdb) FAIL: gdb.base/completion.exp: complete 'info registers '
The problem is caused by a mismatch in the output of 'maint print
registers' and the completion list for 'info registers'. The 'info
registers' completion list contains less registers than
expected. Additionally, the list of registers extracted from the
'maint print registers' list was wrong too, in some cases the test was
grabbing the register number, rather than a register name,
Both of these problems have the same root cause, riscv_register_name
returns nullptr for some registers when it should return an empty
string.
The gdbarch_register_name API is not clearly documented anywhere, and
at first glance it would appear that the function can return either
nullptr, or an empty string to indicate that a register is not
available on the current target. Indeed, there are plenty of places
in GDB where we compare the output of gdbarch_register_name to both
nullptr and '\0' in order to see if a register is supported or not,
and there are plenty of targets that return empty string in some
cases, and nullptr in others.
However, the 'info registers' completion code (reg_or_group_completer)
clearly depends on user_reg_map_regnum_to_name only returning nullptr
when the passed in regnum is greater than the maximum possible
register number (i.e. after all physical registers, pseudo-registers,
and user-registers), this means that gdbarch_register_name should not
be returning nullptr.
I did consider "fixing" user_reg_map_regnum_to_name, if
gdbarch_register_name returns nullptr, I could convert to an empty
string at this point, but that felt like a real hack, so I discarded
that plan.
The next possibility I considered was "fixing" reg_or_group_completer
to not rely on nullptr to indicate the end marker. Or rather, I could
have reg_or_group_completer use gdbarch_num_cooked_regs, we know that
we should check at least that many register numbers. Then, once we're
passed that limit, we keep checking until we hit a nullptr. This
would absolutely work, and didn't actually feel that bad, but, it
still felt a little weird that gdbarch_register_name could return
nullptr OR the empty string to mean the same thing, so I wondered if
the "right" solution was to have gdbarch_register_name not return
nullptr. With this in mind I tried an experiment:
I added a self-test that, for each architecture, calls
gdbarch_register_name for every register number up to the
gdbarch_num_cooked_regs limit, and checks that the name is not
nullptr.
Only a handful of architectures failed this test, RISC-V being one of
them.
This seems to suggest that most architectures agree that the correct
API for gdbarch_register_name is to return an empty string for
registers that are not supported on the current target, and that
returning nullptr is really a mistake.
In this commit I will update the RISC-V target so that GDB no longer
returns nullptr from riscv_register_name, instead we return the empty
string.
In subsequent commits I will add the selftest that I mention above,
and will fix the targets that fail the selftest.
With this change the gdb.base/completion.exp test now passes.
|
|
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
|
|
First, some background on the RISC-V registers fflags, frm, and fcsr.
These three registers all relate to the floating-point status and
control mechanism on RISC-V. The fcsr is the floatint-point control
status register, and consists of two parts, the flags (bits 0 to 4)
and the rounding-mode (bits 5 to 7).
The fcsr register is just one of many control/status registers (or
CSRs) available on RISC-V. The fflags and frm registers are also
CSRs. These CSRs are aliases for the relevant parts of the fcsr
register. So fflags is an alias for bits 0 to 4 of fcsr, and frm is
an alias for bits 5 to 7 of fcsr.
This means that a user can change the floating-point rounding mode
either, by writing a complete new value into fcsr, or by writing just
the rounding mode into frm.
How this impacts on GDB is like this: a target description could,
legitimately include all three registers, fcsr, fflags, and frm. The
QEMU target currently does this, and this makes sense. The target is
emulating the complete system, and has all three CSRs available, so
why not tell GDB about this.
In contrast, the RISC-V native Linux target only has access to the
fcsr. This is because the ptrace data structure that the kernel uses
for reading and writing floating point state only contains a copy of
the fcsr, after all, this one field really contains both the fflags
and frm fields, so why carry around duplicate data.
So, we might expect that the target description for the RISC-V native
Linux GDB would only contain the fcsr register. Unfortunately, this
is not the case. The RISC-V native Linux target uses GDB's builtin
target descriptions by calling riscv_lookup_target_description, this
will then add an fpu feature from gdb/features/riscv, either
32bit-fpu.xml or 64bit-fpu.xml. The problem, is that these features
include an entry for fcsr, fflags, and frm. This means that GDB
expects the target to handle reading and writing these registers. And
the RISC-V native Linux target currently doesn't.
In riscv_linux_nat_target::store_registers and
riscv_linux_nat_target::fetch_registers only the fcsr register is
handled, this means that, for RISC-V native Linux, the fflags and frm
registers always show up as <unavailable> - they are present in the
target description, but the target doesn't know how to access the
registers.
A final complication relating to these floating pointer CSRs is which
target description feature the registers appear in.
These registers are CSRs, so it would seem sensible that these
registers should appear in the CSR target description feature.
However, when I first added RISC-V target description support, I was
using a RISC-V simulator that didn't support any CSRs other than the
floating point related ones. This simulator bundled all the float
related CSRs into the fpu target feature. This didn't feel completely
unreasonable to me, and so I had GDB check for these registers in
either target feature.
In this commit I make some changes relating to how GDB handles the
three floating point CSR:
1. Remove fflags and frm from 32bit-fpu.xml and 64bit-fpu.xml. This
means that the default RISC-V target description (which RISC-V native
FreeBSD), and the target descriptions created for RISC-V native Linux,
will not include these registers. There's nothing stopping some other
target (e.g. QEMU) from continuing to include all three of these CSRs,
the code in riscv-tdep.c continues to check for all three of these
registers, and will handle them correctly if they are present.
2. If a target supplied fcsr, but does not supply fflags and/or frm,
then RISC-V GDB will now create two pseudo registers in order to
emulate the two missing CSRs. These new pseudo-registers do the
obvious thing of just reading and writing the fcsr register.
3. With the new pseudo-registers we can no longer make use of the GDB
register numbers RISCV_CSR_FFLAGS_REGNUM and RISCV_CSR_FRM_REGNUM.
These will be the numbers used if the target supplies the registers in
its target description, but, if GDB falls back to using
pseudo-registers, then new, unique numbers will be used. To handle
this I've added riscv_gdbarch_tdep::fflags_regnum and
riscv_gdbarch_tdep::frm_regnum, I've then updated the RISC-V code to
compare against these fields.
When adding the pseudo-register support, it is important that the
pseudo-register numbers are calculated after the call to
tdesc_use_registers. This is because we don't know the total number
of physical registers until after this call, and the psuedo-register
numbers must follow on from the real (target supplied) registers.
I've updated some tests to include more testing of the fflags and frm
registers, as well as adding a new test.
|
|
On RISC-V the FCSR (float control/status register) is split into two
parts, FFLAGS (the flags) and FRM (the rounding mode). Both of these
two fields are part of the FCSR register, but can also be accessed as
separate registers in their own right. And so, we have three separate
registers, $fflags, $frm, and $fcsr, with the last of these being the
combination of the first two.
Here's how the bits of FCSR are split between FRM and FFLAGS:
,--------- FFLAGS
|---|
76543210 <----- FCSR
|-|
'--------------FRM
Here's how GDB currently displays these registers:
(gdb) info registers $fflags $frm $fcsr
fflags 0x0 RD:0 NV:0 DZ:0 OF:0 UF:0 NX:0
frm 0x0 FRM:0 [RNE (round to nearest; ties to even)]
fcsr 0x0 RD:0 NV:0 DZ:0 OF:0 UF:0 NX:0 FRM:0 [RNE (round to nearest; ties to even)]
Notice the 'RD' field which is present in both $fflags and $fcsr.
This field contains the value of the FRM field, which makes sense when
displaying the $fcsr, but makes no sense when displaying $fflags, as
the $fflags doesn't include the FRM field.
Additionally, the $fcsr already includes an FRM field, so the
information in 'RD' is duplicated. Consider this:
(gdb) set $frm = 0x3
(gdb) info registers $fflags $frm $fcsr │
fflags 0x0 RD:0 NV:0 DZ:0 OF:0 UF:0 NX:0
frm 0x3 FRM:3 [RUP (Round up towards +INF)]
fcsr 0x60 RD:3 NV:0 DZ:0 OF:0 UF:0 NX:0 FRM:3 [RUP (Round up towards +INF)]
See how the 'RD' field in $fflags still displays 0, while the 'RD' and
'FRM' fields in $fcsr show the same information.
The first change I propose in this commit is to remove the 'RD'
field. After this change the output now looks like this:
(gdb) info registers $fflags $frm $fcsr
fflags 0x0 NV:0 DZ:0 OF:0 UF:0 NX:0
frm 0x0 FRM:0 [RNE (round to nearest; ties to even)]
fcsr 0x0 NV:0 DZ:0 OF:0 UF:0 NX:0 FRM:0 [RNE (round to nearest; ties to even)]
Next, I spotted that the text that goes along with the 'FRM' field was
not wrapped in the i18n markers for internationalisation, so I added
those.
Next, I spotted that:
(gdb) set $frm=0x7
(gdb) info registers $fflags $frm $fcsr
fflags 0x0 RD:0 NV:0 DZ:0 OF:0 UF:0 NX:0
frm 0x7 FRM:3 [RUP (Round up towards +INF)]
fcsr 0xe0 RD:7 NV:0 DZ:0 OF:0 UF:0 NX:0 FRM:3 [RUP (Round up towards +INF)]
Notice that despite being a 3-bit field, FRM masks to 2-bits.
Checking the manual I can see that the FRM field is 3-bits, and is
defined for all 8 values. That GDB masks to 2-bits is just a bug I
think, so I've fixed this.
Finally, the 'FRM' text for value 0x7 is wrong. Currently we use the
text 'dynamic rounding mode' for value 0x7. However, this is not
really correct.
A RISC-V instruction can either encode the rounding mode within the
instruction, or a RISC-V instruction can choose to use a global,
dynamic rounding mode.
So, for the rounding-mode field of an _instruction_ the value 0x7
indicates "dynamic round mode", the instruction should defer to the
rounding mode held in the FRM field of the $fcsr.
But it makes no sense for the FRM of $fcsr to itself be set to
0x7 (dynamic rounding mode), and indeed, section 11.2, "Floating-Point
Control and Status Register" of the RISC-V manual, says that a value
of 0x7 in the $fcsr FRM field is invalid, and if an instruction has
_its_ round-mode set to dynamic, and the FRM field is also set to 0x7,
then an illegal instruction exception is raised.
And so, I propose changing the text for value 0x7 of the FRM field to
be "INVALID[7] (Dynamic rounding mode)". We already use the text
"INVALID[5]" and "INVALID[6]" for the two other invalid fields,
however, I think adding the extra "Dynamic round mode" hint might be
helpful.
I've added a new test that uses 'info registers' to check what GDB
prints for the three registers related to this patch. There is one
slight oddity with this test - for the fflags and frm registers, the
test accepts both the "normal" output (as described above), but also
allows these registers to be reported as '<unavailable>'.
The reason why I accept <unavailable> is that currently, the RISC-V,
native Linux target advertises these registers in its target
description, but then doesn't support reading or writing of these
registers, this results in the registers being reported as
unavailable.
A later patch in this series will address this issue, and will remove
this check for <unavailable>.
|
|
There's a comment in riscv-tdep.c that explains some of the background
about how we check for the fcsr, fflags, and frm registers within a
riscv target description.
This comment (and the functionality it describes) relates to how QEMU
advertises these registers within its target description.
Unfortunately, QEMU includes these three registers in both the fpu and
crs target description features. To work around this GDB uses one of
the register declarations, and ignores the other, this means the GDB
user sees a single copy of each register, and things just work.
When I originally wrote the comment I thought it didn't matter which
copy of the register GDB selected, the fpu copy or the csr copy, so
long as we just used one of them. The comment reflected this belief.
Upon further investigation, it turns out I was wrong. GDB has to use
the csr copy of the register. If GDB tries to use the register from
the fpu feature then QEMU will return an error when GDB tries to read
or write the register.
Luckily, the code within GDB (currently) will always select the csr
copy of the register, so nothing is broken, but the comment is wrong.
This commit updates the comment to better describe what is actually
going on.
Of course, I should probably also send a patch to QEMU to fix up the
target description that is sent to GDB.
|
|
The x0 (zero) register is read-only on RISC-V. Implement the
cannot_store_register gdbarch method to tell GDB this.
Without this method GDB will try to write to x0, and relies on the
target to ignore such writes. If you are using a target that
complains (or throws an error) when writing to x0, this change will
prevent this from happening.
The gdb.arch/riscv-reg-aliases.exp test exercises writing to x0, and
will show the errors when using a suitable target.
|
|
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.
|
|
Convert the 7 global, pre-defined, register groups const, and fix the
fall out (a minor tweak required in riscv-tdep.c).
There should be no user visible changes after this commit.
|
|
Convert the reggroup_new and reggroup_gdbarch_new functions to return
a 'const regggroup *', and fix up all the fallout.
There should be no user visible changes after this commit.
|
|
There's a set of 7 default register groups. If we don't add any
gdbarch specific register groups during gdbarch initialisation, then
when we iterate over the register groups using reggroup_next and
reggroup_prev we will make use of these 7 default groups. See the use
of default_groups in gdb/reggroups.c for details on this.
However, if the gdbarch adds its own groups during gdbarch
initialisation, then these groups will be used in preference to the
default groups.
A problem arises though if the particular architecture makes use of
the target description mechanism. If the default target
description(s) (i.e. those internal to GDB that are used when the user
doesn't provide their own) don't mention any additional register
groups then the default register groups will be used.
But if the target description does mention additional groups then the
default groups are not used, and instead, the groups from the target
description are used.
The problem with this is that what usually happens is that the target
description will mention additional groups, e.g. groups for special
registers. Most architectures that use target descriptions work
around this by adding all (or most) of the default register groups in
all cases. See i386_add_reggroups, aarch64_add_reggroups,
riscv_add_reggroups, xtensa_add_reggroups, and others.
In this patch, my suggestion is that we should just add the default
register groups for every architecture, always. This change is in
gdb/reggroups.c.
All the remaining changes are me updating the various architectures to
not add the default groups themselves.
So, where will this change be visible to the user? I think the
following commands will possibly change:
* info registers / info all-registers:
The user can provide a register group to these commands. For example,
on csky, we previously never added the 'vector' group. Now, as a
default group, this will be available, but (presumably) will not
contain any registers. I don't think this is necessarily a bad
thing, there's something to be said for having some consistent
defaults available. There are other architectures that didn't add
all 7 of the defaults, which will now have gained additional groups.
* maint print reggroups
This prints the set of all available groups. As a maintenance
command I'm less concerned with the output changing here.
Obviously, for the architectures that didn't previously add all the
defaults, this list just got bigger.
* maint print register-groups
This prints all the registers, and the groups they are in. If the
defaults were not previously being added then a register (obviously)
can't appear in one of the default groups. Now the groups are
available then registers might be in more groups than previously.
However, this is again a maintenance command, so I'm less concerned
about this changing.
|
