This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH, V850] Add support for V850E2 and V850E2V3
- From: Nick Clifton <nickc at redhat dot com>
- To: "Naveen H. S" <Naveen dot S at kpitcummins dot com>
- Cc: "binutils at sourceware dot org" <binutils at sourceware dot org>
- Date: Thu, 17 Jun 2010 10:34:45 +0100
- Subject: Re: [PATCH, V850] Add support for V850E2 and V850E2V3
- References: <371569CBCFB2E745B891DBB88B2DFDDD19CAD461E9@KCINPUNHJCMS01.kpit.com>
Hi Naveen,
Thanks for submitting this patch, and please accept my apologise for
taking so long to review it.
Unfortunately the patch is not quite ready for inclusion into the
binutils sources yet. In particular it has these problems:
* It renumbers the existing v850 relocs.
This does not make much sense to me - it means that object files
(and libraries) created with an pre-patch v850 toolchain will not
be linked properly by a linker created by a patched toolchain. Why
was this done, and is it really necessary ?
As a side effect of this change the readelf program no longer
recognised the relocs found in debug sections of V850 binaries,
which means that tests like the GAS lns-duplicate test now FAIL.
* It adds new relocations but it does not include them in bfd/reloc.c.
Note - files like bfd-in2.h are automatically generated from the
sources in the bfd/ directory. Try this: Go into the bfd directory
in your build tree. Run "make headers". Then run "make". If you
get errors about missing relocs or broken .texi files then it is
probably because you need to update the reloc.c file.
* It adds new features to the v850 GAS port without documenting them.
In particular the file gas/doc/c-v850.texi needs to be updated
and the gas/NEWS file should have a line added mentioning the
new support.
* It removes the "v850ea" without explaining why.
In fact I see no great reason to remove this particular target,
even if it is only partially supported. It would be much cleaner
to change the configuration files so that match any v850 target.
Ie "v850*-*-*". That way you can remove several duplicates lines
in the configuration files and still keep support for the v850ea.
* It introduces new failures into the binutils testsuites.
My checks showed that these GAS tests now fail:
FAIL: lns-duplicate
FAIL: lns-common-1
FAIL: branch.s: branch operations
FAIL: V850E1 instruction tests
FAIL: V850E split LO16 tests
And this BINUTILS test:
FAIL: unordered .debug_info references to .debug_ranges
And this LD test:
FAIL: ld-elf/merge
* The new code does not always follow the GNU Coding Standards.
In particular there are lots of cases of the "func(args)" without
a space between "func" and "(args)". Plus there are many comments
that are not formatted as sentances, with a capital initial letter
and a full stop followed by two spaces at the end.
That's it. I hope that this review will prove useful to you.
Cheers
Nick