Re: Binutils patch to add support for Open8 MCU[part 1 of 21]

["Joseph S. Myers" <joseph at codesourcery dot com>]

Some general comments on these patches:

* ChangeLog entries for subdirectories should not go in the toplevel 
ChangeLog.  In general each ChangeLog entry should go in the ChangeLog 
closest to the affected file (so binutils/testsuite/ChangeLog for binutils 
testsuite changes, for example), and in no other ChangeLog, and it's best 
to send ChangeLog entries in plain text in the body of the message, not as 
diffs.

* config.sub changes need to go to config-patches@gnu.org, with a new 
version then being imported verbatim from upstream config.git.

* Please avoid adding code to the toplevel configure.ac that disables GCC 
library directories unless (a) GCC supports the target, (b) the libraries 
in question are known not to work for the target and (c) nevertheless, it 
is considered appropriate for some reason to build the associated GCC 
front ends rather than disabling the language with unsupported_languages.  
See recent discussion of patches on gcc-patches and the principles I 
proposed on this list and elsewhere.  If in doubt, avoid making any change 
to the toplevel configure.ac.  (Disabling GDB for the target if you aren't 
going to be contributing a GDB port soon would be reasonable, however.)

* Never include system headers before headers from the relevant bit of 
binutils; for example, it's wrong in gas/config/tc-open8.c to include 
<limits.h> before "as.h"; opcodes/open8-dis.c has a similar problem (this 
may not be an exhaustive list of affected files).  Depending on the 
configuration, feature test macros may be defined in config.h that need to 
be defined before the first system header is included; this can cause 
build failures on some hosts if you get it wrong.  I realise various 
existing files have this problem, but it's not something to repeat.

* Avoid spurious whitespace changes to existing code; for example, the 
second diff hunk to ld/configure.tgt should be removed.

* Is there a reason you really, really need to have your own linker script 
template open8.sc rather than using elf.sc (possibly with additional 
configuration variables added to allow elf.sc to be customised to your 
target).  Linker scripts forked for individual ELF targets are a *bad* 
idea as they don't tend to get updated when elf.sc is - for example, I see 
you are missing the DWARF 3 sections that elf.sc has.  I added significant 
parametrisation to elf.sc when doing the C6X port, and would encourage 
other porters to add further such parametrisation rather than creating 
their own linker scripts.  Cf. my previous comments in 
<http://sourceware.org/ml/binutils/2010-06/msg00317.html> and 
<http://sourceware.org/ml/binutils/2010-03/msg00318.html> - reducing the 
number of such forks would be much better than increasing it.

* I see no changes to the documentation in this patch.  It appears you 
have command-line options to both assembler and linker - these need 
documenting in the relevant manuals.  Assembler directives need 
documenting.  Assembler comment syntax needs documenting.  For options 
documentation, see 
<http://sourceware.org/ml/binutils/2010-11/msg00397.html> for the 
preferred approach to avoid duplicating details of the individual options 
- take care about the bits before and after the place there the .texi file 
gets included for manpage generation, to avoid options for other targets 
disappearing quietly.  Once you've dealt with the user documentation in 
the .texi manuals, then there's the matter of NEWS file updates to mention 
the new port.

* Could you use snprintf instead of sprintf?  It's good style for new code 
to do so, to reduce the risk of buffer overruns if you miscalculate the 
required buffer size.

* Could you confirm that you have 100% clean testsuite results (no 
unexpected FAILs) for binutils, gas and ld testsuites for your new target 
- on both 32-bit and 64-bit hosts if you're able to test both (it's common 
for assemblers to have bugs that only appear on one of 32-bit and 64-bit 
hosts)?

-- 
Joseph S. Myers
joseph@codesourcery.com