[PATCH V2 2/2] Add backend support for C-SKY

Tue Apr 16 07:14:00 GMT 2019

On Mon, Apr 15, 2019 at 01:08:58PM +0200, Mark Wielaard wrote:
> > +	* backends/Makefile.am: Add C-SKY.
> > +	* backends/csky_cfi.c: New file.
> > +	* backends/csky_corenote.c: Likewise.
> > +	* backends/csky_init.c: Likewise.
> > +	* backends/csky_initreg.c: Likewise.
> > +	* backends/csky_regs.c: Likewise.
> > +	* backends/csky_reloc.def: Likewise.
> > +	* backends/csky_symbol.c: Likewise.
> > +	* libebl/eblopenbackend.c: Add C-SKY.
> > +	* src/elflint.c: Likewise.
> 
> We have ChangeLog files per directory.
> So you don't need to prefix with backends/
> And the last two entries should go into their own (libebl and src)
> ChangeLog files.
>
OK

> > diff --git a/backends/csky_corenote.c b/backends/csky_corenote.c
> > new file mode 100644
> > index 0000000..c32b386
> > --- /dev/null
> > +++ b/backends/csky_corenote.c
> > +#include <elf.h>
> > +#include <inttypes.h>
> > +#include <stddef.h>
> > +#include <stdio.h>
> > +#include <sys/time.h>
> > +
> > +#define BACKEND	csky_
> > +#include "libebl_CPU.h"
> > +
> > +#define	ULONG			uint32_t
> > +#define PID_T			int32_t
> > +#define	UID_T			uint32_t
> > +#define	GID_T			uint32_t
> > +#define ALIGN_ULONG		4
> > +#define ALIGN_PID_T		4
> > +#define ALIGN_UID_T		4
> > +#define ALIGN_GID_T		4
> > +#define TYPE_ULONG		ELF_T_WORD
> > +#define TYPE_PID_T		ELF_T_SWORD
> > +#define TYPE_UID_T		ELF_T_WORD
> > +#define TYPE_GID_T		ELF_T_WORD
> > +
> > +static const Ebl_Register_Location prstatus_regs[] =
> > +  {
> > +    { .offset = 0, .regno = 0, .count = 38, .bits = 32 } /* r0..r31 */
> > +  };
> > +#define PRSTATUS_REGS_SIZE	(38 * 4)
> > +
> > +#include "linux-core-note.c"
> 
> OK, that is a very simple 32bit architecture/register setup.

The actual size of pr_reg is 36 word, so changed to
static const Ebl_Register_Location prstatus_regs[] =
  {
    /* pc, a1, a0, sr, r2 ~ r31, reserved, reserved */
    { .offset = 0, .regno = 0, .count = 36, .bits = 32 }
  };
#define PRSTATUS_REGS_SIZE      (36 * 4)

We also have 400 bytes fpu registers .reg2 section, will it
cause any prblem if it is not handled?

> > +  /* gcc/config/ #define DWARF_FRAME_REGISTERS.  */
> > +  eh->frame_nregs = 71;
> > +
> > +  return MODVERSION;
> 
> The 71 matches what can/is being restored through other hooks. But
> seems to not match the c-SKY V2 CPU ABI DWARF register mappings. Also
> maybe it should be 70, because you are using link as return register
> and don't explicitly need to store the pc (see below).

As I mentioned in:
https://sourceware.org/ml/elfutils-devel/2019-q2/msg00006.html
DWARF mappings from the document is mismatch with GCC source code.

71 comes from FIRST_PSEUDO_REGISTER
./gcc/defaults.h:#define DWARF_FRAME_REGISTERS FIRST_PSEUDO_REGISTER
The DWARF register mappings should be some thing like(rr stands 
for reserved):
// r0   r1   r2   r3   r4   r5   r6   r7
// 0    1    2    3    4    5    6    7
// r8   r9   r10  r11  r12  r13  sp   lr
// 8    9    10   11   12   13   14   15
// r16  r17  r18  r19  r20  r21  r22  r23
// 16   17   18   19   20   21   22   23
// r24  r25  r26  r27  r28  r29  r30  r31
// 24   25   26   27   28   29   30   31
// rr   rr   rr   rr   hi   lo   epc
// 32   33   34   35   36   37   72
This part is quite similar to native register numbering.
from 38 to 68 are fpu registers, each fpu register is 
consist of two dwarf register. I was intended to support
the fpu part, but the fpu register mapping is not so clear
at present. So I'll drop the fpu part first, only use 38
DWARF registers.

> > diff --git a/backends/csky_regs.c b/backends/csky_regs.c
> > new file mode 100644
> > index 0000000..0b6706e
> > +
> This confuses me too. So there are 71 registers? But you only handle
> 0..38? The names don't seem to match the usage in other places, but I
> am assuming DWARF register numbers match native register numbering.
> Which might not be how things actually work?
>
Likewise.

> > +  dwarf_regs[3] = user_regs.a3;
> > +  for (int i = 4; i < 14; i++)
> > +    dwarf_regs[i] = user_regs.regs[i - 4];
> > +  /* r ~ r13.  */
> > +  for (int i = 16; i < 31; i++)
> > +    dwarf_regs[i] = user_regs.exregs[i - 16];
> > +  /* tls.  */
> > +  dwarf_regs[31] = user_regs.tls;
> > +  /* hi.  */
> > +  dwarf_regs[34] = user_regs.rhi;
> > +  /* lo.  */
> > +  dwarf_regs[35] = user_regs.rlo;
> > +  /* pc.  */
> > +  dwarf_regs[70] = user_regs.pc;
> > +
> > +  return setfunc (0, 71, dwarf_regs, arg);
> > +#endif
> > +}
> 
> I think you should use setfunc (-1, -1, &regs[70], arg) to set the pc
> explicitly and then setfunc (0, 36, dwarf_regs, arg) to set the rest.
> But you don't seem to actually need the full 71 dwarf_regs array. Again
> I am confused about the actual DWARF register number mapping.
> 
OK

> 
> > +bool
> > +csky_machine_flag_check (GElf_Word flags)
> > +{
> > +  switch (flags & EF_CSKY_ABIMASK)
> > +    {
> > +    case EF_CSKY_ABIV1:
> > +    case EF_CSKY_ABIV2:
> > +      return true;
> > +    default:
> > +      return false;
> > +    }
> > +}
> 
> OK.
> Does the current backend handle both?
ABIV1 is not supported with this patch, so return false for ABIV1?

> > +const char *
> > +csky_section_type_name (int type,
> > +                       char *buf __attribute__ ((unused)),
> > +                       size_t len __attribute__ ((unused)))
> > +{
> > +  if (type == SHT_CSKY_ATTRIBUTES)
> > +    return "CSKY_ATTRIBUTES";
> > +
> > +  return NULL;
> > +}
> 
> OK.
> I couldn't find any description of this section.
> Is it like SHT_ARM_ATTRIBUTES/SHT_GNU_ATTRIBUTES?
> Then you might also want to handle it like that in src/readelf.c
> (print_attributes).
Yes, it is.
static int
process_csky_specific (FILE * file)
{
  return process_attributes (file, "csky", SHT_CSKY_ATTRIBUTES,
			     display_csky_attribute, NULL);
}
binutils-gdb/binutils/readelf.c
https://github.com/c-sky/binutils-gdb/blob/binutils-2_27-branch-csky/binutils/readelf.c

Thanks,
Mao Han