[PATCH, V850] Add support for V850E2 and V850E2V3

Nick Clifton nickc@redhat.com
Thu Jun 17 09:35:00 GMT 2010

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.


More information about the Binutils mailing list