This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: 68hc11/12/s12x/xgate patch


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]