Refactor the process of choosing validators.

Refactor the process of choosing validators.

On startup make a choice of validator implementation: write the choice as
function pointers in NaClApp.  This opens a possibility to add more experimental
validators (such as MIPS) without having to add platform-specific stub
implementations.

BUG=http://code.google.com/p/nativeclient/issues/detail?id=2737
TEST=functionality must be left unchanged

Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=9009

[pasko-google - do not use, 2012-04-25 13:23:10]

I uploaded this in two parts:

1. patchset1 is identical to https://chromiumcodereview.appspot.com/10174016/
(since depending on it)

2. the actual change

I a not sure I placed the code to the best files for the purpose. Glad to hear
how I can help more with refactoring.

[Nick Bray, 2012-04-25 20:57:42]

Updated the BUG= to point to the issue I've been using to track the refactoring.
 Welcome to the club.

My comments are mostly about meshing what I've previously thought about  with
what you're doing - so there's a lot of mushy theory.  As I note below, the
seperating the refactoring from the standalone change would help the review
process significantly - a pain, but please do it.

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/service...
File src/trusted/service_runtime/arch/x86/service_runtime_x86.gyp (right):

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/service...
src/trusted/service_runtime/arch/x86/service_runtime_x86.gyp:41:
['nacl_standalone==0 and target_arch=="ia32"', {
I think you should split the "standalone" change from the bigger refactoring. 
It's not necessary to squish them together.  The standalone change should be
done ASAP, the refactoring we can polish until it shines.

I hope you're using git, it makes splitting CLs less of a pain in the ass. :)

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/service...
File src/trusted/service_runtime/sel_ldr.h (right):

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/service...
src/trusted/service_runtime/sel_ldr.h:105: typedef NaClValidationStatus
(*NaClValidateFunc) (
This is part of the validator interface, not part of sel_ldr.  My instinct is
that these function definitions (and the validation status enums) should be in
src/validator/ncvalidate.h.  The new interface will eventually replace the old,
no reason to add a new file.

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/service...
src/trusted/service_runtime/sel_ldr.h:398: NaClValidateFunc         
validate_func;
I was envisioning these functions in a separate (but embedded) structure that
also contained the bundle_size parameter.  (bundle_size can be dealt with later,
but it is fundamentally a property of the validation model) This would allow
each validator to declare a InitValidatorInterface(...) function to fill out
this structure.  Less coupling.

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/service...
File src/trusted/service_runtime/sel_validate_image.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/service...
src/trusted/service_runtime/sel_validate_image.c:30: static NaClValidationStatus
ValidatorCopyNotImplemented(
I think that each validator that doesn't implement a function should explicitly
provide these stubs itself.  sel_ldr selects a validator, and then the validator
fills out the function table - sel_ldr doesn't need to know the implemented /
unimplemented details.

You could have a .c file in src/trusted/validator containing these stubs, but
linking would be easier if each validator declared their own stub.  (It would
also be better documentation and provide a skeleton function to fill out?  I
realize this may seem to contradict my prior "no stub for ARM" advice, but that
was about adding a new entry point, not about stubbing out the existing entry
points.)

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/service...
src/trusted/service_runtime/sel_validate_image.c:59: nap->validate_func =
NACL_SUBARCH_NAME(ApplyValidator,
I'd reorganize this into mutually exclusive ifdefs that call validator-specific
functions to fill out the function table.  Figure that this is going to be
expanded to deal with x86-32 and x86-64 seperately?  In some ways it seems like
a step back, but if done right it should make what validator is being used
explicit.

I think (maybe not in this CL) that this function should be moved into seperate
file (in src/trusted/validator?) so that it can include header files for
different validators without poluting sel_ldr's namespace, etc.

#if !defined(__arm__)
#if defined(NACL_STANDALONE)
  if (getenv("NACL_DANGEROUS_USE_DFA_VALIDATOR") != NULL) {
    NACL_SUBARCH_NAME(InitDfaValidator,
          NACL_TARGET_ARCH, NACL_TARGET_SUBARCH)(&nap->validator);
  } else {
    NACL_SUBARCH_NAME(InitStandardValidator,
          NACL_TARGET_ARCH, NACL_TARGET_SUBARCH)(&nap->validator);
  }
#else
    NACL_SUBARCH_NAME(InitStandardValidator,
          NACL_TARGET_ARCH, NACL_TARGET_SUBARCH)(&nap->validator);
#endif
#else
    NACL_SUBARCH_NAME(InitStandardValidator,
          NACL_TARGET_ARCH, NACL_TARGET_SUBARCH)(&nap->validator);
#endif

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/validat...
File src/trusted/validator/ncvalidate.h (left):

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/validat...
src/trusted/validator/ncvalidate.h:39: /* Defines possible validation status
values. */
See previous comments, it may make sense to keep the enum here.

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/validat...
File src/trusted/validator_arm/ncvalidate.cc (left):

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/validat...
src/trusted/validator_arm/ncvalidate.cc:89: NaClValidationStatus
NACL_SUBARCH_NAME(ApplyValidatorCodeReplacement, arm, 32)
See previous comments, probably want to keep.

[Nick Bray, 2012-04-25 21:06:02]

Ah, sorry, I misunderstood what you were doing the the patchsets.  I assumed the
CL you were pointing me to was the CL to include ragel, not a new CL to do the
standalone change (I am still crawling though my mailbox and didn't look to
carefully.)  Forget I said anything about splitting the CL.  Me idiot.

If you're using git:

git cl upload a_different_branch will upload a diff between the branch you're
working on and the branch you specify.  You'll need to specify the other branch
every upload, however, or else if will automatically diff against git-svn.

[pasko-google - do not use, 2012-04-26 15:10:09]

On 2012/04/25 21:06:02, Nick Bray wrote:
> Ah, sorry, I misunderstood what you were doing the the patchsets.  I assumed
the
> CL you were pointing me to was the CL to include ragel, not a new CL to do the
> standalone change (I am still crawling though my mailbox and didn't look to
> carefully.)  Forget I said anything about splitting the CL.  Me idiot.
> 
> If you're using git:
> 
> git cl upload a_different_branch will upload a diff between the branch you're
> working on and the branch you specify.  You'll need to specify the other
branch
> every upload, however, or else if will automatically diff against git-svn.

Thanks! I tried various options and they did not work. Nice to know it's a mere
argument that sets the base.

[pasko-google - do not use, 2012-04-26 15:17:01]

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/service...
File src/trusted/service_runtime/sel_ldr.h (right):

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/service...
src/trusted/service_runtime/sel_ldr.h:105: typedef NaClValidationStatus
(*NaClValidateFunc) (
On 2012/04/25 20:57:42, Nick Bray wrote:
> This is part of the validator interface, not part of sel_ldr.  My instinct is
> that these function definitions (and the validation status enums) should be in
> src/validator/ncvalidate.h.  The new interface will eventually replace the
old,
> no reason to add a new file.

Done.

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/service...
src/trusted/service_runtime/sel_ldr.h:398: NaClValidateFunc         
validate_func;
On 2012/04/25 20:57:42, Nick Bray wrote:
> I was envisioning these functions in a separate (but embedded) structure that
> also contained the bundle_size parameter.  (bundle_size can be dealt with
later,
> but it is fundamentally a property of the validation model) This would allow
> each validator to declare a InitValidatorInterface(...) function to fill out
> this structure.  Less coupling.

well, I thought about this same possibility at first, but then on callsites:
nap->validate_func(...) is less clutter than:
nap->validator_interface->validate_func(...)
and then I'm not pleased

Less coupling is good when we have use cases for using components separately or
abstractions that are very natural (and named intuitively). struct
ValidatorInterface does not sound as a positive name for reader's intuition.

Generally I do not mind this decoupling, but for that please give advice on
naming the struct and fields.

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/service...
File src/trusted/service_runtime/sel_validate_image.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/service...
src/trusted/service_runtime/sel_validate_image.c:30: static NaClValidationStatus
ValidatorCopyNotImplemented(
On 2012/04/25 20:57:42, Nick Bray wrote:
> I think that each validator that doesn't implement a function should
explicitly
> provide these stubs itself.  sel_ldr selects a validator, and then the
validator
> fills out the function table - sel_ldr doesn't need to know the implemented /
> unimplemented details.

hm, we seem to agreed that validator selection is a tricky process involving a
lot of parameters: environment vars, platform (maybe OS?), NaClApp, etc. And it
is hard to regularize this process. So, if we leave this choice partially to
each of the "validators", then we risk separating this process into parts
non-trivially which would only increase complication.

Imagine the need to check for the same env var in many files identically. Or
having to pass many args to InitializeValidator(...) which would take more lines
than a simple choice in a single selection function.

If the function grows beyond 50 lines of code, I am more open to reconsidering
it's structure and separation. Decoupling too early is like premature
optimization.

> You could have a .c file in src/trusted/validator containing these stubs, but
> linking would be easier if each validator declared their own stub.  (It would
> also be better documentation and provide a skeleton function to fill out?  I
> realize this may seem to contradict my prior "no stub for ARM" advice, but
that
> was about adding a new entry point, not about stubbing out the existing entry
> points.)

Moving stubs to a separate utility file sounds good. I'll postpone it for a
later iteration of this review. Filename suggestions wanted.

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/service...
src/trusted/service_runtime/sel_validate_image.c:59: nap->validate_func =
NACL_SUBARCH_NAME(ApplyValidator,
On 2012/04/25 20:57:42, Nick Bray wrote:
> I'd reorganize this into mutually exclusive ifdefs that call
validator-specific
> functions to fill out the function table.  Figure that this is going to be
> expanded to deal with x86-32 and x86-64 seperately?  In some ways it seems
like
> a step back, but if done right it should make what validator is being used
> explicit.

That was my first thought as well. Mutually exclusive cases are easy to read.
However, then we get compiler complaints that the stubs are left unused for some
platforms. Then we may decide to make the stubs non-static, but that pollutes
the namespace.

So I can make it non-static mutually exclusive if you express this preference.

> I think (maybe not in this CL) that this function should be moved into
seperate
> file (in src/trusted/validator?) so that it can include header files for
> different validators without poluting sel_ldr's namespace, etc.
> 
> #if !defined(__arm__)
> #if defined(NACL_STANDALONE)
>   if (getenv("NACL_DANGEROUS_USE_DFA_VALIDATOR") != NULL) {
>     NACL_SUBARCH_NAME(InitDfaValidator,
>           NACL_TARGET_ARCH, NACL_TARGET_SUBARCH)(&nap->validator);
>   } else {
>     NACL_SUBARCH_NAME(InitStandardValidator,
>           NACL_TARGET_ARCH, NACL_TARGET_SUBARCH)(&nap->validator);
>   }
> #else
>     NACL_SUBARCH_NAME(InitStandardValidator,
>           NACL_TARGET_ARCH, NACL_TARGET_SUBARCH)(&nap->validator);
> #endif
> #else
>     NACL_SUBARCH_NAME(InitStandardValidator,
>           NACL_TARGET_ARCH, NACL_TARGET_SUBARCH)(&nap->validator);
> #endif

Ouch. So many functions, each of them making a couple of assignments. Are you
sure so much decoupling benefits anyone?

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/validat...
File src/trusted/validator/ncvalidate.h (left):

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/validat...
src/trusted/validator/ncvalidate.h:39: /* Defines possible validation status
values. */
On 2012/04/25 20:57:42, Nick Bray wrote:
> See previous comments, it may make sense to keep the enum here.

Done.

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/validat...
File src/trusted/validator_arm/ncvalidate.cc (left):

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/validat...
src/trusted/validator_arm/ncvalidate.cc:89: NaClValidationStatus
NACL_SUBARCH_NAME(ApplyValidatorCodeReplacement, arm, 32)
On 2012/04/25 20:57:42, Nick Bray wrote:
> See previous comments, probably want to keep.

This would lead us to creating quite a few stubs, keeping these stubs
platform-newtral saves some space. And then they could be moved to some utility
file.

Or we could have platform-newtral ValidatorRunner(nap->some_func) that would
return NotImplemented iff nap->some_func == NULL else run the function

[Nick Bray, 2012-04-27 00:41:30]

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/service...
File src/trusted/service_runtime/sel_ldr.h (right):

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/service...
src/trusted/service_runtime/sel_ldr.h:398: NaClValidateFunc         
validate_func;
> well, I thought about this same possibility at first, but then on callsites:
> nap->validate_func(...) is less clutter than:
> nap->validator_interface->validate_func(...)
> and then I'm not pleased

Yeah, the deref chain isn't pretty, but I figured it was just the cost of doing
business.  You could always do it in two steps:

validator = nap->validator;
validator->validate(...);

... or even 3, if you wanted to go crazy.

> Less coupling is good when we have use cases for using components separately
or
> abstractions that are very natural (and named intuitively). struct
> ValidatorInterface does not sound as a positive name for reader's intuition.
> 
> Generally I do not mind this decoupling, but for that please give advice on
> naming the struct and fields. 

The type could be NaClValidatorInterface, but the field in NaClApp could just be
"validator".

If these functions were put inside a structure, you could also drop the _func
postfix.  validate_copy could also be changed to copy_code - it doesn't
"validate" beyond requiring that it can decode the instructions.

I think there's precedent for function pointers being CamelCase, too.

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/service...
File src/trusted/service_runtime/sel_validate_image.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/service...
src/trusted/service_runtime/sel_validate_image.c:30: static NaClValidationStatus
ValidatorCopyNotImplemented(
> hm, we seem to agreed that validator selection is a tricky process involving a
> lot of parameters: environment vars, platform (maybe OS?), NaClApp, etc. And
it
> is hard to regularize this process. So, if we leave this choice partially to
> each of the "validators", then we risk separating this process into parts
> non-trivially which would only increase complication.

As I see it, each "validator" will always behave the same, and will always have
the same interface functions.  The only choice is which validator.  Can you
describe a specific case where the decision logic would start to leak into the
validator or the interface initialization?  I'll take a shot at figuring out how
it could be cleaned up.

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/service...
src/trusted/service_runtime/sel_validate_image.c:59: nap->validate_func =
NACL_SUBARCH_NAME(ApplyValidator,
> That was my first thought as well. Mutually exclusive cases are easy to read.
> However, then we get compiler complaints that the stubs are left unused for
some
> platforms. Then we may decide to make the stubs non-static, but that pollutes
> the namespace.

But if you require each validator declare its own stubs, no compiler problem. :)

> Ouch. So many functions, each of them making a couple of assignments. Are you
> sure so much decoupling benefits anyone?

Each of those functions will actually be a single assignment:

*validator = {
  ValidatorFunc,
  ReplaceFunc,
  CopyFunc,
  VALIDATOR_BUNDLE_SIZE
}

Or something like that.  Declarative is good.  (I am not sure if I like this
idea, but each validator could just declare a const global interface that
NaClApp could point to.)  If this method is used, modifications to a given
validator will not require changes to the service runtime, except if the name of
the init function changes.  Keeping the initialization in a function keeps the
function table consistent if it's specified multiple times: see standalone vs.
non standalone.  This scheme would also allow us to write generic tests that
iterate over different validators, but that's just a dream right now.

I think, ultimately, I am pushing this design because it allows the validator to
declare its interface.  Declarative is the idea that motivates me.

BTW, that code snippet was ugly, let me take it to its logical conclusion:

#if defined(__i386__) && defined(NACL_STANDALONE)

  if (UseDFAValidator()) {
    NaCl_DFAValidatorInit_x86_32(&nap->validator);
  } else {
    NaCl_ValidatorInit_x86_32(&nap->validator);
  }

#elif defined(__i386__)

  NaCl_ValidatorInit_x86_32(&nap->validator);

#elif defined(__x86_64__) && defined(NACL_STANDALONE)

  if (UseDFAValidator()) {
    NaCl_DFAValidatorInit_x86_64(&nap->validator);
  } else {
    NaCl_ValidatorInit_x86_64(&nap->validator);
  }

#elif defined(__x86_64__)

  NaCl_ValidatorInit_x86_64(&nap->validator);

#elif defined(__arm__)

  NaCl_ValidatorInit_ARM(&nap->validator);

#else
#error "No validator available for this architecture!"
#endif

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/validat...
File src/trusted/validator_arm/ncvalidate.cc (left):

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/validat...
src/trusted/validator_arm/ncvalidate.cc:89: NaClValidationStatus
NACL_SUBARCH_NAME(ApplyValidatorCodeReplacement, arm, 32)
On 2012/04/26 15:17:01, pasko wrote:
> On 2012/04/25 20:57:42, Nick Bray wrote:
> > See previous comments, probably want to keep.
> 
> This would lead us to creating quite a few stubs, keeping these stubs
> platform-newtral saves some space. And then they could be moved to some
utility
> file.
> 
> Or we could have platform-newtral ValidatorRunner(nap->some_func) that would
> return NotImplemented iff nap->some_func == NULL else run the function

Yeah, I was thinking of putting a NULL check in sel_ldr, but I think the stub
function is cleaner.  (I think this is a pattern.  Active Sentinel?  Can't find
a reference to it.)  I do think having each validator declare its own stubs will
be cleaner in the long run, because of build/link mess, but this isn't a big
issue for me - your call.

[pasko-google - do not use, 2012-04-27 17:32:11]

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/service...
File src/trusted/service_runtime/sel_validate_image.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/service...
src/trusted/service_runtime/sel_validate_image.c:30: static NaClValidationStatus
ValidatorCopyNotImplemented(
On 2012/04/27 00:41:30, Nick Bray wrote:
> > hm, we seem to agreed that validator selection is a tricky process involving
a
> > lot of parameters: environment vars, platform (maybe OS?), NaClApp, etc. And
> it
> > is hard to regularize this process. So, if we leave this choice partially to
> > each of the "validators", then we risk separating this process into parts
> > non-trivially which would only increase complication.
> 
> As I see it, each "validator" will always behave the same, and will always
have
> the same interface functions.  The only choice is which validator.  Can you
> describe a specific case where the decision logic would start to leak into the
> validator or the interface initialization?

Purely hypothetical: if we want to choose validator strategy via nacl manifest.
Less hypothetical: we'd want to patch TLS access sequences on some OS/Machine
configurations, but not on others (for speedup), where platform qualification
may be involved.

Okay, I made a draft change with full control over the choice of validation to
the platform-dependent files. I could not separate not-implemented stubs into a
separate lib because the build system won the battle. As a result there is much
more boilerplate code now: stubs, function declarations, trivial implementations
repeated all over. Though anyway, looks better than it was.

I uploaded a draft change. Please, look at the general idea, do not pay
attention to various extra TODOs left there temporarily. Enjoy code duplication
:)

Also, I could not make it to run the validation_cache_test, getting undefined
reference to 'NaCl_mprotect', no idea why and how I could affect that.

Also, one funny/annoying thing, this stuff gets built even if I ask to only
build platform=x86-32:
scons-out/dbg-linux-x86-32/obj/src/trusted/validator_arm/libncvalidate_arm_v2.a

Tired.

[Nick Bray, 2012-04-27 22:21:36]

I am very happy with how this is looking.  I hope you are too, and I haven't
pushed anything "questionable" a little too hard.

Very nice.

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/service...
File src/trusted/service_runtime/sel_validate_image.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/service...
src/trusted/service_runtime/sel_validate_image.c:30: static NaClValidationStatus
ValidatorCopyNotImplemented(
> Purely hypothetical: if we want to choose validator strategy via nacl
manifest.

That would affect the choice of validator and would not leak into any particular
validator.  What we're doing would support this cleanly.

> Less hypothetical: we'd want to patch TLS access sequences on some OS/Machine
> configurations, but not on others (for speedup), where platform qualification
> may be involved.

Hmmm.
1) Add a "patch_TLS" argument to the validator in question.
2) Create two new entry points that wrap this validator, one passing "1", the
other passing "0".
3) Either treat the two new entry points as "different validators", or
parameterize the interface construction function.

Alternatively, this was suggested by bsy, we could move from a table of pointers
to a "object oriented" approach where there was data associated with each
validator.  I rather not make that change until we need to, however.


> Okay, I made a draft change with full control over the choice of validation to
> the platform-dependent files. I could not separate not-implemented stubs into
a
> separate lib because the build system won the battle. As a result there is
much
> more boilerplate code now: stubs, function declarations, trivial
implementations
> repeated all over. Though anyway, looks better than it was.

I take no pleasure that the stubs got duplicated, I just know not to bet against
the build system.

> Also, I could not make it to run the validation_cache_test, getting undefined
> reference to 'NaCl_mprotect', no idea why and how I could affect that.

I suspect that we're relying on accidental dependencies, somewhere, somehow?

> Also, one funny/annoying thing, this stuff gets built even if I ask to only
> build platform=x86-32:
>
scons-out/dbg-linux-x86-32/obj/src/trusted/validator_arm/libncvalidate_arm_v2.a

Yeah, I've thought about fixing that, but I figured as long as I was refactoring
things it was a "feature" - more compile errors per compile!

> Tired.
... of working on this CL?  (I'd happily take over.)  Or are you warning me your
thoughts are not as clear as you'd like?

https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/servic...
File src/trusted/service_runtime/sel_validate_image.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/servic...
src/trusted/service_runtime/sel_validate_image.c:38: /* TODO: make it more
nested. */
I started with it being nested, but unless you indent the preprocessor
directives, it looks horrible.  Hence the linear structure. :/

https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/servic...
src/trusted/service_runtime/sel_validate_image.c:41: fprintf(stderr, "DANGER!
USING THE UNSTABLE DFA VALIDATOR!\n");
Nit: Experimental?  Untested?

https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/servic...
src/trusted/service_runtime/sel_validate_image.c:168: return
NaClValidateStatus(nap->validator->ValidateCopy(
Bad name.  Validating a copy of what?  Hence why I suggested "CopyCode".  I
understand why you want to have all the function have the same prefix, but I
think it confuses what the verb is supposed to be.  (Copy, not Validate.)

https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/valida...
File src/trusted/validator/ncvalidate.h (right):

https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/valida...
src/trusted/validator/ncvalidate.h:126: NaClValidateCopyFunc ValidateCopy;
Nit: swap position of Copy and Replacement?  (Copy is less important that the
replacement function.)  Veeeery nitish.  it bothers me, but I realize I am being
way too picky.  Please ignore me.

https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/valida...
File src/trusted/validator/x86/64/ncvalidate.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/valida...
src/trusted/validator/x86/64/ncvalidate.c:131: (uintptr_t guest_addr,
'(' on previous line; indent.

https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/valida...
src/trusted/validator/x86/64/ncvalidate.c:137: static struct
NaClValidatorInterface validator = {
const?

https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/valida...
src/trusted/validator/x86/64/ncvalidate.c:143: void
NaClValidatorInit_x86_64(struct NaClValidatorInterface **val) {
Maybe this should be a getter (returns the pointer) instead of a setter?

https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/valida...
File src/trusted/validator_arm/ncvalidate.cc (right):

https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/valida...
src/trusted/validator_arm/ncvalidate.cc:48: static NaClValidationStatus
ValidatorCopyNotImplemented(
For consistency with ragel, place these functions next to the interface
declaration.

[pasko-google - do not use, 2012-04-28 16:51:16]

On 2012/04/27 22:21:36, Nick Bray wrote:
> I am very happy with how this is looking.  I hope you are too, and I haven't
> pushed anything "questionable" a little too hard.
> 
> Very nice.

ok good

>
https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/service...
> File src/trusted/service_runtime/sel_validate_image.c (right):
> 
>
https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted/service...
> src/trusted/service_runtime/sel_validate_image.c:30: static
NaClValidationStatus
> ValidatorCopyNotImplemented(
> > Purely hypothetical: if we want to choose validator strategy via nacl
> manifest.
> 
> That would affect the choice of validator and would not leak into any
particular
> validator.  What we're doing would support this cleanly.

I meant that the validator choice should remain the same, while the validator
itself behaves differently. The second example appears to discuss exactly this.

> > Less hypothetical: we'd want to patch TLS access sequences on some
OS/Machine
> > configurations, but not on others (for speedup), where platform
qualification
> > may be involved.
> 
> Hmmm.
> 1) Add a "patch_TLS" argument to the validator in question.
> 2) Create two new entry points that wrap this validator, one passing "1", the
> other passing "0".
> 3) Either treat the two new entry points as "different validators", or
> parameterize the interface construction function.
> 
> Alternatively, this was suggested by bsy, we could move from a table of
pointers
> to a "object oriented" approach where there was data associated with each
> validator.  I rather not make that change until we need to, however.

So you see, this design does not fit very all validator choice strategies that
we envision. And that is good (i.e. less code). Does it fit very well with the
current validator choice? Maybe, but the same thing could be expressed in less
files, less functions, in a more compact and less modular way. The choice is not
trivial.
 
> > Okay, I made a draft change with full control over the choice of validation
to
> > the platform-dependent files. I could not separate not-implemented stubs
into
> a
> > separate lib because the build system won the battle. As a result there is
> much
> > more boilerplate code now: stubs, function declarations, trivial
> implementations
> > repeated all over. Though anyway, looks better than it was.
> 
> I take no pleasure that the stubs got duplicated, I just know not to bet
against
> the build system.

:)

> > Also, I could not make it to run the validation_cache_test, getting
undefined
> > reference to 'NaCl_mprotect', no idea why and how I could affect that.
> 
> I suspect that we're relying on accidental dependencies, somewhere, somehow?

unfortunately, I did not have time today to investigate
 
> > Also, one funny/annoying thing, this stuff gets built even if I ask to only
> > build platform=x86-32:
> >
>
scons-out/dbg-linux-x86-32/obj/src/trusted/validator_arm/libncvalidate_arm_v2.a
> 
> Yeah, I've thought about fixing that, but I figured as long as I was
refactoring
> things it was a "feature" - more compile errors per compile!

Let's be polite, it's an "issue" :)
 
> > Tired.
> ... of working on this CL?  (I'd happily take over.)  Or are you warning me
your
> thoughts are not as clear as you'd like?

no, I'd want to finish things like that myself. Though will be out next 5 days,
hope this does not irritate you being that long in uncommitted state.

>
https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/servic...
> File src/trusted/service_runtime/sel_validate_image.c (right):
> 
>
https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/servic...
> src/trusted/service_runtime/sel_validate_image.c:38: /* TODO: make it more
> nested. */
> I started with it being nested, but unless you indent the preprocessor
> directives, it looks horrible.  Hence the linear structure. :/

I saw indenting preprocessor directives in some projects. Beautiful. Then I
thought it's okay.

>
https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/servic...
> src/trusted/service_runtime/sel_validate_image.c:41: fprintf(stderr, "DANGER!
> USING THE UNSTABLE DFA VALIDATOR!\n");
> Nit: Experimental?  Untested?

experimental is ok

>
https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/servic...
> src/trusted/service_runtime/sel_validate_image.c:168: return
> NaClValidateStatus(nap->validator->ValidateCopy(
> Bad name.  Validating a copy of what?  Hence why I suggested "CopyCode".  I
> understand why you want to have all the function have the same prefix, but I
> think it confuses what the verb is supposed to be.  (Copy, not Validate.)

I was making these names mechanically, did not think how they sound. Will
rename.
 
>
https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/valida...
> File src/trusted/validator/ncvalidate.h (right):
> 
>
https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/valida...
> src/trusted/validator/ncvalidate.h:126: NaClValidateCopyFunc ValidateCopy;
> Nit: swap position of Copy and Replacement?  (Copy is less important that the
> replacement function.)  Veeeery nitish.  it bothers me, but I realize I am
being
> way too picky.  Please ignore me.

no big deal, should change 5 files :)
 
>
https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/valida...
> File src/trusted/validator/x86/64/ncvalidate.c (right):
> 
>
https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/valida...
> src/trusted/validator/x86/64/ncvalidate.c:131: (uintptr_t guest_addr,
> '(' on previous line; indent.

not in final state, probably will drop it in some header

>
https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/valida...
> src/trusted/validator/x86/64/ncvalidate.c:137: static struct
> NaClValidatorInterface validator = {
> const?
> 
>
https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/valida...
> src/trusted/validator/x86/64/ncvalidate.c:143: void
> NaClValidatorInit_x86_64(struct NaClValidatorInterface **val) {
> Maybe this should be a getter (returns the pointer) instead of a setter?

would look better indeed

>
https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/valida...
> File src/trusted/validator_arm/ncvalidate.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/valida...
> src/trusted/validator_arm/ncvalidate.cc:48: static NaClValidationStatus
> ValidatorCopyNotImplemented(
> For consistency with ragel, place these functions next to the interface
> declaration.

Why do you say 'consistency with ragel'? I saw BradC writing 'I added
support for ragel in tests'. Is it a kind of irony here? We are not changing
ragel, only operate on something ragel-based, where ragel is just a tool. In the
same
way we could say 'add support for python in tests' meaning that service runtime
is
built with python code on buildbots.

If it were not too long of a name, I would have referred to it as 'ragel-based
validator' or 'ragel-generated dfa parsing code'. Instead I name it as
'validator_ragel'. Less ambiguous.

As for the note, I'll try to be consistent.

[Nick Bray, 2012-05-10 17:10:21]

What's up?  I am ready to resume if you are ready.

[pasko-google - do not use, 2012-05-11 17:11:15]

On 2012/05/10 17:10:21, Nick Bray wrote:
> What's up?  I am ready to resume if you are ready.

Ready. You are free to avoid looking at it now because it is somewhat
unfinished. Up to you.

I decoupled the validator(s) from the code copying functionality (to avoid
circular dependency). This added some ugliness, but the alternative is to
include all service runtime into all tests, which sounds even worse.

Not all tests pass, currently I checked only validation_cache_test and building
sel_ldr on all linux variants (ia32, amd64, arm). Validator initialization
should be changed in those tests.

The overall structure looks as I want it to be modulo naming of functions/files
and probably some comments that I am going to address real soon. To be
continued..

[pasko-google - do not use, 2012-05-12 12:18:40]

PTAL

https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/servic...
File src/trusted/service_runtime/sel_validate_image.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/servic...
src/trusted/service_runtime/sel_validate_image.c:38: /* TODO: make it more
nested. */
On 2012/04/27 22:21:36, Nick Bray wrote:
> I started with it being nested, but unless you indent the preprocessor
> directives, it looks horrible.  Hence the linear structure. :/

to me it looks almost equally horrible, so I'd better keep your preference

https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/servic...
src/trusted/service_runtime/sel_validate_image.c:41: fprintf(stderr, "DANGER!
USING THE UNSTABLE DFA VALIDATOR!\n");
On 2012/04/27 22:21:36, Nick Bray wrote:
> Nit: Experimental?  Untested?

Done.

https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/valida...
File src/trusted/validator/ncvalidate.h (right):

https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/valida...
src/trusted/validator/ncvalidate.h:126: NaClValidateCopyFunc ValidateCopy;
On 2012/04/27 22:21:36, Nick Bray wrote:
> Nit: swap position of Copy and Replacement?  (Copy is less important that the
> replacement function.)  Veeeery nitish.  it bothers me, but I realize I am
being
> way too picky.  Please ignore me.

no need to apologize :)
Replace is important for JITs.
Copy is important for dynamic linking.
JIT can work without dynamic linking and vice versa. But dynamic linking is more
fundamental.

https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/valida...
File src/trusted/validator/x86/64/ncvalidate.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/valida...
src/trusted/validator/x86/64/ncvalidate.c:131: (uintptr_t guest_addr,
On 2012/04/27 22:21:36, Nick Bray wrote:
> '(' on previous line; indent.

Done.

https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/valida...
src/trusted/validator/x86/64/ncvalidate.c:137: static struct
NaClValidatorInterface validator = {
On 2012/04/27 22:21:36, Nick Bray wrote:
> const?

Done.

https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/valida...
src/trusted/validator/x86/64/ncvalidate.c:143: void
NaClValidatorInit_x86_64(struct NaClValidatorInterface **val) {
On 2012/04/27 22:21:36, Nick Bray wrote:
> Maybe this should be a getter (returns the pointer) instead of a setter?

yep, cleaner
also has a viral effect on NaClSelectValidator() which now turns into
NaClCreateValidator.

https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/valida...
File src/trusted/validator_arm/ncvalidate.cc (right):

https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/valida...
src/trusted/validator_arm/ncvalidate.cc:48: static NaClValidationStatus
ValidatorCopyNotImplemented(
On 2012/04/27 22:21:36, Nick Bray wrote:
> For consistency with ragel, place these functions next to the interface
> declaration.

I do not see an interface declaration here, which one?

[pasko-google - do not use, 2012-05-12 17:09:22]

hm, build issues remaining:

1. GYP on win64 special casing makes it difficult to add an extra file without
too much repeating myself (validator_init.c)

2. MacOS has hit me because it now observes a function prototype that returns
Bool. Lovely.

working to resolve those

[pasko-google - do not use, 2012-05-14 19:14:52]

On 2012/05/12 17:09:22, pasko wrote:
> hm, build issues remaining:
> 
> 1. GYP on win64 special casing makes it difficult to add an extra file without
> too much repeating myself (validator_init.c)
> 
> 2. MacOS has hit me because it now observes a function prototype that returns
> Bool. Lovely.
> 
> working to resolve those

Done that. PTAL.

[pasko-google - do not use, 2012-05-15 16:45:49]

a veeery friendly ping :)

[Nick Bray, 2012-05-15 17:23:34]

I worked on it yesterday.  I'll keep working on it today.  It's big.

On Tue, May 15, 2012 at 9:45 AM, <pasko@google.com> wrote:

> a veeery friendly ping :)
>
>
http://codereview.chromium.**org/10134056/<http://codereview.chromium.org/101...
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[pasko-google - do not use, 2012-05-17 10:20:15]

https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/valida...
File src/trusted/validator/ncvalidate.h (right):

https://chromiumcodereview.appspot.com/10134056/diff/15001/src/trusted/valida...
src/trusted/validator/ncvalidate.h:126: NaClValidateCopyFunc ValidateCopy;
On 2012/05/12 12:18:40, pasko wrote:
> On 2012/04/27 22:21:36, Nick Bray wrote:
> > Nit: swap position of Copy and Replacement?  (Copy is less important that
the
> > replacement function.)  Veeeery nitish.  it bothers me, but I realize I am
> being
> > way too picky.  Please ignore me.
> 
> no need to apologize :)
> Replace is important for JITs.
> Copy is important for dynamic linking.
> JIT can work without dynamic linking and vice versa. But dynamic linking is
more
> fundamental.

I was wrong. ValidateCodeReplacement and CopyCode are used only together (one
after another), and only in dynamic code modification. nacl-glibc does not need
them. The validation goes first, and  then if it succeeds CodeCopy comes in.
This suggests me to put ValidateCodeReplacement before CopyCode in the validator
interface. Maybe in a separate change?

[Nick Bray, 2012-05-22 23:46:31]

The difficulty I have reviewing a CL is roughly quadratic or cubic, so this one
is kicking my ass.  Please back out the portion of the change where you're
injecting the instruction copy function.  As far as I can tell (please correct
me if I am missing something) we're currently living with a circular dependency
between validators and sel.  We're also linking in "too much" to validator tests
because of this.  However - this issue doesn't actually block the rest of the
change?  Backing out that part will help me chew through this faster,
particularly as I am not happy about passing the copy function into the
validator and want to think about how best to do that... not to mention reducing
the n^2.5 cost.  After this CL it's probably something we should look into
fixing.

https://chromiumcodereview.appspot.com/10134056/diff/26024/src/trusted/servic...
File src/trusted/service_runtime/sel_validate_image.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/26024/src/trusted/servic...
src/trusted/service_runtime/sel_validate_image.c:11: #include
"native_client/src/trusted/validator_x86/nccopycode.h"
Uhg.  Copy code as part of the interface _and_ including the header file?

https://chromiumcodereview.appspot.com/10134056/diff/26024/src/trusted/servic...
src/trusted/service_runtime/sel_validate_image.c:110: NaClCopyInstructionFunc
copy_func = NaClCopyInstruction_x86;
Document why this is injected from sel_ldr.

https://chromiumcodereview.appspot.com/10134056/diff/26024/src/trusted/valida...
File src/trusted/validator/ncvalidate.h (right):

https://chromiumcodereview.appspot.com/10134056/diff/26024/src/trusted/valida...
src/trusted/validator/ncvalidate.h:13: #include <stdint.h>
Comment: why?

https://chromiumcodereview.appspot.com/10134056/diff/26024/src/trusted/valida...
src/trusted/validator/ncvalidate.h:106: * Assumes that instruction sizes are the
same. Only allows changes in branches
FYI, I believe this documentation is incorrect.  The x86-32 allows almost any
change that doesn't affect instruction sizes or super instruction groupings. 
The x86-64 validator allows jump targets and some constants.

https://chromiumcodereview.appspot.com/10134056/diff/26024/src/trusted/valida...
src/trusted/validator/ncvalidate.h:136: extern const struct
NaClValidatorInterface* NaClValidatorCreate_x86_64();
"extern" not needed for function declarations.  Scrub it from the file?

I find exposing private functions in this file a little questionable, but I
don't see a better way to set this up, besides adding a private header file. :/

https://chromiumcodereview.appspot.com/10134056/diff/26024/src/trusted/valida...
src/trusted/validator/ncvalidate.h:157: uintptr_t               guest_addr,
You've eliminated whitespace alignment elsewhere (yes!) so get rid of it here,
too.

https://chromiumcodereview.appspot.com/10134056/diff/26024/src/trusted/valida...
File src/trusted/validator/validator_init.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/26024/src/trusted/valida...
src/trusted/validator/validator_init.c:24: #elif NACL_TARGET_SUBARCH == 32 &&
defined(NACL_STANDALONE)
I don't like splitting the ARCH/SUBARCH check - it seems fragile.  The order of
the checks now matter.  Why not stick with the previous implementation?

https://chromiumcodereview.appspot.com/10134056/diff/26024/src/trusted/valida...
File src/trusted/validator/x86/64/ncvalidate.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/26024/src/trusted/valida...
src/trusted/validator/x86/64/ncvalidate.c:138: static int CopyCodeIter(uint8_t
*dst, uint8_t *src,
It feels like this code is in the wrong place.  Previously, this file only
contained entry points.

https://chromiumcodereview.appspot.com/10134056/diff/30044/src/trusted/servic...
File src/trusted/service_runtime/sel_validate_image.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/30044/src/trusted/servic...
src/trusted/service_runtime/sel_validate_image.c:108: NaClCopyInstructionFunc
copy_func = NULL;
The fact this pointer _could_ be NULL (even though it will never be used, if I
think about it) kinda freaks me out.  It makes me start wondering if we need
null pointer checks, etc.  

I think this is a smell - if we need to put this choice in sel_ldr, the
interface isn't working right.

https://chromiumcodereview.appspot.com/10134056/diff/30044/src/trusted/servic...
File src/trusted/service_runtime/service_runtime.gyp (right):

https://chromiumcodereview.appspot.com/10134056/diff/30044/src/trusted/servic...
src/trusted/service_runtime/service_runtime.gyp:209:
'<(DEPTH)/native_client/src/trusted/validator/validator_init.c',
This should be added to the 'target_base=="sel"' declaration - where all the
other sources are.  Note there are 'sel' and 'sel64' targets where this source
list gets injected into.  because you put it here, the sel64 version didn't get
the source file and you papered over the problem by also adding it to the 64-bit
validator's gyp file.  You should be able to remove that change after cleaning
up this file?

Nit: _init may be the wrong file name - _interface?  I don't have a very good
suggestion, but I feel it's not quite right.

Indentation is wrong.

https://chromiumcodereview.appspot.com/10134056/diff/30044/src/trusted/valida...
File src/trusted/validator/validator_init.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/30044/src/trusted/valida...
src/trusted/validator/validator_init.c:19: const struct NaClValidatorInterface*
NaClCreateValidator() {
Name: NaClGetValidatorInterface?  "Create" seems wrong.

[pasko-google - do not use, 2012-05-23 12:13:26]

On 2012/05/22 23:46:31, Nick Bray wrote:
> The difficulty I have reviewing a CL is roughly quadratic or cubic, so this
one
> is kicking my ass.

To make this look less scary I wanted to mention biggest chunks of change:

1. There is a code moving activity: the copying part (nccopycode.c) that is
platform-conditioned was just separated by platform and moved to a more logical
place: each respective ncvalidate.c. It was moved with no change except renaming
one function call.

2. Another visibly big change is the interface declaration in ncvalidate.h
(should look very familiar).

3.  There is a relatively big chunk of boilerplate repeated in each validator
initialization that we agreed is inevitable for this change.

Besides those biggies, all other chunks are rather tiny.

> Please back out the portion of the change where you're
> injecting the instruction copy function.

I am not sure I can finish it today. It is not just undoing across multiple
files. See below for details.

> As far as I can tell (please correct
> me if I am missing something) we're currently living with a circular
dependency
> between validators and sel.

On the library level yes, we have a circular dependency: sel depends on
validator(s) (it calls validator), and validator depend on sel (calls the
function to replace bytes of an instruction safely). But tests do not care about
this dependency yet because on the object level there is no circular dependency
(the object that calls instruction copy does not get included by the linker
because it is not needed, we have no tests for that).

Now we are making a single point of initialization, where we make a decision
about code copying functionality in the validator. And hence we create this
dependency on the object level: now the object containing NaClCreateValidator()
(validator_init.o) depends hardly on NaClValidatorCreate_x86_64() and that guy
although does not call NaClCopyInstruction_x86() in the test, but initializes a
function table with a function pointer that calls it, so the linker sees it as a
dependency.

Now I have to rethink all validator-related tests in terms of libraries they
use, add/reshape some libs, but not the others. I tried that before and it
introduced some unpleasant artifacts in our build system that I did not want to
fight against. Or maybe I should just add everything sel depends on in every
test. Then are they still unittests after this kind of change? Will this lead to
a bigger change in scons files? Do we need to do the same in GYP? Scary.

This may take 2-3 days.

Why do you not like the instruction copy injection? Does it make the code bigger
or it is the design that you do not like?

> We're also linking in "too much" to validator tests
> because of this.  However - this issue doesn't actually block the rest of the
> change?

Unfortunately, it blocks progress by having a single point of choice for all
functions provided by validator. I hope the above description puts it clearly.

> Backing out that part will help me chew through this faster,
> particularly as I am not happy about passing the copy function into the
> validator and want to think about how best to do that... not to mention
reducing
> the n^2.5 cost.  After this CL it's probably something we should look into
> fixing.

I do not see how it could be quadratic. What is n? is it the number of related
components interacting in NaCl? Is it the number of lines I changed in this CL?

If you push stronger with arguments I might agree to reorganize libs or tests
(we should discuss what to do), but I am not yet convinced. The n^2.5 cost is a
mystery to me.

[pasko-google - do not use, 2012-05-23 14:28:11]

uploaded only tiny changes in gyp file, ncvalidate.h, postponing larger changes
until discussion

https://chromiumcodereview.appspot.com/10134056/diff/26024/src/trusted/servic...
File src/trusted/service_runtime/sel_validate_image.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/26024/src/trusted/servic...
src/trusted/service_runtime/sel_validate_image.c:11: #include
"native_client/src/trusted/validator_x86/nccopycode.h"
On 2012/05/22 23:46:31, Nick Bray wrote:
> Uhg.  Copy code as part of the interface _and_ including the header file?

Needed declaration of NaClCopyInstructionFunc, which is easily detachable. If
you like, I'd create a copy_inst.h and include it in nccopycode and validator.
Just did not want to create another header.

https://chromiumcodereview.appspot.com/10134056/diff/26024/src/trusted/valida...
File src/trusted/validator/ncvalidate.h (right):

https://chromiumcodereview.appspot.com/10134056/diff/26024/src/trusted/valida...
src/trusted/validator/ncvalidate.h:13: #include <stdint.h>
On 2012/05/22 23:46:31, Nick Bray wrote:
> Comment: why?

this is old portion of code, I replaced it in the newest patchset with:
#include "native_client/src/include/portability.h"

See:
https://chromiumcodereview.appspot.com/10134056/diff/30044/src/trusted/valida...

Are you reviewing all patchsets? If so, I can guess where cubic time comes from.
Most of the patchsets are not intended for review, they are a result of me
underestimating some platform/build-system differences.

I can delete patchsets that are clearly off. Though I start forgetting what
there was..

https://chromiumcodereview.appspot.com/10134056/diff/26024/src/trusted/valida...
src/trusted/validator/ncvalidate.h:106: * Assumes that instruction sizes are the
same. Only allows changes in branches
On 2012/05/22 23:46:31, Nick Bray wrote:
> FYI, I believe this documentation is incorrect.  The x86-32 allows almost any
> change that doesn't affect instruction sizes or super instruction groupings. 
> The x86-64 validator allows jump targets and some constants.

Yes, when I looked at that later, I found that the x86-32 validator behaves as
you describe. Except it does not allow changing nacljmp at all. I will update
this after we decide on more toplevel things.

https://chromiumcodereview.appspot.com/10134056/diff/26024/src/trusted/valida...
src/trusted/validator/ncvalidate.h:136: extern const struct
NaClValidatorInterface* NaClValidatorCreate_x86_64();
On 2012/05/22 23:46:31, Nick Bray wrote:
> "extern" not needed for function declarations.  Scrub it from the file?
> 
> I find exposing private functions in this file a little questionable, but I
> don't see a better way to set this up, besides adding a private header file.
:/

yes, the main rationale to have it here was to avoid creating another header
file.

Removed the externs.

https://chromiumcodereview.appspot.com/10134056/diff/26024/src/trusted/valida...
src/trusted/validator/ncvalidate.h:157: uintptr_t               guest_addr,
On 2012/05/22 23:46:31, Nick Bray wrote:
> You've eliminated whitespace alignment elsewhere (yes!) so get rid of it here,
> too.

Done.

https://chromiumcodereview.appspot.com/10134056/diff/26024/src/trusted/valida...
File src/trusted/validator/validator_init.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/26024/src/trusted/valida...
src/trusted/validator/validator_init.c:24: #elif NACL_TARGET_SUBARCH == 32 &&
defined(NACL_STANDALONE)
On 2012/05/22 23:46:31, Nick Bray wrote:
> I don't like splitting the ARCH/SUBARCH check - it seems fragile.  The order
of
> the checks now matter.  Why not stick with the previous implementation?

huh, that was funny. The macro __i386__ is not defined on Windows, so I had to
either put windows macro fun here which spans more than one line, or use the
macros we already have for this.

Additionally there were some problems on arm, I do not remember what
specifically, but I can recheck if you want.

https://chromiumcodereview.appspot.com/10134056/diff/26024/src/trusted/valida...
File src/trusted/validator/x86/64/ncvalidate.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/26024/src/trusted/valida...
src/trusted/validator/x86/64/ncvalidate.c:138: static int CopyCodeIter(uint8_t
*dst, uint8_t *src,
On 2012/05/22 23:46:31, Nick Bray wrote:
> It feels like this code is in the wrong place.  Previously, this file only
> contained entry points.

This was previously in nccodecopy.c, that place is more wrong.

I think it fits nicely here since the function is very
old-validator-x86-64-specific and used only within this file. Can create a
separate file, with a separate header, but that would be an overkill.

https://chromiumcodereview.appspot.com/10134056/diff/30044/src/trusted/servic...
File src/trusted/service_runtime/service_runtime.gyp (right):

https://chromiumcodereview.appspot.com/10134056/diff/30044/src/trusted/servic...
src/trusted/service_runtime/service_runtime.gyp:209:
'<(DEPTH)/native_client/src/trusted/validator/validator_init.c',
On 2012/05/22 23:46:31, Nick Bray wrote:
> This should be added to the 'target_base=="sel"' declaration - where all the
> other sources are.  Note there are 'sel' and 'sel64' targets where this source
> list gets injected into.  because you put it here, the sel64 version didn't
get
> the source file and you papered over the problem by also adding it to the
64-bit
> validator's gyp file.  You should be able to remove that change after cleaning
> up this file?

Thank you for this! Seriously, great suggestion! I tried to put this source in
different places and was completely frustrated :)

Things like this:
'target_conditions': [['target_base=="sel"',
... look just sooo cute for a GYP newbie. Freaks me out.

Done.

> Nit: _init may be the wrong file name - _interface?  I don't have a very good
> suggestion, but I feel it's not quite right.

Same thoughts here, maybe validator_selector?

> Indentation is wrong.

Done.

https://chromiumcodereview.appspot.com/10134056/diff/30044/src/trusted/valida...
File src/trusted/validator/validator_init.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/30044/src/trusted/valida...
src/trusted/validator/validator_init.c:19: const struct NaClValidatorInterface*
NaClCreateValidator() {
On 2012/05/22 23:46:31, Nick Bray wrote:
> Name: NaClGetValidatorInterface?  "Create" seems wrong.

I want to emphasize that this needs to be called only once on program startup
since it may one time have side effects like memory allocation. Also, checking
an environment variable is not something we want to do prior to every validation
step.

The prefix "Create" makes a nice hint about possible unpleasant side effects.

[Nick Bray, 2012-05-24 01:17:15]

OK, I played around with your patch and now think I understand the linkage
problem.  Copycode uses these functions, which are part of sel:

NaCl_mprotect
NaCl_page_alloc
NaClFillMemoryRegionWithHalt

Our scons set up chokes on circular library dependencies, we just can't do them
without ugly build hacking.

We could lift the files containing these functions into a separate library,
although the last function might need to be moved from the file(s) it is in -
there's other junk next to it, and trying to put the entire file in a separate
file might get ugly fast.  It's also arch specific whereas the first two
functions are OS specific, so we can't just merge the files.

That's one approach - start breaking up the sel library.  Probably the best
approach, long term.  It may interact with the GYP build, too?  Arg.

The other approach would be to stick with the approach in this CL.  It bugs me
for two reasons. 1) bidirectional indirection between the validator and back. 
Too much indirection?  A very fuzzy objection, I know.  2) it exposes
architecture and implementation issues (null pointer for ARM) across the
validator interface.  Ugly.  This, I think, is the bigger issue for me.  However
- the alternative requires yak shaving.

Now that I actually understand the issue, let's talk tomorrow and decide.

https://chromiumcodereview.appspot.com/10134056/diff/26024/src/trusted/valida...
File src/trusted/validator/validator_init.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/26024/src/trusted/valida...
src/trusted/validator/validator_init.c:24: #elif NACL_TARGET_SUBARCH == 32 &&
defined(NACL_STANDALONE)
On 2012/05/23 14:28:11, pasko wrote:
> On 2012/05/22 23:46:31, Nick Bray wrote:
> > I don't like splitting the ARCH/SUBARCH check - it seems fragile.  The order
> of
> > the checks now matter.  Why not stick with the previous implementation?
> 
> huh, that was funny. The macro __i386__ is not defined on Windows, so I had to
> either put windows macro fun here which spans more than one line, or use the
> macros we already have for this.
> 
> Additionally there were some problems on arm, I do not remember what
> specifically, but I can recheck if you want.

OK, I see why you split the ARCH from the SUBARCH now that I looked everywhere
in our code base - it's just the way things are done.  So silly. :/  The
sensitivity to statement order, however, sill strikes me as begging for bugs. 
It looks like we have our choice of ugly looking code.

1) Leave it as is.
2) Nestled preprocessor blocks (indented?)
3) Decompose into multiple functions (ARM, x86, possibally x86-32 and x86-64),
surrounded by preprocessor switches.  This may or may not make 2) more readable.
4) Live with > 80 character lines.

Getting the formatting right may be more hassle than it's worth, but the small
landmine worries me.  The linkage issues are more important to me, your call.

[pasko-google - do not use, 2012-05-24 10:16:57]

https://chromiumcodereview.appspot.com/10134056/diff/26024/src/trusted/valida...
File src/trusted/validator/validator_init.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/26024/src/trusted/valida...
src/trusted/validator/validator_init.c:24: #elif NACL_TARGET_SUBARCH == 32 &&
defined(NACL_STANDALONE)
On 2012/05/24 01:17:15, Nick Bray wrote:
> On 2012/05/23 14:28:11, pasko wrote:
> > On 2012/05/22 23:46:31, Nick Bray wrote:
> > > I don't like splitting the ARCH/SUBARCH check - it seems fragile.  The
order
> > of
> > > the checks now matter.  Why not stick with the previous implementation?
> > 
> > huh, that was funny. The macro __i386__ is not defined on Windows, so I had
to
> > either put windows macro fun here which spans more than one line, or use the
> > macros we already have for this.
> > 
> > Additionally there were some problems on arm, I do not remember what
> > specifically, but I can recheck if you want.
> 
> OK, I see why you split the ARCH from the SUBARCH now that I looked everywhere
> in our code base - it's just the way things are done.  So silly. :/  The
> sensitivity to statement order, however, sill strikes me as begging for bugs. 
> It looks like we have our choice of ugly looking code.
> 
> 1) Leave it as is.

When writing I thought you'd like this because it is written in the style you
suggested with an exception in a couple of lines. But well, if you do not like
it overall I would better switch to (2).

> 2) Nestled preprocessor blocks (indented?)

I prefer this, indented in the preprocessor lines only. That is a common
practice that I used to accept. When code is short. If the code gets longer, we
will think again.

> 3) Decompose into multiple functions (ARM, x86, possibally x86-32 and x86-64),
> surrounded by preprocessor switches.  This may or may not make 2) more
readable.

This would make the code less localized, longer, harder to make sure that
specific variants are mutually exclusive.

> 4) Live with > 80 character lines.

please no, the question is not in length, but in the number of conditions that
have to be coded in a single line

> Getting the formatting right may be more hassle than it's worth, but the small
> landmine worries me.  The linkage issues are more important to me, your call.

[pasko-google - do not use, 2012-05-24 10:29:52]

On 2012/05/24 01:17:15, Nick Bray wrote:
> OK, I played around with your patch and now think I understand the linkage
> problem.  Copycode uses these functions, which are part of sel:
> 
> NaCl_mprotect
> NaCl_page_alloc
> NaClFillMemoryRegionWithHalt
> 
> Our scons set up chokes on circular library dependencies, we just can't do
them
> without ugly build hacking.
> 
> We could lift the files containing these functions into a separate library,
> although the last function might need to be moved from the file(s) it is in -
> there's other junk next to it, and trying to put the entire file in a separate
> file might get ugly fast.  It's also arch specific whereas the first two
> functions are OS specific, so we can't just merge the files.
> 
> That's one approach - start breaking up the sel library.  Probably the best
> approach, long term.  It may interact with the GYP build, too?  Arg.

... I feel like I have done a bad job explaining the very same thing :/
glad we are on the same page now.
 
> The other approach would be to stick with the approach in this CL.  It bugs me
> for two reasons. 1) bidirectional indirection between the validator and back. 
> Too much indirection?  A very fuzzy objection, I know.  2) it exposes
> architecture and implementation issues (null pointer for ARM) across the
> validator interface.  Ugly.  This, I think, is the bigger issue for me. 
However
> - the alternative requires yak shaving.

If two large components require each other to work objectively, well, that's how
it is. We should not try to hide this detail. Code replacement is a complicated
logic and better stay within the monolithic service runtime. On some
hypothetical systems it may require stopping all threads to modify code. Or even
send data over network? Separation of NaCl_mprotect and friends into a library
that the validator depends on .. would only hurt for these cases. On some not so
hypothetical systems code replacement is not possible at all: so there may be
platform dependency here some time.

Additionally, code replacement is just one function exported from the validator.
Used only once (in sel_alidate_image.c), ugliness is minimal.

[Nick Bray, 2012-05-25 06:41:21]

Let's go crazy.  LGTM w/ order independent #defines for validator selection and
adding a TODO.

The assumption here is that we'll both commit to doing the library refactoring
and make sure it gets done.  I am breaking my rule to never "offer credit" in a
CL.  I've been disappointed in the past when I let something through conditioned
on future work - it seems to never get done.    Please help me make it go right
this time.

https://chromiumcodereview.appspot.com/10134056/diff/41006/src/trusted/servic...
File src/trusted/service_runtime/sel_validate_image.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/41006/src/trusted/servic...
src/trusted/service_runtime/sel_validate_image.c:107: #ifdef __arm__
Add TODO for library refactoring, file issue, cc or list me as owner?

https://chromiumcodereview.appspot.com/10134056/diff/41006/src/trusted/valida...
File src/trusted/validator/validation_cache_test.cc (right):

https://chromiumcodereview.appspot.com/10134056/diff/41006/src/trusted/valida...
src/trusted/validator/validation_cache_test.cc:133: FALSE,  /* stubout_mode= */
If you're moving the comments, remove the "="

https://chromiumcodereview.appspot.com/10134056/diff/41006/src/trusted/valida...
File src/trusted/validator_x86/nccopycode.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/41006/src/trusted/valida...
src/trusted/validator_x86/nccopycode.c:32: #if NACL_ARCH(NACL_TARGET_ARCH) ==
NACL_x86
Remove dead includes?

[pasko-google - do not use, 2012-05-25 10:45:04]

On 2012/05/25 06:41:21, Nick Bray wrote:
> Let's go crazy.  LGTM w/ order independent #defines for validator selection
and
> adding a TODO.

Did I say I like order independence? Is it with line length limit or not? Why
did you choose something I did not choose? Is it a typo?
Sorry I was not available for chat. Had some stuff to do at home long into the
night :(

> The assumption here is that we'll both commit to doing the library refactoring
> and make sure it gets done.  I am breaking my rule to never "offer credit" in
a
> CL.  I've been disappointed in the past when I let something through
conditioned
> on future work - it seems to never get done.    Please help me make it go
right
> this time.

I am not committing this because it is Friday, near vacation time, the CL is
large.

Also, I am not committing because I do not fully understand the exact goal for
library refactoring. Is it to remove the circular dependencies between the
libraries (without injecting instruction copy and such)? Or is it about making
libraries more logical and self contained? Both are not ideal. The first one is,
as I said, trying to move a small part of the core service runtime functionality
to a separate library and can backfire in the long term. The second one is too
vague to be a target (I won't be able to do this, but I would be able to
review).

So let's agree what we want to do with libraries first.

I am not dropping the change. I think it's lovely, and I commit to committing it
later.

>
https://chromiumcodereview.appspot.com/10134056/diff/41006/src/trusted/servic...
> File src/trusted/service_runtime/sel_validate_image.c (right):
> 
>
https://chromiumcodereview.appspot.com/10134056/diff/41006/src/trusted/servic...
> src/trusted/service_runtime/sel_validate_image.c:107: #ifdef __arm__
> Add TODO for library refactoring, file issue, cc or list me as owner?

not sure what to write in the TODO, sorry. There are several options I can see,
but no clear goal before we talk.

>
https://chromiumcodereview.appspot.com/10134056/diff/41006/src/trusted/valida...
> File src/trusted/validator/validation_cache_test.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10134056/diff/41006/src/trusted/valida...
> src/trusted/validator/validation_cache_test.cc:133: FALSE,  /* stubout_mode=
*/
> If you're moving the comments, remove the "="

Done.

>
https://chromiumcodereview.appspot.com/10134056/diff/41006/src/trusted/valida...
> File src/trusted/validator_x86/nccopycode.c (right):
> 
>
https://chromiumcodereview.appspot.com/10134056/diff/41006/src/trusted/valida...
> src/trusted/validator_x86/nccopycode.c:32: #if NACL_ARCH(NACL_TARGET_ARCH) ==
> NACL_x86
> Remove dead includes?

Good catch. Done.

[pasko-google - do not use, 2012-06-14 14:59:34]

Missing LGTM from an OWNER for files in these directories:
    src/trusted/service_runtime

added Mark Seaborn to resolve this

[Mark Seaborn, 2012-06-14 20:11:56]

This is pretty huge and I don't understand it fully yet.  Is there a way you can
split this up into smaller changes?

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/servic...
File src/trusted/service_runtime/sel_validate_image.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/servic...
src/trusted/service_runtime/sel_validate_image.c:110: #ifdef __arm__
This should be "#if NACL_ARCH(NACL_BUILD_ARCH) == NACL_arm" instead.

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/servic...
File src/trusted/service_runtime/service_runtime.gyp (right):

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/servic...
src/trusted/service_runtime/service_runtime.gyp:68:
'<(DEPTH)/native_client/src/trusted/validator/validator_init.c',
This is messy.  service_runtime.gyp shouldn't be building something from
src/trusted/validator.  Also it's not consistent with what you've done in the
Scons build.

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/valida...
File src/trusted/validator/x86/32/ncvalidate.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/valida...
src/trusted/validator/x86/32/ncvalidate.c:111: static Bool
CopyInstruction(NCDecoderStatePair* tthis,
"tthis" is a strange name.  Use "self"?

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/valida...
src/trusted/validator/x86/32/ncvalidate.c:112: NCDecoderInst *dinst_old,
You're mixing "*" spacing style in adjacent lines.  Please check the whole patch
for mixed-up "*" spacing.

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/valida...
src/trusted/validator/x86/32/ncvalidate.c:125: size_t sz, const
NaClCopyInstructionFunc copy_func) {
Nit: this "const" isn't needed.

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/valida...
src/trusted/validator/x86/32/ncvalidate.c:151: const NaClCopyInstructionFunc
copy_func) {
Ditto about the const

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/valida...
File src/trusted/validator/x86/64/ncvalidate.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/valida...
src/trusted/validator/x86/64/ncvalidate.c:140: const NaClCopyInstructionFunc
copy_func) {
Ditto about const

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/valida...
src/trusted/validator/x86/64/ncvalidate.c:141: NaClSegment segment_old,
segment_new;
One variable declaration per line, please - that's the NaCl style.  Same below.

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/valida...
src/trusted/validator/x86/64/ncvalidate.c:210: const NaClCopyInstructionFunc
copy_func) {
Ditto about const

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/valida...
File src/trusted/validator_x86/nccopycode.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/valida...
src/trusted/validator_x86/nccopycode.c:170: int NaClCopyInstruction_x86(uint8_t
*dst, uint8_t *src, uint8_t sz) {
No underscores in function names.  Maybe "NaClCopyInstructionX86()"?

[pasko-google - do not use, 2012-06-17 14:45:49]

On 2012/06/14 20:11:56, Mark Seaborn wrote:
> This is pretty huge and I don't understand it fully yet.  Is there a way you
can
> split this up into smaller changes?

The whole change is LGTMed by Nick Bray. I asked you to review the
src/trusted/service_runtime part, which is 82 lines change. In addition there is
a large interface change in ncvalidate.h containing mostly comments. And there
is validator_init.c that you may want to look at to better understand the GYP
change and overall design shift.

Other large chunks of code is moving the code around with no change (for the
better), it is not that scary.

Also, please, ask Nick Bray about design points that you do not immediately
grasp. He knows every little detail of it and he is in your timezone.

[pasko-google - do not use, 2012-06-17 14:46:45]

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/servic...
File src/trusted/service_runtime/sel_validate_image.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/servic...
src/trusted/service_runtime/sel_validate_image.c:110: #ifdef __arm__
On 2012/06/14 20:11:56, Mark Seaborn wrote:
> This should be "#if NACL_ARCH(NACL_BUILD_ARCH) == NACL_arm" instead.

Done.

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/servic...
File src/trusted/service_runtime/service_runtime.gyp (right):

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/servic...
src/trusted/service_runtime/service_runtime.gyp:68:
'<(DEPTH)/native_client/src/trusted/validator/validator_init.c',
On 2012/06/14 20:11:56, Mark Seaborn wrote:
> This is messy.  service_runtime.gyp shouldn't be building something from
> src/trusted/validator.  Also it's not consistent with what you've done in the
> Scons build.

I agree this looks different than inclusion of other sources to "sel".

In the scons build I created a library out of one file and made it depend on the
rest of the platform-dependent validator libraries (that become conditional in
GYP depending on nacl_standalone). I did not find a way to do it in GYP without
creating one more GYP file, which I considered overkill.

I'd appreciate if your GYP expertise produced an elegant variant of this.

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/valida...
File src/trusted/validator/x86/32/ncvalidate.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/valida...
src/trusted/validator/x86/32/ncvalidate.c:111: static Bool
CopyInstruction(NCDecoderStatePair* tthis,
On 2012/06/14 20:11:56, Mark Seaborn wrote:
> "tthis" is a strange name.  Use "self"?

I moved this code as-is from /src/trusted/validator_x86/nccopycode.c as-is,
without any cleanup changes to make it trivial to compare during review. I can
make suggested cleanup changes in this code in later changes if you are willing
to approve them quickly.

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/valida...
src/trusted/validator/x86/32/ncvalidate.c:112: NCDecoderInst *dinst_old,
On 2012/06/14 20:11:56, Mark Seaborn wrote:
> You're mixing "*" spacing style in adjacent lines.  Please check the whole
patch
> for mixed-up "*" spacing.

same here, not my code

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/valida...
src/trusted/validator/x86/32/ncvalidate.c:125: size_t sz, const
NaClCopyInstructionFunc copy_func) {
On 2012/06/14 20:11:56, Mark Seaborn wrote:
> Nit: this "const" isn't needed.

Done.

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/valida...
src/trusted/validator/x86/32/ncvalidate.c:151: const NaClCopyInstructionFunc
copy_func) {
On 2012/06/14 20:11:56, Mark Seaborn wrote:
> Ditto about the const

Done.

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/valida...
File src/trusted/validator_x86/nccopycode.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/valida...
src/trusted/validator_x86/nccopycode.c:170: int NaClCopyInstruction_x86(uint8_t
*dst, uint8_t *src, uint8_t sz) {
On 2012/06/14 20:11:56, Mark Seaborn wrote:
> No underscores in function names.  Maybe "NaClCopyInstructionX86()"?

sure, my pleasure

[pasko-google - do not use, 2012-06-19 09:27:14]

Mark, did you work on this change yesterday?

We crafted this more concise validator interface together with Nick Bray, and we
consider it a quite important cleanup. I am not comfortable writing this but ..
the changes you committed yesterday to move/reformat a bunch of comments are
less important than this one.

Please take a look.

[pasko-google - do not use, 2012-06-20 17:38:22]

ping

sometimes we should let things go. If we do not know a better way to include a
file into GYP, and researching it takes days, then maybe we should not spend
more time on perfecting this?

[Brad Chen, 2012-06-20 18:46:57]

Waiting for a week is not okay. I will add Nick to the OWNERS file. If Mark
wants to work through issues need to do it soon.

Brad

On Wed, Jun 20, 2012 at 10:38 AM, <pasko@google.com> wrote:

> ping
>
> sometimes we should let things go. If we do not know a better way to
> include a
> file into GYP, and researching it takes days, then maybe we should not
> spend
> more time on perfecting this?
>
>
>
https://chromiumcodereview.**appspot.com/10134056/<https://chromiumcodereview...
>
> --
> You received this message because you are subscribed to the Google Groups
> "Native-Client-Reviews" group.
> To post to this group, send email to native-client-reviews@**
> googlegroups.com <native-client-reviews@googlegroups.com>.
> To unsubscribe from this group, send email to native-client-reviews+**
>
unsubscribe@googlegroups.com<native-client-reviews%2Bunsubscribe@googlegroups...
> .
> For more options, visit this group at http://groups.google.com/**
>
group/native-client-reviews?**hl=en<http://groups.google.com/group/native-client-reviews?hl=en>
> .
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

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

Should be OK with the changes below.  I'd like to do one quick pass after you've
done that, please.

http://codereview.chromium.org/10134056/diff/63002/src/trusted/service_runtim...
File src/trusted/service_runtime/service_runtime.gyp (right):

http://codereview.chromium.org/10134056/diff/63002/src/trusted/service_runtim...
src/trusted/service_runtime/service_runtime.gyp:68:
'<(DEPTH)/native_client/src/trusted/validator/validator_init.c',
On 2012/06/17 14:46:45, pasko wrote:
> On 2012/06/14 20:11:56, Mark Seaborn wrote:
> > This is messy.  service_runtime.gyp shouldn't be building
> > something from src/trusted/validator.  Also it's not
> > consistent with what you've done in the Scons build.
> 
> I agree this looks different than inclusion of other sources
> to "sel".
>
> In the scons build I created a library out of one file and made
> it depend on the rest of the platform-dependent validator
> libraries (that become conditional in GYP depending on
> nacl_standalone). I did not find a way to do it in GYP without
> creating one more GYP file, which I considered overkill.

Creating a new, arch-neutral Gyp file, src/validator/validator.gyp, seems like
the right thing to do.

Your change is creating a single entry point for getting the validator, and it
would make sense to also have a single build-time 'entry point' for library
dependencies.

src/validator/validator.gyp only needs to contain validator_init.c for now. 
(Cleaning up the validator dependencies in
src/trusted/service_runtime/service_runtime.gyp and
src/trusted/service_runtime/arch/x86/service_runtime_x86.gyp can be left to
later changes.)

http://codereview.chromium.org/10134056/diff/78002/src/trusted/validator/ncva...
File src/trusted/validator/ncvalidate.h (right):

http://codereview.chromium.org/10134056/diff/78002/src/trusted/validator/ncva...
src/trusted/validator/ncvalidate.h:36: /* Function type for applying a
Validator, as defined by sel_ldr. That is, run
Nit: "validator" should be lower case?  Unless you mean to say "...for applying
a NaClValidatorInterface"?

http://codereview.chromium.org/10134056/diff/78002/src/trusted/validator/ncva...
src/trusted/validator/ncvalidate.h:58: typedef NaClValidationStatus
(*NaClValidateFunc) (
Nit: there should be no space in "(*NaClValidateFunc)(".  Please fix instances
of this below too.

http://codereview.chromium.org/10134056/diff/78002/src/trusted/validator/ncva...
src/trusted/validator/ncvalidate.h:67: /* Function type to copy instruction
safely. Returns non-zero on success.
"an instruction"

http://codereview.chromium.org/10134056/diff/78002/src/trusted/validator/ncva...
src/trusted/validator/ncvalidate.h:68: * Exposed by the Service Runtime.
"exposed" -> "implemented"?

http://codereview.chromium.org/10134056/diff/78002/src/trusted/validator/ncva...
src/trusted/validator/ncvalidate.h:75: /* Function type for applying a Validator
to copy code from an existing code
Ditto about "Validator".  Same below too...

http://codereview.chromium.org/10134056/diff/78002/src/trusted/validator/ncva...
src/trusted/validator/ncvalidate.h:79: * require that the code segment match
native client rules.
Capitalise: "Native Client"

http://codereview.chromium.org/10134056/diff/78002/src/trusted/validator/ncva...
src/trusted/validator/ncvalidate.h:122: /* The full set of validator API. */
Should be "APIs" plural?

http://codereview.chromium.org/10134056/diff/78002/src/trusted/validator/ncva...
src/trusted/validator/ncvalidate.h:130: const struct NaClValidatorInterface*
NaClCreateValidator();
"*" spacing style: please use " *", as you've done previously in this fine.  Fix
cases below too...

http://codereview.chromium.org/10134056/diff/78002/src/trusted/validator/vali...
File src/trusted/validator/validator_init.c (right):

http://codereview.chromium.org/10134056/diff/78002/src/trusted/validator/vali...
src/trusted/validator/validator_init.c:12: int NaClUseDfaValidator() {
This function can be declared static

http://codereview.chromium.org/10134056/diff/78002/src/trusted/validator/vali...
src/trusted/validator/validator_init.c:20: #if defined(__arm__)
Use NACL_ARCH() again instead of __arm__

http://codereview.chromium.org/10134056/diff/78002/src/trusted/validator/vali...
src/trusted/validator/validator_init.c:23: #error("No validator available for
this architecture!")
No brackets are needed here

http://codereview.chromium.org/10134056/diff/78002/src/trusted/validator/vali...
src/trusted/validator/validator_init.c:26: fprintf(stderr, "DANGER! USING THE
EXPERIMENTAL DFA VALIDATOR!\n");
Please use NaClLog with LOG_WARNING instead of fprintf() and remove #include of
stdio.h.  Same below.

http://codereview.chromium.org/10134056/diff/78002/src/trusted/validator/x86/...
File src/trusted/validator/x86/ncval_seg_sfi/ncdecode.h (right):

http://codereview.chromium.org/10134056/diff/78002/src/trusted/validator/x86/...
src/trusted/validator/x86/ncval_seg_sfi/ncdecode.h:411: const
NaClCopyInstructionFunc copy_func);
Here is another instance of an unnecessary const.  Please grep the patch for
"const NaClCopyInstructionFunc" and fix all cases.

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

Ok, now I am pretty sure the trybots will pass. Feel free to wait until they
finish.

I think I responded to all comments.

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/servic...
File src/trusted/service_runtime/service_runtime.gyp (right):

https://chromiumcodereview.appspot.com/10134056/diff/63002/src/trusted/servic...
src/trusted/service_runtime/service_runtime.gyp:68:
'<(DEPTH)/native_client/src/trusted/validator/validator_init.c',
On 2012/06/20 20:02:26, Mark Seaborn wrote:
> Creating a new, arch-neutral Gyp file, src/validator/validator.gyp, seems like
> the right thing to do.
> 
> Your change is creating a single entry point for getting the validator, and it
> would make sense to also have a single build-time 'entry point' for library
> dependencies.
> 
> src/validator/validator.gyp only needs to contain validator_init.c for now. 
> (Cleaning up the validator dependencies in
> src/trusted/service_runtime/service_runtime.gyp and
> src/trusted/service_runtime/arch/x86/service_runtime_x86.gyp can be left to
> later changes.)

I added the trusted/validator/validator.gyp.
Though I do not fully understand what we should clean up in validator
dependencies. What is wrong with current dependencies between libraries besides
the unclean conditional linking with nacl_standalone?

https://chromiumcodereview.appspot.com/10134056/diff/78002/src/trusted/valida...
File src/trusted/validator/ncvalidate.h (right):

https://chromiumcodereview.appspot.com/10134056/diff/78002/src/trusted/valida...
src/trusted/validator/ncvalidate.h:36: /* Function type for applying a
Validator, as defined by sel_ldr. That is, run
On 2012/06/20 20:02:26, Mark Seaborn wrote:
> Nit: "validator" should be lower case?  Unless you mean to say "...for
applying
> a NaClValidatorInterface"?

Done.

https://chromiumcodereview.appspot.com/10134056/diff/78002/src/trusted/valida...
src/trusted/validator/ncvalidate.h:58: typedef NaClValidationStatus
(*NaClValidateFunc) (
On 2012/06/20 20:02:26, Mark Seaborn wrote:
> Nit: there should be no space in "(*NaClValidateFunc)(".  Please fix instances
> of this below too.

Done.

https://chromiumcodereview.appspot.com/10134056/diff/78002/src/trusted/valida...
src/trusted/validator/ncvalidate.h:67: /* Function type to copy instruction
safely. Returns non-zero on success.
On 2012/06/20 20:02:26, Mark Seaborn wrote:
> "an instruction"

Done.

https://chromiumcodereview.appspot.com/10134056/diff/78002/src/trusted/valida...
src/trusted/validator/ncvalidate.h:68: * Exposed by the Service Runtime.
On 2012/06/20 20:02:26, Mark Seaborn wrote:
> "exposed" -> "implemented"?

Okay, should sound better indeed. Done.

https://chromiumcodereview.appspot.com/10134056/diff/78002/src/trusted/valida...
src/trusted/validator/ncvalidate.h:75: /* Function type for applying a Validator
to copy code from an existing code
On 2012/06/20 20:02:26, Mark Seaborn wrote:
> Ditto about "Validator".  Same below too...

Done.

https://chromiumcodereview.appspot.com/10134056/diff/78002/src/trusted/valida...
src/trusted/validator/ncvalidate.h:79: * require that the code segment match
native client rules.
On 2012/06/20 20:02:26, Mark Seaborn wrote:
> Capitalise: "Native Client"

Done. Added "the".

https://chromiumcodereview.appspot.com/10134056/diff/78002/src/trusted/valida...
src/trusted/validator/ncvalidate.h:122: /* The full set of validator API. */
On 2012/06/20 20:02:26, Mark Seaborn wrote:
> Should be "APIs" plural?

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.

https://chromiumcodereview.appspot.com/10134056/diff/78002/src/trusted/valida...
src/trusted/validator/ncvalidate.h:130: const struct NaClValidatorInterface*
NaClCreateValidator();
On 2012/06/20 20:02:26, Mark Seaborn wrote:
> "*" spacing style: please use " *", as you've done previously in this fine. 
Fix
> cases below too...

Done. Thanks for reminder.

https://chromiumcodereview.appspot.com/10134056/diff/78002/src/trusted/valida...
File src/trusted/validator/validator_init.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/78002/src/trusted/valida...
src/trusted/validator/validator_init.c:12: int NaClUseDfaValidator() {
On 2012/06/20 20:02:26, Mark Seaborn wrote:
> This function can be declared static

That produces an error on ARM: defined but not used

https://chromiumcodereview.appspot.com/10134056/diff/78002/src/trusted/valida...
src/trusted/validator/validator_init.c:20: #if defined(__arm__)
On 2012/06/20 20:02:26, Mark Seaborn wrote:
> Use NACL_ARCH() again instead of __arm__

Done.

https://chromiumcodereview.appspot.com/10134056/diff/78002/src/trusted/valida...
src/trusted/validator/validator_init.c:23: #error("No validator available for
this architecture!")
On 2012/06/20 20:02:26, Mark Seaborn wrote:
> No brackets are needed here

Done.

https://chromiumcodereview.appspot.com/10134056/diff/78002/src/trusted/valida...
src/trusted/validator/validator_init.c:26: fprintf(stderr, "DANGER! USING THE
EXPERIMENTAL DFA VALIDATOR!\n");
On 2012/06/20 20:02:26, Mark Seaborn wrote:
> Please use NaClLog with LOG_WARNING instead of fprintf() and remove #include
of
> stdio.h.  Same below.

Done.

https://chromiumcodereview.appspot.com/10134056/diff/78002/src/trusted/valida...
File src/trusted/validator/x86/ncval_seg_sfi/ncdecode.h (right):

https://chromiumcodereview.appspot.com/10134056/diff/78002/src/trusted/valida...
src/trusted/validator/x86/ncval_seg_sfi/ncdecode.h:411: const
NaClCopyInstructionFunc copy_func);
On 2012/06/20 20:02:26, Mark Seaborn wrote:
> Here is another instance of an unnecessary const.  Please grep the patch for
> "const NaClCopyInstructionFunc" and fix all cases.

Sorry, now done throughout the patch

[Mark Seaborn, 2012-06-21 18:42:01]

LGTM with changes below

https://chromiumcodereview.appspot.com/10134056/diff/104003/src/trusted/servi...
File src/trusted/service_runtime/build.scons (right):

https://chromiumcodereview.appspot.com/10134056/diff/104003/src/trusted/servi...
src/trusted/service_runtime/build.scons:753: filter_group_only = 'true',
Nit: put this immediately after the "filter_regex" argument, to keep related
args together?

https://chromiumcodereview.appspot.com/10134056/diff/104003/src/trusted/valid...
File src/trusted/validator/validator_init.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/104003/src/trusted/valid...
src/trusted/validator/validator_init.c:17: const struct NaClValidatorInterface*
NaClCreateValidator() {
Use " *" spacing style

https://chromiumcodereview.appspot.com/10134056/diff/104003/src/trusted/valid...
File src/trusted/validator/x86/32/ncvalidate.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/104003/src/trusted/valid...
src/trusted/validator/x86/32/ncvalidate.c:165: const struct
NaClValidatorInterface* NaClValidatorCreate_x86_32() {
Use " *" spacing style

https://chromiumcodereview.appspot.com/10134056/diff/104003/src/trusted/valid...
File src/trusted/validator/x86/64/ncvalidate.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/104003/src/trusted/valid...
src/trusted/validator/x86/64/ncvalidate.c:224: const struct
NaClValidatorInterface* NaClValidatorCreate_x86_64() {
Use " *" spacing style

https://chromiumcodereview.appspot.com/10134056/diff/104003/src/trusted/valid...
File src/trusted/validator_arm/ncvalidate.cc (right):

https://chromiumcodereview.appspot.com/10134056/diff/104003/src/trusted/valid...
src/trusted/validator_arm/ncvalidate.cc:128: const struct
NaClValidatorInterface* NaClValidatorCreateArm() {
Use " *" spacing style

https://chromiumcodereview.appspot.com/10134056/diff/104003/src/trusted/valid...
File src/trusted/validator_ragel/unreviewed/dfa_validate_32.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/104003/src/trusted/valid...
src/trusted/validator_ragel/unreviewed/dfa_validate_32.c:88: const struct
NaClValidatorInterface* NaClDfaValidatorCreate_x86_32() {
Use " *" spacing style

https://chromiumcodereview.appspot.com/10134056/diff/104003/src/trusted/valid...
File src/trusted/validator_ragel/unreviewed/dfa_validate_64.c (right):

https://chromiumcodereview.appspot.com/10134056/diff/104003/src/trusted/valid...
src/trusted/validator_ragel/unreviewed/dfa_validate_64.c:87: const struct
NaClValidatorInterface* NaClDfaValidatorCreate_x86_64() {
Use " *" spacing style

[pasko-google - do not use, 2012-06-22 10:11:27]

http://codereview.chromium.org/10134056/diff/104003/src/trusted/service_runti...
File src/trusted/service_runtime/build.scons (right):

http://codereview.chromium.org/10134056/diff/104003/src/trusted/service_runti...
src/trusted/service_runtime/build.scons:753: filter_group_only = 'true',
On 2012/06/21 18:42:01, Mark Seaborn wrote:
> Nit: put this immediately after the "filter_regex" argument, to keep related
> args together?

Ok a little more goodness. Done.

http://codereview.chromium.org/10134056/diff/104003/src/trusted/validator/val...
File src/trusted/validator/validator_init.c (right):

http://codereview.chromium.org/10134056/diff/104003/src/trusted/validator/val...
src/trusted/validator/validator_init.c:17: const struct NaClValidatorInterface*
NaClCreateValidator() {
On 2012/06/21 18:42:01, Mark Seaborn wrote:
> Use " *" spacing style

Done.

http://codereview.chromium.org/10134056/diff/104003/src/trusted/validator/x86...
File src/trusted/validator/x86/32/ncvalidate.c (right):

http://codereview.chromium.org/10134056/diff/104003/src/trusted/validator/x86...
src/trusted/validator/x86/32/ncvalidate.c:165: const struct
NaClValidatorInterface* NaClValidatorCreate_x86_32() {
On 2012/06/21 18:42:01, Mark Seaborn wrote:
> Use " *" spacing style

Done.

http://codereview.chromium.org/10134056/diff/104003/src/trusted/validator/x86...
File src/trusted/validator/x86/64/ncvalidate.c (right):

http://codereview.chromium.org/10134056/diff/104003/src/trusted/validator/x86...
src/trusted/validator/x86/64/ncvalidate.c:224: const struct
NaClValidatorInterface* NaClValidatorCreate_x86_64() {
On 2012/06/21 18:42:01, Mark Seaborn wrote:
> Use " *" spacing style

Done.

http://codereview.chromium.org/10134056/diff/104003/src/trusted/validator_arm...
File src/trusted/validator_arm/ncvalidate.cc (right):

http://codereview.chromium.org/10134056/diff/104003/src/trusted/validator_arm...
src/trusted/validator_arm/ncvalidate.cc:128: const struct
NaClValidatorInterface* NaClValidatorCreateArm() {
On 2012/06/21 18:42:01, Mark Seaborn wrote:
> Use " *" spacing style

Done.

http://codereview.chromium.org/10134056/diff/104003/src/trusted/validator_rag...
File src/trusted/validator_ragel/unreviewed/dfa_validate_32.c (right):

http://codereview.chromium.org/10134056/diff/104003/src/trusted/validator_rag...
src/trusted/validator_ragel/unreviewed/dfa_validate_32.c:88: const struct
NaClValidatorInterface* NaClDfaValidatorCreate_x86_32() {
On 2012/06/21 18:42:01, Mark Seaborn wrote:
> Use " *" spacing style

Done.

http://codereview.chromium.org/10134056/diff/104003/src/trusted/validator_rag...
File src/trusted/validator_ragel/unreviewed/dfa_validate_64.c (right):

http://codereview.chromium.org/10134056/diff/104003/src/trusted/validator_rag...
src/trusted/validator_ragel/unreviewed/dfa_validate_64.c:87: const struct
NaClValidatorInterface* NaClDfaValidatorCreate_x86_64() {
On 2012/06/21 18:42:01, Mark Seaborn wrote:
> Use " *" spacing style

Done.