Move validator_x86_XX.rl out of unreviewed.

src/trusted/validator_ragel/decoder.h

 /*
  * 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.
  */
 
 #ifndef NATIVE_CLIENT_SRC_TRUSTED_VALIDATOR_RAGEL_DECODER_H_
 #define NATIVE_CLIENT_SRC_TRUSTED_VALIDATOR_RAGEL_DECODER_H_
 
 #include "native_client/src/shared/utils/types.h"
 #include "native_client/src/trusted/validator/x86/nacl_cpuid.h"
 
 EXTERN_C_BEGIN
 
 enum operand_type {
 
   /*
    * 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. */

[Brad Chen, 2012/09/28 19:38:34]

Could you make it clear in the comment that it only applies to only the 2_BIT
enum? I initially thought it applies to all of the OPERAND_SIZE things and only
figured out otherwise looking at the internals doc.  

[khim, 2012/09/28 23:22:06]

Done.

   OPERAND_SIZE_8_BIT,
   OPERAND_SIZE_16_BIT,
   OPERAND_SIZE_32_BIT,
   OPERAND_SIZE_64_BIT,

[Brad Chen, 2012/09/28 19:38:34]

Are there other 64-bit operand_types below? In that case, does
OPERAND_SIZE_64_BIT apply or not? If an operand should always have exactly one
type from this list, then it would make sense I think to 

[khim, 2012/09/28 20:26:26]

Operand can only use one items from a list. But
GENERAL_PUSPOSE_REGISTER_OR_MEMORY_OPERAND_OF_64_BIT_SIZE looked too long.

[Brad Chen, 2012/09/28 20:41:01]

... then it would make sense to maybe use more of a common prefix,
OPERAND_TYPE_... just a nit.

On 2012/09/28 19:38:34, Brad Chen wrote:

[khim, 2012/09/28 23:22:06]

Done.

   OPERAND_SIZE_128_BIT,
   OPERAND_SIZE_256_BIT,
 
   /* 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_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_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

[Brad Chen, 2012/09/28 19:38:34]

For my gratification, are there other operand_sizes that need to be added for
AVX?

[khim, 2012/09/28 20:26:26]

There are some for AVX2. AVX is currently supported at the DFA level and
disabled at runtime via "hard CPUID ceiling".

This is done this way because AVX instructions support adds a lot of complexity
to the DFA and we DON'T want to suddenly find themselves with a
brand-spanking-new validator design which we want to throw away and redo from
scratch because we can not, e.g. compile it with MSVC 2010.

 };
 
 enum register_name {
   /* First 16 registers are compatible with encoding of registers in x86 ABI. */
   REG_RAX,
   REG_RCX,
   REG_RDX,
   REG_RBX,
   REG_RSP,
   REG_RBP,
   REG_RSI,
   REG_RDI,
   REG_R8,
   REG_R9,
   REG_R10,
   REG_R11,
   REG_R12,
   REG_R13,
   REG_R14,
   REG_R15,
   /* These are pseudo-registers used in special cases.                        */

[Brad Chen, 2012/09/28 20:41:01]

Q: Why are these better here rather than as a separate enum? Life would be
simpler for the reader if REG_ always meant 'register'.

[khim, 2012/09/28 23:22:06]

They are here to simplify the operations with operands. Operand can be one of
the few registers, it can be memory access operand or immediate, and it may be
some "special" operand like "%ds:(%rbx)" (in xlat).

I agree that register_name is misleading (and it's name does not adhere to the
style guide). Have any better suggestions?

   REG_RM,       /* Address in memory via rm field.                            */
   REG_RIP,      /* RIP - used as base in x86-64 mode.                         */
   REG_RIZ,      /* EIZ/RIZ - used as "always zero index" register.            */
   REG_IMM,      /* Fixed value in imm field.                                  */
   REG_IMM2,     /* Fixed value in second imm field.                           */
   REG_DS_RBX,   /* Fox xlat: %ds(%rbx).                                       */
   REG_ES_RDI,   /* For string instructions: %es:(%rsi).                       */
   REG_DS_RSI,   /* For string instructions: %ds:(%rdi).                       */
   REG_PORT_DX,  /* 16-bit DX: for in/out instructions.                        */
   NO_REG,       /* For modrm: both index and base can be absent.              */
   REG_ST,       /* For x87 instructions: implicit %st.                        */
   JMP_TO        /* Operand is jump target address: usually %rip+offset.       */
 };
 
 /*
  * This enum extends NaClCPUFeatureID to cover instructions not recognized in

[Brad Chen, 2012/09/28 20:41:01]

Something missing here?

[khim, 2012/09/28 23:22:06]

Yeah. CPUID support for decoder. Looks like we are not adding it any time soon.
Better to remove it for now.

  *
  * /
 enum DecoderCPUFeatures {
 };
 */
 
 enum disp_mode {

[Brad Chen, 2012/09/28 20:41:01]

Comment this enum please.

[khim, 2012/09/28 23:22:06]

Done.

   DISPNONE,
   DISP8,
   DISP16,
   DISP32,
   DISP64,
 };
 
 struct instruction {

[Brad Chen, 2012/09/28 20:41:01]

Every non-trival type really merits a comment. While it's obvious in this case
that this struct has fields representing an instruction, it's not obvious where
it will be used, and if not everywhere where it won't be used.

[khim, 2012/09/28 23:22:06]

Done.

   const char *name;
   unsigned char operands_count;
   struct {
     unsigned char rex;        /* Mostly to distingush cases like %ah vs %spl. */
 #ifdef _MSC_VER

[Brad Chen, 2012/09/28 20:41:01]

Yuck. I'm not liking the nearly-identical code here. Is there not a way to do
something like:
#ifndef _MSC_VER
#define Bool _Bool
#endif
That would get rid of the duplication.
Also, please comment any compiler-specific stuff. Don't expect people to know
these differences by heart.

[khim, 2012/09/28 23:22:06]

Unfortunately we already have Bool in NaCl codebase and it
can not be used in place of _Bool because it triggers
warnings when used with GCC.
This one defenitely merits the comment.

     Bool data16:1;            /* "Normal", non-rex prefixes. */
     Bool lock:1;
     Bool repnz:1;
     Bool repz:1;
     Bool branch_not_taken:1;
     Bool branch_taken:1;
 #else
     _Bool data16:1;           /* "Normal", non-rex prefixes. */
     _Bool lock:1;
     _Bool repnz:1;
@@ skipping 17 matching lines @@
 };
 
 typedef void (*process_instruction_func) (const uint8_t *begin,
                                           const uint8_t *end,
                                           struct instruction *instruction,
                                           void *userdata);
 
 typedef void (*process_decoding_error_func) (const uint8_t *ptr,
                                              void *userdata);
 
 /* All possible CPUID features enabled.  */

[Brad Chen, 2012/09/28 20:41:01]

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

A couple other style rules to observe here:
- Please use a prefix like "g_" for global variables.
- For global constants, use kCamelCase, e.g. kFullCpuidFeatures.

[khim, 2012/09/28 23:22:06]

Done.

 extern const NaClCPUFeaturesX86 full_cpuid_features;
 
 int DecodeChunkAMD64(const uint8_t *data, size_t size,
                      process_instruction_func process_instruction,
                      process_decoding_error_func 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);
 
 EXTERN_C_END
 
 #endif  /* NATIVE_CLIENT_SRC_TRUSTED_VALIDATOR_RAGEL_DECODER_H_ */