Move validator_x86_XX.rl out of unreviewed.

src/trusted/validator_ragel/decoder.h

Index: src/trusted/validator_ragel/decoder.h
===================================================================
--- src/trusted/validator_ragel/decoder.h	(revision 9911)
+++ src/trusted/validator_ragel/decoder.h	(working copy)
@@ -12,49 +12,50 @@
 
 EXTERN_C_BEGIN
 
-enum operand_type {
-
+enum OperandType {

[Brad Chen, 2012/10/04 17:26:04]

Thanks; these enum decls look better now.

   /*
    * These are for general-purpose registers, memory access and immediates.
    * They are not used for XMM, MMX etc.
   */
-  OPERAND_SIZE_2_BIT, /* See VPERMIL2Px instruction for description. */
-  OPERAND_SIZE_8_BIT,
-  OPERAND_SIZE_16_BIT,
-  OPERAND_SIZE_32_BIT,
-  OPERAND_SIZE_64_BIT,
-  OPERAND_SIZE_128_BIT,
-  OPERAND_SIZE_256_BIT,
+  /* See VPERMIL2Px instruction for description of 2-bit operand type. */
+  OPERAND_TYPE_2_BIT,
+  /* These are regular-sized operands: GP registers or memory. */
+  OPERAND_TYPE_8_BIT,
+  OPERAND_TYPE_16_BIT,
+  OPERAND_TYPE_32_BIT,
+  OPERAND_TYPE_64_BIT,
+  OPERAND_TYPE_128_BIT,
+  OPERAND_TYPE_256_BIT,
 
+  /* Non-GP registers.  */
+  OPERAND_TYPE_ST,               /* Any X87 register.                       */
+  OPERAND_TYPE_SEGMENT_REGISTER, /* Operand is segment register: %es … %gs. */
+  OPERAND_TYPE_CONTROL_REGISTER, /* Operand is control register: %crX.      */
+  OPERAND_TYPE_DEBUG_REGISTER,   /* Operand is debug register: %drX.        */
+  OPERAND_TYPE_MMX,
+  OPERAND_TYPE_XMM,
+  OPERAND_TYPE_YMM,
+
   /* OPERAND_FLOAT_SIZE_*_BIT are used for in-memory operands. */
-  OPERAND_FLOAT_SIZE_16_BIT,
-  OPERAND_FLOAT_SIZE_32_BIT,
-  OPERAND_FLOAT_SIZE_64_BIT,
-  OPERAND_FLOAT_SIZE_80_BIT,
+  OPERAND_TYPE_FLOAT_32_BIT,
+  OPERAND_TYPE_FLOAT_64_BIT,
+  OPERAND_TYPE_FLOAT_80_BIT,
 
-  /* OPERAND_X87_SIZE_64_BIT are signed integers in memory.*/
-  OPERAND_X87_SIZE_16_BIT,
-  OPERAND_X87_SIZE_32_BIT,
-  OPERAND_X87_SIZE_64_BIT,
+  /* OPERAND_X87_SIZE_*_BIT are signed integers in memory.*/
+  OPERAND_TYPE_X87_16_BIT,
+  OPERAND_TYPE_X87_32_BIT,
+  OPERAND_TYPE_X87_64_BIT,
 
-
-  OPERAND_X87_BCD,          /* 10-byte packed BCD value in memory.           */
-  OPERAND_X87_ENV,          /* A 14-byte or 28-byte x87 environment.         */
-  OPERAND_X87_STATE,        /* A 94-byte or 108-byte x87 state.              */
-  OPERAND_X87_MMX_MM_STATE, /* A 512-byte extended x87/MMX/XMM state.        */
-  OPERAND_SELECTOR,         /* Operand is 6/10 bytes selector in memory.     */
-  OPERAND_FAR_PTR,          /* Operand is 6/10 bytes far pointer in memory.  */
-
-  OPERAND_ST,               /* Any X87 register.                             */
-  OPERAND_SEGMENT_REGISTER, /* Operand is segment register: %{e,c,s,d,f,g}s. */
-  OPERAND_CONTROL_REGISTER, /* Operand is control register: %crX.            */
-  OPERAND_DEBUG_REGISTER,   /* Operand is debug register: %drX.              */
-  OPERAND_MMX,
-  OPERAND_XMM,
-  OPERAND_YMM
+  /* Miscellaneous structures in memory.  */
+  OPERAND_TYPE_X87_BCD,           /* 10-byte packed BCD value.                */
+  OPERAND_TYPE_X87_ENV,           /* A 14-byte or 28-byte x87 environment.    */
+  OPERAND_TYPE_X87_STATE,         /* A 94-byte or 108-byte x87 state.         */
+  OPERAND_TYPE_X87_MMX_XMM_STATE, /* A 512-byte extended x87/MMX/XMM state.   */
+  OPERAND_TYPE_SELECTOR,          /* Operand is 6/10 bytes selector.          */
+  OPERAND_TYPE_FAR_PTR            /* Operand is 6/10 bytes far pointer.       */
 };
 
-enum register_name {
+enum OperandName {
   /* First 16 registers are compatible with encoding of registers in x86 ABI. */
   REG_RAX,
   REG_RCX,
@@ -88,14 +89,13 @@
 };
 
 /*
- * This enum extends NaClCPUFeatureID to cover instructions not recognized in
- *
- * /
-enum DecoderCPUFeatures {
-};
-*/
-
-enum disp_mode {
+ * Displacement can be of four different sizes in x86 instruction set: nothing,
+ * 8-bit, 16-bit, 32-bit, and 64-bit.  These are traditionally threated slightly
+ * differently by decoders: 8-bit are usually printed as signed offset, while
+ * 32-bit (in ia32 mode) and 64-bit (in amd64 mode) are printed as unsigned
+ * offset.
+ */
+enum DisplacementMode {
   DISPNONE,
   DISP8,
   DISP16,
@@ -103,11 +103,21 @@
   DISP64,
 };
 
-struct instruction {
+/*
+ * Structure which encodes the instruction.  Used to pass the information about
+ * the instruction to process_instruction_func callback.
+ */
+struct Instruction {
   const char *name;
   unsigned char operands_count;
   struct {
     unsigned char rex;        /* Mostly to distingush cases like %ah vs %spl. */
+    /*
+     * Here the difference between compilers raises it's ugly head.  What we
+     * really want to have here is C99's _Bool. Unfortunately MSVC does not
+     * offer it.  We have Bool typedef which works fine on MSVC but sadly
+     * generates warnings when used with GCC.
+     */
 #ifdef _MSC_VER

[Brad Chen, 2012/10/04 17:26:04]

Can you try to find a way to get this ifdef out of the defn of "struct
Instruction"? Is there no way to use a typedef above, and then here using
"foo_Bool" instead of "Bool"? I don't like how every one of these fields is
declared twice.

[khim, 2012/10/05 08:22:53]

Done: this makes decoder slightly slower, but we don't care about speed of the
decoder since we only use it for tests.

     Bool data16:1;            /* "Normal", non-rex prefixes. */
     Bool lock:1;
@@ -125,37 +135,37 @@
 #endif
   } prefix;
   struct {
-    enum register_name name;
-    enum operand_type type;
+    enum OperandName name;
+    enum OperandType type;
   } operands[5];
   struct {
-    enum register_name base;
-    enum register_name index;
+    enum OperandName base;  /* Can be RAX ... R15, or NO_REG. */
+    enum OperandName index; /* Can be RAX ... R15, or RIP, RIZ, or NO_REG */
     int scale;
     int64_t offset;
-    enum disp_mode disp_type;
+    enum DisplacementMode disp_type;
   } rm;
   uint64_t imm[2];
 };
 
-typedef void (*process_instruction_func) (const uint8_t *begin,
-                                          const uint8_t *end,
-                                          struct instruction *instruction,
+typedef void (*ProcessInstructionFunc) (const uint8_t *begin,
+                                        const uint8_t *end,
+                                        struct Instruction *instruction,
+                                        void *userdata);
+
+typedef void (*ProcessDecodingErrorFunc) (const uint8_t *ptr,
                                           void *userdata);
 
-typedef void (*process_decoding_error_func) (const uint8_t *ptr,
-                                             void *userdata);
-
 /* All possible CPUID features enabled.  */

[Brad Chen, 2012/10/04 17:26:04]

This comment is too terse. Please mention full_cpuid_features in the comment so
it's more explicit to what it applies.

[khim, 2012/10/05 08:22:53]

Done.

-extern const NaClCPUFeaturesX86 full_cpuid_features;
+extern const NaClCPUFeaturesX86 kFullCPUIDFeatures;
 
 int DecodeChunkAMD64(const uint8_t *data, size_t size,
-                     process_instruction_func process_instruction,
-                     process_decoding_error_func process_error, void *userdata);
+                     ProcessInstructionFunc process_instruction,
+                     ProcessDecodingErrorFunc process_error, void *userdata);
 
 int DecodeChunkIA32(const uint8_t *data, size_t size,
-                    process_instruction_func process_instruction,
-                    process_decoding_error_func process_error, void *userdata);
+                    ProcessInstructionFunc process_instruction,
+                    ProcessDecodingErrorFunc process_error, void *userdata);
 
 EXTERN_C_END