Re: [PATCH 2/2] Support AArch64 architecture

[Mark Wielaard <mjw at redhat dot com>]

Hi Petr,

This all looks good. Just some nitpicks below. Feel free to ignore, I
don't actually expect you to refactor other backends and common methods.
Just noted you had some nice helpers that might be helpful for the next
backend porter.

On Thu, 2013-11-14 at 01:05 +0100, Petr Machata wrote:
> diff --git a/backends/ChangeLog b/backends/ChangeLog
> +2013-11-14  Petr Machata  <pmachata@redhat.com>
> +
> +	* Makefile.am (modules): Add aarch64.
> +	(libebl_pic): Add libebl_aarch64_pic.a.
> +	(aarch64_SRCS): New variable.
> +	(libebl_aarch64_pic_a_SOURCES): Likewise.
> +	(am_libebl_aarch64_pic_a_OBJECTS): Likewise.
> +	(aarch64_regs_no_Wformat): Likewise.

OK, but why do you need no_Wformat?

> +	* aarch64_corenote.c, aarch64_init.c: New files.
> +	* aarch64_regs.c, aarch64_reloc.def: Likewise.
> +	* aarch64_retval.c, aarch64_symbol.c: Likewise.

> +++ b/backends/aarch64_corenote.c
> @@ -0,0 +1,162 @@
> +/* AArch64 specific core note handling.

> +++ b/backends/aarch64_init.c
> @@ -0,0 +1,61 @@
> +/* Initialization of AArch64 specific backend library.

> diff --git a/backends/aarch64_regs.c b/backends/aarch64_regs.c
> +/* Register names and numbers for AArch64 DWARF.
> +  ssize_t
> +  regtype (const char *setname, int type, const char *fmt, int arg)
> +  {
> +    *setnamep = setname;
> +    *typep = type;
> +    int s = snprintf (name, namelen, fmt, arg);
> +    if (s > 0 && (unsigned) s >= namelen)
> +      s = -1;
> +    return s;
> +  }
> +
> +  ssize_t
> +  reserved (void)
> +  {
> +    return regtype (NULL, DW_ATE_void, "", 0);
> +  }

I would just have done *setnamep = NULL; return 0;
But I guess that will be how the compiler eventually optimizes it.

> diff --git a/backends/aarch64_reloc.def b/backends/aarch64_reloc.def
> @@ -0,0 +1,157 @@
> +/* List the relocation types for AArch64.  -*- C -*-

> diff --git a/backends/aarch64_retval.c b/backends/aarch64_retval.c
> +/* Function return value location for Linux/AArch64 ABI.

> +static int
> +peel_type (Dwarf_Die **typediep, Dwarf_Attribute **attrp)
> +{
> +  int tag = DWARF_TAG_OR_RETURN (*typediep);
> +  /* Follow typedefs and qualifiers to get to the actual type.  */
> +  while (tag == DW_TAG_typedef
> +	 || tag == DW_TAG_const_type || tag == DW_TAG_volatile_type
> +	 || tag == DW_TAG_restrict_type || tag == DW_TAG_mutable_type)
> +    {
> +      *attrp = dwarf_attr_integrate (*typediep, DW_AT_type, *attrp);
> +      *typediep = dwarf_formref_die (*attrp, *typediep);
> +      tag = DWARF_TAG_OR_RETURN (*typediep);
> +    }
> +
> +  return tag;
> +}

That might be handy in libebl_CPU.h so other backends can reuse it.

> +static int
> +get_die_type (Dwarf_Die *die, Dwarf_Die *result)
> +{
> +  Dwarf_Attribute attr_mem;
> +  Dwarf_Attribute *attr = dwarf_attr_integrate (die, DW_AT_type, &attr_mem);
> +  if (attr == NULL)
> +    /* The function has no return value, like a `void' function in C.  */
> +    return 0;
> +
> +  if (dwarf_formref_die (attr, result) == NULL)
> +    return -1;
> +
> +  return peel_type (&result, &attr);
> +}

Likewise.

> +static int
> +skip_until (Dwarf_Die *child, int tag)
> +{
> +  int i;
> +  while (DWARF_TAG_OR_RETURN (child) != tag)
> +    if ((i = dwarf_siblingof (child, child)) != 0)
> +      /* If there are no members, then this is not a HFA.  Errors
> +	 are propagated.  */
> +      return i;
> +  return 0;
> +}

Generic function, but specific to hfa_type. Maybe define there?

> +static int hfa_type (Dwarf_Die *ftypedie, int tag,
> +		     Dwarf_Word *sizep, Dwarf_Word *countp);

hfa is "homogeneous floating-point aggregate"?

> +/* Return 0 if MEMBDIE refers to a member with a floating-point or HFA
> +   type, or 1 if it's not.  Return -1 for errors.  The meaning of the
> +   remaining arguments is as documented at hfa_type.  */
> +static int
> +member_is_fp (Dwarf_Die *membdie, Dwarf_Word *sizep, Dwarf_Word *countp)
> +{
> +  Dwarf_Die type_die;
> +  int tag = get_die_type (membdie, &type_die);
> +  switch (tag)
> +    {
> +    case DW_TAG_base_type:;
> +      Dwarf_Word encoding;
> +      Dwarf_Attribute attr_mem;
> +      if (dwarf_attr_integrate (&type_die, DW_AT_encoding, &attr_mem) == NULL
> +	  || dwarf_formudata (&attr_mem, &encoding) != 0)
> +	return -1;
> +
> +      switch (encoding)
> +	{
> +	case DW_ATE_complex_float:
> +	  *countp = 2;
> +	  break;
> +
> +	case DW_ATE_float:
> +	  *countp = 1;
> +	  break;
> +
> +	default:
> +	  return 1;
> +	}
> +
> +      if (dwarf_formudata (dwarf_attr_integrate (&type_die, DW_AT_byte_size,
> +						 &attr_mem), sizep) != 0)
> +	return -1;

I guess we will never actually see DW_AT_bit_size.

> +      *sizep /= *countp;
> +      return 0;
> +
> +    case DW_TAG_structure_type:
> +    case DW_TAG_union_type:
> +    case DW_TAG_array_type:
> +      return hfa_type (&type_die, tag, sizep, countp);
> +    }
> +
> +  return 1;
> +}
> +
> +/* HFA (Homogeneous Floating-point Aggregate) is an aggregate type
> +   whose members are all of the same floating-point type, which is
> +   then base type of this HFA.  Instead of being floating-point types
> +   directly, members can instead themselves be HFA.  Such HFA fields
> +   are handled as if their type were HFA base type.
> +
> +   This function returns 0 if TYPEDIE is HFA, 1 if it is not, or -1 if
> +   there were errors.  In the former case, *SIZEP contains byte size
> +   of the base type (e.g. 8 for IEEE double).  *COUNT is set to the
> +   number of leaf members of the HFA.  */
> +static int
> +hfa_type (Dwarf_Die *ftypedie, int tag, Dwarf_Word *sizep, Dwarf_Word *countp)
> +{
> +  assert (tag == DW_TAG_structure_type || tag == DW_TAG_class_type
> +	  || tag == DW_TAG_union_type || tag == DW_TAG_array_type);
> +
> +  int i;
> +  if (tag == DW_TAG_array_type)
> +    {
> +      Dwarf_Word tot_size;
> +      if (dwarf_aggregate_size (ftypedie, &tot_size) < 0)
> +	return -1;
> +
> +      /* For vector types, we don't care about the underlying
> +	 type, but only about the vector type itself.  */
> +      bool vec;
> +      Dwarf_Attribute attr_mem;
> +      if (dwarf_formflag (dwarf_attr_integrate (ftypedie, DW_AT_GNU_vector,
> +						&attr_mem), &vec) == 0
> +	  && vec)
> +	{
> +	  *sizep = tot_size;
> +	  *countp = 1;
> +
> +	  return 0;
> +	}
> +
> +      if ((i = member_is_fp (ftypedie, sizep, countp)) == 0)
> +	{
> +	  *countp = tot_size / *sizep;
> +	  return 0;
> +	}
> +
> +      return 1;

You already saw this should return i.
I would have missed it.

> +    }
> +
> +  /* Find first DW_TAG_member and determine its type.  */
> +  Dwarf_Die member;
> +  if ((i = dwarf_child (ftypedie, &member) != 0))
> +    return i;
> +
> +  if ((i = skip_until (&member, DW_TAG_member)) != 0)
> +    return i;
> +
> +  *countp = 0;
> +  if ((i = member_is_fp (&member, sizep, countp)) != 0)
> +    return i;
> +
> +  while ((i = dwarf_siblingof (&member, &member)) == 0
> +	 && (i = skip_until (&member, DW_TAG_member)) == 0)
> +    {
> +      Dwarf_Word size, count;
> +      if ((i = member_is_fp (&member, &size, &count)) != 0)
> +	return i;
> +
> +      if (*sizep != size)
> +	return 1;
> +
> +      *countp += count;
> +    }
> +
> +  return i < 0 ? i : 0;
> +}

Wow, would be nice to have some testcases for these.
Your suggested comment would be nice.

The rest looks OK.

> diff --git a/backends/aarch64_symbol.c b/backends/aarch64_symbol.c
> +/* AArch64 specific symbolic name handling.
> [...]
> +/* Check for the simple reloc types.  */
> +Elf_Type
> +aarch64_reloc_simple_type (Ebl *ebl __attribute__ ((unused)), int type)
> +{
> +  switch (type)
> +    {
> +    case R_AARCH64_ABS64:
> +      return ELF_T_XWORD;
> +    case R_AARCH64_ABS32:
> +      return ELF_T_WORD;
> +    case R_AARCH64_ABS16:
> +      return ELF_T_HALF;
> +
> +    default:
> +      return ELF_T_NUM;
> +    }
> +}

Looks OK.
BTW. tests/run-strip-reloc.sh might be nice to add for aarch64.

> diff --git a/libebl/ChangeLog b/libebl/ChangeLog
> index 57f9f46..0ecd16f 100644
> --- a/libebl/ChangeLog
> +++ b/libebl/ChangeLog
> @@ -1,3 +1,7 @@
> +2013-11-14  Petr Machata  <pmachata@redhat.com>
> +
> +	* eblopenbackend.c (machines): Add entry for AArch64.

OK.

> +2013-11-14  Petr Machata  <pmachata@redhat.com>
> +
> +	* elflint.c (valid_e_machine): Add EM_AARCH64.

OK.

> +2013-11-14  Petr Machata  <pmachata@redhat.com>
> +
> +	* testfile_aarch64_core.bz2: New file.
> +	* Makefile.am (EXTRA_DIST): Add it.

OK.

> +	* run-allregs.sh: Use it to add a regs_test.
> +	* run-readelf-mixed-corenote.sh: Use it to add a readelf -n test.

> diff --git a/tests/run-allregs.sh b/tests/run-allregs.sh
> index 885a1d1..e4a40ce 100755
> --- a/tests/run-allregs.sh
> +++ b/tests/run-allregs.sh
> @@ -1,5 +1,5 @@
>  #! /bin/sh
> -# Copyright (C) 2005, 2006, 2007, 2012 Red Hat, Inc.
> +# Copyright (C) 2005, 2006, 2007, 2012, 2013 Red Hat, Inc.
>  # This file is part of elfutils.
>  #
>  # This file is free software; you can redistribute it and/or modify
> @@ -2724,4 +2724,74 @@ VFP registers:
>  	287: d31 (d31), float 64 bits
>  EOF
>  
> +regs_test testfile_aarch64_core <<\EOF
> +integer registers:

Add a comment, see run-readelf-mixed-corenote.sh how to regenerate this
core file.

> diff --git a/tests/run-readelf-mixed-corenote.sh b/tests/run-readelf-mixed-corenote.sh
> index 9a43809..c176e28 100755
> --- a/tests/run-readelf-mixed-corenote.sh
> +++ b/tests/run-readelf-mixed-corenote.sh
> @@ -285,4 +285,144 @@ Note segment of 1476 bytes at offset 0x430:
>        3e001ba000-3e001bc000 001ba000 8192            /usr/lib64/libc-2.17.so
>  EOF
>  
> +# To reproduce this core dump, do this on an aarch64 machine:
> +# $ gcc -x c <(echo 'int main () { return *(int *)0x12345678; }')
> +# $ ./a.out
> +testfiles testfile_aarch64_core
> +testrun_compare ${abs_top_builddir}/src/readelf -n testfile_aarch64_core <<\EOF

OK.