Move validator_x86_XX.rl out of unreviewed.

src/trusted/validator_ragel/decoder_x86_32.rl

 /*
  * Copyright (c) 2011 The Native Client Authors. All rights reserved.

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

2012? Can't remember what the lawyers latest guidance is here.

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

Script accepts both 2011 and 2012. Fixed.

  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include <assert.h>

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

What is this file for? Does it get used when building the validator, or only for
testing the stand-alone decoder? Anything you want to me to anticipate in its
internal structure? Please add an appropriate header comment.

Since most Googlers don't speak Ragel it might also be a good idea to add links
to the ragel documentation and the design doc for the ragel validator.

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

This file is only used to test stand-alone decoder, but it's pretty simple and
short and I don't want to complicate already complex scons magic just to keep it
in unreviewed directory.

 #include <stddef.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 
 #include "native_client/src/shared/utils/types.h"
 #include "native_client/src/trusted/validator_ragel/unreviewed/decoder_internal.h"
 
 #include "native_client/src/trusted/validator_ragel/gen/decoder_x86_32_instruction_consts.h"
 
@@ skipping 31 matching lines @@
     "native_client/src/trusted/validator_ragel/unreviewed/parse_instruction.rl";
   include relative_fields_actions
     "native_client/src/trusted/validator_ragel/unreviewed/parse_instruction.rl";
   include relative_fields_parsing
     "native_client/src/trusted/validator_ragel/unreviewed/parse_instruction.rl";
   include cpuid_actions
     "native_client/src/trusted/validator_ragel/unreviewed/parse_instruction.rl";
 
   include decode_x86_32 "decoder_x86_32_instruction.rl";
 
   main := (one_instruction

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

This procedure would be more readable if you made each switch below into a
function, e.g. instruction.rm.offset = GetInstOffset(instruction, disp, ...).
Performance impact should be zero if it is static inline. Any reason not to make
this change?

     @{
         switch (instruction.rm.disp_type) {
           case DISPNONE: instruction.rm.offset = 0; break;

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

One statement per line? Nothing after the colon please except maybe '{'. Any
reason not to follow C++ coding style conventions in an action like this? BTW
long switch statements are one instance where I expect functions to sometimes be
longer than a page.

           case DISP8: instruction.rm.offset = (int8_t) *disp; break;
           case DISP16: instruction.rm.offset =

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

This is a horrible line break.

             (int16_t) (disp[0] + 256U * disp[1]);
             break;
           case DISP32: instruction.rm.offset = (int32_t)
             (disp[0] + 256U * (disp[1] + 256U * (disp[2] + 256U * (disp[3]))));
             break;
           case DISP64: assert(FALSE);

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

The style guide requires a default case. I might let it slide if you were
committed to building with the compiler flag that makes it a fatal error to not
omit cases from switch statements.

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

This is how our code is built.

         }
         switch (imm_operand) {
           case IMMNONE: instruction.imm[0] = 0; break;
           case IMM2: instruction.imm[0] = imm[0] & 0x03; break;
           case IMM8: instruction.imm[0] = imm[0]; break;
           case IMM16: instruction.imm[0] = (uint64_t) (*imm + 256U * (imm[1]));
             break;
           case IMM32: instruction.imm[0] = (uint64_t)
             (imm[0] + 256U * (imm[1] + 256U * (imm[2] + 256U * (imm[3]))));
             break;
@@ skipping 25 matching lines @@
     })*
     $!{ process_error(current_position, userdata);
         result = FALSE;
         goto error_detected;
     };
 
 }%%
 
 %% write data;
 
 #define GET_VEX_PREFIX3() vex_prefix3

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

It looks like this macro is defined identically in multiple files. Please use a
.h file and a single defn.

[khim, 2013/03/08 17:59:53]

Done.

 #define SET_VEX_PREFIX3(P) vex_prefix3 = (P)
 #define SET_DATA16_PREFIX(S) instruction.prefix.data16 = (S)
 #define SET_LOCK_PREFIX(S) instruction.prefix.lock = (S)
 #define SET_REPZ_PREFIX(S) instruction.prefix.repz = (S)
 #define SET_REPNZ_PREFIX(S) instruction.prefix.repnz = (S)
 #define SET_BRANCH_TAKEN(S) instruction.prefix.branch_taken = (S)
 #define SET_BRANCH_NOT_TAKEN(S) instruction.prefix.branch_not_taken = (S)
 #define SET_INSTRUCTION_NAME(N) instruction.name = (N)
 #define GET_OPERAND_NAME(N) instruction.operands[(N)].name
 #define SET_OPERAND_NAME(N, S) instruction.operands[(N)].name = (S)
@@ skipping 21 matching lines @@
   IMMNONE,
   IMM2,
   IMM8,
   IMM16,
   IMM32
 };
 
 int DecodeChunkIA32(const uint8_t *data, size_t size,
                     process_instruction_func process_instruction,
                     process_decoding_error_func process_error, void *userdata) {
   const uint8_t *current_position = data;

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

Looks like the latest style guide prescribes the 'type* varname' rather than
'type *varname'. This is also what I'm used to seeing in Chromium code. Any
reason not to conform here?

   const uint8_t *end_of_data = data + size;
   const uint8_t *disp = NULL;
   const uint8_t *imm = NULL;
   const uint8_t *imm2 = NULL;
   const uint8_t *instruction_start = current_position;
   uint8_t vex_prefix3 = 0x00;
   enum imm_mode imm_operand = IMMNONE;
   enum imm_mode imm2_operand = IMMNONE;
   struct instruction instruction;
   int result = TRUE;
 
   int current_state;
 
   /* Not used in ia32_mode.  */
   instruction.prefix.rex = 0;
 
   SET_DISP_TYPE(DISPNONE);
   SET_IMM_TYPE(IMMNONE);
   SET_IMM2_TYPE(IMMNONE);
   SET_DATA16_PREFIX(FALSE);
   SET_LOCK_PREFIX(FALSE);
   SET_REPNZ_PREFIX(FALSE);
   SET_REPZ_PREFIX(FALSE);
   SET_BRANCH_NOT_TAKEN(FALSE);
   SET_BRANCH_TAKEN(FALSE);
 
   %% write init;

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

Can you add comments to make it obvious what these will expand to? I see from
the gen files that the stuff above in main := (one_instruction ...) ends up in
here someplace, but it would be nice not to have to reverse engineer such
details.

Comments before each %% would also make it easier to distinguish between the
replacement for %% write init; and %% write exec;

Am I misunderstanding these? Even more of a reason for comments.

[khim, 2013/03/08 17:59:53]

Done.

   %% write exec;
 
 error_detected:
   return result;
 }