Move validator_x86_XX.rl out of unreviewed.

src/trusted/validator_ragel/validator_internal.h

Index: src/trusted/validator_ragel/validator_internal.h
===================================================================
--- src/trusted/validator_ragel/validator_internal.h	(revision 9911)
+++ src/trusted/validator_ragel/validator_internal.h	(working copy)
@@ -14,11 +14,11 @@
 
 #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/decoding.h"
+#include "native_client/src/trusted/validator_ragel/decoding.h"
 #include "native_client/src/trusted/validator_ragel/unreviewed/validator.h"
 
 /* Maximum set of R-DFA allowable CPUID features.  */
-extern const NaClCPUFeaturesX86 validator_cpuid_features;
+extern const NaClCPUFeaturesX86 kValidatorCPUIDFeatures;
 
 /* Macroses to suppport CPUID handling.  */
 #define SET_CPU_FEATURE(F) \
@@ -85,7 +85,7 @@
 #define CPUFeature_XOP      cpu_features->data[NaClCPUFeature_XOP]
 
 #define CPUFeature_3DNOW_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_3DNOW]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_3DNOW]
 /*
  * AMD documentation claims it's always available if CPUFeature_LM is present,
  * But Intel documentation does not even mention it!
@@ -94,91 +94,91 @@
 #define CPUFeature_3DPRFTCH_Allowed \
   CPUFeature_3DNOW_Allowed || CPUFeature_PRE_Allowed
 #define CPUFeature_AES_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_AES]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_AES]
 #define CPUFeature_AESAVX_Allowed \
   CPUFeature_AES_Allowed && CPUFeature_AVX_Allowed
 #define CPUFeature_AVX_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_AVX]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_AVX]
 #define CPUFeature_BMI1_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_BMI1]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_BMI1]
 #define CPUFeature_CLFLUSH_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_CLFLUSH]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_CLFLUSH]
 #define CPUFeature_CLMUL_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_CLMUL]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_CLMUL]
 #define CPUFeature_CLMULAVX_Allowed \
   CPUFeature_CLMUL_Allowed && CPUFeature_AVX_Allowed
 #define CPUFeature_CMOV_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_CMOV]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_CMOV]
 #define CPUFeature_CMOVx87_Allowed \
   CPUFeature_CMOV_Allowed && CPUFeature_x87_Allowed
 #define CPUFeature_CX16_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_CX16]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_CX16]
 #define CPUFeature_CX8_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_CX8]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_CX8]
 #define CPUFeature_E3DNOW_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_E3DNOW]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_E3DNOW]
 #define CPUFeature_EMMX_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_EMMX]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_EMMX]
 #define CPUFeature_EMMXSSE_Allowed \
   CPUFeature_EMMX_Allowed || CPUFeature_SSE_Allowed
 #define CPUFeature_F16C_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_F16C]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_F16C]
 #define CPUFeature_FMA_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_FMA]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_FMA]
 #define CPUFeature_FMA4_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_FMA4]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_FMA4]
 #define CPUFeature_FXSR_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_FXSR]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_FXSR]
 #define CPUFeature_LAHF_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_LAHF]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_LAHF]
 #define CPUFeature_LM_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_LM]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_LM]
 #define CPUFeature_LWP_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_LWP]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_LWP]
 /*
  * We allow lzcnt unconditionally
  * See http://code.google.com/p/nativeclient/issues/detail?id=2869
  */
 #define CPUFeature_LZCNT_Allowed TRUE
 #define CPUFeature_MMX_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_MMX]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_MMX]
 #define CPUFeature_MON_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_MON]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_MON]
 #define CPUFeature_MOVBE_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_MOVBE]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_MOVBE]
 #define CPUFeature_OSXSAVE_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_OSXSAVE]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_OSXSAVE]
 #define CPUFeature_POPCNT_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_POPCNT]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_POPCNT]
 #define CPUFeature_PRE_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_PRE]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_PRE]
 #define CPUFeature_SSE_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_SSE]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_SSE]
 #define CPUFeature_SSE2_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_SSE2]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_SSE2]
 #define CPUFeature_SSE3_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_SSE3]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_SSE3]
 #define CPUFeature_SSE41_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_SSE41]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_SSE41]
 #define CPUFeature_SSE42_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_SSE42]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_SSE42]
 #define CPUFeature_SSE4A_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_SSE4A]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_SSE4A]
 #define CPUFeature_SSSE3_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_SSSE3]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_SSSE3]
 #define CPUFeature_TBM_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_TBM]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_TBM]
 #define CPUFeature_TSC_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_TSC]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_TSC]
 /*
  * We allow tzcnt unconditionally
  * See http://code.google.com/p/nativeclient/issues/detail?id=2869
  */
 #define CPUFeature_TZCNT_Allowed TRUE
 #define CPUFeature_x87_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_x87]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_x87]
 #define CPUFeature_XOP_Allowed \
-  validator_cpuid_features.data[NaClCPUFeature_XOP]
+  kValidatorCPUIDFeatures.data[NaClCPUFeature_XOP]
 
 /* Remember some information about instruction for further processing.  */
 #define GET_REX_PREFIX() rex_prefix
@@ -213,13 +213,13 @@
 
 #define SET_OPERAND_NAME(N, S) operand_states |= ((S) << ((N) << 3))
 #define SET_OPERAND_TYPE(N, T) SET_OPERAND_TYPE_ ## T(N)
-#define SET_OPERAND_TYPE_OPERAND_SIZE_8_BIT(N) \
+#define SET_OPERAND_TYPE_OPERAND_TYPE_8_BIT(N) \
   operand_states |= OperandSandbox8bit << (5 + ((N) << 3))
-#define SET_OPERAND_TYPE_OPERAND_SIZE_16_BIT(N) \
+#define SET_OPERAND_TYPE_OPERAND_TYPE_16_BIT(N) \
   operand_states |= OperandSandboxUnrestricted << (5 + ((N) << 3))
-#define SET_OPERAND_TYPE_OPERAND_SIZE_32_BIT(N) \
+#define SET_OPERAND_TYPE_OPERAND_TYPE_32_BIT(N) \
   operand_states |= OperandSandboxRestricted << (5 + ((N) << 3))
-#define SET_OPERAND_TYPE_OPERAND_SIZE_64_BIT(N) \
+#define SET_OPERAND_TYPE_OPERAND_TYPE_64_BIT(N) \
   operand_states |= OperandSandboxUnrestricted << (5 + ((N) << 3))
 #define CHECK_OPERAND(N, S, T) \
   ((operand_states & (0xff << ((N) << 3))) == ((S | (T << 5)) << ((N) << 3)))
@@ -304,7 +304,8 @@
                ((((bitmap_word)1) << bits) - 1) << (index % NACL_HOST_WORDSIZE);
 }
 
-/* Mark the destination of a jump instruction and make an early validity check:
+/*
+ * Mark the destination of a jump instruction and make an early validity check:
  * to jump outside given code region, the target address must be aligned.
  *
  * Returns TRUE iff the jump passes the early validity check.
@@ -322,13 +323,28 @@
   return TRUE;
 }
 
+/*
+ * Mark the gived address as valid jump target address.

[Brad Chen, 2012/10/05 16:47:21]

"given"?

[khim, 2012/10/15 16:38:57]

Done.

+ */
+static FORCEINLINE void MakeJumpTargetValid(size_t address,
+                                            bitmap_word *valid_targets) {
+  BitmapSetBit(valid_targets, address);
+}
 
+/*
+ * Mark the gived address as invalid jump target address.

[Brad Chen, 2012/10/05 16:47:21]

"given"?

[khim, 2012/10/15 16:38:57]

Done.

+ */
+static FORCEINLINE void MakeInvalidJumpTarget(size_t address,
+                                              bitmap_word *valid_targets) {
+  BitmapClearBit(valid_targets, address);
+}
+
 static INLINE Bool ProcessInvalidJumpTargets(
     const uint8_t *data,
     size_t size,
     bitmap_word *valid_targets,
     bitmap_word *jump_dests,
-    validation_callback_func user_callback,
+    ValidationCallbackFunc user_callback,
     void *callback_data) {
   size_t elements = (size + NACL_HOST_WORDSIZE - 1) / NACL_HOST_WORDSIZE;
   size_t i, j;
@@ -393,8 +409,8 @@
 }
 
 static INLINE void check_access(ptrdiff_t instruction_start,
-                                enum register_name base,
-                                enum register_name index,
+                                enum OperandName base,
+                                enum OperandName index,
                                 uint8_t restricted_register,
                                 bitmap_word *valid_targets,
                                 uint32_t *instruction_info_collected) {
@@ -413,7 +429,7 @@
 }
 
 
-static INLINE void process_0_operands(enum register_name *restricted_register,
+static INLINE void process_0_operands(enum OperandName *restricted_register,
                                       uint32_t *instruction_info_collected) {
   /* Restricted %rsp or %rbp must be processed by appropriate nacl-special
    * instruction, not with regular instruction.  */
@@ -425,7 +441,7 @@
   *restricted_register = NO_REG;
 }
 
-static INLINE void process_1_operand(enum register_name *restricted_register,
+static INLINE void process_1_operand(enum OperandName *restricted_register,
                                      uint32_t *instruction_info_collected,
                                      uint8_t rex_prefix,
                                      uint32_t operand_states) {
@@ -453,7 +469,7 @@
 }
 
 static INLINE void process_1_operand_zero_extends(
-    enum register_name *restricted_register,
+    enum OperandName *restricted_register,
     uint32_t *instruction_info_collected, uint8_t rex_prefix,
     uint32_t operand_states) {
   /* Restricted %rsp or %rbp must be processed by appropriate nacl-special
@@ -481,7 +497,7 @@
   }
 }
 
-static INLINE void process_2_operands(enum register_name *restricted_register,
+static INLINE void process_2_operands(enum OperandName *restricted_register,
                                       uint32_t *instruction_info_collected,
                                       uint8_t rex_prefix,
                                       uint32_t operand_states) {
@@ -518,7 +534,7 @@
 }
 
 static INLINE void process_2_operands_zero_extends(

[Brad Chen, 2012/10/05 16:47:21]

All these non-standard procedure names need to be fixed.

[khim, 2012/10/15 16:38:57]

Done.

-     enum register_name *restricted_register,
+     enum OperandName *restricted_register,
      uint32_t *instruction_info_collected,
      uint8_t rex_prefix, uint32_t operand_states) {
   /* Restricted %rsp or %rbp must be processed by appropriate nacl-special
@@ -562,4 +578,49 @@
   }
 }
 
+/*
+ * This action redefines the range of the superinstruction to include the
+ * preceding sandboxing sequence then invalidates jump targets on the
+ * interior of the superinstructions and finally clears “the restricted
+ * register” variable.
+ */
+static INLINE void ProcessNaclCallOrNaclJmp(

[Brad Chen, 2012/10/05 16:47:21]

This is a big improvement. Now it's much easier to see at the call site what is
happening and where the same thing is happening in two places.

Just as with prose, please eliminate repetitive text that doesn't enhance
meaning, therefore:
  SandboxNaClCallOrJmp
  - drop second redundant nacl. "Sandbox" seems like a better description of
what this routine will do than "Process". Do you have a better idea?
  SandboxRxiSuperInst
  - Rxi easily understood as Rsi or Rdi. Also understood is that sandboxing a
superinst implies eliminating interior targets.

The boolean parameters in this and the next function are readability issues
especially at the call site. Please eliminate them and instead provide different
versions of these routines, i.e.
  ProcessNaClCallOrJmpNoRex(...)
  ProcessNaClCallOrJmpWithREX(...)
These new routines should call the existing version of the routine that takes
the boolean parameter.

[khim, 2012/10/15 16:38:57]

Done.

+    uint32_t *instruction_info_collected, const uint8_t **instruction_start,
+    const uint8_t *current_position, const uint8_t *data,
+    bitmap_word *valid_targets, Bool and_have_rex, Bool add_stores_to_reg) {
+  /* Expand the range.  Only expand by 6 bytes: this covers “add” (second
+   * instruction) and “and” WITHOUT it's REX prefix.
+   */
+  *instruction_start -= 6;
+  /* Compare registers used by “and”, “add”, and “call” or “jmp”.  */
+  if (RMFromModRM((*instruction_start)[1]) !=
+      (add_stores_to_reg ? RegFromModRM((*instruction_start)[5]) :
+                           RMFromModRM((*instruction_start)[5])) ||
+      RMFromModRM((*instruction_start)[1]) != RMFromModRM(current_position[0]))
+    *instruction_info_collected |= UNRECOGNIZED_INSTRUCTION;
+  /* Make start of the second and third instruction not valid targets. */
+  MakeInvalidJumpTarget((*instruction_start - data) + 3, valid_targets);
+  MakeInvalidJumpTarget((*instruction_start - data) + 6, valid_targets);
+  /* Expand the range again.  This time to cover REX prefix (if any).  */
+  if (and_have_rex) --*instruction_start;
+}
+
+/*
+ * This action redefines the range of the superinstruction to include the
+ * preceding sandboxing sequence then invalidates jump targets on the interior
+ * of the superinstruction.
+ */
+static INLINE void SandboxRsiOrRdiMakeInteriorInvalidJumpTargets(
+    const uint8_t **instruction_start, const uint8_t *data,
+    bitmap_word *valid_targets, Bool mov_have_rex) {
+  /* Expand the range.  Only expand by 6 bytes: this covers “lea” (second
+   * instruction) and “mov” WITHOUT it's REX prefix.
+   */
+  *instruction_start -= 6;
+  MakeInvalidJumpTarget((*instruction_start - data) + 2, valid_targets);
+  MakeInvalidJumpTarget((*instruction_start - data) + 6, valid_targets);
+  /* Expand the range again.  This time to cover REX prefix (if any).  */
+  if (mov_have_rex) --*instruction_start;
+}
+
 #endif  /* NATIVE_CLIENT_SRC_TRUSTED_VALIDATOR_RAGEL_VALIDATOR_INTERNAL_H_ */