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
