Re: [PATCH 2/2] Support AArch64 architecture

[Petr Machata <pmachata at redhat dot com>]

Mark Wielaard <mjw@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