Revert 8203: Revert 8202 - Add ragel decoder/validator to SCONS

Redo 8202 - Add ragel decoder/validator to SCONS

This time includes all required files and passes trybots

BUG=http://code.google.com/p/nativeclient/issues/detail?id=2597
TEST=scons decoder-test validator-test ; ls -al scons-out/*/staging
Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=8214

[Nick Bray, 2012-04-09 21:48:52]

LGTM w/ commit message fixes assuming the actual changes are trivial and do not
need to be reviewed.

1) What were the changes?  I cannot incrementally review the CL without knowing.
 Document in the commit message.

2) Nit: you aren't really reverting a revert (this is misleading), you're
recommiting with fixes.  Documenting that the previous CL was reverted in the
commit message is good, however.

[khim, 2012-04-09 22:05:04]

Changed the description. Will commit tomorrow when (hopefully) trybots will
pass.

On Tue, Apr 10, 2012 at 1:48 AM, <ncbray@google.com> wrote:

> LGTM w/ commit message fixes assuming the actual changes are trivial and
> do not
> need to be reviewed.
>
> 1) What were the changes?  I cannot incrementally review the CL without
> knowing.
>  Document in the commit message.
>
> 2) Nit: you aren't really reverting a revert (this is misleading), you're
> recommiting with fixes.  Documenting that the previous CL was reverted in
> the
> commit message is good, however.
>
>
https://chromiumcodereview.**appspot.com/9967015/<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.
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-04-09 22:15:50]

On 9 April 2012 14:48, <ncbray@google.com> wrote:

> LGTM w/ commit message fixes assuming the actual changes are trivial and
> do not
> need to be reviewed.
>
> 1) What were the changes?  I cannot incrementally review the CL without
> knowing.
>  Document in the commit message.
>

Even better is to upload the original version (that is, with "git cl
upload" or gcl) and then upload the new version, then it's possible to
compare the two in Rietveld.

Mark

-- 
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-04-10 05:37:11]

On 2012/04/09 21:48:52, Nick Bray wrote:
> LGTM w/ commit message fixes assuming the actual changes are trivial and do
not
> need to be reviewed.
> 
> 1) What were the changes?  I cannot incrementally review the CL without
knowing.
>  Document in the commit message.

I suppose it's the generated files that were added this time.

> 2) Nit: you aren't really reverting a revert (this is misleading), you're
> recommiting with fixes.  Documenting that the previous CL was reverted in the
> commit message is good, however.

+t, saying "Revert" two times in the first line of the commit message does not
sound neat for the commit log. Especially if it's not true.

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

gen-decoder.cc must *not* be here

you ignored my suggestion for editing EXCLUDE_PROJECT_CHECKS_DIRS in
PRESUBMIT.py, comment or fix

[khim, 2012-04-10 08:14:43]

PTAL

I'll run tests when trybots will regain the ability to run them...

[khim, 2012-04-10 08:14:44]

PTAL

I'll run tests when trybots will regain the ability to run them...

[pasko-google - do not use, 2012-04-10 09:32:41]

https://chromiumcodereview.appspot.com/9967015/diff/5024/src/trusted/validato...
File src/trusted/validator_ragel/build.scons (right):

https://chromiumcodereview.appspot.com/9967015/diff/5024/src/trusted/validato...
src/trusted/validator_ragel/build.scons:32:
gen_env.Append(CCFLAGS=['-DNACL_TRUSTED_BUT_NOT_TCB'])
gen_env only runs ragel and gen-dfa, there should be no need in defining CCFLAGS
in it

https://chromiumcodereview.appspot.com/9967015/diff/5024/src/trusted/validato...
src/trusted/validator_ragel/build.scons:40: tables = []
just noticed: the naming is hm .. copypasted, we do not have decoder tables
(term incorrectly borrowed from the old validator), we have machines processed
by ragel (c_machines? gen_list?).

for later cleanup

https://chromiumcodereview.appspot.com/9967015/diff/5024/src/trusted/validato...
src/trusted/validator_ragel/build.scons:58: #  Source generation step 2:
Generate decoder tables.
there is step 2, but there is no step 1

the comment is useless

https://chromiumcodereview.appspot.com/9967015/diff/5024/src/trusted/validato...
src/trusted/validator_ragel/build.scons:66: # We are cheating here: there are
two autogenerated files:
thank you for this comment

https://chromiumcodereview.appspot.com/9967015/diff/5024/src/trusted/validato...
File src/trusted/validator_ragel/unreviewed/decoder-test.c (right):

https://chromiumcodereview.appspot.com/9967015/diff/5024/src/trusted/validato...
src/trusted/validator_ragel/unreviewed/decoder-test.c:23: *unreviewed/Makefile
is eliminated.
add space after *

https://chromiumcodereview.appspot.com/9967015/diff/5024/src/trusted/validato...
src/trusted/validator_ragel/unreviewed/decoder-test.c:1480:
(size_t)section->sh_size, ProcessInstruction, ProcessError, &state);
should be either shifted by 4 from the previous line or aligned to the first
parameter in the previous line. I prefer the latter.

https://chromiumcodereview.appspot.com/9967015/diff/5024/src/trusted/validato...
File src/trusted/validator_ragel/unreviewed/gen-dfa.cc (right):

https://chromiumcodereview.appspot.com/9967015/diff/5024/src/trusted/validato...
src/trusted/validator_ragel/unreviewed/gen-dfa.cc:2486: const_file_name = NULL;
why assigning NULL? the value is no longer referenced

[khim, 2012-04-10 12:29:38]

https://chromiumcodereview.appspot.com/9967015/diff/5024/src/trusted/validato...
File src/trusted/validator_ragel/build.scons (right):

https://chromiumcodereview.appspot.com/9967015/diff/5024/src/trusted/validato...
src/trusted/validator_ragel/build.scons:32:
gen_env.Append(CCFLAGS=['-DNACL_TRUSTED_BUT_NOT_TCB'])
On 2012/04/10 09:32:41, pasko wrote:
> gen_env only runs ragel and gen-dfa, there should be no need in defining
CCFLAGS
> in it

This exactly why we need to define NACL_TRUSTED_BUT_NOT_TCB here and only here.

Recall where this define is used.

https://chromiumcodereview.appspot.com/9967015/diff/5024/src/trusted/validato...
src/trusted/validator_ragel/build.scons:40: tables = []
On 2012/04/10 09:32:41, pasko wrote:
> just noticed: the naming is hm .. copypasted, we do not have decoder tables
> (term incorrectly borrowed from the old validator), we have machines processed
> by ragel (c_machines? gen_list?).
> 
> for later cleanup

I've replaced tables with automatas...

https://chromiumcodereview.appspot.com/9967015/diff/5024/src/trusted/validato...
src/trusted/validator_ragel/build.scons:58: #  Source generation step 2:
Generate decoder tables.
On 2012/04/10 09:32:41, pasko wrote:
> there is step 2, but there is no step 1
> 
> the comment is useless

It's not useless. The fact that step 1 is not named as step 1 is problematic.
Fixed.

https://chromiumcodereview.appspot.com/9967015/diff/5024/src/trusted/validato...
File src/trusted/validator_ragel/unreviewed/decoder-test.c (right):

https://chromiumcodereview.appspot.com/9967015/diff/5024/src/trusted/validato...
src/trusted/validator_ragel/unreviewed/decoder-test.c:23: *unreviewed/Makefile
is eliminated.
On 2012/04/10 09:32:41, pasko wrote:
> add space after *

Done.

https://chromiumcodereview.appspot.com/9967015/diff/5024/src/trusted/validato...
src/trusted/validator_ragel/unreviewed/decoder-test.c:1480:
(size_t)section->sh_size, ProcessInstruction, ProcessError, &state);
On 2012/04/10 09:32:41, pasko wrote:
> should be either shifted by 4 from the previous line or aligned to the first
> parameter in the previous line. I prefer the latter.

This will create additional useless line. Shift it is.

https://chromiumcodereview.appspot.com/9967015/diff/5024/src/trusted/validato...
File src/trusted/validator_ragel/unreviewed/gen-dfa.cc (right):

https://chromiumcodereview.appspot.com/9967015/diff/5024/src/trusted/validato...
src/trusted/validator_ragel/unreviewed/gen-dfa.cc:2486: const_file_name = NULL;
On 2012/04/10 09:32:41, pasko wrote:
> why assigning NULL? the value is no longer referenced

Exactly to make sure it'll never be referenced and dangling pointer will not
hurt us.

[pasko-google - do not use, 2012-04-10 13:50:53]

lgtm

[bbudge-google, 2012-04-10 17:34:41]

On 2012/04/10 13:50:53, pasko wrote:
> lgtm

The try run is completely red. It looks like the update failed but you should
wait until you can get a good try run before committing. I reverted the CL
because the Mac scons compile is timing out.

[bbudge-google, 2012-04-10 17:35:35]

Also, file properties seem to be messed up.

[Mark Seaborn, 2012-04-10 17:36:01]

This change broke the build.  This one fails on Mac OS X 10.7 and I can see you
have failing try runs that indicate that.  Again, **please do not commit changes
that don't pass on the trybots**.  None of the patch sets on this code review
have successful try runs.

Mark

[pasko-google - do not use, 2012-04-11 10:09:19]

On 2012/04/10 17:36:01, Mark Seaborn wrote:
> This change broke the build.  This one fails on Mac OS X 10.7 and I can see
you
> have failing try runs that indicate that.  Again, **please do not commit
changes
> that don't pass on the trybots**.  None of the patch sets on this code review
> have successful try runs.

Yes, committing with trybots red is not beautiful. Though, as you can see, most
of the trybot failures were due to the some mysterious problems with SVN
checkout, which were unrelated to the matter of the commit, and specific to only
this change, probably because of its size. There was a hope that buildbots would
not repeat the massive failures and it was proven true. A practical way to check
build correctness on all platforms is to use buildbots. I do not know any other.

Victor, please, communicate such complicated deals with sheriff in the future. I
do not like reading English text with double stars.