Re: [PATCH, FT32] Proper support for address <-> pointer conversions

[Yao Qi <qiyaoltc at gmail dot com>]

James Bowman <james.bowman@ftdichip.com> writes:

> FT32 is a Harvard architecture with two address spaces -- RAM and flash.
> This patch properly implements the address to pointer conversion method,
> and its reverse. There are some other small fixes to handle address
> spaces.

Hi James,
Thanks for your patch.  A few comments below...

Please split your patch, in order to place each independent small fix in
one patch.

>
> OK to apply?
>
> 2015-09-21  James Bowman  <james.bowman@ftdichip.com>
>
> 	* ft32-tdep.c: Properly support address <-> pointer conversions,
> 	so that FT32's address spaces (RAM and flash) work correctly
>         when dereferencing pointers in gdb.
> 	* ft32-tdep.h: Add a type for an address space 1 pointer.

The changelog entry looks incorrect to me.  Please read
https://sourceware.org/gdb/wiki/ContributionChecklist

>
>
> diff --git a/gdb/ft32-tdep.c b/gdb/ft32-tdep.c
> index 2e5deca..d6e8c6d 100644
> --- a/gdb/ft32-tdep.c
> +++ b/gdb/ft32-tdep.c
> @@ -117,7 +117,7 @@ static struct type *
>  ft32_register_type (struct gdbarch *gdbarch, int reg_nr)
>  {
>    if (reg_nr == FT32_PC_REGNUM)
> -    return builtin_type (gdbarch)->builtin_func_ptr;
> +    return gdbarch_tdep (gdbarch)->pc_type;
>    else if (reg_nr == FT32_SP_REGNUM || reg_nr == FT32_FP_REGNUM)
>      return builtin_type (gdbarch)->builtin_data_ptr;
>    else
> @@ -270,6 +270,88 @@ ft32_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
>    return pc;
>  }
>  
> +/* Convert from address to pointer and vice-versa.  */
> +
> +static void
> +ft32_address_to_pointer (struct gdbarch *gdbarch,
> +			 struct type *type, gdb_byte *buf, CORE_ADDR addr)
> +{
> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +
> +  store_unsigned_integer (buf, TYPE_LENGTH (type), byte_order,
> +			  addr);
> +}

ft32_address_to_pointer is exactly the same as
unsigned_address_to_pointer, which is the default implementation of
gdbarch method address_to_pointer.  IOW, we don't need this function.

> +
> +static CORE_ADDR
> +ft32_pointer_to_address (struct gdbarch *gdbarch,
> +			 struct type *type, const gdb_byte *buf)

We need comments to this function, like,

"Implementation of ....."

> +{
> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +  CORE_ADDR addr
> +    = extract_unsigned_integer (buf, TYPE_LENGTH (type), byte_order);

A blank line is needed here.

> +  if (TYPE_ADDRESS_CLASS_1 (type))
> +    return addr;
> +  else
> +    return addr | RAM_BIAS;
> +}



> +
> +static CORE_ADDR
> +ft32_integer_to_address (struct gdbarch *gdbarch,
> +			 struct type *type, const gdb_byte *buf)
> +{
> +  ULONGEST addr = unpack_long (type, buf);
> +
> +  return addr;
> +}
> +

Do we really need this function?  Looks it makes no differences after
this hook is installed to gdbarch.

>  
>  static CORE_ADDR
> @@ -498,6 +580,15 @@ ft32_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>    tdep = XNEW (struct gdbarch_tdep);
>    gdbarch = gdbarch_alloc (&info, tdep);
>  
> +  /* Create a type for PC.  We can't use builtin types here, as they may not
> +     be defined.  */
> +  tdep->void_type = arch_type (gdbarch, TYPE_CODE_VOID, 1, "void");
> +  tdep->func_void_type = make_function_type (tdep->void_type, NULL);
> +  tdep->pc_type = arch_type (gdbarch, TYPE_CODE_PTR, 4, NULL);
> +  TYPE_TARGET_TYPE (tdep->pc_type) = tdep->func_void_type;
> +  TYPE_UNSIGNED (tdep->pc_type) = 1;
> +  TYPE_INSTANCE_FLAGS (tdep->pc_type) |= TYPE_INSTANCE_FLAG_ADDRESS_CLASS_1;
> +
>    set_gdbarch_read_pc (gdbarch, ft32_read_pc);
>    set_gdbarch_write_pc (gdbarch, ft32_write_pc);
>    set_gdbarch_unwind_sp (gdbarch, ft32_unwind_sp);
> @@ -510,6 +601,10 @@ ft32_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>  
>    set_gdbarch_return_value (gdbarch, ft32_return_value);
>  
> +  set_gdbarch_address_to_pointer (gdbarch, ft32_address_to_pointer);
> +  set_gdbarch_pointer_to_address (gdbarch, ft32_pointer_to_address);
> +  set_gdbarch_integer_to_address (gdbarch, ft32_integer_to_address);
> +
>    set_gdbarch_skip_prologue (gdbarch, ft32_skip_prologue);
>    set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
>    set_gdbarch_breakpoint_from_pc (gdbarch, ft32_breakpoint_from_pc);
> @@ -535,6 +630,12 @@ ft32_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>    /* Support simple overlay manager.  */
>    set_gdbarch_overlay_update (gdbarch, simple_overlay_update);
>  
> +  set_gdbarch_address_class_type_flags (gdbarch, ft32_address_class_type_flags);
> +  set_gdbarch_address_class_name_to_type_flags
> +    (gdbarch, ft32_address_class_name_to_type_flags);
> +  set_gdbarch_address_class_type_flags_to_name
> +    (gdbarch, ft32_address_class_type_flags_to_name);
> +
>    return gdbarch;
>  }
>  
> diff --git a/gdb/ft32-tdep.h b/gdb/ft32-tdep.h
> index 5c52480..f236163 100644
> --- a/gdb/ft32-tdep.h
> +++ b/gdb/ft32-tdep.h
> @@ -22,7 +22,12 @@
>  
>  struct gdbarch_tdep
>  {
> -  /* gdbarch target dependent data here.  Currently unused for FT32.  */
> +  /* Type for void.  */
> +  struct type *void_type;
> +  /* Type for a function returning void.  */
> +  struct type *func_void_type;
> +  /* Type for a pointer to a function.  Used for the type of PC.  */
> +  struct type *pc_type;

Only pc_type is used (in ft32_register_type), so we don't need to add
fields void_type and func_void_type.

-- 
Yao (éå)