Add ragel machine generators to SCONS

src/trusted/validator_ragel/build.scons

Index: src/trusted/validator_ragel/build.scons
===================================================================
--- src/trusted/validator_ragel/build.scons	(revision 0)
+++ src/trusted/validator_ragel/build.scons	(revision 0)
@@ -0,0 +1,120 @@
+# -*- python -*-
+# Copyright (c) 2012 The Native Client Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+Import('env')
+
+#
+#
+# Build on x86 only.
+#
+#
+if not env.Bit('target_x86'):
+  Return()
+
+# General adjustments to the environment for builds.
+
+# TODO(khim): eliminate need for the following piece

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

Document why, and/or link to a bug.

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

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).

+if env.Bit('windows'):
+  env.FilterOut(CCFLAGS=['/O1'])
+  env.Append(CCFLAGS=['/O0'])
+
+# Defines the source directory where validator generated files should be added.
+rl_src_dir = '$OBJ_ROOT/src/trusted/validator_ragel'
+val_src_dir = '$MAIN_DIR/src/trusted/validator_ragel/generated'
+
+# Source generation:
+#

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

Comments for space aren't done anywhere else.  Reformat?

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

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

+#   valgen : Regenerate any autogenerated source files.
+
+gen_env = env.Clone();
+gen_env.Append(CCFLAGS=['-DNACL_TRUSTED_BUT_NOT_TCB'])
+
+generate = False
+if 'valgen' in COMMAND_LINE_TARGETS: generate = True
+if 'valclean' in COMMAND_LINE_TARGETS: generate = True

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

Two lines for each if, merge the conditions if you want.

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

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

+
+# Set of generated (source) decoder tables.
+tables = []
+
+# Ragel generator:
+#
+# We have generator which reads .def files and produced automata definition.
+
+env_gen_dfa = env.Clone()
+env_gen_dfa.Append(CCFLAGS=['-std=c++0x', '-DNACL_TRUSTED_BUT_NOT_TCB'])
+
+gen_dfa = env_gen_dfa.ComponentProgram(
+    'gen-dfa',
+    ['unreviewed/gen-dfa.cc'])
+
+#  Source generation step 2: Generate decoder tables.
+#
+# Now we are back to conditionally defining the large tables generated
+# by gen-dfa.
+
+# Generate 32 and 64 bit versions of decoders and validators.
+if generate:
+  for bits in ['32', '64']:
+    for automata in ['decoder', 'validator']:
+      rl_file = '%s-x86_%s-instruction.rl' % (automata, bits)
+      const_file = '%s/%s-x86_%s-instruction-consts.c' % (
+          rl_src_dir, automata, bits)
+      exe_path = '${STAGING_DIR}/${PROGPREFIX}gen-dfa${PROGSUFFIX}'
+      gen_env.Command(
+          target=rl_file,
+          source=[exe_path,

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

Put on its own line line so it lines up with its siblings.

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

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

+            'unreviewed/general-purpose-instructions.def',
+            'unreviewed/system-instructions.def',
+            'unreviewed/x87-instructions.def',
+            'unreviewed/mmx-instructions.def',
+            'unreviewed/xmm-instructions.def',
+            'unreviewed/nops.def'],
+          action=[('${SOURCES[0]} -o ${TARGET} -m %s -d %s ${SOURCES[1]} '
+            '${SOURCES[2]} ${SOURCES[3]} ${SOURCES[4]} ${SOURCES[5]} '
+            '${SOURCES[6]} && { ! test -e %s || mv -f %s %s/ ; }') % (

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

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]

Why is it unexpected? This is normal practice in make...
I think it's easier to fold in gen-dfa.c
Because Egor asked me to do that?
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.
There are only three arguments now. I've added comments to make more easily
parseable.
We already have "names": each argument comes after gen-dfa.c option.

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

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.

+            {'32': 'ia32', '64': 'amd64'}[bits],
+            {'decoder32':
+              'check_access,opcode,parse_operands_states,mark_data_fields',
+             'decoder64':
+              'check_access,opcode,parse_operands_states,mark_data_fields',
+             'validator32':
+              ('check_access,opcode,parse_operands,parse_operands_states,'
+                'instruction_name,mark_data_fields,nacl-forbidden,'
+                'imm_operand_action,rel_operand_action'),
+            'validator64':
+              'opcode,instruction_name,mark_data_fields,rel_operand_action,' +
+              'nacl-forbidden'}[automata+bits],
+            const_file, const_file, val_src_dir)]
+      )
+      tables.append(rl_file)
+      c_file = '%s/%s-x86_%s.c' % (val_src_dir, automata, bits)
+      gen_env.Command(
+          target=c_file,
+          source=['unreviewed/%s-x86_%s.rl' % (automata, bits), rl_file],
+          action=['ragel -G2 -I%s ${SOURCES[0]} -o ${TARGET}' % (rl_src_dir)]
+      )
+      tables.append(c_file)
+
+  # Generate 32 and 64 bit versions of decoders and validators
+  gen_env.AlwaysBuild(
+      gen_env.Alias('valgen', tables))
+  gen_env.AlwaysBuild(
+      gen_env.Alias('valclean', action=[Delete(x) for x in tables]))
+
+# Generate 32 and 64 bit versions of decoders and validators.
+for bits in ['32', '64']:
+  for automata in ['decoder', 'validator']:
+    env.ComponentLibrary('%s-x86_%s' % (automata, bits),
+                         ['generated/%s-x86_%s.c' % (automata, bits)])
+
+env.ComponentProgram(
+    'decoder-test',
+    ['unreviewed/decoder-test.c'],
+    EXTRA_LIBS=['decoder-x86_32', 'decoder-x86_64'])
+
+env.ComponentProgram(
+    'validator-test',
+    ['unreviewed/validator-test.c'],
+    EXTRA_LIBS=['validator-x86_32', 'validator-x86_64'])