Move validator_x86_XX.rl out of unreviewed.

src/trusted/validator_ragel/unreviewed/decoder_test.c

 /*

[Dmitry Polukhin, 2012/11/02 17:31:06]

I think this test need to be converted to C++ and simplified. See some hints
below.

  * Copyright (c) 2012 The Native Client Authors. All rights reserved.
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include <assert.h>
 #include <stdio.h>
 #include <stdarg.h>
 #include <stdlib.h>
 #include <string.h>
 
 #include "native_client/src/include/elf32.h"
 #include "native_client/src/include/elf64.h"
 #include "native_client/src/shared/platform/nacl_check.h"
 #include "native_client/src/shared/utils/types.h"
-#include "native_client/src/trusted/validator_ragel/unreviewed/decoder.h"
+#include "native_client/src/trusted/validator_ragel/decoder.h"
 
 /* This is a copy of NaClLog from shared/platform/nacl_log.c to avoid
  * linking in code in NaCl shared code in the unreviewed/Makefile and be able to
  *  use CHECK().
 
  * TODO(khim): remove the copy of NaClLog implementation as soon as
  * unreviewed/Makefile is eliminated.
  */
 void NaClLog(int detail_level, char const  *fmt, ...) {
   va_list ap;
@@ skipping 96 matching lines @@
           NULL, "cr15", "db15", "tr15" }
     };
     assert(kRegisterNames[name][type]);
     return kRegisterNames[name][type];
   } else {
     assert(FALSE);
     return NULL;
   }
 }
 
 Bool IsNameInList(const char *name, ...) {

[Brad Chen, 2012/10/22 21:29:05]

Could you use an array rather than varargs here? I'm worried about fragility of
varargs.
Can you make this function static, assuming it is not used outside this file?

[Dmitry Polukhin, 2012/11/02 17:31:06]

This function can be converted to function with two arguments and va_args can be
passed as single string with some separator like ';' or '|'.

   va_list value_list;
 
   va_start(value_list, name);
 
   for (;;) {
     const char *element = va_arg(value_list, const char*);
     if (element) {
       if (strcmp(name, element) == 0) {
         va_end(value_list);
         return TRUE;
       }
     } else {
       va_end(value_list);
       return FALSE;
     }
   }
 }
 
+/*
+ * "fwait" is nasty: few of them will be included in other X87 instructions
+ * ("fclex", "finit", "fstcw", "fstsw", "fsave" have two names, other
+ * instructions are unchanged) - but if after them we see regular instruction
+ * then we must print them all.  This convoluted logic is not needed when we
+ * don't print anything so decoder does not include it.
+ */
+static Bool ProcessFWait(const uint8_t **begin, const uint8_t *end,

[Dmitry Polukhin, 2012/11/02 17:31:06]

Too many arguments, class with state should solve this issue (see below).

+                         struct Instruction *instruction,
+                         struct DecodeState *userdata,
+                         unsigned char *rex_prefix, Bool *spurious_rex_prefix,
+                         enum OperandName *rm_base,
+                         enum OperandName *rm_index) {
+  /* Instruction is "fwait" if it's 0x9b or if it's REX prefix plus 0x9b.  */
+  if (((end == *begin + 1) && ((*begin)[0] == 0x9b)) ||
+      ((end == *begin + 2) &&
+       (((*begin)[0] & 0xf0) == 0x40) && ((*begin)[1] == 0x9b))) {
+    if (!(userdata->fwait)) {
+      userdata->fwait = *begin;
+    }
+    return TRUE;
+  } else if (userdata->fwait) {
+    /*
+     * If it's x87 instruction then we can fold some fwait's in the instruction
+     * itself.
+     *
+     * Instruction is x87 instruction if it has opcode from 0xd8 to 0xdf, but it
+     * can include either 0x66 prefix or REX prefix, or both.
+     */
+    if ((((*begin)[0] & 0xf8) == 0xd8) ||
+        (((((*begin)[0] & 0xf0) == 0x40) || ((*begin)[0] == 0x66)) &&
+         (((*begin)[1] & 0xf8) == 0xd8)) ||
+        (((*begin)[0] == 0x66) && (((*begin)[1] & 0xf0) == 0x40) &&
+         (((*begin)[2] & 0xf8) == 0xd8))) {
+      /*
+       * fwait "prefix" can only include two 0x9b bytes or one rex byte - and
+       * then only if the instruction itself have no rex prefix.
+       */
+      int fwait_count = !!(instruction->prefix.data16);
+      int rex_count = (!!*rex_prefix) | (!!(instruction->prefix.data16));
+      for (;;) {
+        if (*begin == userdata->fwait)
+          break;
+        if (((*begin)[-1]) == 0x9b) {
+          if (fwait_count < 2) {
+            --*begin;
+            ++fwait_count;
+            if (((*begin)[1] & 0xf0) == 0x40)
+              break;
+          } else {
+            break;
+          }
+        } else if (((*begin)[-1] & 0xf0) == 0x40) {
+          if (rex_count >= 1)
+            break;
+          --*begin;
+          ++rex_count;
+          if (!*rex_prefix) {
+            *rex_prefix = **begin;
+            /*
+             * Bug-to-bug compatibility: objdump will erroneously use bits from
+             * first REX prefix (attached to "fwait") and not from the
+             * second REX prefix (attached to the instruction itself).
+             *
+             * Duplicate this error here till it'll be fixed in objdump.
+             */
+            if ((*rex_prefix & 0x01) && (*rm_base <= REG_RDI)) {
+              if (instruction->operands_count == 1 &&
+                  instruction->operands[0].name == REG_RM)
+                *rm_base |= REG_R8;
+              else
+                *spurious_rex_prefix = TRUE;
+            }
+            if (*rex_prefix & 0x02) {
+              if (instruction->operands_count == 1 &&
+                  instruction->operands[0].name == REG_RM) {
+                if (*rm_index <= REG_RDI)
+                  *rm_index |= REG_R8;
+                else if (*rm_index == REG_RIZ)
+                  *rm_index = REG_R12;
+                else if (*rm_index == NO_REG)
+                  *spurious_rex_prefix = TRUE;
+              } else {
+                *spurious_rex_prefix = TRUE;
+              }
+            }
+            if (*rex_prefix & 0x0c)
+              *spurious_rex_prefix = TRUE;
+          }
+        }
+      }
+      if (*begin != userdata->fwait) {
+        while (userdata->fwait < *begin) {
+          printf("%*lx:\t%02x                   \tfwait\n",
+            userdata->width, (long)(userdata->fwait - userdata->offset),
+            *userdata->fwait);
+            ++(userdata->fwait);
+        }
+      }
+    } else {
+      while ((userdata->fwait) < *begin) {
+        printf("%*lx:\t%02x                   \tfwait\n",
+          userdata->width, (long)(userdata->fwait - userdata->offset),
+          *userdata->fwait);
+        ++(userdata->fwait);
+      }
+    }
+    userdata->fwait = FALSE;
+  }
+  return FALSE;
+}
+
 void ProcessInstruction(const uint8_t *begin, const uint8_t *end,

[Dmitry Polukhin, 2012/11/02 17:31:06]

I think it should be converted to so class InstructionParser that will have
state of parsing with methods to parse prefixes, suffixes, etc. so you could
avoid passing tons of arguments and will pass only 'this' + really needed
params.

-                        struct Instruction *instruction, void *userdata) {
+                        struct Instruction *instruction, void *callback_data) {
+  struct DecodeState *userdata = callback_data;
   const char *instruction_name = instruction->name;
   unsigned char operands_count = instruction->operands_count;
   unsigned char rex_prefix = instruction->prefix.rex;
   enum OperandName rm_index = instruction->rm.index;
   enum OperandName rm_base = instruction->rm.base;
   Bool data16_prefix = instruction->prefix.data16;
   const uint8_t *p;
   char delimeter = ' ';
   Bool print_rip = FALSE;
   Bool empty_rex_prefix_ok = FALSE;
   Bool spurious_rex_prefix = FALSE;
 #define print_name(x) (printf((x)), shown_name += strlen((x)))

[Brad Chen, 2012/10/22 21:29:05]

What is your goal in making this a macro rather than a function? I notice you
#undef it below as well. Macros generally make code harder to understand and
harder to maintain, better to avoid. If you use one you should have a really
good reason and document it clearly.

   size_t shown_name = 0;
   int i, operand_type;
 
+  if (ProcessFWait(&begin, end, instruction, callback_data,
+                   &rex_prefix, &spurious_rex_prefix, &rm_base, &rm_index))
+    return;
+
   /*
-   * "fwait" is nasty: few of them will be included in other X87 instructions
-   * ("fclex", "finit", "fstcw", "fstsw", "fsave" have two names, other
-   * instructions are unchanged) - but if after them we see regular instruction
-   * then we must print them all.  This convoluted logic is not needed when we
-   * don't print anything so decoder does not include it.
+   * Objdump will print data16 (0x66) prefix on a separate line for "fbld",
+   * "fbstp", "fild", "fistp", "fld", and "fstp" instructions.
    */
-  if (((end == begin + 1) && (begin[0] == 0x9b)) ||
-      ((end == begin + 2) &&
-                           ((begin[0] & 0xf0) == 0x40) && (begin[1] == 0x9b))) {
-    if (!(((struct DecodeState *)userdata)->fwait)) {
-      ((struct DecodeState *)userdata)->fwait = begin;
-    }
-    return;
-  } else if (((struct DecodeState *)userdata)->fwait) {
-    /* If it's x87 instruction then we can fold some fwait's in the instruction
-       itself.  */
-    if (((begin[0] >= 0xd8) && (begin[0] <= 0xdf)) ||
-        ((((begin[0] & 0xf0) == 0x40) || (begin[0] == 0x66)) &&
-                                    (begin[1] >= 0xd8) && (begin[1] <= 0xdf)) ||
-        ((begin[0] == 0x66) || ((begin[1] & 0xf0) == 0x40) ||
-                                    (begin[2] >= 0xd8) || (begin[2] <= 0xdf))) {
-      /* fwait "prefix" can only include two 0x9b bytes or one rex byte - and
-       * then only if the instruction itself have no rex prefix.  */
-      int fwait_count = !!data16_prefix;
-      int rex_count = (!!rex_prefix) | (!!data16_prefix);
-      for (;;) {
-        if (begin == ((struct DecodeState *)userdata)->fwait)
-          break;
-        if ((begin[-1]) == 0x9b) {
-          if (fwait_count < 2) {
-            --begin;
-            ++fwait_count;
-            if ((begin[1] & 0xf0) == 0x40)
-              break;
-          } else {
-            break;
-          }
-        } else if ((begin[-1] & 0xf0) == 0x40) {
-          if (rex_count >= 1)
-            break;
-          --begin;
-          ++rex_count;
-          if (!rex_prefix) {
-            rex_prefix = *begin;
-            /* Bug-to-bug compatibility, fun... */
-            if ((rex_prefix & 0x01) && (rm_base <= REG_RDI)) {
-              if (operands_count == 1 &&
-                  instruction->operands[0].name == REG_RM)
-                rm_base |= REG_R8;
-              else
-                spurious_rex_prefix = TRUE;
-            }
-            if (rex_prefix & 0x02) {
-              if (operands_count == 1 &&
-                  instruction->operands[0].name == REG_RM) {
-                if (rm_index <= REG_RDI)
-                  rm_index |= REG_R8;
-                else if (rm_index == REG_RIZ)
-                  rm_index = REG_R12;
-                else if (rm_index == NO_REG)
-                  spurious_rex_prefix = TRUE;
-              } else {
-                spurious_rex_prefix = TRUE;
-              }
-            }
-            if (rex_prefix & 0x0c)
-              spurious_rex_prefix = TRUE;
-          }
-        }
-      }
-      if (begin != ((struct DecodeState *)userdata)->fwait) {
-        while ((((struct DecodeState *)userdata)->fwait) < begin) {
-          printf("%*lx:\t%02x                   \tfwait\n",
-            ((struct DecodeState *)userdata)->width,
-            (long)((((struct DecodeState *)userdata)->fwait) -
-                                    (((struct DecodeState *)userdata)->offset)),
-            *((struct DecodeState *)userdata)->fwait);
-            ++(((struct DecodeState *)userdata)->fwait);
-        }
-      }
-    } else {
-      while ((((struct DecodeState *)userdata)->fwait) < begin) {
-        printf("%*lx:\t%02x                   \tfwait\n",
-          ((struct DecodeState *)userdata)->width,
-          (long)((((struct DecodeState *)userdata)->fwait) -
-                                    (((struct DecodeState *)userdata)->offset)),
-          *((struct DecodeState *)userdata)->fwait);
-        ++(((struct DecodeState *)userdata)->fwait);
-      }
-    }
-    ((struct DecodeState *)userdata)->fwait = FALSE;
-  }
-
   if ((data16_prefix) && (begin[0] == 0x66) && (!(rex_prefix & 0x08)) &&
       (IsNameInList(instruction_name,
                     "fbld", "fbstp", "fild", "fistp", "fld", "fstp", NULL))) {
     printf("%*lx:\t66                   \tdata16\n",
-           ((struct DecodeState *)userdata)->width,
-           (long)(begin - (((struct DecodeState *)userdata)->offset)));
+           userdata->width, (long)(begin - userdata->offset));
     data16_prefix = FALSE;
     ++begin;
   }
-  printf("%*lx:\t", ((struct DecodeState *)userdata)->width,
-                    (long)(begin - (((struct DecodeState *)userdata)->offset)));
+
+  /* Start the main processing part: print offset here.  */
+  printf("%*lx:\t", userdata->width, (long)(begin - userdata->offset));
   for (p = begin; p < begin + 7; ++p) {
     if (p >= end)
       printf("   ");
     else
       printf("%02x ", *p);
   }
   printf("\t");
+
   /*
    * "pclmulqdq" has two-operand mnemonic names for "imm8" equal to 0x01, 0x01,
    * 0x10, and 0x11.  Objdump incorrectly mixes them up with 0x2 and 0x03.
    */
   if (!strcmp(instruction_name, "pclmulqdq")) {
     if (instruction->imm[0] < 0x04) {
       switch (instruction->imm[0]) {
         case 0x02: instruction_name = "pclmullqhqdq"; break;
         case 0x03: instruction_name = "pclmulhqhqdq"; break;
       }
       --operands_count;
     }
   }
+
   /*
    * "vpclmulqdq" has two-operand mnemonic names for "imm8" equal to 0x01, 0x01,
    * 0x10, and 0x11.  Objdump mixes them with 0x2 and 0x03.
    */
   if (!strcmp(instruction_name, "vpclmulqdq")) {
     if (instruction->imm[0] < 0x04) {
       switch (instruction->imm[0]) {
         case 0x02: instruction_name = "vpclmullqhqdq"; break;
         case 0x03: instruction_name = "vpclmulhqhqdq"; break;
       }
       --operands_count;
     }
   }
+
   spurious_rex_prefix |=
     rex_prefix &&
     (instruction->prefix.rex_b_spurious ||
      instruction->prefix.rex_x_spurious ||
      instruction->prefix.rex_r_spurious ||
      instruction->prefix.rex_w_spurious);
+
+  /*
+   * Objdump sometimes does not show spurious rex prefixes.  Adjust the
+   * spurious_rex_prefix variable here for these cases.
+   */
   if (operands_count > 0) {
-    if (!((struct DecodeState *)userdata)->ia32_mode)
+    if (!userdata->ia32_mode)
       for (i=0; i<operands_count; ++i)
         /*
          * Objdump mistakenly allows "lock" with "mov %crX,%rXX" only in ia32
          * mode.  It's perfectly valid in amd64, too, so instead of changing
          * the decoder we fix it here.
          */
         if (instruction->operands[i].type == OPERAND_TYPE_CONTROL_REGISTER &&
             *begin == 0xf0 && !instruction->prefix.lock) {
           print_name("lock ");
           if (rex_prefix & 0x04) {
             if (!instruction->prefix.rex_b_spurious &&
                 instruction->prefix.rex_r_spurious &&
                 !instruction->prefix.rex_x_spurious &&
                 !instruction->prefix.rex_w_spurious)
               spurious_rex_prefix = FALSE;
           } else {
             instruction->operands[i].name -= 8;
           }
         }
   }
-  /* Only few rare instructions show spurious REX.B in objdump.  */
-  if (!spurious_rex_prefix && instruction->prefix.rex_b_spurious)
-    if (IsNameInList(instruction_name,
-                     "ja", "jae", "jbe", "jb", "je", "jg", "jge", "jle",
-                     "jl", "jne", "jno", "jnp", "jns", "jo", "jp", "js",
-                     "jecxz", "jrcxz", "loop", "loope", "loopne", NULL))
-      spurious_rex_prefix = TRUE;
+
   /* Some instructions don't show spurious REX.B in objdump.  */
   if (spurious_rex_prefix &&
       instruction->prefix.rex_b_spurious &&
       !instruction->prefix.rex_r_spurious &&
       !instruction->prefix.rex_x_spurious &&
       !instruction->prefix.rex_w_spurious) {
     if (operands_count > 0)
       for (i=0; i<operands_count; ++i)
         if (instruction->operands[i].name == REG_RM &&
             instruction->rm.disp_type != DISP64) {
           spurious_rex_prefix = FALSE;
           break;
         }
   }
+
   /* Some instructions don't show spurious REX.W in objdump.  */
   if (spurious_rex_prefix &&
       !instruction->prefix.rex_b_spurious &&
       !instruction->prefix.rex_r_spurious &&
       !instruction->prefix.rex_x_spurious &&
       instruction->prefix.rex_w_spurious) {
     if (IsNameInList(instruction_name,
                      "ja", "jae", "jbe", "jb", "je", "jg", "jge", "jle",
                      "jl", "jne", "jno", "jnp", "jns", "jo", "jp", "js",
                      "jecxz", "jrcxz", "loop", "loope", "loopne",
@@ skipping 20 matching lines @@
   if (!spurious_rex_prefix && (rex_prefix & 0x08) &&
       ((IsNameInList(instruction_name, "call", "jmp", "lcall", "ljmp", NULL) &&
         instruction->operands[0].name != JMP_TO) ||
        IsNameInList(instruction_name,
                     "fldenvs", "fnstenvs", "fnsaves",
                     "frstors", "fsaves", "fstenvs",
                     "in", "ins", "out", "outs",
                     "popf", "push", "pushf", NULL)))
     spurious_rex_prefix = TRUE;
 
+  /*
+   * Print prefixes. For the case where two prefixes are present we must print
+   * them in a corrent order.  First print prefixes from begin[0], then handle
+   * prefixes from begin[1] (at this point we don't support more then two
+   * prefixes).
+   */
   if (instruction->prefix.lock && (begin[0] == 0xf0))
     print_name("lock ");
   if (instruction->prefix.repnz && (begin[0] == 0xf2))
     print_name("repnz ");
   if (instruction->prefix.repz && (begin[0] == 0xf3)) {
     /*
      * This prefix is "rep" for "ins", "lods", "movs", "outs", "stos". For other
      * instructions print "repz".
      */
     if (IsNameInList(instruction_name,
                      "ins", "lods", "movs", "outs", "stos", NULL))
       print_name("rep ");
     else
       print_name("repz ");
   }
-
   if (((data16_prefix) && (rex_prefix & 0x08)) &&
       !IsNameInList(instruction_name,
                     "bsf", "bsr", "fldenvs", "fnstenvs", "fnsaves", "frstors",
                     "fsaves", "fstenvs", "movbe", NULL)) {
     if ((end - begin) != 3 ||
         (begin[0] != 0x66) || ((begin[1] & 0x48) != 0x48) || (begin[2] != 0x90))
       print_name("data32 ");
   }
-
   if (instruction->prefix.lock && (begin[0] != 0xf0)) {
     print_name("lock ");
   }
   if (instruction->prefix.repnz && (begin[0] != 0xf2)) {
     print_name("repnz ");
   }
   if (instruction->prefix.repz && (begin[0] != 0xf3)) {
     /*
      * This prefix is "rep" for "ins", "lods", "movs", "outs", "stos". For other
      * instructions print "repz".
      */
     if (IsNameInList(instruction_name,
                      "ins", "lods", "movs", "outs", "stos", NULL))
       print_name("rep ");
     else
       print_name("repz ");
   }
 
+  /*
+   * REX prefix.  Empty REX prefix (without REX.B, REX.X, REX.R, or REX.W bits)
+   * is not always spurious: it can be used to select between 8-bit registers
+   * (%ah vs %spl, %ch vs %bpl, %dh vs %dil, and %bh vs %sil).
+   */
   if (rex_prefix == 0x40) {
     if (operands_count > 0)
       for (i=0; i<operands_count; ++i)
         /*
          * "Empty" rex prefix (0x40) is used to select "sil"/"dil"/"spl"/"bpl".
          */
         if (instruction->operands[i].type == OPERAND_TYPE_8_BIT &&
             instruction->operands[i].name <= REG_RDI) {
           empty_rex_prefix_ok = TRUE;
         }
     if (!empty_rex_prefix_ok)
       print_name("rex ");
+   /*
+    * Non-empty REX prefix is shown if and only if if it's spurious.

[Brad Chen, 2012/10/22 21:29:05]

if if

+    */
   } else if (spurious_rex_prefix) {
     print_name("rex.");
     if (rex_prefix & 0x08) {
       print_name("W");
     }
     if (rex_prefix & 0x04) {
       print_name("R");
     }
     if (rex_prefix & 0x02) {
       print_name("X");
     }
     if (rex_prefix & 0x01) {
       print_name("B");
     }
     print_name(" ");
   }
 
+  /* Prefixes are printed.  Print the instruction name.  */
   printf("%s", instruction_name);
   shown_name += strlen(instruction_name);
 
+  /* In some cases AT&T instruction uses suffix to show the size of operand.  */

[Brad Chen, 2012/10/22 21:29:05]

It looks to me like it may be the case that the structure of this procedure is a
sequence of handlers for cases of work-arounds for unexpected behavior from
objdump. I would be very happy if I could look at a one page procedure and that
this structure would be obvious.

In the end, this is just one test, and probably not worth stopping the entire
train. So if it truly is more than you can deal with right now then you could
propose something like filing a bug and adding a TODO() comment. More important
to me than the actual changes is that you have understood and internalized the
how such changes make the system better.

   if (instruction->att_instruction_suffix) {
+    /* But special versions of "nop" never use these suffixes.  */
     if (!IsNameInList(instruction_name,
                       "nopw   0x0(%eax,%eax,1)",
                       "nopw   0x0(%rax,%rax,1)",
                       NULL)) {
       printf("%s", instruction->att_instruction_suffix);
       shown_name += strlen(instruction->att_instruction_suffix);
     }
   } else {
     /*
      * Objdump prints size suffixes for "crc32" instruction even when it's
      * not really needed.
      */
     if (strcmp(instruction_name, "crc32") == 0) {
       if (instruction->operands[1].type == OPERAND_TYPE_8_BIT)
         print_name("b");
       else if (instruction->operands[1].type == OPERAND_TYPE_16_BIT)
         print_name("w");
       else if (instruction->operands[1].type == OPERAND_TYPE_32_BIT)
         print_name("l");
       else if (instruction->operands[1].type == OPERAND_TYPE_64_BIT)
         print_name("q");
     }
   }
+
+  /* Regular "mov" with 64-bit immediate is printed as "movabs" by objdump.  */
   if (strcmp(instruction_name, "mov") == 0 &&
       instruction->operands[1].name == REG_IMM &&
       instruction->operands[1].type == OPERAND_TYPE_64_BIT)
     print_name("abs");
 
+  /*
+   * Jump instructions can use branch-prediction prefixes.  They are shown as
+   * suffixes by objdump.
+   */
   if (IsNameInList(instruction_name,
                    "ja", "jae", "jbe", "jb", "je", "jg", "jge", "jle",
                    "jl", "jne", "jno", "jnp", "jns", "jo", "jp", "js",
                    "jecxz", "jrcxz", "loop", "loope", "loopne", NULL)) {
     if (instruction->prefix.branch_not_taken)
       print_name(",pn");
     else if (instruction->prefix.branch_taken)
       print_name(",pt");
   }
+#undef print_name
 
-#undef print_name
+  /* Objdump does not print spaces after some "special" instructions.  */
   if ((strcmp(instruction_name, "nop") != 0 || operands_count != 0) &&
       !IsNameInList(
         instruction_name,
         "fwait",
         "nopw   0x0(%eax,%eax,1)",
         "nopw   0x0(%rax,%rax,1)",
         "nopw %cs:0x0(%eax,%eax,1)",
         "nopw %cs:0x0(%rax,%rax,1)",
         "nopw   %cs:0x0(%eax,%eax,1)",
         "nopw   %cs:0x0(%rax,%rax,1)",
         "data32 nopw %cs:0x0(%eax,%eax,1)",
         "data32 nopw %cs:0x0(%rax,%rax,1)",
         "data32 data32 nopw %cs:0x0(%eax,%eax,1)",
         "data32 data32 nopw %cs:0x0(%rax,%rax,1)",
         "data32 data32 data32 nopw %cs:0x0(%eax,%eax,1)",
         "data32 data32 data32 nopw %cs:0x0(%rax,%rax,1)",
         "data32 data32 data32 data32 nopw %cs:0x0(%eax,%eax,1)",
         "data32 data32 data32 data32 nopw %cs:0x0(%rax,%rax,1)",
         "data32 data32 data32 data32 data32 nopw %cs:0x0(%eax,%eax,1)",
         "data32 data32 data32 data32 data32 nopw %cs:0x0(%rax,%rax,1)",
         NULL)) {
+    /*
+     * In this is "regular" instruction with a short name then it's operands are
+     * aligned.
+     */
     while (shown_name < 6) {
       printf(" ");
       ++shown_name;
     }
     if (operands_count == 0)
       printf(" ");
   }
+
+  /* Print instruction operands.  */
   for (i=operands_count-1; i>=0; --i) {
     printf("%c", delimeter);
+    /* If the instruction is "call" or "jmp" then we need to print asterics.  */
     if (IsNameInList(instruction_name, "call", "jmp", "lcall", "ljmp", NULL) &&
         instruction->operands[i].name != JMP_TO)
       printf("*");
     /*
      * Both AMD manual and Intel manual agree that mov from general purpose
      * register to segment register has signature "mov Ew Sw", but objdump
      * insists on 32bit/64 bit.  This is clearly an error in objdump so we fix
      * it here and not in decoder.
      */
     if ((begin[0] >= 0x48) && (begin[0] <= 0x4f) && (begin[1] == 0x8e) &&
         (instruction->operands[i].type == OPERAND_TYPE_16_BIT)) {
       operand_type = OPERAND_TYPE_64_BIT;
     } else if (((begin[0] == 0x8e) ||
        ((begin[0] >= 0x40) && (begin[0] <= 0x4f) && (begin[1] == 0x8e))) &&
         (instruction->operands[i].type == OPERAND_TYPE_16_BIT)) {
       operand_type = OPERAND_TYPE_32_BIT;
     } else {
       operand_type = instruction->operands[i].type;
     }
+
+    /* All the special cases are handled, let's print the normal operands!  */
     switch (instruction->operands[i].name) {
       case REG_RAX:
       case REG_RCX:
       case REG_RDX:
       case REG_RBX:
       case REG_RSP:
       case REG_RBP:
       case REG_RSI:
       case REG_RDI:
       case  REG_R8:
@@ skipping 12 matching lines @@
         printf("%%st");
         break;
       case REG_RM:
         if (instruction->rm.disp_type != DISPNONE) {
           if ((instruction->rm.disp_type == DISP64) ||
               (instruction->rm.offset >= 0))
             printf("0x%"NACL_PRIx64, instruction->rm.offset);
           else
             printf("-0x%"NACL_PRIx64, -instruction->rm.offset);
         }
-        if (((struct DecodeState *)userdata)->ia32_mode) {
+        if (userdata->ia32_mode) {
           if ((rm_base != NO_REG) ||
               (rm_index != NO_REG) ||
               (instruction->rm.scale != 0))
             printf("(");
           if (rm_base != NO_REG)
             printf("%%%s",
                    RegisterNameAsString(rm_base, OPERAND_TYPE_32_BIT, FALSE));
           if (rm_index == REG_RIZ) {
             if ((rm_base != REG_RSP) || (instruction->rm.scale != 0))
               printf(",%%eiz,%d", 1 << instruction->rm.scale);
@@ skipping 38 matching lines @@
       case REG_IMM:
         printf("$0x%"NACL_PRIx64,instruction->imm[0]);
         break;
       case REG_IMM2:
         printf("$0x%"NACL_PRIx64,instruction->imm[1]);
         break;
       case REG_PORT_DX:
         printf("(%%dx)");
         break;
       case REG_DS_RBX:
-        if (((struct DecodeState *)userdata)->ia32_mode)
+        if (userdata->ia32_mode)
           printf("%%ds:(%%ebx)");
         else
           printf("%%ds:(%%rbx)");
         break;
       case REG_ES_RDI:
-        if (((struct DecodeState *)userdata)->ia32_mode)
+        if (userdata->ia32_mode)
           printf("%%es:(%%edi)");
         else
           printf("%%es:(%%rdi)");
         break;
       case REG_DS_RSI:
-        if (((struct DecodeState *)userdata)->ia32_mode)
+        if (userdata->ia32_mode)
           printf("%%ds:(%%esi)");
         else
           printf("%%ds:(%%rsi)");
         break;
       case JMP_TO:
         if (instruction->operands[0].type == OPERAND_TYPE_16_BIT)
           printf("0x%lx", (long)((end + instruction->rm.offset -
-                         (((struct DecodeState *)userdata)->offset)) & 0xffff));
+                         (userdata->offset)) & 0xffff));
         else
           printf("0x%lx", (long)(end + instruction->rm.offset -
-                                   (((struct DecodeState *)userdata)->offset)));
+                                   (userdata->offset)));
         break;
       case REG_RIP:
       case REG_RIZ:
       case NO_REG:
         assert(FALSE);
     }
     delimeter = ',';
   }
+
+  /*
+   * If %rip was used then objdump will show the actual address as the comment.
+   */
   if (print_rip) {
     printf("        # 0x%8"NACL_PRIx64,
            (uint64_t) (end + instruction->rm.offset -
-               (((struct DecodeState *)userdata)->offset)));
+               (userdata->offset)));
   }
+
+  /* First line of instruction decoding is printed.  Finish the line.  */
   printf("\n");
   begin += 7;
+  /* if there are more then seven bytes we need to print the rest.  */
   while (begin < end) {
-    printf("%*"NACL_PRIx64":\t", ((struct DecodeState *)userdata)->width,
-           (uint64_t) (begin - (((struct DecodeState *)userdata)->offset)));
+    printf("%*"NACL_PRIx64":\t", userdata->width,
+           (uint64_t) (begin - (userdata->offset)));
     for (p = begin; p < begin + 7; ++p) {
       if (p >= end) {
         printf("\n");
         return;
       } else {
         printf("%02x ", *p);
       }
     }
     printf("\n");
     if (p >= end)
       return;
     begin += 7;
   }
 }
 
 void ProcessError (const uint8_t *ptr, void *userdata) {
   printf("rejected at %"NACL_PRIx64" (byte 0x%02"NACL_PRIx32")\n",
          (uint64_t) (ptr - (((struct DecodeState *)userdata)->offset)),
          *ptr);
 }
 
 int DecodeFile(const char *filename, int repeat_count) {

[Brad Chen, 2012/10/22 21:29:05]

The two if statements that follow the calls to CheckBounds() strike me as
opportunities for procedural abstraction.

   size_t data_size;
   uint8_t *data;
   int count;
 
   ReadImage(filename, &data, &data_size);
   if (data[4] == 1) {
     for (count = 0; count < repeat_count; ++count) {
       Elf32_Ehdr *header;
       int index;
 
@@ skipping 90 matching lines @@
   return 0;
 }
 
 int main(int argc, char **argv) {
   int index, initial_index = 1, repeat_count = 1;
   if (argc == 1) {
     printf("%s: no input files\n", argv[0]);
     exit(1);
   }
   if (!strcmp(argv[1], "--repeat"))
     repeat_count = atoi(argv[2]),

[Dmitry Polukhin, 2012/11/02 17:31:06]

Please don't use comma to separate statements. This economy doesn't give us
anything.

     initial_index += 2;
   for (index = initial_index; index < argc; ++index) {
     const char *filename = argv[index];
     int rc = DecodeFile(filename, repeat_count);
     if (!rc) {
       printf("file '%s' can not be fully decoded\n", filename);
       return 1;
     }
   }
   return 0;
 }