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

Mao Han han_mao@c-sky.com
Wed Jul 17 01:21:00 GMT 2019


On Tue, Jul 16, 2019 at 03:48:22PM +0200, Mark Wielaard wrote:
> > > > 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).

Yes, csky follows this assumption, so it is handled correctly already.
 
> 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.

Yes, please. Thanks for your review and help improveing the patch.

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

Thanks,
Mao Han



More information about the Elfutils-devel mailing list