PATCH: support for NEC SX architecture

PATCH: support for NEC SX architecture

[Jaka Močnik]

[-- Attachment #1: Type: text/plain, Size: 1870 bytes --]

hi, Nick (& other folks)!

attached you will find a (long ago promised) set of patches against
current binutils CVS head that implement support for the NEC SX series
of vector CPUs.

one patch per binutils subproject. note, however, that they really do
make sense only as a whole.

the patch provides support for two new targets:
- sx?-nec-superux*: for Super-UX OS on NEC SX, and
- sx?-nec-linux*: for Kitten light-weight kernel on NEC SX.

both targets are COFF targets, and the latter currently only differs in
the linker scripts.

while most certainly there are still some bugs hidden, the port is fully
functional and used in our GCC ( http://code.google.com/p/sx-gcc/ ) and
Kitten ( https://software.sandia.gov/trac/kitten ) porting efforts. as
it can build a functioning OS kernel, I suppose it's good enough to be
included in the binutils mainline. ;)

the port does, however, touch parts of the target-independent COFF code,
as SX COFF format is a bit peculiar. the most invasive of these changes
implements support for variable (that is: per-target variable) sized
symbol name filed in symbol entries.

changelogs for each subproject are attached in a separate file.

generated files are not included in the patch. after patching, running
autoreconf is required in bfd/, opcodes/, gas/ and ld/ dirs; also, one
should do a "make headers" in bfd/.

patched binutils show no regressions (when compared to an unpatched CVS
HEAD); I have regression tested the following configurations (on an ia32
host):
--enable-target=all --enable-64bit-bfd
--target=i386-coff
--target=i686-pc-linux-gnu
--target=alpha-linuxecoff
--target=i586-pc-msdosdjgpp
--target=x86_64-pc-mingw32
--target=arm-vxworks
--target=i686-pc-cygwin
--target=z8k-coff

looking forward to your comments. thanks & best regards,
  jaKa

-- 
email: jaka@xlab.si
w3:    http://www.gmajna.net/svojat/jaka/

[-- Attachment #2: 20090507-sx-binutils-diff.tar.gz --]
[-- Type: application/x-compressed-tar, Size: 85615 bytes --]

[-- Attachment #3: ChangeLog --]
[-- Type: text/x-changelog, Size: 3548 bytes --]

opcodes/ChangeLog:

2009-05-07  Jaka Mocnik  <jaka@xlab.si>

	* Makefile.am: Added sx-[dis|opc].c to build.
	* configure.in: Added NEC SX target CPU.
	* disassemble.c: Added dissassembler hook for NEC SX CPU.
	* sx-dis.c: Implementation of disassembler for NEC SX CPU.
	* sx-opc.c: Definition of opcodes for NEC SX CPU.

ld/ChangeLog:

2009-05-07  Jaka Mocnik  <jaka@xlab.si>

	* Makefile.am: Added generation of linker scripts for
	sx?-nec-[superux|linux] targets.
	* configure.tgt: Added sx?-nec-[superux|linux] targets.
	* emulparams/sxcoff.sh, emulparams/sxcoff_linux.sh: Defined emulation
	parameters for SX targets.
	* emultempl/sxcoff.em, emultempl/sxcoff_linux.em: Emulation scripts
	for SX targets.
	* scripttempl/sxcoff.sc, scripttempl/sxcoff_linux.sc: Linker scripts
	for SX targets.
	* Updated NEWS file.
	
include/ChangeLog

2009-05-07  Jaka Mocnik  <jaka@xlab.si>

	* coff/external.h (GET_LINENO_SYMNDX, PUT_LINENO_SYMNDX): Added
	accessors for a few more fields in the COFF header that have
	non-standard sizes in SX COFF format.
	(E_SYMNMLEN, E_DIMNUM): Conditionally define only if not defined by
	the target headers.
	* coff/internal.h: Increased sizes for some internal representation
	fields, in order to accomodate non-standard sizes in SX COFF format.
	Added some extra SX COFF specific flags, storage classes, etc.
	Introduce MAX_FILNMLEN and MAX_SYMNMLEN macros that allow for variable
	(with an upper limit) sizes of symbol and file names; use them instead
	of SYMNMLEN and FILNMLEN, respectively.
	* coff/sx.h: Defined SX COFF external representation.
	* opcode/sx.h: Defined SX opcode related data types.

gas/ChangeLog
	
2009-05-07  Jaka Mocnik  <jaka@xlab.si>

	* config/atof-ieee.c: Added support for SX quad precision fp format
	(almost, but not quite IEEE compliant ...).
	* config/obj-coff.h: Include NEC SX target headers when required.
	* config/tc-sx.c: Implemented an assembler for the NEC SX CPU.
	* configure.tgt: Added sx?-nec-[superux|linux] targets.
	* Updated NEWS file.
	
binutils/ChangeLog

2009-05-07  Jaka Mocnik  <jaka@xlab.si>

	* Updated NEWS file.

bfd/ChangeLog

2009-05-07  Jaka Mocnik  <jaka@xlab.si>

	* Makefile.am: Added SX sources to build.
	* archures.c: Added sx[4-9] machine types.
	* coff-sx.c: Implemented SX COFF bfd support.
	* coff-alpha.c, coff-arm.c, coff-i960.c, coff-mcore.c, coff-mips.c,
	coff-or32.c, coff-ppc.c, coff-rs6000.c, coff-sh.c, coff-tic80.c,
	coff64-rs6000.c, peXXigen.c, pei-ppc.c, ticoff.h, xcofflink.c: Minor
	changes to accomodate variable symbol name lengths in internal COFF
	representation.
	* coffcode.h: Added SX COFF support.
	Implemented handling of variable symbol name lengths.
	(bfd_coff_symnmlen) New function: returns target-specific max symbol
	name length, as specified in the bfd descriptor.
	Minor changes to accomodate larger sizes of some COFF fields in SX COFF.
	* cofflink.c: Implemented handling of variable symbol name lengths.
	* coffswap.h: Added more field accessors in cases where SX COFF has
	non-standard field sizes. Provided sane, backwards-compatible defaults.
	Use these accessors instead of the generic ones used before.
	Implemented handling of variable symbol name lengths.
	* config.bfd: Added sx?-nec-[superux|linux] targets.
	* configure.in: Added sxcoff_big_vec target.
	* cpu-sx.c: Defined NEC SX architecture variants.
	* libbfd.c (bfd_write_bigendian_8byte_long_long): New function.
	* libcoff-in.h: Added some SX COFF specific fields.
	* reloc.c: Added SX COFF specific relocations.
	* targets.c: Added NEC SX target.

Re: PATCH: support for NEC SX architecture

[Jaka Močnik]

hi!

not wanting to be pushy, but still: is anyone looking at this? ;)

> attached you will find a (long ago promised) set of patches against
> current binutils CVS head that implement support for the NEC SX series
> of vector CPUs.

regards,
  jaKa

-- 
email: jaka@xlab.si
w3:    http://www.gmajna.net/svojat/jaka/

Re: PATCH: support for NEC SX architecture

[Tristan Gingold]

On Jun 3, 2009, at 12:13 PM, Jaka Močnik wrote:

> hi!
>
> not wanting to be pushy, but still: is anyone looking at this? ;)

Hi,

this is a weird but interesting port :-)

You'd better to submit your patches inline (not as a tar.gz) as it is  
easier to review.
Don't forget to capitalize the first letter and put a double space  
after the dot.  There are still a few
places where this GNU style is not followed, eg:

+#ifdef NEC_SX_COFF
+  /* the optional header magic number and the stack size are provided  
as
+     commandline arguments of the linker. */

It is strange to use bfd_vma type for f_nsyms in include/coff/internal.h

Why are you using SX_PACKED in declaration of external_filhdr,  
external_aouthdr ... ?

Although I am ready to read your patch (when inlined), I can't approve  
it.

Tristan.

BTW: in there a documentation for the cpu ?

Re: PATCH: support for NEC SX architecture

[Jaka Močnik]

hi!

On Wed, 2009-06-03 at 12:56 +0200, Tristan Gingold wrote:
> Hi,
> 
> this is a weird but interesting port :-)
weird by all means.

> You'd better to submit your patches inline (not as a tar.gz) as it is  
> easier to review.
well, the patches are quite big, so I though it would be nicer to pack
them - didnt really expect someone would review them in their email
client ... I can resubmit them inline, if necessary.

> Don't forget to capitalize the first letter and put a double space  
> after the dot.  There are still a few
> places where this GNU style is not followed, eg:
> 
> +#ifdef NEC_SX_COFF
> +  /* the optional header magic number and the stack size are provided  
> as
> +     commandline arguments of the linker. */
ok. wasn't aware of there was comment related stuff in the coding style
- just tried to mimic the existing one.

> It is strange to use bfd_vma type for f_nsyms in include/coff/internal.h
true. however, I needed a type with the same domain as the virtual
addresses of the target CPU: any suggestions?

> Why are you using SX_PACKED in declaration of external_filhdr,  
> external_aouthdr ... ?
just to make sure that the compiler does not introduce any padding as
these structures should correspond to the memory/file contents of an SX
COFF file, bit by bit. it's an sx target specific define that translates
to gcc packed attribute. I tend to abstract gccisms away with defines
whenever possible.

> Although I am ready to read your patch (when inlined), I can't approve  
> it.
I thought so, it was more a question for Nick or anybody else who can
say yes to some of the changes in the core coff code - it's those that
probably need review most, not the target specific code.

> BTW: in there a documentation for the cpu ?
sure there is. ;) it is unfortunately not (or, better, is not supposed
to be) publicly available. however, asking google about it, you just
might find something. ;)

thanks and regards,
  jaKa 

-- 
email: jaka@xlab.si
w3:    http://www.gmajna.net/svojat/jaka/

Re: PATCH: support for NEC SX architecture

[Dave Korn]

Jaka Močnik wrote:

>> Although I am ready to read your patch (when inlined), I can't approve  
>> it.
> I thought so, it was more a question for Nick or anybody else who can
> say yes to some of the changes in the core coff code - it's those that
> probably need review most, not the target specific code.

  I can review the COFF code, but I'm hideously busy until the other side of
the weekend now; at least if noone else can do it sooner, I'll be able to look
at it then.  Sorry for not spotting your patch earlier.

    cheers,
      DaveK

Re: PATCH: support for NEC SX architecture

[Tristan Gingold]

>> You'd better to submit your patches inline (not as a tar.gz) as it is
>> easier to review.
> well, the patches are quite big, so I though it would be nicer to pack
> them - didnt really expect someone would review them in their email
> client ... I can resubmit them inline, if necessary.

One usual way is to split the patch (as you did) and post part by part.

>> It is strange to use bfd_vma type for f_nsyms in include/coff/ 
>> internal.h
> true. however, I needed a type with the same domain as the virtual
> addresses of the target CPU: any suggestions?

Requiring the same domain is too strong here.  You can have 32bits  
targets even when
bfd_vma is 64bits.  If you need a domain larger or equal than virtual  
addresses domain,
bfd_vma will fit this purpose although you may prefer bfd_size_type.

>> Why are you using SX_PACKED in declaration of external_filhdr,
>> external_aouthdr ... ?
> just to make sure that the compiler does not introduce any padding as
> these structures should correspond to the memory/file contents of an  
> SX
> COFF file, bit by bit. it's an sx target specific define that  
> translates
> to gcc packed attribute. I tend to abstract gccisms away with defines
> whenever possible.

All the structures (but external_syment) have only char[] fields.   
external_syment has a char * field
but within a union with at least a larger field.  So padding won't be  
inserted and I think this is not
necessary.
Using a gcc specific attribute may break other compilers (but I agree  
this is not critical).

>> Although I am ready to read your patch (when inlined), I can't  
>> approve
>> it.
> I thought so, it was more a question for Nick or anybody else who can
> say yes to some of the changes in the core coff code - it's those that
> probably need review most, not the target specific code.

Fine!

>> BTW: in there a documentation for the cpu ?
> sure there is. ;) it is unfortunately not (or, better, is not supposed
> to be) publicly available. however, asking google about it, you just
> might find something. ;)

Thanks,

Tristan.

Re: PATCH: support for NEC SX architecture

[Dave Korn]

Jaka Močnik wrote:
> On Wed, 2009-06-03 at 12:56 +0200, Tristan Gingold wrote:

>> BTW: in there a documentation for the cpu ?
> sure there is. ;) it is unfortunately not (or, better, is not supposed
> to be) publicly available. however, asking google about it, you just
> might find something. ;)

  All of a sudden I'm uncertain whether it's suitable to include support for a
somewhat private undocumented target in FSF sources.  I don't know if there's
a policy about it but it stands out from other targets in this regard.  Do you
have any idea how many of these supercomputers there are in use out there?

    cheers,
      DaveK

Re: PATCH: support for NEC SX architecture

[Jaka Močnik]

hi!

On Wed, 2009-06-03 at 15:39 +0100, Dave Korn wrote:
>   All of a sudden I'm uncertain whether it's suitable to include support for a
> somewhat private undocumented target in FSF sources.  I don't know if there's
> a policy about it but it stands out from other targets in this regard.  Do you
> have any idea how many of these supercomputers there are in use out there?
well, the sources are GPLed, implemented independently of any existing,
proprietary code, and well documented. the submission has also been
approved by erich (cc-ed above). so as far as the legal issues (and
probably philosophical ones as well) are concerned, I'd say the patch is
suitable.

also, I suppose that having a free toolchain for these machines, which
is at the base of a free system software stack that we're aiming at,
can't be a bad thing.

however, erich, who is directly affiliated with NEC will be a better
address for this and related questions.

regards,
  jaKa

-- 
email: jaka@xlab.si
w3:    http://www.gmajna.net/svojat/jaka/

Re: PATCH: support for NEC SX architecture

[Ian Lance Taylor]

Dave Korn <dave.korn.cygwin@googlemail.com> writes:

> Jaka Močnik wrote:
>> On Wed, 2009-06-03 at 12:56 +0200, Tristan Gingold wrote:
>
>>> BTW: in there a documentation for the cpu ?
>> sure there is. ;) it is unfortunately not (or, better, is not supposed
>> to be) publicly available. however, asking google about it, you just
>> might find something. ;)
>
>   All of a sudden I'm uncertain whether it's suitable to include support for a
> somewhat private undocumented target in FSF sources.  I don't know if there's
> a policy about it but it stands out from other targets in this regard.  Do you
> have any idea how many of these supercomputers there are in use out there?

I think it's OK.  In effect a public binutils/gcc port documents the
target.

What does tend to happen with these private ports is that the maintainer
changes jobs, the port bitrots, and gets removed.

Ian

Re: PATCH: support for NEC SX architecture

[Dave Korn]

Jaka Močnik wrote:
> hi!
> 
> On Wed, 2009-06-03 at 15:39 +0100, Dave Korn wrote:
>> All of a sudden I'm uncertain whether it's suitable to include support
>> for a somewhat private undocumented target in FSF sources.  I don't know
>> if there's a policy about it but it stands out from other targets in this
>> regard.  Do you have any idea how many of these supercomputers there are
>> in use out there?
> well, the sources are GPLed, implemented independently of any existing, 
> proprietary code, and well documented. the submission has also been 
> approved by erich (cc-ed above).

  Sorry, I've only barely started looking at them so far.  I took a browse
through the sx-gcc website wiki and only found skeletal documentation.  I now
see you have quite a comprehensive instruction listing in include/opcode/sx.h,
but it's not something that anyone could just compile binutils and start
writing assembler code for.

> so as far as the legal issues (and 
> probably philosophical ones as well) are concerned, I'd say the patch is 
> suitable.
> 
> also, I suppose that having a free toolchain for these machines, which is
> at the base of a free system software stack that we're aiming at, can't be
> a bad thing.
> 
> however, erich, who is directly affiliated with NEC will be a better 
> address for this and related questions.

  Erich, is there any chance at all NEC could be persuaded to change policy
and release the CPU and ISA documentation?  That would be the ideal situation.

    cheers,
      DaveK

Re: PATCH: support for NEC SX architecture

[Dave Korn]

Ian Lance Taylor wrote:
> Dave Korn <dave.korn.cygwin@googlemail.com> writes:
> 
>> Jaka Močnik wrote:
>>> On Wed, 2009-06-03 at 12:56 +0200, Tristan Gingold wrote:
>>>> BTW: in there a documentation for the cpu ?
>>> sure there is. ;) it is unfortunately not (or, better, is not supposed
>>> to be) publicly available. however, asking google about it, you just
>>> might find something. ;)
>>   All of a sudden I'm uncertain whether it's suitable to include support for a
>> somewhat private undocumented target in FSF sources.  I don't know if there's
>> a policy about it but it stands out from other targets in this regard.  Do you
>> have any idea how many of these supercomputers there are in use out there?
> 
> I think it's OK.  In effect a public binutils/gcc port documents the
> target.

  I'll defer to your greater experience here; if you're happy it's OK, I have
no objection.

> What does tend to happen with these private ports is that the maintainer
> changes jobs, the port bitrots, and gets removed.

  This also sounds like the voice of experience speaking :-/  Opening the
documentation would be one way NEC could help avoid that happening.

    cheers,
      DaveK

Re: PATCH: support for NEC SX architecture

[Erich Focht]

Hi Dave,

Dave Korn wrote:
> Jaka Močnik wrote:
>> On Wed, 2009-06-03 at 15:39 +0100, Dave Korn wrote:
>>> All of a sudden I'm uncertain whether it's suitable to include support
>>> for a somewhat private undocumented target in FSF sources.  I don't know
>>> if there's a policy about it but it stands out from other targets in this
>>> regard.  Do you have any idea how many of these supercomputers there are
>>> in use out there?
>> well, the sources are GPLed, implemented independently of any existing, 
>> proprietary code, and well documented. the submission has also been 
>> approved by erich (cc-ed above).
> 
>   Erich, is there any chance at all NEC could be persuaded to change policy
> and release the CPU and ISA documentation?  That would be the ideal situation.

since every customer gets the documentation and most of the programmers 
who request it get a quick reference guide, we didn't think this is a 
problem. There's no place on the docs stating "do not disclose" or 
"secret" (though: reproduction rights stay with the company, as usual 
for books). But I see the problem: nobody could just start coding SX 
assembler with just the info we have on the sx-gcc page. So we'll do two 
things: start improving the docs on the sx-gcc web page and find out 
about possibilities to put the official SX CPU docs somewhere onto the 
web. This might take a while...

Best regards,
Erich

Re: PATCH: support for NEC SX architecture

[Nick Clifton]

[-- Attachment #1: Type: text/plain, Size: 2197 bytes --]

Hi Jaka,

> attached you will find a (long ago promised) set of patches against
> current binutils CVS head that implement support for the NEC SX series
> of vector CPUs.

Thank you very much for submitting this patch, and please accept my 
apologise for taking so long to review it.

The patch does apply cleanly and apart from some minor formatting issues 
it seems internally OK.  There are three problems however:

   * You do not appear to have an FSF binutils copyright assignment.
     Without such an assignment we cannot accept your contribution.
     I have attached a form for you to fill out and email off in order
     to start this application process rolling.

   * The new files are licensed under version 2 of the GPL.
     The binutils sources are now being distributed under version 3
     of the GPL.  Do you have any objections to changing the license
     on your contributed files to GPLv3 ?

   * There are lots of failures in the various testsuites which indicate
     problems with the new toolchain.  Specifically for a toolchain
     configured as "--target=sx-linux" I see 34 unexpected failures in
     the GAS testsuite, 104 unexpected failures in the LD testsuite and
     10 unexpected failures in the BINUTILS testsuite.

     Looking in the logs there are various worrying entries, such as:

       gas/testsuite/gas/all/float.s: Assembler messages:
       gas/testsuite/gas/all/float.s:2: Internal error!
       Assertion failure in float_cons at gas/read.c line 4679.

     and:

       ld/testsuite/ld-discard/extern.s: Assembler messages:
       ld/testsuite/ld-discard/extern.s:13: Fatal error: BUG: Fix-up 
(relocation type 2) can not be handled.
       Reloc generated at line 13

     I suspect some of the problems are due to the fact that the test
     harnesses are assuming that a Linux targeted port is also an ELF
     based port.  Are you really running a COFF based linux using this
     toolchain ?

If these problems can be resolved then I will be happy to accept your 
contribution.  Would you also be willing to act as a maintainer for the 
SX port in order to keep it working as the binutils sources change in 
the future ?

Cheers
   Nick


[-- Attachment #2: future --]
[-- Type: text/plain, Size: 5611 bytes --]

The way to assign copyright to the Foundation is to sign an assignment
contract.  This is what legally makes the FSF the copyright holder so
that we can register the copyright on the new version.
I'm assuming that you wrote these changes yourself;
if other people wrote parts, we may need papers from them.

If you are employed to do writing (even at a university), or have
made an agreement with your employer or school saying it owns text
you write, then you and we need a signed piece of paper from your
employer disclaiming rights to your changes.

The disclaimer should be signed by a vice president or general manager
of the company.  If you can't get at them, anyone else authorized to
license manuals produced there will do.  Here is a sample wording:

  Digital Stimulation Corporation hereby disclaims all copyright
  interest in the changes and enhancements made by Hugh Heffner to the
  manual "The Seduction Manual", including both those he has already
  made and those he will make in the future.  We do not consider them 
  as works made for hire for us.

  <signature of Ty Coon>, 1 April 1987
  Ty Coon, President of Vice, Digital Stimulation Corp.

IMPORTANT: When you talk to your employer, *no matter what
instructions they have given you*, don't fail to show them the sample
disclaimer above, or a disclaimer with the details filled in for your
specific case.  Companies are usually willing to sign a disclaimer
without any fuss.  If you make your request less specific, you may
open Pandora's box and cause a long and unnecessary delay.

Below is the assignment contract that we usually use.  You would need
to print it out, sign it, and snail it to:

Richard Stallman
77 Mass Ave rm 32-381
Cambridge MA 02139
USA

Snail a copy of the employer's disclaimer as well.

Please send me email about what you decide to do.  If you have any
questions, or would like something to be changed, ask rms@ai.mit.edu via email.
\f			    ASSIGNMENT

   For good and valuable consideration, receipt of which I acknowledge, I,
NAME OF PERSON, hereby transfer to the Free Software Foundation, Inc. (the
"Foundation") my entire right, title, and interest (including all rights
under copyright) in my changes and enhancements to the manual "NAME OF
MANUAL", including those I have already made and those I shall make in the
future, subject to the conditions below.  These changes and enhancements
are herein called the "Work".

   Upon thirty days' prior written notice, the Foundation agrees to
grant me non-exclusive rights to use the Work (i.e. my changes and
enhancements, not the manual which I enhanced) as I see fit; (and the
Foundation's rights shall otherwise continue unchanged).

   For the purposes of this contract, a work "based on the Work" means
any work that in whole or in part incorporates or is derived from all or
part of the Work.

   The Foundation promises that all distribution of the Work, or of any
work "based on the Work", that takes place under the control of the
Foundation or its assignees, shall be on terms that explicitly and
perpetually permit anyone possessing a copy of the work to which the terms
apply, and possessing accurate notice of these terms, to redistribute
copies of the work to anyone on the same terms.  These terms shall not
restrict which members of the public copies may be distributed to.  These
terms shall not require a member of the public to pay any royalty to the
Foundation or to anyone else for any permitted use of the work they apply
to, or to communicate with the Foundation or its agents in any way either
when redistribution is performed or on any other occasion.

   The Foundation promises to give or send me, upon reasonable prior notice
and payment of a fee no more than twenty times the cost of the necessary
materials and postage, a copy of any or all of the works "based on the
Work" that it offers to the public or that it has offered within the
past six months, or that it distributed for the first time within the past
six months.  My request shall detail whether I wish to receive all
such works or specific works.  My choice of works to request may affect the
cost and therefore the fee.

   I hereby represent and warrant that I am the sole copyright holder for the
Work and that I have the right and power to enter into this contract.  I
hereby indemnify and hold harmless the Foundation, its officers, employees,
and agents against any and all claims, actions or damages (including
attorney's reasonable fees) asserted by or paid to any party on account of a
breach or alleged breach of the foregoing warranty.  I make no other express
or implied warranty (including without limitation, in this disclaimer of
warranty, any warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE).

Agreed:  [signature]			Date:


For the Free Software Foundation,
 	
ACCEPTED: FREE SOFTWARE FOUNDATION - Thanks for contributing to the GNU project!

Richard M. Stallman, President
\f
Please do not delete the control-l character before this line.
Please print this as a separate page.

Please email a copy of the information on this page to
fsf-records@gnu.org, if you can, so that our clerk doesn't have
to type it in.  Use your full name as the subject line.

Otherwise, please write down the answers and snail this with
your assignment.

[For the copyright registration, what country are you a citizen of?]


[What year were you born?]


[What is your email address?]


[Please write your snail address here, so we can snail a copy back to you.]






[Which files have you changed so far, and which new files have you written
so far?]

Re: PATCH: support for NEC SX architecture

[Dave Korn]

Nick Clifton wrote:
> Hi Jaka,

  Hi also Jaka!

>> attached you will find a (long ago promised) set of patches against
>> current binutils CVS head that implement support for the NEC SX series
>> of vector CPUs.
> 
> Thank you very much for submitting this patch, and please accept my
> apologise for taking so long to review it.
> 
> The patch does apply cleanly and apart from some minor formatting issues
> it seems internally OK.

  I had just started taking a look at this patch too, as I had offered to
check over the COFF parts.  I'll post a more detailed review for you later
today; there were a few things I wanted to ask about.

    cheers,
      DaveK

Re: PATCH: support for NEC SX architecture

[Jaka Močnik]

hi, nick, dave!

first of all, thanks for the review; appreciate the comments (and
eagerly awaiting yours, dave).

On Thu, 2009-06-18 at 07:51 +0100, Nick Clifton wrote:
>    * You do not appear to have an FSF binutils copyright assignment.
>      Without such an assignment we cannot accept your contribution.
>      I have attached a form for you to fill out and email off in order
>      to start this application process rolling.
sure: I expect erich to have the final say in the copyright assignment
issues, so he'll comment on this.

>    * The new files are licensed under version 2 of the GPL.
>      The binutils sources are now being distributed under version 3
>      of the GPL.  Do you have any objections to changing the license
>      on your contributed files to GPLv3 ?
not me. once again, it's erich's call.

>      I suspect some of the problems are due to the fact that the test
>      harnesses are assuming that a Linux targeted port is also an ELF
>      based port.  Are you really running a COFF based linux using this
>      toolchain ?
well, the linux toolchain is a bit experimental and not really ready for
public consumption yet (there is also no publicly available kernel yet),
so perhaps it would be best to leave it out of the upstream patch - I
can maintain it in a private tree easily.

(it really is used to build a variant of a radically trimmed down linux
kernel, https://software.sandia.gov/trac/kitten , which we are currently
porting to SX and is intended for use in computational clusters, not
unlike CNL. and yes, our port really is a COFF based kernel as we dont
have an ELF bootloader yet. anyway, it is expected that it will be
obsoleted by a future ELF toolchain for Linux on SX.)

> If these problems can be resolved then I will be happy to accept your 
> contribution.
sure. I'll check the testsuite status for the *-superux toolchain and
fix any outstanding issues.

that said, I'll be happy to provide another patch that omits the linux
toolchain and fixes any possible SX testsuite issues - as it will be
almost identical to this one, I suppose another in-depth review will not
be necessary. let's just wait for dave's comments about COFF internals
first ...

>   Would you also be willing to act as a maintainer for the 
> SX port in order to keep it working as the binutils sources change in 
> the future ?
it's a shrink-wrapped offer: a maintainer comes with the patch. ;) not
sure about the name yet, but there will be one, of course.

regards,
  jaKa

-- 
email: jaka@xlab.si
w3:    http://www.gmajna.net/svojat/jaka/

Re: PATCH: support for NEC SX architecture

[Dave Korn]

Jaka Močnik wrote:

> attached you will find a (long ago promised) set of patches against
> current binutils CVS head that implement support for the NEC SX series
> of vector CPUs.

> both targets are COFF targets, and the latter currently only differs in
> the linker scripts.

  As I said, I can review COFF changes.  You'll still need a build maintainer
to OK the various configury stuff and presumably a GWP maintainer to OK the
new gas/opcodes/ld ports.  (Sorry it took longer than I'd hoped to get time to
get back to this.)

> include/ChangeLog
> 
> 2009-05-07  Jaka Mocnik  <jaka@xlab.si>
> 
> 	* coff/external.h (GET_LINENO_SYMNDX, PUT_LINENO_SYMNDX): Added
> 	accessors for a few more fields in the COFF header that have
> 	non-standard sizes in SX COFF format.
> 	(E_SYMNMLEN, E_DIMNUM): Conditionally define only if not defined by
> 	the target headers.
> 	* coff/internal.h: Increased sizes for some internal representation
> 	fields, in order to accomodate non-standard sizes in SX COFF format.
> 	Added some extra SX COFF specific flags, storage classes, etc.
> 	Introduce MAX_FILNMLEN and MAX_SYMNMLEN macros that allow for variable
> 	(with an upper limit) sizes of symbol and file names; use them instead
> 	of SYMNMLEN and FILNMLEN, respectively.

  This piece of ChangeLog is incorrectly formatted; it should be like the one
above, rather than a chunk of narrative.  Also, I think ChangeLogs are
supposed to say "what, not why", so I'd remove the motivation parts "that have
non-standard ..." and "in order to accomodate ...".  I'd put these as:

 	* coff/external.h (GET_LINENO_SYMNDX, PUT_LINENO_SYMNDX): Added
 	accessors for a few more fields in the COFF header.
 	(E_SYMNMLEN, E_DIMNUM): Conditionally define only if not defined by
 	the target headers.
 	* coff/internal.h (struct internal_filehdr):  Expand f_nscns, f_nsyms
	and f_opthdr fields and add f_flags2, f_flags3 and f_flttype.
	(F2_LP64, F2_SX3MEM, F2_CLANG, F2_USEB6, F2_INT64, F2_VECINS):  New
	flag defines for f_flags2 field, only meaningful on SX target.
	(F3_SIZE_TMIX, F3_SIZE_T64, F3_VL512ONLY, F3_VL512MAX, F3_P64,
	F3_PG0PRIV, F3_MEMFRT, F3_MLTTSK):  Likewise for f_flags3.
	(FT_NON, FT_FL1, FT_FL2, FT_MIX, FT_FL0):  Likewise for f_flttype.
	(struct internal_aouthdr):  Add ssize field, only meaningful on SX.

... etc, etc.

> 	* coff/sx.h: Defined SX COFF external representation.
> 	* opcode/sx.h: Defined SX opcode related data types.

  And likewise for a lot of your other ChangeLogs; we really need blow-by-blow
accounts of what you changed.  The relevant section of the coding standard is at:

  http://www.gnu.org/prep/standards/html_node/Change-Logs.html

  (I admit that some of these are more honoured in the breach than the
observance, particularly the bits about conditional changes and continuation
brackets; but take a look through the existing ChangeLogs and try and mirror
what they do.)

  There are also several other formatting rules from the GCS that your patch
doesn't strictly follow.  Comments should be English sentences; they should
begin with a capital letter and end with a period and *two* spaces before the
closing '*/'.  When wrapping a comment across multiple lines, align the text
on the second line; don't do this:

+/* Directs long word size (64 bit) reference to the local common symbol
+virtual address.  */

do it like this instead:

+/* Directs long word size (64 bit) reference to the local common symbol
+   virtual address.  */

  (Apologies for tampering slightly with that example to avoid my mailer
wrapping it in a confusing way, I repositioned 'virtual' but it's otherwise an
actual example from the patch).

  Indent depth is 2, which you have right, but any multiples of 8 spaces at
the start of a line should use real hard TABs.  And there are some stray
little whitespace errors such as

+  if(sx_ssize != 0 && sx_layout != SX_LAYOUT_512G)
+	  {
+      printf("WARNING: stack size parameter is only meaningful with "
+             "512G memory layout. It will be set to 0.\n");
+      sx_ssize = 0;
+    }

  I don't want to delay you spending load of time on minor formatting issues,
but please at least take a sweep across the non-sx-specific files you touch in
your patch and tidy them up.  It leads to a mess that only gets worse as
subsequent people come in and work on the same files using different editor
configurations ...

  OK, now to the actual patches.

opcodes/ChangeLog:

> 	* Makefile.am: Added sx-[dis|opc].c to build.
> 	* configure.in: Added NEC SX target CPU.

  Can't approve these, not my area; needs build maintainer approval.

> 	* disassemble.c: Added dissassembler hook for NEC SX CPU.
> 	* sx-dis.c: Implementation of disassembler for NEC SX CPU.
> 	* sx-opc.c: Definition of opcodes for NEC SX CPU.

  Can't approve these, not my area; needs maintainer approval.  Will just
comment that you've got a lot of code that is indented with TABs instead of
spaces (or by mixed assortments of TABs and spaces that don't appear to hold
to a consistent definition of whether a TAB is 2, 4 or 8 spaces wide!), and
you also have a number of instances of non-GNU brace style, e.g.:

+        else {
+			vdreg = ((CHARbit(4, field) << 4) + dfield);
+        }

ld/ChangeLog:

> 	* Makefile.am: Added generation of linker scripts for
> 	sx?-nec-[superux|linux] targets.
> 	* configure.tgt: Added sx?-nec-[superux|linux] targets.

  Needs build maintainer.

> 	* emulparams/sxcoff.sh, emulparams/sxcoff_linux.sh: Defined emulation
> 	parameters for SX targets.
> 	* emultempl/sxcoff.em, emultempl/sxcoff_linux.em: Emulation scripts
> 	for SX targets.

  This didn't build directly for me: in this new function,

+static bfd_vma
+sx_parse_stack_size(char *str_ssize)
+{
+  bfd_vma multiplier;
+  bfd_vma num_ssize;
+  size_t str_len;
+  char buff[20];
+  bfd_boolean remove_unit;
+  const char *end;
+
+  /* get the unit (Kilo-, Mega- or Gigabytes)*/
+  str_len = strlen(str_ssize);
+  if (isdigit((int)*(str_ssize + str_len - 1)))
+    {
+      /* there is no unit. we are dealing with the number in bytes. */
+      multiplier = 1;
+      remove_unit = FALSE;
+    }
+  else if (tolower(*(str_ssize + str_len - 1)) == 'k')
+    {
+      /* we are dealing with the number in kilobytes. */
+      multiplier = 1024;
+      remove_unit = TRUE;
+    }
+  else if (tolower(*(str_ssize + str_len - 1)) == 'm')
+    {
+      /* we are dealing with the number in megabytes. */
+      multiplier = 1024 * 1024;
+      remove_unit = TRUE;
+    }
+  else if (tolower(*(str_ssize + str_len - 1)) == 'g')
+    {

... I got errors from the final three ctype.h calls where there is no cast to
(int) present:

cc1: warnings being treated as errors
esxcoff_linux.c: In function 'sx_parse_stack_size':
esxcoff_linux.c:144: error: array subscript has type 'char'
esxcoff_linux.c:150: error: array subscript has type 'char'
esxcoff_linux.c:156: error: array subscript has type 'char'

  Adding the casts fixed it.  There is also a trivial buffer overflow
vulnerability:

> +static bfd_vma
> +sx_parse_stack_size(char *str_ssize)
> +{

> +  size_t str_len;
> +  char buff[20];

> +  str_len = strlen(str_ssize);

> +      strncpy(buff, str_ssize, str_len-1);
> +      buff[str_len-1] = '\0';

which should be avoided.  Also,

> +      fprintf(stderr, "Invalid stack size unit.");

don't do this; use the error/warning indication functions from ld/ldmisc.c,
and likewise where you have a bunch of printfs later in the file that don't
even send their error messages to stderr.  You most likely want to use einfo()
for all of these.  You should probably also be wrapping these text strings in
the _() macro so that they will get picked out for the message files and be
available to language translation.

> 	* scripttempl/sxcoff.sc, scripttempl/sxcoff_linux.sc: Linker scripts
> 	for SX targets.

  Once the formatting problems are fixed, these are all OK from the COFF point
of view.  A GWP maintainer should probably also take a look, as I don't know
all the conventions and internal interface usage, but I didn't spot anything
obviously or egregiously wrong.

> 	* Updated NEWS file.

  Normally I think we just push new stuff on the top of the NEWS file, like a
stack, rather than adding it to the end of the currently-active section.

> include/ChangeLog

> 	* coff/external.h (GET_LINENO_SYMNDX, PUT_LINENO_SYMNDX): Added
> 	accessors for a few more fields in the COFF header that have
> 	non-standard sizes in SX COFF format.
> 	(E_SYMNMLEN, E_DIMNUM): Conditionally define only if not defined by
> 	the target headers.

  This is all OK.

> 	* coff/internal.h: Increased sizes for some internal representation
> 	fields, in order to accomodate non-standard sizes in SX COFF format.
> 	Added some extra SX COFF specific flags, storage classes, etc.
> 	Introduce MAX_FILNMLEN and MAX_SYMNMLEN macros that allow for variable
> 	(with an upper limit) sizes of symbol and file names; use them instead
> 	of SYMNMLEN and FILNMLEN, respectively.

> -  unsigned short f_nscns;	/* number of sections		*/
> +  unsigned int  f_nscns;	/* number of sections		*/

  Stray extra space.

> -  long f_nsyms;			/* number of symtab entries	*/
> -  unsigned short f_opthdr;	/* sizeof(optional hdr)		*/
> +  bfd_vma f_nsyms;		/* number of symtab entries	*/
> +  unsigned int f_opthdr;	/* sizeof(optional hdr)		*/

  As Tristan mentioned, bfd_vma is a bit peculiar.  I'd like a GWP maintainer
to suggest if there's a better type to use such as bfd_size_type but if nobody
has any strong feelings on the matter I don't object.

>    unsigned short f_flags;	/* flags			*/
> +  
>    unsigned short f_target_id;	/* (TI COFF specific)		*/

  Stray extra blank line.

> +/* extra flags for SX COFF format */
> +/* extra extra flags for SX COFF format */
> +/* floating point mode for SX COFF format */

  Comment style doesn't match comments immediately above and below.

> +/* storage classes for SX */
> +/* global lcomm and taskcomm symbols */
> +#define C_GLCOMM        19
> +/* global slcomm and staskcomm symbols */
> +#define C_GSLCOMM       24
> +/* static slcomm symbols */
> +#define C_SSLCOMM       27
> +/* static lcomm symbols */
> +#define C_SLCOMM        28
> +/* end of storage classes for SX */

  Likewise also please format like the sections above; comments on same line
as definitions, and no need for an end marker.  Please also move the whole
section below the "#if defined _AIX52 || defined AIX_WEAK_SUPPORT" chunk so
you don't separate it from the RS/6000 C_AIX_WEAKEXT definition above.

> -#define SYMNMLEN	8	/* # characters in a symbol name	*/
> -#define FILNMLEN	14	/* # characters in a file name		*/
> +/* define maximum sizes for file and symbol name lengths in COFF
> +   files. this must be a maximum over all COFF flavours (currently
> +   8 and 14 for all flavours except for COFF/SX, which has 16 and 36,
> +   so this is our current maximum) */
> +#define MAX_FILNMLEN        36
> +#define MAX_SYMNMLEN        16
> +
> +/* defaults for most COFF targets (except SX and PE) */
> +#ifndef FILNMLEN
> +#define FILNMLEN            14
> +#endif
> +
> +#ifndef SYMNMLEN
> +#define SYMNMLEN             8
> +#endif
> +
> +#ifndef DIMNUM
>  #define DIMNUM		4	/* # array dimensions in auxiliary entry */
> +#endif

  Please tweak these so the numbers stay vertically aligned; use a single TAB
between each of the new #defines' symbol and its value.

>      struct
>      {
> -      bfd_hostptr_t _n_zeroes;	/* new == 0			*/
> -      bfd_hostptr_t _n_offset;	/* offset into string table	*/
> +      bfd_vma _n_zeroes;		/* new == 0		*/
> +      bfd_vma _n_offset;		/* offset into string table */
>      }      _n_n;
>      char *_n_nptr[2];		/* allows for overlaying	*/

  I think this is wrong.  First, these are pointers into the host's memory,
not the target's; internal structures describe the layout of data in the host
so there's simply no point in using a different pointer size; we're not going
to be able to read a BFD that's greater than four gig into memory on a 32 bit
machine, and as far as I can see we just have to live with that.  Second, if
the target vma size doesn't match the host's pointer size, the overlaying with
_n_nptr[] won't work right.  What would break for you if you didn't make this
change?

> -  unsigned short n_type;	/* type and derived type	*/
> +  unsigned int n_type;	        /* type and derived type	*/

  Space-vs-TAB problem.

> +    /* NOTE: changed data type sizes in x_misc to accomodate
> +       larger types in COFF/SX */

  History belongs in ChangeLogs and CVS revision metadata, not in comments; a
statement about what sizes these things used-to-be-but-are-not-any-more is not
particularly useful, so please delete it.

> -      long x_fsize;		/* size of function */
> +      bfd_vma x_fsize;		/* size of function */

  I'd also think this might want to be bfd_size_type, but I'm not certain of
its semantics.

> -    long x_scnlen;		/* section length */
> -    unsigned short x_nreloc;	/* # relocation entries */
> -    unsigned short x_nlinno;	/* # line numbers */
> +    bfd_vma x_scnlen;	        /* section length */

  And again here.  I hope a GWP maintainer can comment on these.

>  #define R_W65_DP       10  /* direct page 8 bits only   */
>  
> +
> +/* SX relocation modes */
> +

  Excess blank line before, as compared to the other target-specific reloc
mode definition chunks above.  I wouldn't mention it but since you're
respinning the patch anyway might as well delete it.

> 	* coff/sx.h: Defined SX COFF external representation.

> +#define SX_PACKED __attribute__((__packed__))

  Don't do this.  If you need it, there's already ATTRIBUTE_PACKED in
include/ansidecl.h - it's a GCC feature, not an SX feature - but really, I
don't think you need do it at all.  The structs to which you've applied it are
all simple bunches of chars and char arrays; no sane compiler will be padding
them.  The one exception you mentioned is the "char *_e_nptr[2]" in your
external_syment, and I think that's wrong too; it's meaningless to talk about
having a pointer to memory in a file on a disk, isn't it?  You don't use
_e_nptr anywhere in your patchset, so I'd suggest you remove it and SX_PACKED
altogether.

  Aside from that (and the usual formatting issues) this is OK, I think.

> 	* opcode/sx.h: Defined SX opcode related data types.

  Formatting issues.  I can't approve this but it looks fine aside from that
to me.

  You also omitted to mention a couple of other files that were edited in your
patch: include/coff/xcoff.h and include/dis-asm.h; please supply ChangeLogs
for those when you do.  The xcoff.h patch may need adjusting, depending what
we decide about the bfd_vma vs. bfd_size_type issue, but is OK otherwise; the
dis-asm.h patch I can't approve.

> gas/ChangeLog

> 	* config/atof-ieee.c: Added support for SX quad precision fp format
> 	(almost, but not quite IEEE compliant ...).

  Can't approve.

> 	* config/obj-coff.h: Include NEC SX target headers when required.

  OK.

> 	* config/tc-sx.c: Implemented an assembler for the NEC SX CPU.
> 	* configure.tgt: Added sx?-nec-[superux|linux] targets.
> 	* Updated NEWS file.

  Can't approve.  Could move NEWS entry to top.

> binutils/ChangeLog

> 	* Updated NEWS file.

  Can't approve.  Could move NEWS entry to top.

> bfd/ChangeLog

  Look, I've kept you waiting long enough for this as it is; I'll send this
now and follow-up with the review of the BFD part shortly.  One final thing
for this email, about the amount of test failures that Nick mentioned: a lot
of the failing tests do indeed assume that linux implies ELF object format.
At least the ld testsuite has a function to fix this: I made the following change

Index: ld/testsuite/lib/ld-lib.exp
===================================================================
RCS file: /cvs/src/src/ld/testsuite/lib/ld-lib.exp,v
retrieving revision 1.64
diff -p -u -r1.64 ld-lib.exp
--- ld/testsuite/lib/ld-lib.exp 18 Jun 2009 02:47:51 -0000      1.64
+++ ld/testsuite/lib/ld-lib.exp 20 Jun 2009 15:14:44 -0000
@@ -370,7 +370,8 @@ proc is_elf_format {} {
     }

     if { [istarget *-*-linux*aout*] \
-        || [istarget *-*-linux*oldld*] } {
+        || [istarget *-*-linux*oldld*] \
+        || [istarget sx*-*-linux*] } {
        return 0
     }

and it removed a bunch of the FAILs; you should take a look through the other
testsuites to see if they have similar checks you can modify.

    cheers,
      DaveK

Re: PATCH: support for NEC SX architecture

[Dave Korn]

Dave Korn wrote:

>> bfd/ChangeLog
> 
>   Look, I've kept you waiting long enough for this as it is; I'll send this
> now and follow-up with the review of the BFD part shortly.  

  I'm working on that now, but I've got one overall question I'd like to ask
you separately about the SYMNMLEN change to BFD.  It's largely mechanical,
sure, but it's still very intrusive.  I wondered if you had at any point
considered the approach of making SX COFF a fully-fledged derived sub-class of
plain COFF in the same way as XCOFF and ECOFF are?  Wouldn't you just need to
override the definition of SYMNMLEN with your own value, then you could reuse
all the standard coff code, and end up with a much simpler patch overall?

    cheers,
      DaveK

Re: PATCH: support for NEC SX architecture

[Jaka Močnik]

hi, dave!

first of all, sorry for the late reply: I was away, sailing, until this
weekend.

On Tue, 2009-06-23 at 18:32 +0100, Dave Korn wrote:
> Dave Korn wrote:
> 
> >> bfd/ChangeLog
> > 
> >   Look, I've kept you waiting long enough for this as it is; I'll send this
> > now and follow-up with the review of the BFD part shortly.  
> 
>   I'm working on that now, but I've got one overall question I'd like to ask
> you separately about the SYMNMLEN change to BFD.  It's largely mechanical,
> sure, but it's still very intrusive.  I wondered if you had at any point
> considered the approach of making SX COFF a fully-fledged derived sub-class of
> plain COFF in the same way as XCOFF and ECOFF are?  Wouldn't you just need to
> override the definition of SYMNMLEN with your own value, then you could reuse
> all the standard coff code, and end up with a much simpler patch overall?
well, the answer is twofold...

first of all the SX COFF flavour did not seem that much different when
compared to ordinary COFF to require another subvariant along the lines
of [E|X]COFF which would only be used on a single architecture.

however, the main reason was that my change (which is in the end only
exposed to BFD users by addition of bfd_coff_symnmlen(abfd) to "public"
BFD API) looked quite similar to the already existing
bfd_coff_filnmlen(abfd) and similar functions.

regards,
  jaKa

-- 
email: jaka@xlab.si
w3:    http://www.gmajna.net/svojat/jaka/

Re: PATCH: support for NEC SX architecture

[Dave Korn]

Jaka Močnik wrote:
> hi, dave!
> 
> first of all, sorry for the late reply: I was away, sailing, until this
> weekend.

  My turn to apologise, I've got awfully tied up in other stuff.

> first of all the SX COFF flavour did not seem that much different when
> compared to ordinary COFF to require another subvariant along the lines
> of [E|X]COFF which would only be used on a single architecture.
> 
> however, the main reason was that my change (which is in the end only
> exposed to BFD users by addition of bfd_coff_symnmlen(abfd) to "public"
> BFD API) looked quite similar to the already existing
> bfd_coff_filnmlen(abfd) and similar functions.

  Yep, I agree.  I had a look at it myself and concluded that doing a derivative
type wasn't the best fit for the solution.  So, here's the review of the
remaining COFF-specific parts of your port.  You still need to get the paperwork
done, get reviews of the non-COFF-specific parts, and address the comments that
I made in the first part of this review(*), and Nick's comments still hold, but
that's all fairly small beer.

> bfd/ChangeLog
> 
> 2009-05-07  Jaka Mocnik  <jaka@xlab.si>
> 
> 	* Makefile.am: Added SX sources to build.

  Can't approve.  Notice it only has dependencies for cpu-sx, not coff-sx; it
needs a run of "make dep-am".

> 	* archures.c: Added sx[4-9] machine types.

  Can't approve, but looks right.

> 	* coff-sx.c: Implemented SX COFF bfd support.

  Don't have much to say about this, since it's going to be your file to
maintain!  Modulo the usual formatting issues, this is OK.

> 	* coff-alpha.c, coff-arm.c, coff-i960.c, coff-mcore.c, coff-mips.c,
> 	coff-or32.c, coff-ppc.c, coff-rs6000.c, coff-sh.c, coff-tic80.c,
> 	coff64-rs6000.c, peXXigen.c, pei-ppc.c, ticoff.h, xcofflink.c: Minor
> 	changes to accomodate variable symbol name lengths in internal COFF
> 	representation.

  Missing space between bfd_coff_symnmlen and open bracket:

> +++ src-test-fresh-patch/bfd/coff-i960.c	2009-05-06 16:54:42.000000000 +0200
> @@ -362,7 +362,7 @@
>      {
>        struct internal_syment isym;
>  
> -      strncpy (isym._n._n_name, o->name, SYMNMLEN);
> +      strncpy (isym._n._n_name, o->name, bfd_coff_symnmlen(abfd));

also in other places.  Apart from that these are all OK.

> 	* coffcode.h: Added SX COFF support.
> 	Implemented handling of variable symbol name lengths.
> 	(bfd_coff_symnmlen) New function: returns target-specific max symbol
> 	name length, as specified in the bfd descriptor.
> 	Minor changes to accomodate larger sizes of some COFF fields in SX COFF.

  OK, with same minor formatting issues.


> -	    maxlen = bfd_coff_force_symnames_in_strings (abfd) ? 0 : SYMNMLEN;
> +	    maxlen = bfd_coff_force_symnames_in_strings (abfd) ?
> +	      0 : bfd_coff_symnmlen(abfd);

  Coding standards say break the line just before an operator, not after, so
this should look like:

-	    maxlen = bfd_coff_force_symnames_in_strings (abfd) ? 0 : SYMNMLEN;
+	    maxlen = bfd_coff_force_symnames_in_strings (abfd)
+	      ? 0 : bfd_coff_symnmlen(abfd);

and also you forgot to mention coffgen.c (from which this hunk is taken) in the
ChangeLog (unless I've somehow overlooked it).  The rest of the coffgen.c
changes are OK.

> 	* cofflink.c: Implemented handling of variable symbol name lengths.

  OK.

> 	* coffswap.h: Added more field accessors in cases where SX COFF has
> 	non-standard field sizes. Provided sane, backwards-compatible defaults.
> 	Use these accessors instead of the generic ones used before.
> 	Implemented handling of variable symbol name lengths.

  From this file:

> +      bzero(ext->e.e.e_zeroes, sizeof(ext->e.e.e_zeroes));

  Another example of missing space between function name and brackets.

> +          /* should never occur with COFF/SX: we will bomb out if it does
> +             just to be sure */
> +          BFD_ASSERT(strcmp(bfd_get_target(abfd), "coff-sx64"));

  Wrong spacing, these lines should begin with TABs.

> @@ -399,22 +480,38 @@
>      case C_FILE:
>        if (ext->x_file.x_fname[0] == 0)
>  	{
> +          /* should never occur with COFF/SX: we will bomb out if it does
> +             just to be sure */
> +          BFD_ASSERT(strcmp(bfd_get_target(abfd), "coff-sx64"));
>  	  in->x_file.x_n.x_zeroes = 0;
>  	  in->x_file.x_n.x_offset = H_GET_32 (abfd, ext->x_file.x_n.x_offset);
>  	}
>        else
>  	{
> -#if FILNMLEN != E_FILNMLEN
> -#error we need to cope with truncating or extending FILNMLEN
> +#if MAX_FILNMLEN < E_FILNMLEN
> +#error E_FILNMLEN must be <= MAX_FILNMLEN
>  #else
> +          memcpy(in->x_file.x_fname,
> +                 ext->x_file.x_fname,
> +                 bfd_coff_filnmlen(abfd));
> +
> +#if 0
>  	  if (numaux > 1)
>  	    {
> +              int i;
> +
> +              /* we can't just do a blind copy over multiple ext auxents
> +                 as internal and external x_fname array sizes may not be (and
> +                 usually aren't!) equal. */
>  	      if (indx == 0)
>  		memcpy (in->x_file.x_fname, ext->x_file.x_fname,
>  			numaux * sizeof (AUXENT));
>  	    }
>  	  else
> -	    memcpy (in->x_file.x_fname, ext->x_file.x_fname, FILNMLEN);
> +            memcpy (in->x_file.x_fname, ext->x_file.x_fname,
> +                    bfd_coff_filnmlen(abfd));
> +#endif /* 0/1 */

  Uh, what is this about?  Not OK without detailed explanation and some
guarantees that this won't affect other targets.  And in general we prefer to
delete code rather than #if-0 it out.  This bit looks like unfinished
work-in-progress.

> 	* config.bfd: Added sx?-nec-[superux|linux] targets.
> 	* configure.in: Added sxcoff_big_vec target.
> 	* cpu-sx.c: Defined NEC SX architecture variants.
> 	* libbfd.c (bfd_write_bigendian_8byte_long_long): New function.

  Can't approve.

> 	* libcoff-in.h: Added some SX COFF specific fields.

  Ok.

> 	* reloc.c: Added SX COFF specific relocations.
> 	* targets.c: Added NEC SX target.

  Can't approve.

  Overall it all looks quite good.  There are minor formatting issues throughout
that won't be hard to tidy up, and there's that one scary-looking bit of
unfinished-seeming #if 0'd stuff.

  Once that's sorted, the formatting fixed, and the patches have been merged up
to CVS HEAD and retested, all the COFF changes are OK.

    cheers,
      DaveK
-- 
(*) - http://sourceware.org/ml/binutils/2009-06/msg00347.html