Re: [PATCH V4 0/1] Add C-SKY support

[Mark Wielaard <mark at klomp dot org>]

On Tue, 2019-07-16 at 19:14 +0800, Mao Han wrote:
> > > The ELF_ARCH was defined as 39 inside linux kernel(before 4.20),
> > > so
> > > the the core file generated was EM_RCE.
> > > I'v regenerated the core file with ELF_ARCH defined as 252:
> > > https://github.com/c-sky/test-result/blob/master/elfutils/zeroptr
> > > 
https://github.com/c-sky/test-result/raw/master/elfutils/core.zeroptr
> > > We have extended our core dump reg set last year, linux kernel
> > > have
> > > upstreamed this change, but the gdb does not. I'v only got an old
> > > released toolchain(EM_RCE) and the upstream toolchain(out of date
> > > reg set) in my enviroment, so I can't generate/verify EM_CSKY
> > > core
> > > file with any toolchain I have.
> > 
> > So the zeroptr binary looks correct now.
> > But the core.zeroptr is probably generated by an older linux kernel
> > and
> > still is EM_RCE. This confusion is a bit confusing since I makes it
> > really hard to check the details (register numbering, etc.) are
> > really
> > correct. I can tweak the code a bit to accept EM_RCE as EM_CSKY but
> > then the PRSTATUS core note doesn't look correct. I am not sure
> > that is
> > because the register set as dumped by the kernel is bogus or
> > because
> > the prstatus_regs definition in csky_corenote.c is incorrect.
> 
> vmh@vmh-VirtualBox:~/workspace/test-result/elfutils$ file
> core.zeroptr
> core.zeroptr: ELF 32-bit LSB core file *unknown arch 0xfc* version 1
> (SYSV), SVR4-style, from 'root/zeroptr'
> 
> The core.zeroptr should be EM_CSKY now. I've tested the core file
> with
> non-public released toolchain with EM_CSKY(the upstream gdb don't
> support csky coredump). GPR seems loaded correctly. I think the
> prstatus_regs have the correct reg size (36 * 4), problem may come
> from the coredump regs->dwarf regs mapping(the mapping and coredump
> support for binutil is not upsteamed).

You are right. Things look a little garbled because we use
ebl_register_info () and the hook to get the names and types for the
register numbers in various places. But ebl_register_info assumes DWARF
register numbers. We do have ebl_dwarf_to_regno, but we really also
need ebl_regno_to_dwarf. But we don't have it and we should audit the
code to make sure it is used in all the right places.

We have been going over this new backend for a while now. I think the
register mapping are slightly unique in this case (ppc has some
oddness, but we hacked around it without proper hooks...). Lets decide
to tackle this after the release, when we are going to update the ebl
backends anyway. Hopefully by then the rest of the toolchain has been
updated too to the C-SKY ABIv2 making creating testcases easier.

> > > These attributes including cpu name and some other ISA related
> > > descriptions.
> > > Some thing like:
> > > CSKY_ARCH_NAME: "ck810"
> > > CSKY_CPU_NAME:  "ck810f"
> > > CSKY_ISA_FLAG:  0x12345678
> > > CSKY_ISA_EXT_FLAG:  5
> > > They are not documented yet.
> > > I'v ask the person who is responsible for these to update the ABI
> > > documents, but I think it will take a quite long time for them to
> > > do that. They are quite busy at present.
> > 
> > OK. If you can add that tweak to src/readelf.c and add an
> > check_object_attribute hook that handles the above attributes that
> > would be good.
> > 
> > Ideally you also add a testcase for tests/readelf-A.sh
> > Some of those tests cheat and create the attributes by hand.
> > But it would be nice if you could generate a small .o file with the
> > latest toolchain to be used as testcase in some other tests.
> 
> I'm not sure about how to handle different data type here. It seems
> only tag_name is required when data type is string, I could not
> found how to handle int here.
> The binary with csky.attribute currently can not be generate with
> public
> released toolchain, so I don't know how to add the testcase.

OK, lets add a testcase once this has gone upstream in the rest of the
toolchain. Good point about the tag representing a string or number.
But I think this is (accidentially) handled correctly already. See this
comment in readelf.c (print_attributes):

   /* GNU style tags have either a uleb128 value,
      when lowest bit is not set, or a string
      when the lowest bit is set.
      "compatibility" (32) is special.  It has
      both a string and a uleb128 value.  For
      non-gnu we assume 6 till 31 only take ints.
      XXX see arm backend, do we need a separate
      hook?  */

We probably need another hook one day, but it looks like csky follows
this assumption (4 and 5 are strings, 6 and 7 are numbers).

There is one bug in the implementation though. The vendor check is
wrong, checks for "gnu", should obviously be "csky":

diff --git a/backends/csky_attrs.c b/backends/csky_attrs.c
index 9b236f1c..177f0ba2 100644
--- a/backends/csky_attrs.c
+++ b/backends/csky_attrs.c
@@ -43,7 +43,7 @@ csky_check_object_attribute (Ebl *ebl __attribute__ ((unused)),
                            const char **tag_name,
                            const char **value_name __attribute__ ((unused)))
 {
-  if (!strcmp (vendor, "gnu"))
+  if (!strcmp (vendor, "csky"))
     switch (tag)
       {
       case 4:

> Tested on x86
> ============================================================================
> Testsuite summary for elfutils 0.176
> ============================================================================
> # TOTAL: 209
> # PASS:  204
> # SKIP:  5
> # XFAIL: 0
> # FAIL:  0
> # XPASS: 0
> # ERROR: 0
> ============================================================================
> 
> Changes since v1:
>   - Add the Signed-off-by line and the copyright
> 
> Changes since v2:
>   - move changelog to corresponding entries
>   - correct core dump registers size
>   - remove unused fpu DWARF register
> 
> Changes since v3:
>   - add testfilecsky.bz2 and hello_csky.ko.bz2
>   - add csky_check_object_attribute
> 
> Mao Han (1):
>   Add backend support for C-SKY

The new patch looks really good. Thanks. The addition of the testcases
really helps showing things look good. I can make that one small fix
s/gnu/csky/ in csky_attrs.c if you agree that is what was intended.
Then I'll push it to master.

And after the next release we can add some more testcases and handle to
register mappings more correctly.

Thanks,

Mark