Mark Wielaard <mjw(a)redhat.com> writes:
> + (aarch64_regs_no_Wformat): Likewise.
OK, but why do you need no_Wformat?
For the following:
> + regtype (const char *setname, int type, const char *fmt, int
arg)
> + [...]
> + int s = snprintf (name, namelen, fmt, arg);
GCC warns that it can't verify the argument types, as FMT is not a
literal.
> +/* 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.
The code is actually buggy. ebl_register_info should return number of
bytes written including the terminating zero. So "return s + 1;". For
reserved registers, we don't even need to bother setting *SETNAME,
simply returning 0 will do and we get rid of reserved() altogether.
> +static int
> +peel_type (Dwarf_Die **typediep, Dwarf_Attribute **attrp)
That might be handy in libebl_CPU.h so other backends can reuse it.
Agreed (and for get_die_type as well).
> +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?
I think it's cleaner like this. Internal functions in C are somewhat
suspicious in that one never knows whether they touch the surrounding
scopes.
> +static int hfa_type (Dwarf_Die *ftypedie, int tag,
> + Dwarf_Word *sizep, Dwarf_Word *countp);
hfa is "homogeneous floating-point aggregate"?
Yes. I guess I should move the comment from down there up here, it
expands the abbreviation.
> + 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.
Hmm, hmm, I suppose we actually might! I'll get this (and the one other
case) fixed.
Wow, would be nice to have some testcases for these.
Your suggested comment would be nice.
I have a test binary that I debugged this on. I'll add that to the test
suite together with the source (which is fairly large). That will just
tell us that what aarch64_return_value_location returns matches what we
think the ABI is, not whether our idea of the ABI is the correct one,
but at least it will work as a regression test and we can fix bugs in
the test case as we go.
BTW. tests/run-strip-reloc.sh might be nice to add for aarch64.
OK, I'll add it.
> +regs_test testfile_aarch64_core <<\EOF
> +integer registers:
Add a comment, see run-readelf-mixed-corenote.sh how to regenerate this
core file.
OK.
Thanks for the review, I'll send an updated patch.
Petr