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