This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 3/7] Arm: Add read_description read funcs and use in GDB
On 2019-07-05 5:45 a.m., Alan Hayward wrote:
> Switch the Arm target to get target descriptions via arm_read_description
> and aarch32_read_description, in the same style as other feature targets.
> Add an enum to specify the different types - this will also be of use to
> gdbserver in a later patch.
>
> Call arm_create_mprofile_target_description directly as these will only be
> called the once, therefore they do not need caching.
I was going to point out "called the once", because it sounds strange to me,
but there is another instance of it in function arm_register_g_packet_guesses,
so maybe it's ok?
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 7308ea5767..9352dd92ff 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -665,7 +665,9 @@ ALL_64_TARGET_OBS = \
>
> # All other target-dependent objects files (used with --enable-targets=all).
> ALL_TARGET_OBS = \
> + aarch32-tdep.o \
> arc-tdep.o \
> + arch/aarch32.o \
> arch/arm.o \
> arch/arm-get-next-pcs.o \
> arch/arm-linux.o \
> @@ -1184,6 +1186,7 @@ SFILES = \
> # right, it is probably easiest just to list .h files here directly.
>
> HFILES_NO_SRCDIR = \
> + aarch32-tdep.h \
> aarch64-ravenscar-thread.h \
> aarch64-tdep.h \
> ada-lang.h \
> @@ -1431,6 +1434,7 @@ HFILES_NO_SRCDIR = \
> xtensa-tdep.h \
> arch/aarch64.h \
> arch/aarch64-insn.h \
> + arch/aarch32.h \
This line should go above the aarch64 ones.
> diff --git a/gdb/aarch32-tdep.c b/gdb/aarch32-tdep.c
> new file mode 100644
> index 0000000000..d9355d0665
> --- /dev/null
> +++ b/gdb/aarch32-tdep.c
> @@ -0,0 +1,33 @@
> +/* Copyright (C) 2019 Free Software Foundation, Inc.
> +
> + This file is part of GDB.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
> +
> +#include "common/common-defs.h"
> +#include "common/common-regcache.h"
> +#include "arch/aarch32.h"
> +
> +struct target_desc *tdesc_aarch32;
static
> +
> +/* See linux-aarch32-tdep.h. */
/* See aarch32-tdep.h. */
> diff --git a/gdb/arch/aarch32.c b/gdb/arch/aarch32.c
> new file mode 100644
> index 0000000000..f3cb8c7855
> --- /dev/null
> +++ b/gdb/arch/aarch32.c
> @@ -0,0 +1,29 @@
> +/* Copyright (C) 2019 Free Software Foundation, Inc.
> +
> + This file is part of GDB.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
> +
> +#include "common/common-defs.h"
> +#include "aarch32.h"
> +
> +extern struct target_desc *tdesc_arm_with_neon;
> +
> +/* See aarch32.h. */
> +
> +target_desc *
> +aarch32_create_target_description ()
> +{
> + return tdesc_arm_with_neon;
> +}
Can you briefly explain the difference (from the point of view of GDB) between the
ARM and AArch32 architecture? My current understanding is that AArch32 is Arm(v7?)
emulation on AArch64, so they are very close. But there might be some subtle
differences, requiring GDB to consider them as different architectures.
Edit: I now went through the patch and saw the configure.in change. My guess would
now be that it allows to include in an AArch64 build just what's actually supported
by the AArch32 mode, which is arm-with-neon, without having to pull the entire Arm
support. Does that sound right?
> diff --git a/gdb/arch/arm.c b/gdb/arch/arm.c
> index 93738f0a0f..37806753cb 100644
> --- a/gdb/arch/arm.c
> +++ b/gdb/arch/arm.c
> @@ -21,6 +21,15 @@
> #include "common/common-regcache.h"
> #include "arm.h"
>
> +extern struct target_desc *tdesc_arm_with_vfpv2;
> +extern struct target_desc *tdesc_arm_with_vfpv3;
> +extern struct target_desc *tdesc_arm_with_iwmmxt;
> +#ifndef GDBSERVER
> +extern struct target_desc *tdesc_arm_with_m;
> +extern struct target_desc *tdesc_arm_with_m_vfp_d16;
> +extern struct target_desc *tdesc_arm_with_m_fpa_layout;
> +#endif
> +
> /* See arm.h. */
>
> int
> @@ -372,3 +381,49 @@ shifted_reg_val (struct regcache *regcache, unsigned long inst,
>
> return res & 0xffffffff;
> }
> +
> +/* See arch/arm.h. */
> +
> +target_desc *
> +arm_create_target_description (arm_fp_type fp_type)
> +{
> + switch (fp_type)
> + {
> + case ARM_FP_TYPE_NONE:
> + return nullptr;
> +
> + case ARM_FP_TYPE_VFPV2:
> + return tdesc_arm_with_vfpv2;
> +
> + case ARM_FP_TYPE_VFPV3:
> + return tdesc_arm_with_vfpv3;
> +
> + case ARM_FP_TYPE_IWMMXT:
> + return tdesc_arm_with_iwmmxt;
> +
> + default:
> + error (_("Invalid Arm FP type: %d"), fp_type);
> + }
> +}
> +
> +/* See arch/arm.h. */
> +
> +target_desc *
> +arm_create_mprofile_target_description (arm_m_profile_type m_type)
> +{
> + switch (m_type)
> + {
> +#ifndef GDBSERVER
> + case ARM_M_TYPE_M_PROFILE:
> + return tdesc_arm_with_m;
> +
> + case ARM_M_TYPE_VFP_D16:
> + return tdesc_arm_with_m_fpa_layout;
> +
> + case ARM_M_TYPE_WITH_FPA:
> + return tdesc_arm_with_m_vfp_d16;
> +#endif
> + default:
> + error (_("Invalid Arm M type: %d"), m_type);
> + }
> +}
If it doesn't make sense to have this function shared with GDBserver (given
that GDBserver doesn't run on Cortex-Ms), it should probably go in gdb/arm-tdep.c.
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 8e3607cdea..f3f6458a27 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -240,6 +240,9 @@ static const char **valid_disassembly_styles;
> /* Disassembly style to use. Default to "std" register names. */
> static const char *disassembly_style;
>
> +/* All possible arm target descriptors. */
> +struct target_desc *tdesc_arm_list[ARM_FP_TYPE_INVALID];
static
> +
> /* This is used to keep the bfd arch_info in sync with the disassembly
> style. */
> static void set_disassembly_style_sfunc (const char *, int,
> @@ -8763,7 +8766,6 @@ arm_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
> return default_register_reggroup_p (gdbarch, regnum, group);
> }
>
> -
> /* For backward-compatibility we allow two 'g' packet lengths with
> the remote protocol depending on whether FPA registers are
> supplied. M-profile targets do not have FPA registers, but some
> @@ -8777,21 +8779,29 @@ arm_register_g_packet_guesses (struct gdbarch *gdbarch)
> {
> if (gdbarch_tdep (gdbarch)->is_m)
> {
> + const target_desc *tdesc;
> +
> + /* This function is only called the once, therefore it's safe to call the
> + tdesc creation function directly. */
The other instance of "called the once".
Would it be "unsafe" to call tdesc creation functions multiple times in general? I
thought it was just a question of efficiency/caching. If so, I'd say "therefore
it's not worth caching the descriptions".
It might be true that they are called only once today (I guess because the only way to
debug them in practice is through some server that only support debugging one at the
time), but it could change eventually. For example, with multi-target, you could
connect to two of them. Since it's not really difficult, I'd use the same caching pattern
as for the other ones.
Thanks,
Simon