Add ragel machine generators to SCONS

Add ragel decoder/validator to SCONS

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=8202

[pasko-google - do not use, 2012-04-03 15:09:40]

first round of review

non-scons changes look fine, though for build.scons I would suggest inviting
more familiar people such as bradnelson@

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

https://chromiumcodereview.appspot.com/9968039/diff/6001/src/trusted/validato...
src/trusted/validator_ragel/build.scons:56:
env_gen_decoder.FilterOut(CCFLAGS=['-pedantic', '-Wsign-compare'])
filtering out these options smells wrong

https://chromiumcodereview.appspot.com/9968039/diff/6001/src/trusted/validato...
src/trusted/validator_ragel/build.scons:57:
env_gen_decoder.Append(CCFLAGS=['-std=c++0x', '-O3', '-finline-limit=10000',
no reason for doing -O3, only reduces overall time
-finline-limit -- needs explanation in commants

https://chromiumcodereview.appspot.com/9968039/diff/6001/src/trusted/validato...
src/trusted/validator_ragel/build.scons:91: [('${SOURCES[0]} -o ${TARGET} -m %s
-d %s ${SOURCES[1]} '
this manipulation with SOURCES scares me away since I do not know where these
SOURCES came from, is this something internal to scons? can this be done
differently using names rather than numbers?

[halyavin, 2012-04-03 15:15:12]

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

https://chromiumcodereview.appspot.com/9968039/diff/6001/src/trusted/validato...
src/trusted/validator_ragel/build.scons:91: [('${SOURCES[0]} -o ${TARGET} -m %s
-d %s ${SOURCES[1]} '
These are Command parameters (second argument of env.Command).

[khim, 2012-04-03 16:45:52]

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

https://chromiumcodereview.appspot.com/9968039/diff/6001/src/trusted/validato...
src/trusted/validator_ragel/build.scons:57:
env_gen_decoder.Append(CCFLAGS=['-std=c++0x', '-O3', '-finline-limit=10000',
On 2012/04/03 15:09:40, pasko wrote:
> no reason for doing -O3, only reduces overall time
> -finline-limit -- needs explanation in commants

Done.

https://chromiumcodereview.appspot.com/9968039/diff/6001/src/trusted/validato...
src/trusted/validator_ragel/build.scons:91: [('${SOURCES[0]} -o ${TARGET} -m %s
-d %s ${SOURCES[1]} '
On 2012/04/03 15:09:40, pasko wrote:
> this manipulation with SOURCES scares me away since I do not know where these
> SOURCES came from, is this something internal to scons? 

Well, yeah: these are files listed immediately before that line.

can this be done
> differently using names rather than numbers?

I've tried and it looks even worse. If we use SOURCES[N] then scons inself
creates FQFN, otherwise we'll need to it manually.

[Mark Seaborn, 2012-04-03 17:13:37]

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

https://chromiumcodereview.appspot.com/9968039/diff/2004/src/trusted/validato...
src/trusted/validator_ragel/build.scons:15: if not env.Bit('target_x86'):
Return()
Our Python style is to use multiple lines:

if X:
  Y

https://chromiumcodereview.appspot.com/9968039/diff/2004/src/trusted/validato...
src/trusted/validator_ragel/build.scons:17: #
------------------------------------------------------
Please don't put spacers like this in comments.

https://chromiumcodereview.appspot.com/9968039/diff/2004/src/trusted/validato...
src/trusted/validator_ragel/build.scons:20: # Make a copy of debug CRT for now.
This appears to be cargo-culted.  Please remove this CRT stuff from here; we
don't need it.

https://chromiumcodereview.appspot.com/9968039/diff/2004/src/trusted/validato...
src/trusted/validator_ragel/build.scons:30: # TODO(bradchen): eliminate need for
the following line
TODOs are supposed to contain the name of the person who added them, so this
should be "TODO(khim)".

https://chromiumcodereview.appspot.com/9968039/diff/2004/src/trusted/validato...
src/trusted/validator_ragel/build.scons:31: env.FilterOut(CCFLAGS=['-Wextra',
'-Wswitch-enum', '-Wsign-compare'])
Do you really need this?  Can you fix your code to not need these?

https://chromiumcodereview.appspot.com/9968039/diff/2004/src/trusted/validato...
src/trusted/validator_ragel/build.scons:57:
env_gen_dfa.Append(CCFLAGS=['-std=c++0x', '-Wno-sign-compare',
I thought you guys were going to eliminate the use of C++0x...?

https://chromiumcodereview.appspot.com/9968039/diff/2004/src/trusted/validato...
src/trusted/validator_ragel/build.scons:58: '-DNACL_TRUSTED_BUT_NOT_TCB'])
Fix indentation to line up with '['

[khim, 2012-04-03 17:48:04]

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

https://chromiumcodereview.appspot.com/9968039/diff/2004/src/trusted/validato...
src/trusted/validator_ragel/build.scons:15: if not env.Bit('target_x86'):
Return()
On 2012/04/03 17:13:37, Mark Seaborn wrote:
> Our Python style is to use multiple lines:
> 
> if X:
>   Y

Hmm... All validator files use this style, but Ok. Done.

https://chromiumcodereview.appspot.com/9968039/diff/2004/src/trusted/validato...
src/trusted/validator_ragel/build.scons:17: #
------------------------------------------------------
On 2012/04/03 17:13:37, Mark Seaborn wrote:
> Please don't put spacers like this in comments.

Again: our validator's build.scons files use them. Done.

https://chromiumcodereview.appspot.com/9968039/diff/2004/src/trusted/validato...
src/trusted/validator_ragel/build.scons:20: # Make a copy of debug CRT for now.
On 2012/04/03 17:13:37, Mark Seaborn wrote:
> This appears to be cargo-culted.  Please remove this CRT stuff from here; we
> don't need it.

I'm not sure if we need it or not, but it's included in most files which build
binaries. Do you know what breaks if I remove that? Or if nothing breaks then
why we have that?

https://chromiumcodereview.appspot.com/9968039/diff/2004/src/trusted/validato...
src/trusted/validator_ragel/build.scons:31: env.FilterOut(CCFLAGS=['-Wextra',
'-Wswitch-enum', '-Wsign-compare'])
On 2012/04/03 17:13:37, Mark Seaborn wrote:
> Do you really need this?  Can you fix your code to not need these?

I'll check: I think only generator needs these and it has it's own set of
options.

https://chromiumcodereview.appspot.com/9968039/diff/2004/src/trusted/validato...
src/trusted/validator_ragel/build.scons:57:
env_gen_dfa.Append(CCFLAGS=['-std=c++0x', '-Wno-sign-compare',
On 2012/04/03 17:13:37, Mark Seaborn wrote:
> I thought you guys were going to eliminate the use of C++0x...?

No. We've removed C++11 features not implemented in GCC 4.4.x

https://chromiumcodereview.appspot.com/9968039/diff/2004/src/trusted/validato...
src/trusted/validator_ragel/build.scons:58: '-DNACL_TRUSTED_BUT_NOT_TCB'])
On 2012/04/03 17:13:37, Mark Seaborn wrote:
> Fix indentation to line up with '['

Done.

[pasko-google - do not use, 2012-04-05 09:41:37]

https://chromiumcodereview.appspot.com/9968039/diff/13003/src/trusted/validat...
File src/trusted/validator_ragel/build.scons (right):

https://chromiumcodereview.appspot.com/9968039/diff/13003/src/trusted/validat...
src/trusted/validator_ragel/build.scons:64: gen_env.Command(
nit:
see in src/trusted/validator_arm/build.scons:
env.Command(target=blah,
            source=blah,
            action=blah)
improves readability

[pasko-google - do not use, 2012-04-05 11:41:24]

https://chromiumcodereview.appspot.com/9968039/diff/13003/src/trusted/validat...
File src/trusted/validator_ragel/build.scons (right):

https://chromiumcodereview.appspot.com/9968039/diff/13003/src/trusted/validat...
src/trusted/validator_ragel/build.scons:93: ['unreviewed/%s-x86_%s.rl' %
(automata, bits), rl_file],
I think it can happen that after a change in a .def/gen-dfa.cc all .rl files
will be left unchanged, but a -consts.c file has a change. In that case scons
would not notice the change and will refuse to rebuild decoder/validator when it
looks at hashes of files instead of timestamps.

suggestion: add -consts.c as targets or eliminate them, i.e. print everything
into one file (with forward declarations it is super easy).

[khim, 2012-04-05 13:39:33]

PTAL

2BradN: my initial version included cargo-culted piece related to
Microsoft.VC80.DebugCRT and this piece included the following todo:

# Make a copy of debug CRT for now.
# TODO(bradnelson): there should be a better way to generalize this requirement.
# NOTE: debug builds on windows break without this

Mark implied that this should not be needed and I've tried and it works for me
on Windows without this piece.

Do you remember where and why was it important? Buildbots? Some unusual system
configuration?

https://chromiumcodereview.appspot.com/9968039/diff/13003/src/trusted/validat...
File src/trusted/validator_ragel/build.scons (right):

https://chromiumcodereview.appspot.com/9968039/diff/13003/src/trusted/validat...
src/trusted/validator_ragel/build.scons:64: gen_env.Command(
On 2012/04/05 09:41:37, pasko wrote:
> nit:
> see in src/trusted/validator_arm/build.scons:
> env.Command(target=blah,
>             source=blah,
>             action=blah)
> improves readability

Done.

https://chromiumcodereview.appspot.com/9968039/diff/13003/src/trusted/validat...
src/trusted/validator_ragel/build.scons:93: ['unreviewed/%s-x86_%s.rl' %
(automata, bits), rl_file],
If all .rl files are unchanged then -consts.c file is unchanged period.

It can happen that -consts.c file will not change when .rl file is changed, but
this is not a problem.

[pasko-google - do not use, 2012-04-05 14:16:38]

you should also fix the unreviewed/Makefile, it is too early now to break 'make
check'

https://chromiumcodereview.appspot.com/9968039/diff/19001/src/trusted/validat...
File src/trusted/validator_ragel/unreviewed/decoder-test.c (right):

https://chromiumcodereview.appspot.com/9968039/diff/19001/src/trusted/validat...
src/trusted/validator_ragel/unreviewed/decoder-test.c:23: /* We only use
argyuments in assert(3) and that generates warnings in release
arguments

As far as I can see the convention in TCB is to use UNREFERENCED_PARAMETER(),
please, use that

https://chromiumcodereview.appspot.com/9968039/diff/19001/src/trusted/validat...
File src/trusted/validator_ragel/unreviewed/decoder-x86_32.rl (right):

https://chromiumcodereview.appspot.com/9968039/diff/19001/src/trusted/validat...
src/trusted/validator_ragel/unreviewed/decoder-x86_32.rl:16: #define TRUE    1
also fixable with #include "native_client/src/shared/utils/types.h"

https://chromiumcodereview.appspot.com/9968039/diff/19001/src/trusted/validat...
File src/trusted/validator_ragel/unreviewed/decoder.h (right):

https://chromiumcodereview.appspot.com/9968039/diff/19001/src/trusted/validat...
src/trusted/validator_ragel/unreviewed/decoder.h:18: typedef int _Bool;
The convention in other validator sources seems to be using Bool from:
native_client/src/shared/utils/types.h

And you'll "love" how it is defined

https://chromiumcodereview.appspot.com/9968039/diff/19001/src/trusted/validat...
File src/trusted/validator_ragel/unreviewed/gen-dfa.cc (right):

https://chromiumcodereview.appspot.com/9968039/diff/19001/src/trusted/validat...
src/trusted/validator_ragel/unreviewed/gen-dfa.cc:1942: !strncmp(&*it, symbolic,
strlen(symbolic))) {
I do not like how this is formatted. Should be shifted by 4, mot aligned to the
right border.

btw, this can look less ugly if you added in front of the loop:
size_t symbolic_len = strlen(symbolic);

https://chromiumcodereview.appspot.com/9968039/diff/19001/src/trusted/validat...
src/trusted/validator_ragel/unreviewed/gen-dfa.cc:1962: third_byte.length());
spacing, better be:
bool third_byte_ok =
    (arraysize(third_byte_check) == third_byte.length());

https://chromiumcodereview.appspot.com/9968039/diff/19001/src/trusted/validat...
src/trusted/validator_ragel/unreviewed/gen-dfa.cc:1965: ++set_it) {
shiftwidth = 4

[Mark Seaborn, 2012-04-05 14:57:50]

On 5 April 2012 06:39, <khim@chromium.org> wrote:

> PTAL
>
> 2BradN: my initial version included cargo-culted piece related to
> Microsoft.VC80.DebugCRT and this piece included the following todo:
>
>
> # Make a copy of debug CRT for now.
> # TODO(bradnelson): there should be a better way to generalize this
> requirement.
> # NOTE: debug builds on windows break without this
>
> Mark implied that this should not be needed and I've tried and it works
> for me
> on Windows without this piece.
>
> Do you remember where and why was it important? Buildbots? Some unusual
> system
> configuration?
>

I asked Brad N about this recently for a reason unrelated to this review.
It was something to do with debugging "in the field" on a machine with a
different CRT installed and/or a CRT without debugging information
installed.  But given that MSVS and Windbg have the ability to download
debugging info automatically, this is probably obsolete.  Maybe this
Scons/CRT stuff predates this ability in the debuggers.

Copying this CRT stuff into every file is definitely the wrong thing to
do.  If it's going to be done at all, it should be done globally, but it
probably doesn't need to be done at all.

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.

[khim, 2012-04-05 16:38:43]

Small fixes suggested by Egor

PTAL

https://chromiumcodereview.appspot.com/9968039/diff/19001/src/trusted/validat...
File src/trusted/validator_ragel/unreviewed/decoder-test.c (right):

https://chromiumcodereview.appspot.com/9968039/diff/19001/src/trusted/validat...
src/trusted/validator_ragel/unreviewed/decoder-test.c:23: /* We only use
argyuments in assert(3) and that generates warnings in release
On 2012/04/05 14:16:38, pasko wrote:
> arguments
> 
> As far as I can see the convention in TCB is to use UNREFERENCED_PARAMETER(),
> please, use that

Done.

https://chromiumcodereview.appspot.com/9968039/diff/19001/src/trusted/validat...
File src/trusted/validator_ragel/unreviewed/decoder-x86_32.rl (right):

https://chromiumcodereview.appspot.com/9968039/diff/19001/src/trusted/validat...
src/trusted/validator_ragel/unreviewed/decoder-x86_32.rl:16: #define TRUE    1
On 2012/04/05 14:16:38, pasko wrote:
> also fixable with #include "native_client/src/shared/utils/types.h"

Done.

https://chromiumcodereview.appspot.com/9968039/diff/19001/src/trusted/validat...
File src/trusted/validator_ragel/unreviewed/decoder.h (right):

https://chromiumcodereview.appspot.com/9968039/diff/19001/src/trusted/validat...
src/trusted/validator_ragel/unreviewed/decoder.h:18: typedef int _Bool;
On 2012/04/05 14:16:38, pasko wrote:
> The convention in other validator sources seems to be using Bool from:
> native_client/src/shared/utils/types.h
> 
> And you'll "love" how it is defi

Oh yeah. I especially love how Bool is unusable on non-MSVC systems. Ugh.
Separate clean fork for POSIX systems looks more and more feasible...

Done.

https://chromiumcodereview.appspot.com/9968039/diff/19001/src/trusted/validat...
File src/trusted/validator_ragel/unreviewed/gen-dfa.cc (right):

https://chromiumcodereview.appspot.com/9968039/diff/19001/src/trusted/validat...
src/trusted/validator_ragel/unreviewed/gen-dfa.cc:1942: !strncmp(&*it, symbolic,
strlen(symbolic))) {
On 2012/04/05 14:16:38, pasko wrote:
> I do not like how this is formatted. Should be shifted by 4, mot aligned to
the
> right border.
> 
> btw, this can look less ugly if you added in front of the loop:
> size_t symbolic_len = strlen(symbolic);

Whatever. I don't really care about this file (I have clean version anyway).

Done.

https://chromiumcodereview.appspot.com/9968039/diff/19001/src/trusted/validat...
src/trusted/validator_ragel/unreviewed/gen-dfa.cc:1962: third_byte.length());
On 2012/04/05 14:16:38, pasko wrote:
> spacing, better be:
> bool third_byte_ok =
>     (arraysize(third_byte_check) == third_byte.length());

Done.

https://chromiumcodereview.appspot.com/9968039/diff/19001/src/trusted/validat...
src/trusted/validator_ragel/unreviewed/gen-dfa.cc:1965: ++set_it) {
On 2012/04/05 14:16:38, pasko wrote:
> shiftwidth = 4

Done.

[Mark Seaborn, 2012-04-05 16:58:52]

https://chromiumcodereview.appspot.com/9968039/diff/21002/src/trusted/validat...
File src/trusted/validator_ragel/unreviewed/decoder-test.c (right):

https://chromiumcodereview.appspot.com/9968039/diff/21002/src/trusted/validat...
src/trusted/validator_ragel/unreviewed/decoder-test.c:22:
UNREFERENCED_PARAMETER(data);
It's not unreferenced: it's referenced immediately above.  You should be using
CHECK() in trusted code rather than assert() to avoid this problem.  (You could
use DCHECK() but that's not appropriate for a test.)

[Mark Seaborn, 2012-04-05 16:59:45]

Please set BUG= and TEST= fields, BTW.

Mark

[khim, 2012-04-05 17:59:16]

https://chromiumcodereview.appspot.com/9968039/diff/21002/src/trusted/validat...
File src/trusted/validator_ragel/unreviewed/decoder-test.c (right):

https://chromiumcodereview.appspot.com/9968039/diff/21002/src/trusted/validat...
src/trusted/validator_ragel/unreviewed/decoder-test.c:22:
UNREFERENCED_PARAMETER(data);
On 2012/04/05 16:58:53, Mark Seaborn wrote:
> It's not unreferenced: it's referenced immediately above.  You should be using
> CHECK() in trusted code rather than assert() to avoid this problem.  (You
could
> use DCHECK() but that's not appropriate for a test.)

Done.

[pasko-google - do not use, 2012-04-06 11:15:58]

More general comments.

These generated files are missing from this CL:
decoder-x86_32.c
decoder-x86_64.c
validator-x86_64.c

Generated .rl files are not added to generated/ in this CL, but they are put to
generated/ as part of the 'scons valgen'. 'scons valclean' will remove the .rl
files, of course, but also the generated .c files. These generated/*.rl files
would annoyingly pollute the sources, someone may accidentally 'git add' them
while adding a top-level directory. I propose one of the following:
* generated .rl files are also committed to generated/
* generated .rl files are put to scons-out/...

I would prefer to commit generated files as a separate CL to separate meaningful
code from the large chunks. With that I would be able to download a patchset and
try it locally (guess what, I wanted to, but the patchset is too large to be
downloaded). Alternatively, you could prepare a patchset without generated
files, but it is less convenient IMHO.

[khim, 2012-04-06 12:24:11]

All files are in this CL. I'm not responsible for rietveld limitations.

I can remove them from CL if you wish.

[pasko-google - do not use, 2012-04-06 15:23:58]

LGTM for everything except build.scons

https://chromiumcodereview.appspot.com/9968039/diff/22011/src/trusted/validat...
File src/trusted/validator_ragel/unreviewed/decoder-test.c (right):

https://chromiumcodereview.appspot.com/9968039/diff/22011/src/trusted/validat...
src/trusted/validator_ragel/unreviewed/decoder-test.c:18: void
NaClLog_Function(int detail_level, char const  *fmt, ...) {
/* This is a copy of NaClLog_Function from shared/platform/nacl_log.c to avoid
linking in code in NaCl shared code in the unreviewed/Makefile and be able to
use CHECK().

TODO(khim): remove the copy of NaClLog_Function implementation as soon as
unreviewed/Makefile is eliminated.
*/

https://chromiumcodereview.appspot.com/9968039/diff/22011/src/trusted/validat...
File src/trusted/validator_ragel/unreviewed/validator-test.c (right):

https://chromiumcodereview.appspot.com/9968039/diff/22011/src/trusted/validat...
src/trusted/validator_ragel/unreviewed/validator-test.c:18: void
NaClLog_Function(int detail_level, char const  *fmt, ...) {
same TODO here please

https://chromiumcodereview.appspot.com/9968039/diff/22011/src/trusted/validat...
File src/trusted/validator_ragel/unreviewed/validator-x86_64.rl (right):

https://chromiumcodereview.appspot.com/9968039/diff/22011/src/trusted/validat...
src/trusted/validator_ragel/unreviewed/validator-x86_64.rl:461: uint8_t
rex_prefix = FALSE;
uint8_t rex_prefix = 0;

[khim, 2012-04-06 15:30:04]

https://chromiumcodereview.appspot.com/9968039/diff/22011/src/trusted/validat...
File src/trusted/validator_ragel/unreviewed/decoder-test.c (right):

https://chromiumcodereview.appspot.com/9968039/diff/22011/src/trusted/validat...
src/trusted/validator_ragel/unreviewed/decoder-test.c:18: void
NaClLog_Function(int detail_level, char const  *fmt, ...) {
On 2012/04/06 15:23:58, pasko wrote:
> /* This is a copy of NaClLog_Function from shared/platform/nacl_log.c to avoid
> linking in code in NaCl shared code in the unreviewed/Makefile and be able to
> use CHECK().
> 
> TODO(khim): remove the copy of NaClLog_Function implementation as soon as
> unreviewed/Makefile is eliminated.
> */

Done.

https://chromiumcodereview.appspot.com/9968039/diff/22011/src/trusted/validat...
File src/trusted/validator_ragel/unreviewed/validator-test.c (right):

https://chromiumcodereview.appspot.com/9968039/diff/22011/src/trusted/validat...
src/trusted/validator_ragel/unreviewed/validator-test.c:18: void
NaClLog_Function(int detail_level, char const  *fmt, ...) {
On 2012/04/06 15:23:58, pasko wrote:
> same TODO here please

Done.

https://chromiumcodereview.appspot.com/9968039/diff/22011/src/trusted/validat...
File src/trusted/validator_ragel/unreviewed/validator-x86_64.rl (right):

https://chromiumcodereview.appspot.com/9968039/diff/22011/src/trusted/validat...
src/trusted/validator_ragel/unreviewed/validator-x86_64.rl:461: uint8_t
rex_prefix = FALSE;
On 2012/04/06 15:23:58, pasko wrote:
> uint8_t rex_prefix = 0;

Done.

[Nick Bray, 2012-04-07 00:17:46]

Here's my initial reaction, just looking at the scons file and not having much
context.

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

https://chromiumcodereview.appspot.com/9968039/diff/2005/src/trusted/validato...
src/trusted/validator_ragel/build.scons:95:
'check_access,opcode,parse_operands_states,mark_data_fields',
4x spaces.

https://chromiumcodereview.appspot.com/9968039/diff/2005/src/trusted/validato...
src/trusted/validator_ragel/build.scons:102: 'validator64':
Horizontal alignment.

https://chromiumcodereview.appspot.com/9968039/diff/23016/src/trusted/validat...
File src/trusted/validator_ragel/build.scons (right):

https://chromiumcodereview.appspot.com/9968039/diff/23016/src/trusted/validat...
src/trusted/validator_ragel/build.scons:18: # TODO(khim): eliminate need for the
following piece
Document why, and/or link to a bug.

https://chromiumcodereview.appspot.com/9968039/diff/23016/src/trusted/validat...
src/trusted/validator_ragel/build.scons:28: #
Comments for space aren't done anywhere else.  Reformat?

https://chromiumcodereview.appspot.com/9968039/diff/23016/src/trusted/validat...
src/trusted/validator_ragel/build.scons:36: if 'valclean' in
COMMAND_LINE_TARGETS: generate = True
Two lines for each if, merge the conditions if you want.

https://chromiumcodereview.appspot.com/9968039/diff/23016/src/trusted/validat...
src/trusted/validator_ragel/build.scons:67: source=[exe_path,
Put on its own line line so it lines up with its siblings.

https://chromiumcodereview.appspot.com/9968039/diff/23016/src/trusted/validat...
src/trusted/validator_ragel/build.scons:76: '${SOURCES[6]} && { ! test -e %s ||
mv -f %s %s/ ; }') % (
Embedding shell scripts into a SCons action seems questionable because it's
unexpected.  Move it to an external script?  Fold this functionality into
gen-dfa?  Create a custom scons action to do the move that depends on the
generation command?

You probably want to explain why you're going to all this trouble, too.

One factor that is unclear is what SCons commands are supported on what
platforms.  Embedded shell script is an obvious sign, but documentation about
what commands are expected to be run in what contexts would help.  A short
user's manual for the build targets, as it were.

Another issue is that the format string is an absolute bear to understand - you
might want to lift the dict indexing above the action declaration so you can
interpolate a simple, named list of variables.  Simple names would help document
what's actually going on.

[khim, 2012-04-07 13:26:53]

https://chromiumcodereview.appspot.com/9968039/diff/23016/src/trusted/validat...
File src/trusted/validator_ragel/build.scons (right):

https://chromiumcodereview.appspot.com/9968039/diff/23016/src/trusted/validat...
src/trusted/validator_ragel/build.scons:18: # TODO(khim): eliminate need for the
following piece
Created a bug. This is serious work (we've already spent more then week trying
easy fixes thus it's separate work with separate issue).

https://chromiumcodereview.appspot.com/9968039/diff/23016/src/trusted/validat...
src/trusted/validator_ragel/build.scons:28: #
On 2012/04/07 00:17:46, Nick Bray wrote:
> Comments for space aren't done anywhere else.  Reformat?

How? This is copy-pasted from Karl's validator file so I'm not sure what kind of
comments "new rules" demand here.

https://chromiumcodereview.appspot.com/9968039/diff/23016/src/trusted/validat...
src/trusted/validator_ragel/build.scons:36: if 'valclean' in
COMMAND_LINE_TARGETS: generate = True
On 2012/04/07 00:17:46, Nick Bray wrote:
> Two lines for each if, merge the conditions if you want.

Again: this is copy-pasted from Karl's validator. Done.

https://chromiumcodereview.appspot.com/9968039/diff/23016/src/trusted/validat...
src/trusted/validator_ragel/build.scons:67: source=[exe_path,
On 2012/04/07 00:17:46, Nick Bray wrote:
> Put on its own line line so it lines up with its siblings.

Well... It's kinda special, but Ok.

https://chromiumcodereview.appspot.com/9968039/diff/23016/src/trusted/validat...
src/trusted/validator_ragel/build.scons:76: '${SOURCES[6]} && { ! test -e %s ||
mv -f %s %s/ ; }') % (
On 2012/04/07 00:17:46, Nick Bray wrote:
> Embedding shell scripts into a SCons action seems questionable because it's
unexpected.

Why is it unexpected? This is normal practice in make...

> Move it to an external script?  Fold this functionality into
> gen-dfa?  Create a custom scons action to do the move that depends on the
> generation command?
> 
I think it's easier to fold in gen-dfa.c

> You probably want to explain why you're going to all this trouble, too.

Because Egor asked me to do that?

> 
> One factor that is unclear is what SCons commands are supported on what
> platforms.  Embedded shell script is an obvious sign, but documentation about
> what commands are expected to be run in what contexts would help.  A short
> user's manual for the build targets, as it were.
> 
I've added appropriate comment. I *think* it should be possible to make gen-dfa
usable on Windows (if we'll add ragel to third-party for example), I'm just not
sure if it's what we want to spend time doing.

These files must only be touched by someone who knows what he's doing, it's good
idea to run exhaustive tests after that (takes ~3hours on z600 thus it's not a
good idea to add them to buildbots), etc.

> Another issue is that the format string is an absolute bear to understand -
you
> might want to lift the dict indexing above the action declaration so you can
> interpolate a simple, named list of variables.

There are only three arguments now. I've added comments to make more easily
parseable.

> Simple names would help document
> what's actually going on.

We already have "names": each argument comes after gen-dfa.c option.

[pasko-google - do not use, 2012-04-09 08:35:10]

https://chromiumcodereview.appspot.com/9968039/diff/23016/src/trusted/validat...
File src/trusted/validator_ragel/build.scons (right):

https://chromiumcodereview.appspot.com/9968039/diff/23016/src/trusted/validat...
src/trusted/validator_ragel/build.scons:76: '${SOURCES[6]} && { ! test -e %s ||
mv -f %s %s/ ; }') % (
On 2012/04/07 13:26:53, khim wrote:
> On 2012/04/07 00:17:46, Nick Bray wrote:
> > Embedding shell scripts into a SCons action seems questionable because it's
> unexpected.
> 
> Why is it unexpected? This is normal practice in make...
> 
> > Move it to an external script?  Fold this functionality into
> > gen-dfa?  Create a custom scons action to do the move that depends on the
> > generation command?
> > 
> I think it's easier to fold in gen-dfa.c
> 
> > You probably want to explain why you're going to all this trouble, too.
> 
> Because Egor asked me to do that?

I do not quite understand what you guys are referring to. The purpose of "the
trouble" is to take *.def files as input to gen-dfa and produce *.rl files as
output. Maybe Victor means that I asked to not pollute the source directories
with the generated uncommitted .rl files, and gen-dfa generates all files in the
same dir, so we'd have to move some files afterwards.

Well, I do not like embedded shell in scons as well, just because it is a mix of
concepts that are hard to read in the code. A better separation of concepts
would be to call a shell script that takes TARGET, bits, SOURCES as parameters
(directories and other options are wired in), and just call the script here.
gen_dfa_for_platform.sh?

Alternatively we could do it in gen-dfa by providing an extra option where to
put const_file. But I like it less, for verbosity.

Alternatively, get rid of the extra consts_file in gen-dfa. There is no serious
need in generating this file separately, can put all consts in the same .rl
file. For some reason unknown to me Victor does not like this, though I consider
it the most appropriate.

[khim, 2012-04-09 09:02:04]

On 2012/04/09 08:35:10, pasko wrote:
> Alternatively, get rid of the extra consts_file in gen-dfa.

Well, this will increase size of generated code, but yes, it's possible.

> There is no serious need in generating this file separately,
> can put all consts in the same .rl file.

No. This is impossible. We can get rid of them and embed the required code in
actions. Will add few KB to the size of decoder (should not affect validator all
that much since it does not do decoding currently).

> For some reason unknown to me Victor does not like this, though I consider it
the most appropriate.

I don't like code duplication and prefer to have one const instead of hundreds
of them.

[pasko-google - do not use, 2012-04-09 09:35:23]

On 2012/04/09 09:02:04, khim wrote:
> On 2012/04/09 08:35:10, pasko wrote:
> > Alternatively, get rid of the extra consts_file in gen-dfa.
> 
> Well, this will increase size of generated code, but yes, it's possible.

We do not reduce the size by the amount of lines #incuded, right? How big is the
increase for a single file? is it a matter of a single forward declaration or
something less trivial? Maybe you mean the code in the generator?

> > There is no serious need in generating this file separately,
> > can put all consts in the same .rl file.
> 
> No. This is impossible.

Just above you said it would increase the size, now you are saying it's
impossible. Mystery.

> We can get rid of them and embed the required code in
> actions. Will add few KB to the size of decoder (should not affect validator
all
> that much since it does not do decoding currently).
> 
> > For some reason unknown to me Victor does not like this, though I consider
it
> the most appropriate.
> 
> I don't like code duplication and prefer to have one const instead of hundreds
> of them.

Let's discuss it offline.

[pasko-google - do not use, 2012-04-09 09:37:58]

oh by the way, you should add something to EXCLUDE_PROJECT_CHECKS_DIRS in
PRESUBMIT.py

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

https://chromiumcodereview.appspot.com/9968039/diff/28001/src/trusted/validat...
File src/trusted/validator_ragel/build.scons (right):

https://chromiumcodereview.appspot.com/9968039/diff/28001/src/trusted/validat...
src/trusted/validator_ragel/build.scons:124: env.ComponentLibrary('%s-x86_%s' %
(automata, bits),
you suggest a validator library be named like:
validator-x86_64.a

I'd like to:
* distinguish it from the existing validator
* bring more consistency between dash and underscore
* refer to it as 'validate', not 'validator' because:
  (a) it is not a standalone validator, only a library
  (b) the current library of similar purpose is called 'ncvalidate', would be
more natural to people knowing about it

Proposed name:
dfa_validate_x86_64.a

[khim, 2012-04-09 10:30:22]

On 2012/04/09 09:35:23, pasko wrote:
> On 2012/04/09 09:02:04, khim wrote:
> > On 2012/04/09 08:35:10, pasko wrote:
> > > Alternatively, get rid of the extra consts_file in gen-dfa.
> > 
> > Well, this will increase size of generated code, but yes, it's possible.
> 
> We do not reduce the size by the amount of lines #incuded, right? How big is
the
> increase for a single file? is it a matter of a single forward declaration or
> something less trivial? Maybe you mean the code in the generator?
> 
I mean the simple fact: you can not declare consts in generated *-x86_*.rl file.
You can remove declaration of consts entirely and put them in the places where
they are needed, but then each const will be included quite a few files and they
will not be merged (at least GCC 4.4 will not merge all of them; perhaps other
compiler will do that).

> > > There is no serious need in generating this file separately,
> > > can put all consts in the same .rl file.
> > 
> > No. This is impossible.
> 
> Just above you said it would increase the size, now you are saying it's
> impossible. Mystery.
> 
No mystery. Here I've said that you can not stuff consts in generated .rl file -
and this is true. Above I've said that you can get rid of *-consts.c files - and
this is true as well.

> > We can get rid of them and embed the required code in
> > actions. Will add few KB to the size of decoder (should not affect validator
> all
> > that much since it does not do decoding currently).
> > 
> > > For some reason unknown to me Victor does not like this, though I consider
> it
> > the most appropriate.
> > 
> > I don't like code duplication and prefer to have one const instead of
hundreds
> > of them.
> 
> Let's discuss it offline.

Ok.

[pasko-google - do not use, 2012-04-09 10:38:54]

https://chromiumcodereview.appspot.com/9968039/diff/28001/src/trusted/validat...
File src/trusted/validator_ragel/build.scons (right):

https://chromiumcodereview.appspot.com/9968039/diff/28001/src/trusted/validat...
src/trusted/validator_ragel/build.scons:124: env.ComponentLibrary('%s-x86_%s' %
(automata, bits),
On 2012/04/09 10:04:27, pasko wrote:
> you suggest a validator library be named like:
> validator-x86_64.a

hm, in fact it is called libvalidator-x86_64.a, but has to be referenced as
"validator..." in scons files

[khim, 2012-04-09 10:41:52]

On 2012/04/09 10:04:26, pasko wrote:
>
https://chromiumcodereview.appspot.com/9968039/diff/28001/src/trusted/validat...
> File src/trusted/validator_ragel/build.scons (right):
> 
>
https://chromiumcodereview.appspot.com/9968039/diff/28001/src/trusted/validat...
> src/trusted/validator_ragel/build.scons:124: env.ComponentLibrary('%s-x86_%s'
%
> (automata, bits),
> you suggest a validator library be named like:
> validator-x86_64.a
> 
> I'd like to:
> * distinguish it from the existing validator
> * bring more consistency between dash and underscore
> * refer to it as 'validate', not 'validator' because:
>   (a) it is not a standalone validator, only a library
>   (b) the current library of similar purpose is called 'ncvalidate', would be
> more natural to people knowing about it
> 
> Proposed name:
> dfa_validate_x86_64.a

I've followed GNU conventions so far WRT dashes and underscores (for example
full name compiler in Googbuntu is x86_64-linux-gnu-cpp-4.4, not
x86_64_linux_gnu_cpp_4.4 and not x86-64-linux-gnu-cpp-4.4) but looks like NaCl
uses underscores everywhere.

Should we rename source files, too?

[pasko-google - do not use, 2012-04-09 11:28:07]

On 2012/04/09 10:41:52, khim wrote:
> I've followed GNU conventions so far WRT dashes and underscores (for example
> full name compiler in Googbuntu is x86_64-linux-gnu-cpp-4.4, not
> x86_64_linux_gnu_cpp_4.4 and not x86-64-linux-gnu-cpp-4.4) but looks like NaCl
> uses underscores everywhere.
> 
> Should we rename source files, too?

probably yes, though this is bad news

[khim, 2012-04-09 15:17:11]

PTAL

I don't want to add renaming of source files in this CL, it's already busy
enough.

[Nick Bray, 2012-04-09 19:04:36]

SCons looks good.

> Why is it unexpected? This is normal practice in make...

Principle of least astonishment.

http://en.wikipedia.org/wiki/Principle_of_least_astonishment

I am unaware of any place in our build system that chains shell commands inside
of a SCons Command.  I believe this is a result of trying to keep the build
specification platform neutral.  "Normal practice" depends entirely on where
you're standing.

https://chromiumcodereview.appspot.com/9968039/diff/28004/src/trusted/validat...
File src/trusted/validator_ragel/build.scons (right):

https://chromiumcodereview.appspot.com/9968039/diff/28004/src/trusted/validat...
src/trusted/validator_ragel/build.scons:77: target=rl_file,
Not necessary, but you might be able to make this a list of the two targets and
save yourself some comments?  I am pretty sure Commands can have multiple
outputs.  If you're not sure -consts will exist, however, you're probably doing
the right thing, but you might want to mention that oddity.

[khim, 2012-04-09 19:13:29]

https://chromiumcodereview.appspot.com/9968039/diff/28004/src/trusted/validat...
File src/trusted/validator_ragel/build.scons (right):

https://chromiumcodereview.appspot.com/9968039/diff/28004/src/trusted/validat...
src/trusted/validator_ragel/build.scons:77: target=rl_file,
Today -const.c file is created 3 times out of 4, but logic which determines if
it's created or not sits in gen-dfa.c (and depends on the arguments supplied to
it!) and I don't want to duplicate it in build.scons

[Mark Seaborn, 2012-04-09 19:31:17]

This broke the build.  You don't seem to have run any try jobs, at least
according to Rietveld.  *Please do not commit without try runs.*

Mark