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, ®s[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
