This is the mail archive of the
elfutils-devel@sourceware.org
mailing list for the elfutils project.
Re: [PATCH] Support AArch64 architecture
- From: Mark Wielaard <mjw at redhat dot com>
- To: elfutils-devel at lists dot fedorahosted dot org
- Date: Fri, 22 Nov 2013 21:31:06 +0100
- Subject: 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