Re: [PATCH] Patch 2 of 2 for aarch64 ILP32 support in gdb

[Yao Qi <qiyaoltc at gmail dot com>]

On 17-01-24 16:20:39, Steve Ellcey wrote:
> @@ -2851,10 +2852,14 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>    const struct tdesc_feature *feature;
>    int num_regs = 0;
>    int num_pseudo_regs = 0;
> +  int ilp32 = FALSE;
> +

We can use bool now, because we moved to C++ :)

> +  if (info.bfd_arch_info->mach == bfd_mach_aarch64_ilp32)
> +    ilp32 = TRUE;
>  
>    /* Ensure we always have a target descriptor.  */
>    if (!tdesc_has_registers (tdesc))
> -    tdesc = tdesc_aarch64;
> +    tdesc = ilp32 ? tdesc_aarch64_ilp32 : tdesc_aarch64;
>  
>    gdb_assert (tdesc);
>  
> diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
> index 85c6a97..4c9b58d 100644
> --- a/gdb/aarch64-tdep.h
> +++ b/gdb/aarch64-tdep.h
> @@ -97,6 +97,9 @@ struct gdbarch_tdep
>  
>    /* syscall record.  */
>    int (*aarch64_syscall_record) (struct regcache *regcache, unsigned long svc_number);
> +  /* If this is ILP32 or LP64.  */
> +  int ilp32;

bool ilp32;

> diff --git a/gdb/features/aarch64_ilp32-core.xml b/gdb/features/aarch64_ilp32-core.xml
> index e69de29..ace8d62 100644
> --- a/gdb/features/aarch64_ilp32-core.xml
> +++ b/gdb/features/aarch64_ilp32-core.xml
> @@ -0,0 +1,67 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2009-2016 Free Software Foundation, Inc.

2016 - 2017

> +     Contributed by ARM Ltd.

Remove this line according to the attribution policy
https://sourceware.org/gdb/wiki/ContributionChecklist#Attribution

> +
> +     Copying and distribution of this file, with or without modification,
> +     are permitted in any medium without royalty provided the copyright
> +     notice and this notice are preserved.  -->
> +
> +<!DOCTYPE feature SYSTEM "gdb-target.dtd">
> +<feature name="org.gnu.gdb.aarch64.core">
> +  <reg name="x0" bitsize="64"/>
> +  <reg name="x1" bitsize="64"/>
> +  <reg name="x2" bitsize="64"/>
> +  <reg name="x3" bitsize="64"/>
> +  <reg name="x4" bitsize="64"/>
> +  <reg name="x5" bitsize="64"/>
> +  <reg name="x6" bitsize="64"/>
> +  <reg name="x7" bitsize="64"/>
> +  <reg name="x8" bitsize="64"/>
> +  <reg name="x9" bitsize="64"/>
> +  <reg name="x10" bitsize="64"/>
> +  <reg name="x11" bitsize="64"/>
> +  <reg name="x12" bitsize="64"/>
> +  <reg name="x13" bitsize="64"/>
> +  <reg name="x14" bitsize="64"/>
> +  <reg name="x15" bitsize="64"/>
> +  <reg name="x16" bitsize="64"/>
> +  <reg name="x17" bitsize="64"/>
> +  <reg name="x18" bitsize="64"/>
> +  <reg name="x19" bitsize="64"/>
> +  <reg name="x20" bitsize="64"/>
> +  <reg name="x21" bitsize="64"/>
> +  <reg name="x22" bitsize="64"/>
> +  <reg name="x23" bitsize="64"/>
> +  <reg name="x24" bitsize="64"/>
> +  <reg name="x25" bitsize="64"/>
> +  <reg name="x26" bitsize="64"/>
> +  <reg name="x27" bitsize="64"/>
> +  <reg name="x28" bitsize="64"/>
> +  <reg name="x29" bitsize="64"/>
> +  <reg name="x30" bitsize="64"/>

I diff aarch64-core.xml and aarch64_ilp32-core.xml, and see a lot of
differences,

> +  <reg name="sp" bitsize="64" type="int64"/>

s/int64/uint64/ ?

> +
> +  <reg name="pc" bitsize="64" type="uint64"/>

-  <reg name="sp" bitsize="64" type="data_ptr"/>
+  <reg name="sp" bitsize="64" type="int64"/>
 
-  <reg name="pc" bitsize="64" type="code_ptr"/>
+  <reg name="pc" bitsize="64" type="uint64"/>

Do we really need these changes? I know data_ptr and code_ptr is 32-bit
on ilp32, but registers are 64-bit.  However, I still think we shouldn't
create different target descriptions for different ABIs, because target
description is about the hardware stuff.  If we add target descriptions
for different ABIs, we have to add two copies of target descriptions for
two ABIs every time a new hardware feature is added.  That is a pain of
maintenance.

Could you remove these ilp32 xml files, use the existing aarch64 target
description, and try the patch below?

Note that x32 adds xml files some years ago
https://sourceware.org/ml/gdb-patches/2012-04/msg00018.html but I think
the idea is still worth baking and the x32 patch was not fully reviewed.

> +
> +  <flags id="cpsr_flags" size="4">
> +    <field name="SP" start="0" type="bool"/>

-    <field name="SP" start="0" end="0"/>
+    <field name="SP" start="0" type="bool"/>

Why is that? out of sync, maybe?  That may be no longer a question if we
don't need ilp32 target description.

> diff --git a/gdb/gdbserver/linux-aarch64-low.c b/gdb/gdbserver/linux-aarch64-low.c
> index 334310b..7258c83 100644
> --- a/gdb/gdbserver/linux-aarch64-low.c
> +++ b/gdb/gdbserver/linux-aarch64-low.c
> @@ -42,6 +42,8 @@
>  /* Defined in auto-generated files.  */
>  void init_registers_aarch64 (void);
>  extern const struct target_desc *tdesc_aarch64;
> +void init_registers_aarch64_ilp32 (void);
> +extern const struct target_desc *tdesc_aarch64_ilp32;
>  
>  #ifdef HAVE_SYS_REG_H
>  #include <sys/reg.h>
> @@ -484,8 +486,13 @@ aarch64_linux_read_description (void)
>  
>    is_elf64 = linux_pid_exe_is_elf_64_file (tid, &machine);
>  
> -  if (is_elf64)
> +  if (sizeof (void *) == 4 && is_elf64)
> +    error (_("Can't debug 64-bit process with 32-bit GDBserver"));

ilp32 GDBserver can't debug lp64 program?

> +
> +  if (machine == EM_AARCH64 && is_elf64)
>      return tdesc_aarch64;
> +  else if (machine == EM_AARCH64 && !is_elf64)
> +    return tdesc_aarch64_ilp32;
>    else
>      return tdesc_arm_with_neon;

-- 
Yao (齐尧)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 801c03d..e23ac75 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -2091,6 +2091,22 @@ aarch64_gen_return_address (struct gdbarch *gdbarch,
 }
 
 
+/* Implement the "register_type" gdbarch method.
+   Adjust the register type of $PC and $SP on ILP32.  */
+
+static struct type *
+aarch64_ilp32_register_type (struct gdbarch *gdbarch, int regnum)
+{
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
+  gdb_assert (tdep->ilp32);
+
+  if (regnum == AARCH64_SP_REGNUM || regnum == AARCH64_PC_REGNUM)
+    return builtin_type (gdbarch)->builtin_uint64;
+  else
+    return tdesc_register_type (gdbarch, regnum);
+}
+
 /* Return the pseudo register name corresponding to register regnum.  */
 
 static const char *
@@ -3012,6 +3028,13 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 
   tdesc_use_registers (gdbarch, tdesc, tdesc_data);
 
+  if (ilp32)
+    {
+      /* Override tdesc_register_type to adjust the types of $PC and
+	 $SP in ILP32.  */
+      set_gdbarch_register_type (gdbarch, aarch64_ilp32_register_type);
+    }
+
   /* Add standard register aliases.  */
   for (i = 0; i < ARRAY_SIZE (aarch64_register_aliases); i++)
     user_reg_add (gdbarch, aarch64_register_aliases[i].name,