Refactor the process of choosing validators.

src/trusted/validator/ncvalidate.h

Index: src/trusted/validator/ncvalidate.h
diff --git a/src/trusted/validator/ncvalidate.h b/src/trusted/validator/ncvalidate.h
index a46e6960814f49ed0fbd0036ab1a21303d1fc7bf..e3ba24a744d7a002e1c2ef65d2fe9aa050c94c9c 100644
--- a/src/trusted/validator/ncvalidate.h
+++ b/src/trusted/validator/ncvalidate.h
@@ -7,29 +7,10 @@
 #ifndef NATIVE_CLIENT_SRC_TRUSTED_VALIDATOR_X86_NCVALIDATE_H__
 #define NATIVE_CLIENT_SRC_TRUSTED_VALIDATOR_X86_NCVALIDATE_H__
 
-/* Defines the API to the native client validation code.
- *
- * Note that the API is designed to use the NACL_SUBARCH_NAME
- * macro. This macro encapsulates the target architecture the
- * code is being compiled for into the name of the function.
- * This lets the API call different functions based on the
- * target architecture the code is being compiled for. It also allows
- * the maintainers of the compiled libraries to change the implementation
- * without having to update the API callers.
- *
- * The macro NACL_SUBARCH_NAME(name, arch, subarch) prepends "NaCl_"
- * to the function name, and suffixes it with the architecture and
- * corresponding subarchitecture (with underscores separating it).
- * Hence, for example, NACL_SUBARCH_NAME(ApplyValidator, x86, 64)
- * would generate the function name NaCL_ApplyValidator_x86_64.
- */
-/* TODO(bradchen): Cleanup the interface to the validator. There
- * are too many combinations of different calls and input parameters,
- * making this interface hard to understand and hard to modify.
- */
+/* Defines the API exposed by the Native Client validators. */
 
 #include "native_client/src/include/nacl_base.h"
-#include "native_client/src/shared/utils/types.h"
+#include "native_client/src/include/portability.h"
 #include "native_client/src/trusted/validator/cpufeatures.h"
 
 EXTERN_C_BEGIN
@@ -52,11 +33,10 @@ typedef enum NaClValidationStatus {
   NaClValidationFailedSegmentationIssue
 } NaClValidationStatus;
 
-/* Applies the validator, as defined by sel_ldr. That is, run the
- * validator where performance is critical.
+/* Function type for applying a Validator, as defined by sel_ldr. That is, run

[Mark Seaborn, 2012/06/20 20:02:26]

Nit: "validator" should be lower case?  Unless you mean to say "...for applying
a NaClValidatorInterface"?

[pasko-google - do not use, 2012/06/21 15:24:18]

Done.

+ * the validator where performance is critical.
+ *
  * Parameters are:
- *    local_cpu: True if local cpu rules should be applied.
- *           Otherwise, assume no cpu specific rules.
  *    guest_addr - The virtual pc to assume with the beginning address of the
  *           code segment. Typically, this is the corresponding addresss that
  *           will be used by objdump.
@@ -75,58 +55,54 @@ typedef enum NaClValidationStatus {
  *    cpu_features - The CPU features to support while validating.
  *    cache - Pointer to NaCl validation cache.
  */
-extern NaClValidationStatus NACL_SUBARCH_NAME(ApplyValidator,
-                                              NACL_TARGET_ARCH,
-                                              NACL_TARGET_SUBARCH)(
-    uintptr_t               guest_addr,
-    uint8_t                 *data,
-    size_t                  size,
-    int                     stubout_mode,
-    int                     readonly_text,
-    const NaClCPUFeatures   *cpu_features,
+typedef NaClValidationStatus (*NaClValidateFunc) (

[Mark Seaborn, 2012/06/20 20:02:26]

Nit: there should be no space in "(*NaClValidateFunc)(".  Please fix instances
of this below too.

[pasko-google - do not use, 2012/06/21 15:24:18]

Done.

+    uintptr_t guest_addr,
+    uint8_t *data,
+    size_t size,
+    int stubout_mode,
+    int readonly_text,
+    const NaClCPUFeatures *cpu_features,
     struct NaClValidationCache *cache);
 
-/* Applies the DFA-based validator as in the ApplyValidator case described
- * above.  The interface of this new validator must remain the same as of the
- * ApplyValidator.  Less arguments will be ignored as the new validator
- * implements features, such as stubout mode.
+/* Function type to copy instruction safely. Returns non-zero on success.

[Mark Seaborn, 2012/06/20 20:02:26]

"an instruction"

[pasko-google - do not use, 2012/06/21 15:24:18]

Done.

+ * Exposed by the Service Runtime.

[Mark Seaborn, 2012/06/20 20:02:26]

"exposed" -> "implemented"?

[pasko-google - do not use, 2012/06/21 15:24:18]

Okay, should sound better indeed. Done.

  */
-extern NaClValidationStatus NACL_SUBARCH_NAME(ApplyDfaValidator,
-                                              NACL_TARGET_ARCH,
-                                              NACL_TARGET_SUBARCH)(
-    uintptr_t               guest_addr,
-    uint8_t                 *data,
-    size_t                  size,
-    int                     stubout_mode,
-    int                     readonly_text,
-    const NaClCPUFeatures   *cpu_features,
-    struct NaClValidationCache *cache);
+typedef int (*NaClCopyInstructionFunc) (
+    uint8_t *dst,
+    uint8_t *src,
+    uint8_t sz);
 
-/* Applies the validator, as used in a command-line tool to report issues.
- * Note: This is intentionally separated from ApplyValidator, since it need
- * not be performance critical.
+/* Function type for applying a Validator to copy code from an existing code

[Mark Seaborn, 2012/06/20 20:02:26]

Ditto about "Validator".  Same below too...

[pasko-google - do not use, 2012/06/21 15:24:18]

Done.

+ * segment to a new code segment.
+ *
+ * Note: Current implementations use the instruction decoders, which may
+ * require that the code segment match native client rules.

[Mark Seaborn, 2012/06/20 20:02:26]

Capitalise: "Native Client"

[pasko-google - do not use, 2012/06/21 15:24:18]

Done. Added "the".

+ *
  * Parameters are:
- *    local_cpu: True if local cpu rules should be applied.
- *           Otherwise, assume no cpu specific rules.
  *    guest_addr - The virtual pc to assume with the beginning address of the
  *           code segment. Typically, this is the corresponding addresss that
  *           will be used by objdump.
- *    data - The contents of the code segment to be validated.
- *    size - The size of the code segment to be validated.
+ *    data_old - The contents of the original code segment.
+ *    data_new - The addres of the new code segment for which the original
+ *           code segment should be copied into.
+ *    size - The size of the passed code segments.
  *    cpu_features - The CPU features to support while validating.
+ *    copy_func - Function to perform copying with.
  */
-extern NaClValidationStatus NACL_SUBARCH_NAME(ApplyValidatorVerbosely,
-                                              NACL_TARGET_ARCH,
-                                              NACL_TARGET_SUBARCH)(
-    uintptr_t               guest_addr,
-    uint8_t                 *data,
-    size_t                  size,
-    const NaClCPUFeatures   *cpu_features);
-
-/* Applies validator to new code segment, assuming that it was updated
- * from the previously validated old code segment. Assumes that instruction
- * sizes are the same. Only allows changes in branches that don't change
- * instruction sizes.
+typedef NaClValidationStatus (*NaClCopyCodeFunc) (
+    uintptr_t guest_addr,
+    uint8_t *data_old,
+    uint8_t *data_new,
+    size_t size,
+    const NaClCPUFeatures *cpu_features,
+    NaClCopyInstructionFunc copy_func);
+
+/* Function type for applying a Validator on small updates to previously
+ * validated code segments.
+ *
+ * Assumes that instruction sizes are the same. Only allows changes in branches
+ * that don't change instruction sizes.
+ *
  * Parameters are:
  *    guest_addr - The virtual pc to assume with the beginning address of the
  *           code segment. Typically, this is the corresponding addresss that
@@ -136,38 +112,48 @@ extern NaClValidationStatus NACL_SUBARCH_NAME(ApplyValidatorVerbosely,
  *    size - The size of the passed code segments.
  *    cpu_features - The CPU features to support while validating.
  */
-extern NaClValidationStatus NACL_SUBARCH_NAME(ApplyValidatorCodeReplacement,
-                                              NACL_TARGET_ARCH,
-                                              NACL_TARGET_SUBARCH)(
-      uintptr_t             guest_addr,
-      uint8_t               *data_old,
-      uint8_t               *data_new,
-      size_t                size,
-      const NaClCPUFeatures *cpu_features);
-
-/* Runs the validator to copy code from an existing code segment to a new
- * code segment.
- *
- * Note: Current implementations uses the instruction decoders, which may
- * require that the code segment match native client rules.
+typedef NaClValidationStatus (*NaClValidateCodeReplacementFunc) (
+    uintptr_t guest_addr,
+    uint8_t *data_old,
+    uint8_t *data_new,
+    size_t size,
+    const NaClCPUFeatures *cpu_features);
+
+/* The full set of validator API. */

[Mark Seaborn, 2012/06/20 20:02:26]

Should be "APIs" plural?

[pasko-google - do not use, 2012/06/21 15:24:18]

hm, probably, I am not aware of the boundary between API and APIs. Is one
function an API and two functions already APIs?

Anyway, I do not feel strongly about any of the variants (probably I should
be?:), so I am taking what you propose.

Done.

+struct NaClValidatorInterface {
+  NaClValidateFunc Validate;
+  NaClCopyCodeFunc CopyCode;
+  NaClValidateCodeReplacementFunc ValidateCodeReplacement;
+};
+
+/* Make a choice of validating functions. */
+const struct NaClValidatorInterface* NaClCreateValidator();

[Mark Seaborn, 2012/06/20 20:02:26]

"*" spacing style: please use " *", as you've done previously in this fine.  Fix
cases below too...

[pasko-google - do not use, 2012/06/21 15:24:18]

Done. Thanks for reminder.

+
+/* Known Validator API initializers. Private. Do not use outside validator. */
+const struct NaClValidatorInterface* NaClValidatorCreate_x86_64();
+const struct NaClValidatorInterface* NaClValidatorCreate_x86_32();
+const struct NaClValidatorInterface* NaClDfaValidatorCreate_x86_32();
+const struct NaClValidatorInterface* NaClDfaValidatorCreate_x86_64();
+const struct NaClValidatorInterface* NaClValidatorCreateArm();
+
+/* Applies the validator, as used in a command-line tool to report issues.
+ * Note: This is intentionally separated from ApplyValidator, since it need
+ * not be performance critical.
  *
  * Parameters are:
  *    guest_addr - The virtual pc to assume with the beginning address of the
  *           code segment. Typically, this is the corresponding addresss that
  *           will be used by objdump.
- *    data_old - The contents of the original code segment.
- *    data_new - The addres of the new code segment for which the original
- *           code segment should be copied into.
- *    size - The size of the passed code segments.
+ *    data - The contents of the code segment to be validated.
+ *    size - The size of the code segment to be validated.
  *    cpu_features - The CPU features to support while validating.
  */
-extern NaClValidationStatus NACL_SUBARCH_NAME(ApplyValidatorCopy,
-                                              NACL_TARGET_ARCH,
-                                              NACL_TARGET_SUBARCH)(
-    uintptr_t             guest_addr,
-    uint8_t               *data_old,
-    uint8_t               *data_new,
-    size_t                size,
+NaClValidationStatus NACL_SUBARCH_NAME(ApplyValidatorVerbosely,
+                                       NACL_TARGET_ARCH,
+                                       NACL_TARGET_SUBARCH)(
+    uintptr_t guest_addr,
+    uint8_t *data,
+    size_t size,
     const NaClCPUFeatures *cpu_features);
 
 EXTERN_C_END