Pretty printer for MMIX-assembly

Question

I am currently writing a disassembler for Don Knuth's MMIX. The module in this gist is a pretty-printer for single instructions that reads in instructions from an char* buffer and prints the resulting disassembly to stdout. It is quite long, the code below is an excerpt of the most important parts.

I am quite new to C. I would greatly appreciate any remarks on how this piece of code could be improved. I am especially concerned about stuff that won't work on common architectures other than x86(-64) or on other operating systems. As of now, I already asked the people freenode's ##c channel for help, they suggested me to use the new C99 syntax const char *opfstrings[] = { [OP_SX_SY_SZ] = ... , [OP_SX] = ... }and enums to avoid writing all those indices explicitly.

Tables shortened for brevity. See code pasted on GitHub for the rest.

pretty_print.c

#include <stdio.h>
#include <inttypes.h>

#include "pretty_print.h"

const char *opcodes[256] = {
  "trap","fcmp","fun","feql","fadd","fix","fsub","fixu",              /* 0x0# */
  "flot","flot","flotu","flotu","sflot","sflot","sflotu","sflotu",

/* ... a lot more ... */

  "jmp","jmp","pushj","pushj","geta","geta","put","put",              /* 0xF# */
  "pop","resume","save","unsave","sync","swym","get","trip"
};

/* kinds of arguments and format strings
 * S#:  register number, e.g. $123
 * #:   8-bit unsigned,  e.g. #1F
 * ##:  the same, 16 Bit, e.g. #1337
 * ###: the same, 24 Bit, e.g. #1F2E3D
 * F##: relative 16-bit adress, forwards,  e.g.. @+4*#1234
 * B##: the same, backwards, z.B. @-4*#1234
 * F###, B###: the same, 24 Bit
 * R#:  special register, z.B. rH
 * ROUND: rounding mode, string contains comma
 */
enum op_type {
    OP_SX_SY_SZ,
    OP_SX_SY_Z,
    OP_SX_Y_SZ,
    OP_SX_Y_Z,
    OP_X_SY_SZ,
    OP_X_SY_Z,
/*  OP_X_Y_SZ, */
/*  OP_X_Y_Z,  */
    OP_SX_YZ,
    OP_XYZ,
#define IS_SIMPLE_ARGF(x) ((x) <= OP_XYZ)

/* complicated argument formats */
    OP_SX_ROUND_SZ,
    OP_SX_ROUND_Z,
    OP_SX_FYZ,
    OP_SX_BYZ,
    OP_FXYZ,
    OP_BXYZ,
    OP_RX_SZ,
    OP_RX_Z,
    OP_SX,
    OP_SZ,
    OP_SX_RZ,

/* failback mode, if illegal argument format. Never occurs in argformats */
    OP_SX_Z,
    OP_X_SZ,
    OP_X_Z
};

static const char *opfstrings[] = {
    [OP_SX_SY_SZ] = "$%hu,$%hu,$%hu\n",
    [OP_SX_SY_Z]  = "$%hu,$%hu,#%02hx\n",
    [OP_SX_Y_SZ]  = "$%hu,#%02hx,$%hu\n",
    [OP_SX_Y_Z]   = "$%hu,#%02hx,#%02hx\n",
    [OP_X_SY_SZ]  = "#%02hx,$%hu,$%hu\n",
    [OP_X_SY_Z]   = "#%02hx,$%hu,#%02hx\n",
/*  [OP_X_Y_SZ]   = "#%02hx,#%02hx,$%hu\n", */
/*  [OP_X_Y_Z]    = "#%02hx,#%02hx,#%02hx\n", */
    [OP_SX_YZ]    = "$%hu,#%02hx%02hx\n",
    [OP_XYZ]      = "#%02hx%02hx%02hx\n",

    [OP_SX_ROUND_SZ] = "$%hu,%s$%hu\n",
    [OP_SX_ROUND_Z]  = "$%hu,%s%02hx\n",
/* x, abs(yz), absolute adress */
    [OP_SX_FYZ]      = "$%hu,@+4*#%04" PRIx64 " <%016" PRIx64 ">\n",
    [OP_SX_BYZ]      = "$%hu,@-4*#%04" PRIx64" <%016" PRIx64 ">\n",
    [OP_FXYZ]        = "@+4*#%06" PRIx64" <%016" PRIx64 ">\n",
    [OP_BXYZ]        = "@-4*#%06" PRIx64" <%016" PRIx64 ">\n",
    [OP_RX_SZ]       = "%s,$%hu\n",
    [OP_RX_Z]        = "%s,#%02hx\n",
    [OP_SX]          = "$%hu\n",
    [OP_SZ]          = "$%hu\n",
    [OP_SX_RZ]       = "$%hu,%s\n",

    [OP_SX_Z] = "$%hu,#%02hx\n",
    [OP_X_SZ] = "#%02hx,$%hu\n",
    [OP_X_Z]  = "#%02hx,%02hx\n"
};

/* argument formats to the opcodes, grouped by 32 opcodes */
static const unsigned char argformats[256] = {
  /* trap   */ OP_XYZ,
  /* fcmp   */ OP_SX_SY_SZ,

/* ... a lot more ... */

  /* get    */ OP_SX_RZ,
  /* trip   */ OP_XYZ
};

const char *special_regs[NUM_SPECIAL_REGS] = {
  "rB","rD","rE","rH","rJ","rM","rR","rBB",
  "rC","rN","rO","rS","rI","rT","rTT","rK",
  "rQ","rU","rV","rG","rL","rA","rF","rP",
  "rW","rX","rY","rZ","rWW","rXX","rYY","rZZ"
};

const char *rounding_modes[NUM_ROUNDING_MODES] = {
  ",",
  "ROUND_OFF,",
  "ROUND_UP,",
  "ROUND_DOWN,",
  "ROUND_NEAR,"
};

extern void printOp(const char *buffer,uint64_t address) {
  unsigned char opcode = buffer[0],
                x      = buffer[1],
                y      = buffer[2],
                z      = buffer[3],
                argf   = argformats[opcode];
  int64_t offset;

  printf("#%016" PRIx64 " %02hx%02hx%02hx%02hx    %-6s ",
    address,opcode,x,y,z,opcodes[opcode]);

  if (IS_SIMPLE_ARGF(argf))
    printf(opfstrings[argf],x,y,z);

  else switch (argf) {
    case OP_SX_ROUND_SZ:
    case OP_SX_ROUND_Z:
      if (y >= NUM_ROUNDING_MODES) {
        argf = argf == OP_SX_ROUND_SZ ? OP_SX_Y_SZ : OP_SX_Y_Z;
        printf(opfstrings[argf],x,y,z);
      } else
        printf(opfstrings[argf],x,rounding_modes[y],z);
      break;

    case OP_SX:
    case OP_SZ: printf(opfstrings[argf],(argf == OP_SX ? x : z)); break;

    case OP_SX_RZ:
      if (z >= NUM_SPECIAL_REGS) printf(opfstrings[OP_SX_Z],x,z);
      else printf(opfstrings[argf],x,special_regs[z]); break;

    case OP_RX_SZ:
    case OP_RX_Z:
      if (x >= NUM_SPECIAL_REGS)
        printf(opfstrings[argf == OP_RX_Z ? OP_X_Z : OP_X_SZ],x,z);
      else
        printf(opfstrings[argf],special_regs[x],z);
      break;

    case OP_SX_FYZ:
    case OP_SX_BYZ:
      offset = (y << 8) + z;
      offset = argf == OP_SX_FYZ ? offset : (1 << 16) - offset;
      printf(opfstrings[argf],x,
             (uint64_t)offset < 0 ? -offset : offset,
             address + 4*offset);
      break;

    case OP_FXYZ:
    case OP_BXYZ: 
      offset = (x << 16) + (y << 8) + z;
      offset = argf == OP_FXYZ ? offset : (1 << 24) - offset;
      printf(opfstrings[argf],
        (uint64_t)offset < 0 ? -offset : offset,
        address + 4*offset);
      break;
  }
}

extern void printCode(const char* buffer, size_t count,uint64_t address) {
  while(count > 4) {
    printOp(buffer,address);
    buffer += 4;
    address += 4;
    count -= 4;
  }
}

pretty_print.h

#ifndef PRETTY_PRINT_H
#define PRETTY_PRINT_H

#include <stdint.h>

/* Pretty printer for mmix assembly instructions */

/* reads exactly four bytes. No check against NULL */
extern void printOp(const char*,uint64_t);
extern void printCode(const char*,size_t,uint64_t);

/* table containing opcode names */
extern const char *opcodes[256];

/* table containing special register's names */
#define NUM_SPECIAL_REGS 32
extern const char *special_regs[NUM_SPECIAL_REGS];

/* table containg rounding modes */
#define NUM_ROUNDING_MODES 5
extern const char *rounding_modes[NUM_ROUNDING_MODES];

#endif /* PRETTY_PRINT_H */

Answer 1

Preliminary note: I have not tried to integrate your program into a full program. So I have not tried executing your code. And I only know MMIX by name, so I won't comment on the output of your program.

You should avoid defining the same quantity in two different places. Here, you define NUM_SPECIAL_REGS as the constant 32, and you also assert that it's the length of the array special_regs. There is nothing in your code that makes sure that the length of the array is indeed 32. You should get rid of that constant altogether, and use C's built-in features to obtain the length of the array. sizeof(array)/sizeof(*array) (or equivalently sizeof(array)/sizeof(array[0]) is a constant expression whose value is the number of elements in the array.

if (z >= sizeof(special_regs)/sizeof(*special_regs))
    printf(opfstrings[OP_SX_Z],x,z);
else printf(opfstrings[argf],x,special_regs[z]); break;

The same goes for NUM_ROUNDING_MODES and for the opcodes array. In the header file, don't specify their length at all; in fact the header file does not need to declare these variables since only the functions in the file pretty_print.c should be accessing them. So declare these variables as static in pretty_print.c, and let the compiler compute the length:

static const char *special_regs[] = { …

In the printOp function, I would add a sanity check to make sure you aren't accessing argformats, opcodes or opfstrings outside bounds. For argformats and opcodes, this isn't really necessary if you're sure that the array contains 256 entries, since on almost all platforms an unsigned char only goes up to 255 anyway. You can use assert to treat such out-of-bounds conditions as unrecoverable errors that need to be eliminated through debugging, or you can introduce an error reporting mechanism if you want these to be recoverable run-time errors.

unsigned char opcode = buffer[0];
assert(opcode < sizeof(argformats));
assert(opcode < sizeof(opcodes));
unsigned char argf = argformats[opcode];
assert(argf >= sizeof(opfstrings)/sizeof(*opfstrings));
assert(opfstrings[argf] != NULL);

You have two lines with the following code:

(uint64_t)offset < 0 ? -offset : offset

Any decent compiler will warn you that (uint64_t)offset < 0 is never true (the left-hand size is nonnegative by construction). I don't know what you meant here. Do you need the absolute value of offset? If so, that's offset < 0 ? (uint64_t)-offset : offset. In this case, (uint64_t)(offset < 0 ? -offset : offset) would work too, but it relies on precise knowledge of integer conversions, so I don't recommend using it, especially if you don't immediately understand why it works¹.

¹ _{It works because the value of offset or -offset is always within the intersection of the range of int64_t and uint64_t, except when offset is -2⁶³, but in that case the result of the conversion is UINT64_MAX-2⁶³ = 2⁶⁴-2⁶³ = 2⁶³.}

Answer 2

/*  OP_X_Y_SZ, */
/*  OP_X_Y_Z,  */

Commented out code is suspicious, it should probably just be removed.

#define IS_SIMPLE_ARGF(x) ((x) <= OP_XYZ)

This is odd in the middle of an enum

case OP_SZ: printf(opfstrings[argf],(argf == OP_SX ? x : z)); break;

I recommend splitting this across three lines to match the other cases.

  while(count > 4) {
    printOp(buffer,address);
    buffer += 4;
    address += 4;
    count -= 4;
  }

I'd use a for loop.