validator_ragel: Link into TCB, use under env var

validator_ragel: Link into TCB, use under env var

In both Chrome and sel_ldr environment variable
NACL_DANGEROUS_USE_DFA_VALIDATOR is used to replace the current validator with
the new one.

WARNING:
WARNING:
WARNING: The size increase of the runnable binaries resulted by this change will
likely trigger the "perf_expectation" regression when this change gets pulled
into Chrome via DEPS roll.

Size implications:

0. absolute values of size increase are equal among sel_ldr, nacl_helper and the
   chrome binary. On Linux the validator library gets pulled into the
   downloadable chrome package twice (in nacl_helper and chrome).

1. 32bit validator size is negligible (within 40K on each platform uncompressed)

2. with 64 validator there are 2 cases: Linux and Windows.

2.1. On Linux the optimized stripped sel_ldr gets increased by 400K (was: 510K).
     The 7zipped size increase for nacl_helper is 108K (was: 884K).

2.2. On Windows the optimized sel_ldr gets increased by 1MB (was: 955K). When
     7zipped the increase is 101K (great compression!). When we get to optimize
     the Windows64 build the uncompressed size increase is expected to be
     significantly smaller: about 350K. This is a work in progress.

Overall package size increase is expected to be:
Windows64:
  7zipped      - about 101K
  uncompressed - about 1MB (can be reduced later)
Linux64:
  7zipped      - about 216K
  uncompressed - about 800K
Windows32, all MacOS variants:
  uncompressed - less than 40K
Linux32:
  uncompressed - less than 80K

Linking another validator in statically has the advantage of being able to run
two validators side by side for some time taking the current validator's answer
as authoritative. Also, it does not increase the hassle in the build and
deployment systems which we have a lot already.

BUG=http://code.google.com/p/nativeclient/issues/detail?id=2597
TEST=./scons --mode=dbg-host platform=x86-64 run_dfa_validator_hello_world_test

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

[Mark Seaborn, 2012-04-19 22:11:39]

http://codereview.chromium.org/10070010/diff/1025/src/trusted/service_runtime...
File src/trusted/service_runtime/sel_main_chrome.h (right):

http://codereview.chromium.org/10070010/diff/1025/src/trusted/service_runtime...
src/trusted/service_runtime/sel_main_chrome.h:64: int enable_dfa_validator;
I don't think this belongs here.  Chromium does not need to know about the
validators, so don't add this to NaClChromeMainArgs, please.

Using an env var is preferable.  If you want to add something to about:flags, we
can discuss that.

http://codereview.chromium.org/10070010/diff/1025/src/trusted/service_runtime...
File src/trusted/service_runtime/testdata/dfa_validator_hello.stderr (right):

http://codereview.chromium.org/10070010/diff/1025/src/trusted/service_runtime...
src/trusted/service_runtime/testdata/dfa_validator_hello.stderr:1: DANGER! USING
THE UNSTABLE DFA VALIDATOR!
I don't think we want validator-implementation-specific stuff in
service_runtime.  Nick will probably have more to say about how we want to do
this cleanly.

http://codereview.chromium.org/10070010/diff/1025/tests/minnacl/nacl.scons
File tests/minnacl/nacl.scons (right):

http://codereview.chromium.org/10070010/diff/1025/tests/minnacl/nacl.scons#ne...
tests/minnacl/nacl.scons:25: VALIDATOR_LIBS +=
[trusted_env.NaClTargetArchSuffix('ncvalidate'),
Can you put this information into site_scons/site_tools/library_deps.py rather
than changing all these test/*/nacl.scons files, please?

[pasko-google - do not use, 2012-04-19 22:36:26]

http://codereview.chromium.org/10070010/diff/1025/src/trusted/service_runtime...
File src/trusted/service_runtime/sel_main_chrome.h (right):

http://codereview.chromium.org/10070010/diff/1025/src/trusted/service_runtime...
src/trusted/service_runtime/sel_main_chrome.h:64: int enable_dfa_validator;
On 2012/04/19 22:11:39, Mark Seaborn wrote:
> I don't think this belongs here.  Chromium does not need to know about the
> validators, so don't add this to NaClChromeMainArgs, please.
> 
> Using an env var is preferable.  If you want to add something to about:flags,
we
> can discuss that.

I do not have a strong preference on flag vs. env var for chrome. Can do either
way.

The env var seems to take less code because we can check for it in the deep
validator corners. On the other side, having all switches visible in one place
has it's advantages. Easier to review, less security risks, etc.

Reminds me of a story where engineers forgot to remove env var check deep in
their compiler, performed different optimization passes depending on $USER.

http://codereview.chromium.org/10070010/diff/1025/src/trusted/service_runtime...
File src/trusted/service_runtime/testdata/dfa_validator_hello.stderr (right):

http://codereview.chromium.org/10070010/diff/1025/src/trusted/service_runtime...
src/trusted/service_runtime/testdata/dfa_validator_hello.stderr:1: DANGER! USING
THE UNSTABLE DFA VALIDATOR!
On 2012/04/19 22:11:39, Mark Seaborn wrote:
> I don't think we want validator-implementation-specific stuff in
> service_runtime.  Nick will probably have more to say about how we want to do
> this cleanly.

I'd be curious to know a cleaner way.

[Nick Bray, 2012-04-19 23:11:07]

Here's some high-level advice.  Feel free to debate, a lot of it is based on how
I envision the future and my reasoning may be unclear.

http://codereview.chromium.org/10070010/diff/1025/src/trusted/service_runtime...
File src/trusted/service_runtime/build.scons (right):

http://codereview.chromium.org/10070010/diff/1025/src/trusted/service_runtime...
src/trusted/service_runtime/build.scons:769: env.AddNodeToTestSuite(node,
['medium_tests'],
Also add to suite validator_tests ?

http://codereview.chromium.org/10070010/diff/1025/src/trusted/service_runtime...
File src/trusted/service_runtime/sel_main_chrome.h (right):

http://codereview.chromium.org/10070010/diff/1025/src/trusted/service_runtime...
src/trusted/service_runtime/sel_main_chrome.h:64: int enable_dfa_validator;
In general, plumbing stuff from Chrome is a pain and using env vars to gate
experimental features helps keep things simple.  You probably want to keep the
check as high as possible, but modifying the browser interface seems excessive.

http://codereview.chromium.org/10070010/diff/1025/src/trusted/service_runtime...
File src/trusted/service_runtime/sel_validate_image.c (right):

http://codereview.chromium.org/10070010/diff/1025/src/trusted/service_runtime...
src/trusted/service_runtime/sel_validate_image.c:30: typedef
NaClValidationStatus (*ValidateFunc) (enum NaClSBKind,
FYI, removing SBKind is next on my hit list - but I expect you'll get this
committed before I make that change.

http://codereview.chromium.org/10070010/diff/1025/src/trusted/service_runtime...
src/trusted/service_runtime/sel_validate_image.c:41: const ValidateFunc
dfa_validate_func = NACL_SUBARCH_NAME(ApplyDfaValidator,
Don't stub this out on ARM, just ifdef out the reference.

http://codereview.chromium.org/10070010/diff/1025/src/trusted/service_runtime...
src/trusted/service_runtime/sel_validate_image.c:75: if
(nap->enable_dfa_validator) {
Move this up to keep the validate_func selection grouped.  In fact, I'd
encapsulate the validator selection logic in a separate function.  (Running the
validator should not care where the validator comes from.)  The logic will get a
little more complicate when you take ARM into account.

Once you stick this in a function you could also create a "Warn about
experimental validator" function that is always called when you select an
experimental validator, but maintains a global Boolean (on nap?) to suppress
multiple output.  This would keep the validator selection logic grouped.  Or,
screw it, you could just warn every time the validator is called. Much simpler.
:)

I plan to eventually refactor this so that the validator is selected once at
startup and the function pointers are cached.

http://codereview.chromium.org/10070010/diff/1025/src/trusted/validator/ncval...
File src/trusted/validator/ncvalidate.h (right):

http://codereview.chromium.org/10070010/diff/1025/src/trusted/validator/ncval...
src/trusted/validator/ncvalidate.h:106: extern NaClValidationStatus
NACL_SUBARCH_NAME(ApplyDfaValidator,
Don't specify this interface into ncvalidate, just pull a headerfile from ragel
into sel_ldr.

http://codereview.chromium.org/10070010/diff/1025/src/trusted/validator_arm/n...
File src/trusted/validator_arm/ncvalidate.cc (right):

http://codereview.chromium.org/10070010/diff/1025/src/trusted/validator_arm/n...
src/trusted/validator_arm/ncvalidate.cc:123: NaClValidationStatus
NACL_SUBARCH_NAME(ApplyDfaValidator, arm, 32) (
Don't add this stub.  Just ifdef out references to ApplyDfaValidator on ARM. 
For example, how would we handle Mark's validator?  The MIPS validator?  Etc. 
There's a combinational explosion as we add more validators and architectures. 
Ultimately this "global name" pattern needs to be replaced with dependency
injection, and every validator entry point will have a different name.

http://codereview.chromium.org/10070010/diff/1025/src/trusted/validator_ragel...
File src/trusted/validator_ragel/unreviewed/dfa_validate_32.c (right):

http://codereview.chromium.org/10070010/diff/1025/src/trusted/validator_ragel...
src/trusted/validator_ragel/unreviewed/dfa_validate_32.c:39:
UNREFERENCED_PARAMETER(readonly_text);
I'd check if readonly_text and error if it's requested, see the ARM validator.

http://codereview.chromium.org/10070010/diff/1025/src/trusted/validator_ragel...
src/trusted/validator_ragel/unreviewed/dfa_validate_32.c:42: if (stubout_mode ||
sb_kind != NACL_SB_DEFAULT) {
Keep the checks separate, it makes it easier to refactor.

http://codereview.chromium.org/10070010/diff/1025/src/trusted/validator_ragel...
src/trusted/validator_ragel/unreviewed/dfa_validate_32.c:48: if
(!ValidateChunkIA32(data, size, ProcessError, 0)) {
Nit: == 0?  This would make it clearer that you're expecting an "error code"
return rather than a true / false return.

http://codereview.chromium.org/10070010/diff/1025/src/trusted/validator_ragel...
File src/trusted/validator_ragel/unreviewed/dfa_validate_64.c (right):

http://codereview.chromium.org/10070010/diff/1025/src/trusted/validator_ragel...
src/trusted/validator_ragel/unreviewed/dfa_validate_64.c:38:
UNREFERENCED_PARAMETER(readonly_text);
Dito.

http://codereview.chromium.org/10070010/diff/1025/src/trusted/validator_ragel...
src/trusted/validator_ragel/unreviewed/dfa_validate_64.c:41: if (stubout_mode ||
sb_kind != NACL_SB_DEFAULT) {
Dito.

http://codereview.chromium.org/10070010/diff/1025/src/trusted/validator_ragel...
src/trusted/validator_ragel/unreviewed/dfa_validate_64.c:47: if
(!ValidateChunkAMD64(data, size, ProcessError, 0)) {
Dito.

[pasko-google - do not use, 2012-04-20 14:30:38]

http://codereview.chromium.org/10070010/diff/1025/src/trusted/service_runtime...
File src/trusted/service_runtime/build.scons (right):

http://codereview.chromium.org/10070010/diff/1025/src/trusted/service_runtime...
src/trusted/service_runtime/build.scons:769: env.AddNodeToTestSuite(node,
['medium_tests'],
On 2012/04/19 23:11:08, Nick Bray wrote:
> Also add to suite validator_tests ?

Yeah, why not. Done.

http://codereview.chromium.org/10070010/diff/1025/src/trusted/service_runtime...
File src/trusted/service_runtime/sel_main_chrome.h (right):

http://codereview.chromium.org/10070010/diff/1025/src/trusted/service_runtime...
src/trusted/service_runtime/sel_main_chrome.h:64: int enable_dfa_validator;
On 2012/04/19 23:11:08, Nick Bray wrote:
> In general, plumbing stuff from Chrome is a pain and using env vars to gate
> experimental features helps keep things simple.  You probably want to keep the
> check as high as possible, but modifying the browser interface seems
excessive.

Yes, keeping the check high has the benefit of keeping it close to other similar
checks. So in one glance anyone can see what kind of things can be requested
from service_runtime when running in the browser. This serves as a good
self-documentation.

But yes, since it is an experimental/temporary feature it may be OK to go with
an environment variable check buried deep in the woods. I am doing that since
both you and Mark clearly expressed your preference.

http://codereview.chromium.org/10070010/diff/1025/src/trusted/service_runtime...
File src/trusted/service_runtime/sel_validate_image.c (right):

http://codereview.chromium.org/10070010/diff/1025/src/trusted/service_runtime...
src/trusted/service_runtime/sel_validate_image.c:30: typedef
NaClValidationStatus (*ValidateFunc) (enum NaClSBKind,
On 2012/04/19 23:11:08, Nick Bray wrote:
> FYI, removing SBKind is next on my hit list - but I expect you'll get this
> committed before I make that change.

not a problem

http://codereview.chromium.org/10070010/diff/1025/src/trusted/service_runtime...
src/trusted/service_runtime/sel_validate_image.c:41: const ValidateFunc
dfa_validate_func = NACL_SUBARCH_NAME(ApplyDfaValidator,
On 2012/04/19 23:11:08, Nick Bray wrote:
> Don't stub this out on ARM, just ifdef out the reference.

across all trusted code I do not see a single #ifdef ARM, is it a good time to
break the rule? Specifically this file seems to have been designed to look as
platform-independent. If it stops to be, let's discuss design vision in more
detail. A document?

http://codereview.chromium.org/10070010/diff/1025/src/trusted/service_runtime...
src/trusted/service_runtime/sel_validate_image.c:75: if
(nap->enable_dfa_validator) {
On 2012/04/19 23:11:08, Nick Bray wrote:
> Move this up to keep the validate_func selection grouped.

For the grouping I'd better move the validate_func selection down just before it
is used. In C it would need an extra {} block, which is well .. more or less
okay. Done.

> In fact, I'd
> encapsulate the validator selection logic in a separate function.  (Running
the
> validator should not care where the validator comes from.)  The logic will get
a
> little more complicate when you take ARM into account.

NaClValidateCode() is already a function that selects an appropriate validating
function and additionally performs some work: run the function, check
compatibility and decide on caching. If it were a lengthy function I would fully
support the separation, but as of now it would only create another abstract
layer that complicates reading. Are there clear plans for more validation
features that require separating into multiple functions?

Choosing a validating function in a platform-dependent way would be reasonable
if we had numerous validation methods to choose from. I am trying to make things
as simple as possible (but not simpler). I.e. making a nice extensible
infrastructure for something that is not going to be extended is something I do
not support.

> Once you stick this in a function you could also create a "Warn about
> experimental validator" function that is always called when you select an
> experimental validator, but maintains a global Boolean (on nap?) to suppress
> multiple output.  This would keep the validator selection logic grouped.  Or,
> screw it, you could just warn every time the validator is called. Much
simpler.
> :)

Take a different look: the two (three?) validators will be there simultaneously
for some time, then the dispatch function will have to be reverted because it
would only make a choice among one option.

> I plan to eventually refactor this so that the validator is selected once at
> startup and the function pointers are cached.

If it is for speedup, then it is overly premature. Global state like that might
look more elegant than now, but not for a large extent, so I would not bother.

http://codereview.chromium.org/10070010/diff/1025/src/trusted/validator/ncval...
File src/trusted/validator/ncvalidate.h (right):

http://codereview.chromium.org/10070010/diff/1025/src/trusted/validator/ncval...
src/trusted/validator/ncvalidate.h:106: extern NaClValidationStatus
NACL_SUBARCH_NAME(ApplyDfaValidator,
On 2012/04/19 23:11:08, Nick Bray wrote:
> Don't specify this interface into ncvalidate, just pull a headerfile from
ragel
> into sel_ldr.

I did not want to include directly anything from unreviewed/*. Also, ragel
validating function names differ for 32bit and 64bit sandboxes
(ValidateChunkAMD64,ValidateChunkIA32)  to be able to keep them in the same
binary (good for SDK). Pulling directly into platform-independent parts would
require #ifdef-ing. So I rather went with the existing mechanism for handling
platform differences in validator.

http://codereview.chromium.org/10070010/diff/1025/src/trusted/validator_arm/n...
File src/trusted/validator_arm/ncvalidate.cc (right):

http://codereview.chromium.org/10070010/diff/1025/src/trusted/validator_arm/n...
src/trusted/validator_arm/ncvalidate.cc:123: NaClValidationStatus
NACL_SUBARCH_NAME(ApplyDfaValidator, arm, 32) (
On 2012/04/19 23:11:08, Nick Bray wrote:
> Don't add this stub.  Just ifdef out references to ApplyDfaValidator on ARM. 
> For example, how would we handle Mark's validator?

Mark's validator exists for 2 platforms out of 3, adding an extra stub in ARM
for it is similarly ugly as #ifdef-ing the code in sel_validate_image that
otherwise operates on platform-independent abstractions.

> The MIPS validator?  Etc. 

The MIPS validator would live in ApplyValidator, will define a stub for
DfaValidator and one for Mark's validator.

Under assumption that "validator strategies differ a lot depending on platform"
better would be to define _only_ ApplyValidator function for each platform that
would deal with various blows and whistles like caching, choosing an exact
validator etc. Making so would be quite a huge refactoring. I'd ask to bypass it
now.

Under assumption that "validator strategies are pretty much the same for all
platforms" we add all validation methods for all platforms, some of which are
stubs. This seems to be the original assumption.

Under some assumptions we can implement a hybrid scheme with dynamic dispatch
and caching. Only good for complicated cases. I'll try to avoid that until there
actually is a lot of existing dispatch code that can be regularized. Better
refactor a dumb system than eliminate an overly complex system for the task.

> There's a combinational explosion as we add more validators and architectures.


Yes, adding ApplyDfaValidatorCopy, ApplyDfaValidatorCodeReplacement for all is a
bit explosive. I did not think about it yet. 

> Ultimately this "global name" pattern needs to be replaced with dependency
> injection, and every validator entry point will have a different name.

Probably. Though I would first try to reduce the amount of unneeded global names
(introducing a damn simple new argument?), then among the remaining names decide
whether we actually need runtime dispatch/injection. Not clear to me at this
point.

What I think is clear is that a hybrid approach with some "global names" and
some dynamic validator selection is worse than any of the two strategies alone.
So, looking at this I decided to go with what is common in the codebase, then
refactor, maybe. 

Yeah, increases technical debt. But without clear scope of validator features
planned it is a reasonable approach IMHO.

http://codereview.chromium.org/10070010/diff/1025/src/trusted/validator_ragel...
File src/trusted/validator_ragel/unreviewed/dfa_validate_32.c (right):

http://codereview.chromium.org/10070010/diff/1025/src/trusted/validator_ragel...
src/trusted/validator_ragel/unreviewed/dfa_validate_32.c:39:
UNREFERENCED_PARAMETER(readonly_text);
On 2012/04/19 23:11:08, Nick Bray wrote:
> I'd check if readonly_text and error if it's requested, see the ARM validator.

Done.

http://codereview.chromium.org/10070010/diff/1025/src/trusted/validator_ragel...
src/trusted/validator_ragel/unreviewed/dfa_validate_32.c:42: if (stubout_mode ||
sb_kind != NACL_SB_DEFAULT) {
On 2012/04/19 23:11:08, Nick Bray wrote:
> Keep the checks separate, it makes it easier to refactor.

Code should be optimized for readability, not for refactoring pleasures.
Besides, I do not see how your suggestion makes refactoring easier. Do you mean
it would make conflicts more likely? I think returning NotImplemented in one
condition makes the code more clear because the reader does not have to look
through multiple conditionals and hunt for differences in returned codes that
are lengthy words looking similarly.

Not a big deal. If you insist I can put conditions on different lines.

http://codereview.chromium.org/10070010/diff/1025/src/trusted/validator_ragel...
src/trusted/validator_ragel/unreviewed/dfa_validate_32.c:48: if
(!ValidateChunkIA32(data, size, ProcessError, 0)) {
On 2012/04/19 23:11:08, Nick Bray wrote:
> Nit: == 0?  This would make it clearer that you're expecting an "error code"
> return rather than a true / false return.

Good nit. Done.

http://codereview.chromium.org/10070010/diff/1025/src/trusted/validator_ragel...
File src/trusted/validator_ragel/unreviewed/dfa_validate_64.c (right):

http://codereview.chromium.org/10070010/diff/1025/src/trusted/validator_ragel...
src/trusted/validator_ragel/unreviewed/dfa_validate_64.c:38:
UNREFERENCED_PARAMETER(readonly_text);
On 2012/04/19 23:11:08, Nick Bray wrote:
> Dito.

Done.

http://codereview.chromium.org/10070010/diff/1025/src/trusted/validator_ragel...
src/trusted/validator_ragel/unreviewed/dfa_validate_64.c:41: if (stubout_mode ||
sb_kind != NACL_SB_DEFAULT) {
On 2012/04/19 23:11:08, Nick Bray wrote:
> Dito.

Done.

http://codereview.chromium.org/10070010/diff/1025/src/trusted/validator_ragel...
src/trusted/validator_ragel/unreviewed/dfa_validate_64.c:47: if
(!ValidateChunkAMD64(data, size, ProcessError, 0)) {
On 2012/04/19 23:11:08, Nick Bray wrote:
> Dito.

Done.

http://codereview.chromium.org/10070010/diff/1025/tests/minnacl/nacl.scons
File tests/minnacl/nacl.scons (right):

http://codereview.chromium.org/10070010/diff/1025/tests/minnacl/nacl.scons#ne...
tests/minnacl/nacl.scons:25: VALIDATOR_LIBS +=
[trusted_env.NaClTargetArchSuffix('ncvalidate'),
On 2012/04/19 22:11:39, Mark Seaborn wrote:
> Can you put this information into site_scons/site_tools/library_deps.py rather
> than changing all these test/*/nacl.scons files, please?

looking forward to merging it with your cleanup

[pasko-google - do not use, 2012-04-20 16:41:21]

Merged Mark's comments from https://chromiumcodereview.appspot.com/10158002

PTAL

[Nick Bray, 2012-04-23 04:30:17]

> across all trusted code I do not see a single #ifdef ARM, is it a good time to
> break the rule? Specifically this file seems to have been designed to look as
> platform-independent. If it stops to be, let's discuss design vision in more
> detail. A document?

I suppose I could, although the design doc would pretty much look like a CL. 
The key is that selecting a validator is, in fact, messy - despite appearances. 
We've papered over it by exploiting the fact there is only one validator per
architecture. We're using macros (NACL_SUBARCH_NAME) to make it appear as if
there is one entry point, but if you take a long look at that macro, that's
where your ifdef is hiding (strictly speaking not an ifdef, but preprocessor
magic all the same).  We're in a world, however, where we're experimenting the
validators and on ARM we may even support two ISAs.  Selecting a validator is a
decision, so my belief is that would should embrace that decision, make it
explicit in the code, and then promptly confine it to a single function so it
doesn't mess up the rest of the code base.  The macro approach doesn't scale (it
cannot fully encode how we decide what validator to use) and I think you needing
to stub out the new validator on ARM demonstrates this - you're having to
dynamically select between two macro magic "global names", one of which is bogus
on ARM.  Ouch.  In my view, an ifdef in the validator selection logic tells the
reader _exactly_ what is going on.  It isn't pretty, but it's the least ugly.

If you aren't feeling what's I am saying, I don't know what else to say.  I
don't want this to be a brutal review.  Did I sway you?  Is there a third person
you'd like input from?  Or are you convinced it's good as is?

Also, on a side note, I suspect that running two validators side-by-side will
require a large amount of yak shaving, because we have no way to communicate the
results.  We can't do UMA from the sel_ldr process (yet) so what's the plan?
 
> NaClValidateCode() is already a function that selects an appropriate
validating
> function and additionally performs some work: run the function, check
> compatibility and decide on caching. If it were a lengthy function I would
fully
> support the separation, but as of now it would only create another abstract
> layer that complicates reading. Are there clear plans for more validation
> features that require separating into multiple functions?

One function, one task.  Even without any future plans, selecting the validator
has nothing to do with running it.  I don't understand (but I am trying) how
adding a function to perform a clearly defined and seperable task does anything
but help readability?  Programming is fundamentally about building good
abstractions that help us understand a horrendously complicated problem.

> If it is for speedup, then it is overly premature. Global state like that
might
> look more elegant than now, but not for a large extent, so I would not bother.

Not an issue of speedup: sticking a function table on nap->... would make it
clear we're not switching validators at runtime, or anything awful like that. 
Simplicity.

> What I think is clear is that a hybrid approach with some "global names" and
> some dynamic validator selection is worse than any of the two strategies
alone.
> So, looking at this I decided to go with what is common in the codebase, then
> refactor, maybe. 

Yeah, I just end up being the sucker who does the refactoring. :)  The ultimate
design I see: each validator has unique function names.  At startup a table of
validator functions is built and attached to "nap".  All the macros go away.

> Code should be optimized for readability, not for refactoring pleasures.
> Besides, I do not see how your suggestion makes refactoring easier. Do you
mean
> it would make conflicts more likely? I think returning NotImplemented in one
> condition makes the code more clear because the reader does not have to look
> through multiple conditionals and hunt for differences in returned codes that
> are lengthy words looking similarly.

I wouldn't have suggested it if I thought it reduced readability.  (Have some
faith.)  In fact, I thought it would improve readability - a series of simple
checks, just like the other validators.  It isn't worth the discussion, however,
so don't worry about it.



So, in the interests of avoiding time zone ping pong, LGTM with the following
strong suggestions:

1) Move validator selection in a function.
2) Don't create a stub function for the ARM validator.

I would be happy to discuss the design issues outside this CL, but I don't want
it to hold the CL up.

[pasko-google - do not use, 2012-04-23 16:55:01]

On 2012/04/23 04:30:17, Nick Bray wrote:
> > across all trusted code I do not see a single #ifdef ARM, is it a good time
to
> > break the rule? Specifically this file seems to have been designed to look
as
> > platform-independent. If it stops to be, let's discuss design vision in more
> > detail. A document?
> 
> I suppose I could, although the design doc would pretty much look like a CL. 
> The key is that selecting a validator is, in fact, messy - despite
appearances. 

I seem to understand your point better now. So assuming the validator selection
is messy we would create a function SelectValidator(nap) that should return a
validating function. All validating functions share the same function type (i.e.
same API).

> We've papered over it by exploiting the fact there is only one validator per
> architecture. We're using macros (NACL_SUBARCH_NAME) to make it appear as if
> there is one entry point, but if you take a long look at that macro, that's
> where your ifdef is hiding (strictly speaking not an ifdef, but preprocessor
> magic all the same).  We're in a world, however, where we're experimenting the
> validators and on ARM we may even support two ISAs.  Selecting a validator is
a
> decision, so my belief is that would should embrace that decision, make it
> explicit in the code, and then promptly confine it to a single function so it
> doesn't mess up the rest of the code base.  The macro approach doesn't scale
(it
> cannot fully encode how we decide what validator to use) and I think you
needing
> to stub out the new validator on ARM demonstrates this - you're having to
> dynamically select between two macro magic "global names", one of which is
bogus
> on ARM.  Ouch.  In my view, an ifdef in the validator selection logic tells
the
> reader _exactly_ what is going on.  It isn't pretty, but it's the least ugly.
>
> If you aren't feeling what's I am saying, I don't know what else to say.

Great! I agree to these points. A lone #ifdef is better than the whole name
expansion plus stub(s) for ARM. It's just the original authors seemed to have
the opposite opinion, and I wanted to double-check.

> I don't want this to be a brutal review.  Did I sway you?  Is there a third
person
> you'd like input from?  Or are you convinced it's good as is?

The review is going fine given all objective constraints. Not brutal :) Nothing
to worry about. So please don't worry.

> Also, on a side note, I suspect that running two validators side-by-side will
> require a large amount of yak shaving, because we have no way to communicate
the
> results.  We can't do UMA from the sel_ldr process (yet) so what's the plan?

Oops. I did not think over these details. Sounds pretty bad. We want to
communicate the difference (url, hash, ret1, ret2) to UMA via an established
channel to (Chrome? Breakpad?). How hard would that be? Can you point me briefly
at the yak shaving spots?

> > NaClValidateCode() is already a function that selects an appropriate
> validating
> > function and additionally performs some work: run the function, check
> > compatibility and decide on caching. If it were a lengthy function I would
> fully
> > support the separation, but as of now it would only create another abstract
> > layer that complicates reading. Are there clear plans for more validation
> > features that require separating into multiple functions?
> 
> One function, one task.  Even without any future plans, selecting the
validator
> has nothing to do with running it.  I don't understand (but I am trying) how
> adding a function to perform a clearly defined and seperable task does
anything
> but help readability?  Programming is fundamentally about building good
> abstractions that help us understand a horrendously complicated problem.

This can go into a lengthy debate on adding an extra function for a task vs. not
adding. Each line of code performs a separate task, after all.

I was not specific enough. If we select a validating function, and then run it,
then we need an extra composite function that would stubout+revalidate. And we
will need a function like this for two platforms: x86-32 and x86-64, but not for
ARM.  I was thinking that it forces us to duplicate the list of arguments at
callsites.

Other than that I do not have objections, will be doing it now.

> > If it is for speedup, then it is overly premature. Global state like that
> might
> > look more elegant than now, but not for a large extent, so I would not
bother.
> 
> Not an issue of speedup: sticking a function table on nap->... would make it
> clear we're not switching validators at runtime, or anything awful like that. 
> Simplicity.

Sounds Good.

> > What I think is clear is that a hybrid approach with some "global names" and
> > some dynamic validator selection is worse than any of the two strategies
> alone.
> > So, looking at this I decided to go with what is common in the codebase,
then
> > refactor, maybe. 
> 
> Yeah, I just end up being the sucker who does the refactoring. :)  The
ultimate
> design I see: each validator has unique function names.  At startup a table of
> validator functions is built and attached to "nap".  All the macros go away.

It is possible to make other people do refactoring on separate CLs. For example,
we
could have an agreement now that I would eliminate NACL_SUBARCH_NAME, and then
you'd move the selection logic to startup. Or something like that.

What throughput are we talking about here? Would one change like that per week
be helpful?

> > Code should be optimized for readability, not for refactoring pleasures.
> > Besides, I do not see how your suggestion makes refactoring easier. Do you
> mean
> > it would make conflicts more likely? I think returning NotImplemented in one
> > condition makes the code more clear because the reader does not have to look
> > through multiple conditionals and hunt for differences in returned codes
that
> > are lengthy words looking similarly.
> 
> I wouldn't have suggested it if I thought it reduced readability.  (Have some
> faith.)  In fact, I thought it would improve readability - a series of simple
> checks, just like the other validators.  It isn't worth the discussion,
however,
> so don't worry about it.

Yes, those are minor. I could go either way.

> So, in the interests of avoiding time zone ping pong, LGTM with the following
> strong suggestions:
> 
> 1) Move validator selection in a function.

At first I thought you mean this:
SelectValidator(nap) returns a pointer to one of:
StuboutAndRevalidateX86_32(...)
StuboutAndRevalidateX86_64(...)
ApplyValidatorX86_32(...)
ApplyValidatorX86_64(...)
ApplyValidatorArm(...)

But now it seems like at this stage would be enough to:
SelectValidator(nap) returns a pointer to one of:
NaCl_ApplyValidator_X86_32(...)
NaCl_ApplyValidator_X86_64(...)
NaCl_ApplyValidator_Arm(...)

not sure what to do with these:
  ApplyValidatorCodeReplacement,
  ApplyValidatorCopy

should we set one field for each of them in nap? Seems so.

> 2) Don't create a stub function for the ARM validator.

OK, doing that.

> I would be happy to discuss the design issues outside this CL, but I don't
want
> it to hold the CL up.

I see no problem discussing it here or in another review. If there is a more
suitable place for you, I can probably switch to that.

[Nick Bray, 2012-04-24 00:24:47]

> Oops. I did not think over these details. Sounds pretty bad. We want to
> communicate the difference (url, hash, ret1, ret2) to UMA via an established
> channel to (Chrome? Breakpad?). How hard would that be? Can you point me
briefly
> at the yak shaving spots?

It might be possible to tunnel UMA back to the render process, but this could be
a little tricky because some validation is going to be performed before the
proxy is set up.  The communication channels may be limited, and bsy would be
your best reference on what can actually be sent when.

Alternatively, you could make sel_ldr act like a render process that accumulates
UMA stats and then dumps them directly to the browser process.  Long term I
think we need to do this, but it's going to require a lot of Chrome magic (and
also making sure we don't mess up the address space on 32-bit).  I suggest you
avoid taking this on for the sake of expedience.

But... UMA doesn't actually give you everything you want.  It's designed to
produce histograms with a finite number of buckets.  the best you can do is log
the # of times the validators disagree, but if they do disagree you'll have
problems determining why.  You want to log hash values?  There's a ugly, ugly
hack for doing that that I cannot recommend (4 256 entry histograms, look for
the biggest spikes to determine the hash value of the biggest contributor.)  You
want URLs?  No way to do that.  If you want to know more about the limitations
of UMA, we can chat about it.  It's a special purpose tool with well defined
limitations.

Assuming disagreements are rare, you probably want to create fake crash dumps. 
(See the email I just sent out to nacl-eng).  Doing that from inside the sel_ldr
process will require some yak shaving, however - no one has tried it before.  It
also won't tell you what bytes caused the problem, so either you hope the
disagreements occur on publicly accessible nexes or you figure our how to add
data to the crash dump (there are in memory structures breakpad scrapes that you
can add data to).