68hc11/12/s12x/xgate patch

nick clifton nickc@redhat.com
Tue Jan 10 14:42:00 GMT 2012


Hi James,

> Find attached my latest revision of a patch to the m68hc11 port

Thank you for submitting this.  Here are some observations and requests 
for improvements on the patch:

*) Are you offering to act as a maintainer for the S12X target ?

*) You have included a patch to the top level configure file, but not 
the top level configure.ac file (from which the configure file is 
generated).

*) You have included a patch to the top level config.sub file.  Are you 
aware that this patch needs to be submitted to a different project ? 
(config-patches@gnu.org)

*) You have included patches to various ChangeLogs.  Common practice is 
just to include changelog entries as plain text, since they almost never 
apply cleanly as patches.

*) You have added new options to the m68hc11 GAS port, but not added 
documentation for these new options to the gas/doc/c-m68hc11.texi file.

*) You have added support for a new processor, but not mentioned it in 
either gas/NEWS or ld/NEWS.

*) There are some formatting problems.  Ideally we like code that 
follows the GNU Coding Standard: http://www.gnu.org/prep/standards/

Problems such as lines that end with opening curly braces:

    if (r_type != R_M68HC11_NONE) {

Comments that are not treated as sentences:

  /* the -2 here is due to the way XGATE PCREL9, PCREL10 work */

Lines that are excessively long and could be broken up:

   if (move_insn && !(val >= -16 && val <= 15) && ((!(mode & 
M6812_OP_IDX) && !(mode & M6812_OP_D_IDX_2)) || !(current_architecture & 
cpu9s12x)))

Function calls without a space between the function name and the opening 
parentheses.

   as_bad("Invalid register\n");

*) There appears to be some kind of problem with assembling or 
disassembling NOP insns for the xgate target.  I tried this:

   % cat fred.s
         .text
   fred:
         nop

   % as -mm9s12xg fred.s -o fred.o

   % objdump -d fred.o

   fred.o:     file format elf32-m68hc12

   Disassembly of section .text:

   00000000 <fred>:
      0:   01              mem
           ...

Assembling with -mm9s12x gives a disassembly of "nop" as expected...


Other than that though, this patch looks fine.  If you could address the 
issues raised above and resubmit the patch I will be happy to review it 
again.

Cheers
   Nick



More information about the Binutils mailing list