Re: [PATCH, V850] Add support for V850E2 and V850E2V3

[Nick Clifton <nickc at redhat dot 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