Re: PATCH: support for NEC SX architecture

[Dave Korn <dave dot korn dot cygwin at googlemail dot com>]

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