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

["khays at hayshaus dot com" <khays at hayshaus dot com>]

Joseph,
Â
thanks for the catches - specific items addressed below.
Â
Kindest,
Kirk HaysÂ
Â
On March 30, 2011 at 4:24 PM "Joseph S. Myers" <joseph@codesourcery.com> wrote:

> 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.
Â
Backed out all the ChangeLog entries. Will conform to standard methodology in
the future.
Â
> * config.sub changes need to go to config-patches@gnu.org, with a new
> version then being imported verbatim from upstream config.git.
Â
Submitted this morning.
Â
> * 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.)
Â
Changes backed out, and testing shows they were not needed.
Â
> * 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.
Â
Fixed.
Â
> * Avoid spurious whitespace changes to existing code; for example, the
> second diff hunk to ld/configure.tgt should be removed.
Â
Fixed.
Â
> * 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.
Â
Agreed, will add to the list of tasks.
Â
> * 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.
Â
Agreed, already on the list of tasks.
Â
> * 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.
Â
Fixed, also caught a use of strcpy().
Â
> * 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)?
Â
Confirm 100% clean testsuites on 64 bits host (openSuSE 11.3 x86_64) and 32 bit
host (openSuSE 11.4, i586).
Â