Move validator_x86_XX.rl out of unreviewed.

src/trusted/validator_ragel/decoding.h

Index: src/trusted/validator_ragel/decoding.h
===================================================================
--- src/trusted/validator_ragel/decoding.h	(revision 9911)
+++ src/trusted/validator_ragel/decoding.h	(working copy)
@@ -12,7 +12,7 @@
 #ifndef NATIVE_CLIENT_SRC_TRUSTED_VALIDATOR_RAGEL_DECODING_H_
 #define NATIVE_CLIENT_SRC_TRUSTED_VALIDATOR_RAGEL_DECODING_H_
 
-#include "native_client/src/trusted/validator_ragel/unreviewed/decoder.h"
+#include "native_client/src/trusted/validator_ragel/decoder.h"
 
 #if NACL_WINDOWS
 # define FORCEINLINE __forceinline

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

This seems a bit heavy-handed. Why not let the compiler decide?

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

Mostly because MSVC consistently makes wrong decision: apparently when function
is large it stops inlining other functions into it even when said functions are
so tiny inlining actually reduces the final size of code!

Our *SAN (ASAN/MSAN/TSAN/etc) guys are constantly fighting with it, too - and so
far have found that __forceinline is the only tool that works.

GCC is better in that regard, but also sometimes decides not to inline these
functions.

FORCEINLINE is only used for tiny "accessor"-type functions, larger functions in
validator_internal.h are marked with just plain INLINE decoration.

@@ -84,6 +84,62 @@
   return is4 >> 4;
 }
 
+/*
+ * SignExtendXXBit is used to sign-extend XX-bit value to unsigned 64-bit value.
+ *
+ * To do that you need to pass unsigned value of smaller then 64-bit size
+ * to this function: it will be converted to signed value and then
+ * sign-extended to become 64-bit value.
+ *
+ * Smaller values can be obtained by restricting this value further (which is
+ * safe according to the C language specification: see 6.2.1.2 in C90 and
+ * 6.3.1.3.2 in C99 specification).
+ *
+ * Note that these operations are safe but slightly unusual: they come very

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

This comment is too long. Can you please stick to just the facts and eliminate
extra details and opinions? Sign extension is not that big a deal is it?

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

Yes it is. If someone will replace "int32_t" with "int" here then s/he'll turn
safe program into unsafe one: is it immediately obvious to you?

Sure, this code works today and noone have seen any problems with it, but is it
actually guaranteed? Once upon time handling of singed integer overflow was also
easy and simple - till compiler writers found that this triggers undefined
behaviour. We *don't* want such suprises in our TCB.

I've spent about a hour reading the documentation and even asked Dmitry about
this one line of code before we've found it that this code is supposed to work
*if* it's compileable at all.

As I've already said: the very same function may become incorrect in some
hyphothetoical compiler if I will replace "intN_t" with plain "int", for example
- because compiler may use not two's complement arythmetic, but ones'-complement
one!

Standard makes it very clear that C compiler may very well handle sign-extension
incorrectly - but such compilers are not supposed to have "intN_t" types (they
should use "int_leastN_t" and "int_fastN_t" types instead) which means that *if*
compiler can compile this code *then* it works. 

I think it's entirely appropriate to leave the trail for the future readers with
pointers to relevant pieces of the standard when such subtletities are involved.

+ * close to the edge of what “well-behaved C program is not supposed to do”,
+ * but they stay on the “safe” side of this boundary.  Specifically: this
+ * behavior triggers “implementation-defined behavior” (see 6.2.1.2 in C90
+ * specification and 6.3.1.3.3 in C99 specification) which sounds suspiciously
+ * similar to the dreaded “undefined behavior”, but in reality these two are
+ * quite different: any program which triggers “undefined behavior” is not a
+ * valid C program at all, but program which tirggers “implementation-defined
+ * behavior” is quite valid C program.  What this program actually *does*
+ * depends on the specification of a given C compiler: each particular
+ * implementation must decide for itself what it'll do in this particular case
+ * and *stick* *to* *it*.  If the implementation uses two's-complement negative
+ * numbers (and all the implementation which can compile this code *must*
+ * support two's-complement arythmetic—see 7.18.1.1 in C99 specification) then
+ * the easiest thing to do is to do what we need here—this is what all known
+ * compilers for all known platforms are actually doing.
+ */
+static FORCEINLINE uint64_t SignExtend8Bit(int8_t value) {
+  return value;
+}
+
+static FORCEINLINE uint64_t SignExtend16Bit(int16_t value) {
+  return value;
+}
+
+static FORCEINLINE uint64_t SignExtend32Bit(int32_t value) {
+  return value;
+}
+
+static FORCEINLINE uint64_t AnyFieldValue8bit(const uint8_t *start) {
+  return *start;
+}
+
+static FORCEINLINE uint64_t AnyFieldValue16bit(const uint8_t *start) {
+  return (start[0] + 256U * start[1]);
+}
+
+static FORCEINLINE uint64_t AnyFieldValue32bit(const uint8_t *start) {
+  return (start[0] + 256U * (start[1] + 256U * (start[2] + 256U * (start[3]))));
+}
+static FORCEINLINE uint64_t AnyFieldValue64bit(const uint8_t *start) {
+  return (*start + 256ULL * (start[1] + 256ULL * (start[2] + 256ULL *
+          (start[3] + 256ULL * (start[4] + 256ULL * (start[5] + 256ULL *
+           (start[6] + 256ULL * start[7])))))));
+}
 static const uint8_t index_registers[] = {
   /* Note how REG_RIZ falls out of the pattern. */
   REG_RAX, REG_RCX, REG_RDX, REG_RBX,