10
\$\begingroup\$

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 */
\$\endgroup\$
2
\$\begingroup\$

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 -263, but in that case the result of the conversion is UINT64_MAX-263 = 264-263 = 263.

\$\endgroup\$
  • \$\begingroup\$ What is if I need to access the tables opcodes and special_regs from outside? I have fixed thos bugs. Is a sanity check really needed? An unsigned char is never bigger than 255, thus an access is always in-bounds. And for opfstrings, I initialize all elements with an explicit value that is in bounds by construction. Thank you for your comments. \$\endgroup\$ – FUZxxl Nov 14 '11 at 16:11
  • \$\begingroup\$ @FUZxxl On a few exotic platforms, unsigned char is more than 8 bits. Sanity checks aren't always needed, but it's better to have a program that says “I have a bug. Fix me.” than a program that silently returns wrong data in an unrelated part of the code. If you need opcodes and friends from outside, you'll have to export them, and you may need to export the length as well; depending on your needs, you could export it as a global variable that you initialize statically (extern size_t opcodes_length; in the header and size_t opcodes_length = isizeof(opcodes)/sizeof(*opcodes) in the .c). \$\endgroup\$ – Gilles 'SO- stop being evil' Nov 14 '11 at 22:20
  • \$\begingroup\$ If you have a look at pretty_print.h, you might see that I already export those. Is it sensible to leave the constant hardwired, as I don't plan to add more constructors? The compiler could also generate better code from that. \$\endgroup\$ – FUZxxl Nov 14 '11 at 22:23
  • \$\begingroup\$ @FUZxxl You don't plan to add more constructors today. As a general programming habit, such hard-wired constants are a bad idea because in 99% of the cases they end up changing at some point and the maintainer who changes them forgets one of them. Since the definition of MMIX isn't going to change, you're in the 1% of cases where the only reason for the constants to change is if someone is porting your code to a significantly different system (MMMIX?) and expecting major changes anyway. Efficiency-wise, it won't make a measurable difference. \$\endgroup\$ – Gilles 'SO- stop being evil' Nov 14 '11 at 22:29
2
\$\begingroup\$
/*  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.

\$\endgroup\$
  • \$\begingroup\$ Well, I thought it would make it simpler to remember altering the definition of IS_SIMPLE_ARGF if I want to change the code later on if I place it after the last 'simple' entry. Thanks for the comments. \$\endgroup\$ – FUZxxl Nov 13 '11 at 21:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.