Patches for tic54x target (includes TI COFF support)

Nick Clifton nickc@cygnus.com
Wed Jan 12 16:31:00 GMT 2000


Hi Tim,

  OK, I have had a chance to look at your tic54x patch now and here
  are my comments:

Summary
=======
  I do not think that this patch is in an acceptable state.

General
=======
  This patch is too big, and too complicated.  I think that you ought
to split it up into several smaller patches and submit them
individually.  I would suggest splitting them as follows: 

   * The octet vs bytes patch.

   * The patch to replace SCNHSZ (and friends) with bfd_coff_scnhsz()
     (and friends).

   * The section load page patch.

   * The conditional section load patch.  [If it is really needed].

   * The disassembler patch.

   * The .elseif assembler pseudo op patch.

   * The new assembler md macros patch.

   * The new linker command line switch --errors-to-file.

   * The new linker script SECTION directive syntax.

   * The new macros: COFF_DECODE_ALIGNMENT, COFF_ENCODE_AIGNMENT,
     SECTION_RELATIVE_ABSOLUTE_SYMBOL_P, COFF_ADJUST_SYM_IN_POST,
     COFF_ADJUST_SYM_OUT_PRE.  [Note - please include documenation for
     these new macros].

   * The adding a tic54x target patch.

   * Anything else, possible as several patches, not just one.

  It is OK that some patches may depend upon earlier patches being 
accepted, although it would be best if this dependency was one way (ie
later patches depending upon earlier patch, but not the other way
around). 

  Another, minor point is that your comments do not always follow the
GNU standard:  ie reading like a proper English sentance, with a
capitol first letter, a terminating period, two spaces after the final
period, followed immediately by the end-of-comment marker.


Specific
========
----------------------------------------------------------------------

+ /*
+ FUNCTION
+ 	bfd_octets_per_byte
+ 
+ SYNOPSIS
+ 	int bfd_octets_per_byte(bfd *abfd);
+ 
+ DESCRIPTION
+ 	Return the number of host (8-bit) bytes per target byte (minimum
+   addressable unit).  In most cases, this will be one, but some DSP
+   targets have 16, 32, or even 48 bits per byte.
+ 
+ */

  I think it is important to be extremely clear and exact when talking
  about the difference between octets and bytes, thus I think that the
  DESCRIPTION field above is not quite right.  Especially since it
  assumes that the host has an 8 bit byte.  I think that it ought to
  read: 

    DESCRIPTION
      Return the number of octets (8-bit quantities) per target byte
      (minimum addressable unit).  In most cases, this will be one,
      but some DSP targets have 16, 32, or even 48 bits per byte.

-------------------------------------------------------------------------

           /* If this section is going to be output, then this value is the
             offset into the output section of the first byte in the input
!            section. E.g., in most cases, if this was going to start at the
!            100th byte in the output section, this value would be 100; if
!            bfd_octets_per_byte were "2", this value would be 50. */

  Again I think that this ought to be reworded to make the distinction
  between octets and bytes clear.  ie:

         /* If this section is going to be output, then this value is the
            *byte* offset into the output section of the first byte in 
            the input section.  (byte => smallest addressable unit on
            the target).  E.g., in most cases, if the section was
            going to start at the 100th octet (octet => an 8-bit
            quantity) in the output section, this value would be 100;
            if bfd_octets_per_byte were "2", this value would be 50.  */  

---------------------------------------------------------------------------

+         /* Conditionally link this section; link only if there are no
+           references found to any symbol in the section */
+ #define SEC_CLINK 0x10000000

  Why is this flag necessary.  Doesn't the --gc-sections switch to the
  linker already perform this function ?  Is this flag necessayr for
  compatability with another linker ?

*Phew*  There are probably other things to say, but I think it would
best to wait until this patch is broken down into its component
changes.

Cheers
	Nick


More information about the Binutils mailing list