This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: PATCH: support for NEC SX architecture
- From: Dave Korn <dave dot korn dot cygwin at googlemail dot com>
- To: Jaka MoÄnik <jaka at xlab dot si>
- Cc: Dave Korn <dave dot korn dot cygwin at googlemail dot com>, binutils at sourceware dot org, Erich Focht <efocht at hpce dot nec dot com>
- Date: Mon, 10 Aug 2009 05:13:14 +0100
- Subject: Re: PATCH: support for NEC SX architecture
- References: <1241711023.4770.70.camel@wall.xlab.lan> <4A3CFFF7.6000903@gmail.com> <4A4111B9.8030903@gmail.com> <1246872826.30224.93.camel@wall.xlab.lan>
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