Re: [PATCH v2 1/5] Arm: Add read_description read funcs and use in GDB

Alan Hayward
2019-07-11T18:04:13Z

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 20109 bytes --]

> On 11 Jul 2019, at 14:57, Simon Marchi <simark@simark.ca> wrote:
>
> Oh, also please test rebuilding each patch against latest master, you will find the
> places that need to be changed from "common" to "gdbsupport", such as here:
>
>>

I had rebased, but looks like I had something left over causing the build to work.
Fixed.

> On 11 Jul 2019, at 15:19, Simon Marchi <simark@simark.ca> wrote:
>
> On 2019-07-11 9:46 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.
>>
>> Under the hood return the same existing pre-feature target descriptions.
>>
>> Note: This commit will break the AArch64 gdbserver build.
>
> Sorry for spamming you, one last thing :)
>
> I forgot to ask you, what is it in this patch that is breaking the AArch64
> gdbserver build in this? And what would it take for you not to break it?
>
> We generally try to keep things building at each commit, so I'd just like
> a bit more justifications about why it's worth breaking it here, rather than
> doing the necessary work to keep it building.
>

Annoyingly (for me) it’s just a single ifdef needed in arch/arm.c. I originally
had patch 4 directly after patch 1, which would have then required adding the
Arm files that get generated at build time into the aarch64 build.
Anyway, fixed.

Updated patch below:

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index b6680b9afd..62a59c90ea 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 \
@@ -1429,6 +1432,7 @@ HFILES_NO_SRCDIR = \
xml-syscall.h \
xml-tdesc.h \
xtensa-tdep.h \
+ arch/aarch32.h \
arch/aarch64.h \
arch/aarch64-insn.h \
arch/arm.h \
@@ -2133,6 +2137,7 @@ force_update:
MAKEOVERRIDES =

ALLDEPFILES = \
+ aarch32-tdep.c \
aarch64-fbsd-nat.c \
aarch64-fbsd-tdep.c \
aarch64-linux-nat.c \
diff --git a/gdb/aarch32-tdep.c b/gdb/aarch32-tdep.c
new file mode 100644
index 0000000000..12d464171c
--- /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 "gdbsupport/common-defs.h"
+#include "gdbsupport/common-regcache.h"
+#include "arch/aarch32.h"
+
+static struct target_desc *tdesc_aarch32;
+
+/* See aarch32-tdep.h. */
+
+const target_desc *
+aarch32_read_description ()
+{
+ if (tdesc_aarch32 == nullptr)
+ tdesc_aarch32 = aarch32_create_target_description ();
+
+ return tdesc_aarch32;
+}
diff --git a/gdb/aarch32-tdep.h b/gdb/aarch32-tdep.h
new file mode 100644
index 0000000000..7fcea0adb9
--- /dev/null
+++ b/gdb/aarch32-tdep.h
@@ -0,0 +1,25 @@
+/* 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/>. */
+
+#ifndef AARCH32_TDEP_H
+#define AARCH32_TDEP_H
+
+/* Get the AArch32 target description. */
+
+const target_desc *aarch32_read_description ();
+
+#endif /* aarch32-tdep.h. */
diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index 7b60a9a0c3..2c1f4d9f98 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -30,6 +30,7 @@
#include "aarch64-tdep.h"
#include "aarch64-linux-tdep.h"
#include "aarch32-linux-nat.h"
+#include "aarch32-tdep.h"
#include "arch/arm.h"
#include "nat/aarch64-linux.h"
#include "nat/aarch64-linux-hw-point.h"
@@ -631,8 +632,6 @@ aarch64_linux_nat_target::post_attach (int pid)
linux_nat_target::post_attach (pid);
}

-extern struct target_desc *tdesc_arm_with_neon;
-
/* Implement the "read_description" target_ops method. */

const struct target_desc *
@@ -649,7 +648,7 @@ aarch64_linux_nat_target::read_description ()

ret = ptrace (PTRACE_GETREGSET, tid, NT_ARM_VFP, &iovec);
if (ret == 0)
- return tdesc_arm_with_neon;
+ return aarch32_read_description ();

CORE_ADDR hwcap = linux_get_hwcap (this);

diff --git a/gdb/arch/aarch32.c b/gdb/arch/aarch32.c
new file mode 100644
index 0000000000..14d6987d3f
--- /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 "gdbsupport/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;
+}
diff --git a/gdb/arch/aarch32.h b/gdb/arch/aarch32.h
new file mode 100644
index 0000000000..d2c0047216
--- /dev/null
+++ b/gdb/arch/aarch32.h
@@ -0,0 +1,27 @@
+/* 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/>. */
+
+#ifndef ARCH_AARCH32_H
+#define ARCH_AARCH32_H
+
+#include "gdbsupport/tdesc.h"
+
+/* Create the AArch32 target description. */
+
+target_desc *aarch32_create_target_description ();
+
+#endif /* aarch32.h. */
diff --git a/gdb/arch/arm.c b/gdb/arch/arm.c
index 860ce02858..7a0f36e4c4 100644
--- a/gdb/arch/arm.c
+++ b/gdb/arch/arm.c
@@ -21,6 +21,17 @@
#include "gdbsupport/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;
+
+/* Temporary ifdef. Will be removed when target descriptions are switched. */
+#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 +383,52 @@ 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;
+/* Temporary ifdef. Will be removed when target descriptions are switched. */
+#ifndef GDBSERVER
+ 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;
+#endif
+ 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)
+ {
+/* Temporary ifdef. Will be removed when target descriptions are switched. */
+#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);
+ }
+}
+
diff --git a/gdb/arch/arm.h b/gdb/arch/arm.h
index dfbbd56d28..58511c7c6b 100644
--- a/gdb/arch/arm.h
+++ b/gdb/arch/arm.h
@@ -19,6 +19,8 @@
#ifndef ARCH_ARM_H
#define ARCH_ARM_H

+#include "gdbsupport/tdesc.h"
+
/* Register numbers of various important registers. */

enum gdb_regnum {
@@ -66,6 +68,23 @@ enum arm_breakpoint_kinds
ARM_BP_KIND_ARM = 4,
};

+/* Supported Arm FP hardware types. */
+enum arm_fp_type {
+ ARM_FP_TYPE_NONE = 0,
+ ARM_FP_TYPE_VFPV2,
+ ARM_FP_TYPE_VFPV3,
+ ARM_FP_TYPE_IWMMXT,
+ ARM_FP_TYPE_INVALID
+};
+
+/* Supported M-profile Arm types. */
+enum arm_m_profile_type {
+ ARM_M_TYPE_M_PROFILE,
+ ARM_M_TYPE_VFP_D16,
+ ARM_M_TYPE_WITH_FPA,
+ ARM_M_TYPE_INVALID
+};
+
/* Instruction condition field values. */
#define INST_EQ 0x0
#define INST_NE 0x1
@@ -165,4 +184,12 @@ unsigned long shifted_reg_val (struct regcache *regcache,
unsigned long pc_val,
unsigned long status_reg);

+/* Create an Arm target description with the given FP hardware type. */
+
+target_desc *arm_create_target_description (arm_fp_type fp_type);
+
+/* Create an Arm M-profile target description with the given hardware type. */
+
+target_desc *arm_create_mprofile_target_description (arm_m_profile_type m_type);
+
#endif /* ARCH_ARM_H */
diff --git a/gdb/arm-fbsd-tdep.c b/gdb/arm-fbsd-tdep.c
index dea3abbdd3..6e1af9cda3 100644
--- a/gdb/arm-fbsd-tdep.c
+++ b/gdb/arm-fbsd-tdep.c
@@ -20,6 +20,8 @@
#include "defs.h"

#include "elf/common.h"
+#include "target-descriptions.h"
+#include "aarch32-tdep.h"
#include "arm-tdep.h"
#include "arm-fbsd-tdep.h"
#include "auxv.h"
@@ -178,20 +180,20 @@ arm_fbsd_read_description_auxv (struct target_ops *target)
CORE_ADDR arm_hwcap = 0;

if (target_auxv_search (target, AT_FREEBSD_HWCAP, &arm_hwcap) != 1)
- return NULL;
+ return nullptr;

if (arm_hwcap & HWCAP_VFP)
{
if (arm_hwcap & HWCAP_NEON)
- return tdesc_arm_with_neon;
+ return aarch32_read_description ();
else if ((arm_hwcap & (HWCAP_VFPv3 | HWCAP_VFPD32))
== (HWCAP_VFPv3 | HWCAP_VFPD32))
- return tdesc_arm_with_vfpv3;
+ return arm_read_description (ARM_FP_TYPE_VFPV3);
else
- return tdesc_arm_with_vfpv2;
+ return arm_read_description (ARM_FP_TYPE_VFPV2);
}

- return NULL;
+ return nullptr;
}

/* Implement the "core_read_description" gdbarch method. */
diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c
index fe8a113a27..6a374bbc74 100644
--- a/gdb/arm-linux-nat.c
+++ b/gdb/arm-linux-nat.c
@@ -27,6 +27,7 @@
#include "observable.h"
#include "gdbthread.h"

+#include "aarch32-tdep.h"
#include "arm-tdep.h"
#include "arm-linux-tdep.h"
#include "aarch32-linux-nat.h"
@@ -551,7 +552,7 @@ arm_linux_nat_target::read_description ()
}

if (arm_hwcap & HWCAP_IWMMXT)
- return tdesc_arm_with_iwmmxt;
+ return arm_read_description (ARM_FP_TYPE_IWMMXT);

if (arm_hwcap & HWCAP_VFP)
{
@@ -566,11 +567,11 @@ arm_linux_nat_target::read_description ()
/* NEON implies VFPv3-D32 or no-VFP unit. Say that we only support
Neon with VFPv3-D32. */
if (arm_hwcap & HWCAP_NEON)
- return tdesc_arm_with_neon;
+ return aarch32_read_description ();
else if ((arm_hwcap & (HWCAP_VFPv3 | HWCAP_VFPv3D16)) == HWCAP_VFPv3)
- return tdesc_arm_with_vfpv3;
- else
- return tdesc_arm_with_vfpv2;
+ return arm_read_description (ARM_FP_TYPE_VFPV3);
+
+ return arm_read_description (ARM_FP_TYPE_VFPV2);
}

return this->beneath ()->read_description ();
diff --git a/gdb/arm-linux-tdep.c b/gdb/arm-linux-tdep.c
index d846749e0b..aec20877d9 100644
--- a/gdb/arm-linux-tdep.c
+++ b/gdb/arm-linux-tdep.c
@@ -33,6 +33,7 @@
#include "auxv.h"
#include "xml-syscall.h"

+#include "aarch32-tdep.h"
#include "arch/arm.h"
#include "arch/arm-get-next-pcs.h"
#include "arch/arm-linux.h"
@@ -738,14 +739,14 @@ arm_linux_core_read_description (struct gdbarch *gdbarch,
/* NEON implies VFPv3-D32 or no-VFP unit. Say that we only support
Neon with VFPv3-D32. */
if (arm_hwcap & HWCAP_NEON)
- return tdesc_arm_with_neon;
+ return aarch32_read_description ();
else if ((arm_hwcap & (HWCAP_VFPv3 | HWCAP_VFPv3D16)) == HWCAP_VFPv3)
- return tdesc_arm_with_vfpv3;
- else
- return tdesc_arm_with_vfpv2;
+ return arm_read_description (ARM_FP_TYPE_VFPV3);
+
+ return arm_read_description (ARM_FP_TYPE_VFPV2);
}

- return NULL;
+ return nullptr;
}

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index c1ee39714f..1b19b72ca1 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -240,6 +240,10 @@ 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. */
+static struct target_desc *tdesc_arm_list[ARM_FP_TYPE_INVALID];
+static struct target_desc *tdesc_arm_mprofile_list[ARM_M_TYPE_INVALID];
+
/* This is used to keep the bfd arch_info in sync with the disassembly
style. */
static void set_disassembly_style_sfunc (const char *, int,
@@ -8739,7 +8743,6 @@ arm_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
return default_register_reggroup_p (gdbarch, regnum, group);
}

-^L
/* 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
@@ -8753,21 +8756,26 @@ arm_register_g_packet_guesses (struct gdbarch *gdbarch)
{
if (gdbarch_tdep (gdbarch)->is_m)
{
+ const target_desc *tdesc;
+
/* If we know from the executable this is an M-profile target,
cater for remote targets whose register set layout is the
same as the FPA layout. */
+ tdesc = arm_read_mprofile_description (ARM_M_TYPE_WITH_FPA);
register_remote_g_packet_guess (gdbarch,
ARM_CORE_REGS_SIZE + ARM_FP_REGS_SIZE,
- tdesc_arm_with_m_fpa_layout);
+ tdesc);

/* The regular M-profile layout. */
+ tdesc = arm_read_mprofile_description (ARM_M_TYPE_M_PROFILE);
register_remote_g_packet_guess (gdbarch, ARM_CORE_REGS_SIZE,
- tdesc_arm_with_m);
+ tdesc);

/* M-profile plus M4F VFP. */
+ tdesc = arm_read_mprofile_description (ARM_M_TYPE_VFP_D16);
register_remote_g_packet_guess (gdbarch,
ARM_CORE_REGS_SIZE + ARM_VFP2_REGS_SIZE,
- tdesc_arm_with_m_vfp_d16);
+ tdesc);
}

/* Otherwise we don't have a useful guess. */
@@ -13310,3 +13318,35 @@ arm_process_record (struct gdbarch *gdbarch, struct regcache *regcache,

return ret;
}
+
+/* See arm-tdep.h. */
+
+const target_desc *
+arm_read_description (arm_fp_type fp_type)
+{
+ struct target_desc *tdesc = tdesc_arm_list[fp_type];
+
+ if (tdesc == nullptr)
+ {
+ tdesc = arm_create_target_description (fp_type);
+ tdesc_arm_list[fp_type] = tdesc;
+ }
+
+ return tdesc;
+}
+
+/* See arm-tdep.h. */
+
+const target_desc *
+arm_read_mprofile_description (arm_m_profile_type m_type)
+{
+ struct target_desc *tdesc = tdesc_arm_mprofile_list[m_type];
+
+ if (tdesc == nullptr)
+ {
+ tdesc = arm_create_mprofile_target_description (m_type);
+ tdesc_arm_mprofile_list[m_type] = tdesc;
+ }
+
+ return tdesc;
+}
diff --git a/gdb/arm-tdep.h b/gdb/arm-tdep.h
index 36d2d381cf..6d1a91ca35 100644
--- a/gdb/arm-tdep.h
+++ b/gdb/arm-tdep.h
@@ -281,11 +281,11 @@ extern void
void *cb_data,
const struct regcache *regcache);

-/* Target descriptions. */
-extern struct target_desc *tdesc_arm_with_m;
-extern struct target_desc *tdesc_arm_with_iwmmxt;
-extern struct target_desc *tdesc_arm_with_vfpv2;
-extern struct target_desc *tdesc_arm_with_vfpv3;
-extern struct target_desc *tdesc_arm_with_neon;
+/* Get the correct Arm target description with given FP hardware type. */
+const target_desc *arm_read_description (arm_fp_type fp_type);
+
+/* Get the correct Arm M-Profile target description with given hardware
+ type. */
+const target_desc *arm_read_mprofile_description (arm_m_profile_type m_type);

#endif /* arm-tdep.h */
diff --git a/gdb/configure.tgt b/gdb/configure.tgt
index 27f122ad04..7c0215e89a 100644
--- a/gdb/configure.tgt
+++ b/gdb/configure.tgt
@@ -48,8 +48,9 @@ amd64_tobjs="amd64-tdep.o arch/amd64.o ${x86_tobjs}"

case "${targ}" in
aarch64*-*-*)
- cpu_obs="aarch64-tdep.o arch/aarch64-insn.o arch/aarch64.o \
- ravenscar-thread.o aarch64-ravenscar-thread.o";;
+ cpu_obs="aarch32-tdep.o aarch64-tdep.o arch/aarch32.o \
+ arch/aarch64-insn.o arch/aarch64.o ravenscar-thread.o \
+ aarch64-ravenscar-thread.o";;

alpha*-*-*)
# Target: Alpha
@@ -62,7 +63,8 @@ arc*-*-*)
;;

arm*-*-*)
- cpu_obs="arch/arm.o arch/arm-get-next-pcs.o arm-tdep.o";;
+ cpu_obs="aarch32-tdep.o arch/aarch32.o arch/arm.o \
+ arch/arm-get-next-pcs.o arm-tdep.o";;

hppa*-*-*)
# Target: HP PA-RISC

\x16º&Öéj×!zÊÞ¶êç×žùÓ‰b²Ö«r\x18\x1dn–­r\x17¬