Move validator_x86_XX.rl out of unreviewed.

src/trusted/validator_ragel/validator_x86_64.rl

 /*
  * 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 <errno.h>
 #include <stddef.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 
-#include "native_client/src/trusted/validator_ragel/unreviewed/validator_internal.h"
+#include "native_client/src/trusted/validator_ragel/validator_internal.h"
 
 %%{
   machine x86_64_validator;
   alphtype unsigned char;
   variable p current_position;
   variable pe end_of_bundle;
   variable eof end_of_bundle;
   variable cs current_state;
 
   include byte_machine "byte_machines.rl";
@@ skipping 186 matching lines @@
      @{
        instruction_start -= 7;
        if (RMFromModRM(instruction_start[2]) !=
                                             RMFromModRM(instruction_start[6]) ||
            RMFromModRM(instruction_start[2]) != RMFromModRM(*current_position))
          instruction_info_collected |= UNRECOGNIZED_INSTRUCTION;
        BitmapClearBit(valid_targets, (instruction_start - data) + 4);
        BitmapClearBit(valid_targets, (instruction_start - data) + 7);
        restricted_register = NO_REG;
      } |
 

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

It would be helpful to have a comment before every block of stuff saying what
the block of stuff is. For example, something like
# This block encodes call and jump instructions of the form:
#   41 83 e3 e0             and    $0xffffffe0,%r11d
#   4d 01 fb                add    %r15,%r11
#   41 ff e3                jmpq   *%r11

      # rex.R?X? and $~0x1f, %eax/%ecx/%edx/%ebx/%esp/%ebp/%esi/%edi
     ((REX_RX 0x83 (0xe0|0xe1|0xe2|0xe3|0xe4|0xe5|0xe6|0xe7) 0xe0
      # add %r15,%rax/%rcx/%rdx/%rbx/%rsp/%rbp/%rsi/%rdi

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

A comment stating explicitly this is computing the effective address of the
call/jump would be nice.

I don't find the lists of registers useful at all; I know what the x86 registers
are, so I'm not going to read through a list like this when I see it. Is there a
way to only write about the interesting parts? For example, are there registers
omitted from this list that I might not be expecting?

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

Well, there two groups in two different places:
%rax/%rcx/%rdx/%rbx/%rsp/%rbp/%rsi/%rdi (first eight registers)
%r8/%r9/%r10/%r11/%r12/%r13/%r14 (that is: %r15 is omitted).

Surprisingly enough you CAN use %rbp and %rsp with naclcall/nacljmp and we even
have such files in our tests (but not on CWS).

%r15 is absolutely off-limits.

       b_0100_10x1 0x03 (0xc7|0xcf|0xd7|0xdf|0xe7|0xef|0xf7|0xff)
      # callq %rax/%rcx/%rdx/%rbx/%rsp/%rbp/%rsi/%rdi
       ((REX_WRX? 0xff (0xd0|0xd1|0xd2|0xd3|0xd4|0xd5|0xd6|0xd7)) |
      # jmpq %rax/%rcx/%rdx/%rbx/%rsp/%rbp/%rsi/%rdi
       (REX_WRX? 0xff (0xe0|0xe1|0xe2|0xe3|0xe4|0xe5|0xe6|0xe7)))) |
 
      # and $~0x1f, %r8d/%r9d/%r10d/%r11d/%r12d/%r13d/%r14d
      (b_0100_0xx1 0x83 (0xe0|0xe1|0xe2|0xe3|0xe4|0xe5|0xe6) 0xe0
      # add %r15, %r8d/%r9d/%r10d/%r11d/%r12d/%r13d/%r14d
       b_0100_11x1 0x03 (0xc7|0xcf|0xd7|0xdf|0xe7|0xef|0xf7)
      # callq %r8/%r9/%r10/%r11/%r12/%r13/%r14
      ((b_0100_xxx1 0xff (0xd0|0xd1|0xd2|0xd3|0xd4|0xd5|0xd6)) |
      # jmpq %r8/%r9/%r10/%r11/%r12/%r13/%r14
       (b_0100_xxx1 0xff (0xe0|0xe1|0xe2|0xe3|0xe4|0xe5|0xe6)))))

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

The more repetitive the comments are, the less useful they will tend to be in
the context of this review. Part of what I need to understand is the interesting
differences in a case, so repetition in comments works against that.

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

Well, I need to somehow show that we are handling the same groups here (%r8 and
%rax are not the same even if they are encoded identically in the ModR/M).

[Brad Chen, 2012/09/28 23:21:02]

Thanks for the feedback. Just in case it's not clear, in general for every
comment I make I am looking for at least one change. (I will try to note
exceptions clearly.) These comments are not acceptable in their current form. If
it's not clear to you how to make things better then we should work through the
details together.

On 2012/09/28 20:26:26, khim wrote:

      @{
        instruction_start -= 7;
        if (RMFromModRM(instruction_start[2]) !=
                                            RegFromModRM(instruction_start[6]) ||
            RMFromModRM(instruction_start[2]) != RMFromModRM(*current_position))
          instruction_info_collected |= UNRECOGNIZED_INSTRUCTION;
        BitmapClearBit(valid_targets, (instruction_start - data) + 4);

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

Could you add a level of procedural indirection so that it is obvious that this
BitmapClearBit is actually removing a possible target from an instruction list?

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

Done.

        BitmapClearBit(valid_targets, (instruction_start - data) + 7);
        restricted_register = NO_REG;
      };
 
   # EMMS/SSE2/AVX instructions which have implicit %ds:(%rsi) operand
   # maskmovq %mmX,%mmY
   maskmovq =
       REX_WRXB? (0x0f 0xf7)
       @CPUFeature_EMMX modrm_registers;
   # maskmovdqu %xmmX, %xmmY
@@ skipping 277 matching lines @@
                                       user_callback, callback_data);
 
   /* We only use malloc for a large code sequences  */
   if (size > sizeof valid_targets_small) {
     free(jump_dests);
     free(valid_targets);
   }
   if (!result) errno = EINVAL;
   return result;
 }