Re: [PATCH 3/7] Arm: Add read_description read funcs and use in GDB

[Alan Hayward <Alan.Hayward@arm.com>]

> On 10 Jul 2019, at 04:45, Simon Marchi <simark@simark.ca> wrote:
> 
> 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?

Should probably read “called the once for each target description type”. But,
removed due to comments below.

> 
>> 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.

Done.

> 
>> 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

Done.

> 
>> +
>> +/* See linux-aarch32-tdep.h.  */
> 
> /* See aarch32-tdep.h.  */

Gah! Done.

> 
>> 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.
> 

AArch32 is the 32bit mode in Arm v8. Compatible with Arm v7, but new features continue
to go into it via v8-M and v8-R.  Emulation is probably not quite the correct word, as
it’s all down in hardware.


> 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?

Yes. Yao split the AArch32 parts out from Arm in order to support multi-arch debugging
on AArch64. I’m not sure exactly what works on it.

> 
>> 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.

Right. I wasn’t thinking of arch as “architecture code shared with gdbserver”,
but it makes sense.

The GDBSERVER defines do vanish later in the series.

I think I preferred having the two functions together in arch, but I’ve moved it.


> 
>> 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

Done.

> 
>> +
>> /* 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);
>> }
>> 
>> -\f
>> /* 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”.

Yes, it’s just efficiency/caching.

> 
> 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.
> 

Agreed and done.

The above changes are going to require rebasing the other patches too. I’ll repost the
set as a V2.


Alan.