This is the mail archive of the elfutils-devel@sourceware.org mailing list for the elfutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Support AArch64 architecture


On Fri, Nov 22, 2013 at 12:53:57AM +0100, Petr Machata wrote:
> Besides nits this
> includes the following fairly large changes:
> 
>  - added hello_aarch64.ko.bz2 and enabled it in run-strip-reloc.sh.
> 
>  - added funcretval_test.c with source (not compiled, just for
>    reference, as it is too large and unwieldy to stick into a testfile
>    comment).  This yields funcretval_test_aarch64.bz2, which is used in
>    a new test suite run-funcretval.sh.

Thanks a lot for these new testcase. Everything looks fine, but could
you take a look at my comment for regtype (). With that fixed or explained
it looks good to go in.

> diff --git a/backends/ChangeLog b/backends/ChangeLog
> index 3c57f8c..e7d1268 100644
> --- a/backends/ChangeLog
> +++ b/backends/ChangeLog
> @@ -1,3 +1,17 @@
> +2013-11-22  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.
> +	* aarch64_corenote.c, aarch64_init.c: New files.
> +	* aarch64_regs.c, aarch64_reloc.def: Likewise.
> +	* aarch64_retval.c, aarch64_symbol.c: Likewise.
> +	* libebl_CPU.h (dwarf_peel_type): New function.
> +	(dwarf_peeled_die_type): Likewise.

OK.

> diff --git a/backends/aarch64_regs.c b/backends/aarch64_regs.c
> +  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;

I would have expected if (s < 0 || (unsigned) s >= namelen)
to signal a snprintf error.

> +    return s + 1;
> +  }

So if there is an error (given name array is too short?) then
s = -1 + 1 = 0. But that looks like an unused register number.

> diff --git a/backends/aarch64_retval.c b/backends/aarch64_retval.c
> +static int
> +dwarf_bytesize_aux (Dwarf_Die *die, Dwarf_Word *sizep)
> +{
> +  int bits;
> +  if (((bits = 8 * dwarf_bytesize (die)) < 0
> +       && (bits = dwarf_bitsize (die)) < 0)
> +      || bits % 8 != 0)
> +    return -1;
> +
> +  *sizep = bits / 8;
> +  return 0;
> +}

Cute.

> +static int
> +member_is_fp (Dwarf_Die *membdie, Dwarf_Word *sizep, Dwarf_Word *countp)
> +{
> +  Dwarf_Die typedie;
> +  int tag = dwarf_peeled_die_type (membdie, &typedie);
> +  switch (tag)
> +    {
> +    case DW_TAG_base_type:;
> +      Dwarf_Word encoding;
> +      Dwarf_Attribute attr_mem;
> +      if (dwarf_attr_integrate (&typedie, DW_AT_encoding, &attr_mem) == NULL
> +	  || dwarf_formudata (&attr_mem, &encoding) != 0)
> +	return -1;

Nitpick. A block is nicer in this case than the :; case thingy. IMHO.

> diff --git a/libebl/ChangeLog b/libebl/ChangeLog
> +2013-11-22  Petr Machata  <pmachata@redhat.com>
> +
> +	* eblopenbackend.c (machines): Add entry for AArch64.

OK.

> diff --git a/src/ChangeLog b/src/ChangeLog
> +2013-11-22  Petr Machata  <pmachata@redhat.com>
> +
> +	* elflint.c (valid_e_machine): Add EM_AARCH64.

OK.

> diff --git a/tests/ChangeLog b/tests/ChangeLog
> +2013-11-22  Petr Machata  <pmachata@redhat.com>
> +
> +	* testfile_aarch64_core.bz2, hello_aarch64.ko.bz2: New files.
> +	* funcretval_test.c, funcretval_test_aarch64.bz2: Likewise.
> +	* Makefile.am (EXTRA_DIST): Add these.
> +	(TESTS): Add run-funcretval.sh.
> +	* run-allregs.sh: Use testfile_aarch64_core.bz2 for a regs_test.
> +	* run-readelf-mixed-corenote.sh: ... and for a readelf -n test.
> +	* run-strip-reloc.sh: Add a test on hello_aarch64.ko.bz2.
> +	* run-funcretval.sh: New file.

OK.

It would be nice if funcretval printed the ops in a more human
readable form. varlocs.c has some code for that. But that is for
another time.

Thanks,

Mark

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]