Home Original page

PATCH: support for NEC SX architecture

Dave Korn dave.korn.cygwin@googlemail.com
Sun Jun 21 04:47:00 GMT 2009 
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
More information about the Binutils mailing list