Add a scons pnacl finalize step for pnacl_generate_pexe tests.

SConstruct

Index: SConstruct
diff --git a/SConstruct b/SConstruct
index 55b483b47c6efd03ddac58a1badca3270dbe8049..cfd94d376a8563d991f4a5b370cba038e38912da 100755
--- a/SConstruct
+++ b/SConstruct
@@ -352,6 +352,9 @@ def SetUpArgumentBits(env):
   BitFromArgument(env, 'arm_hard_float', default=False,
                   desc='Build for hard float ARM ABI')
 
+  BitFromArgument(env, 'skip_nonstable_bitcode', default=False,
+                  desc='Skip tests involving non-stable bitcode')
+
   #########################################################################
   # EXPERIMENTAL
   # This is for generating a testing library for use within private test
@@ -1560,31 +1563,46 @@ pre_base_env.AddMethod(ShouldUseVerboseOptions)
 
 DeclareBit('tests_use_irt', 'Non-browser tests also load the IRT image', False)
 
-# Translate the given pexe.
-def GetTranslatedNexe(env, pexe):
+# Bit to be set by individual test/nacl.scons files that need to opt out.
+DeclareBit('nonstable_bitcode', 'Tests use non-stable bitcode features', False)
+
+def GetFinalizedPexe(env, pexe):

[Mark Seaborn, 2013/03/28 18:44:17]

Nit: 2 empty lines before this to separate top-level decls

[jvoung (off chromium), 2013/03/28 19:45:31]

Done.

+  """ Prep and finalize the ABI for a given pexe if needed.
+  """
+  if not env.Bit('pnacl_generate_pexe') or env.Bit('nonstable_bitcode'):
+    return pexe
+
+  # There is no bitcode for trusted code.

[Mark Seaborn, 2013/03/28 18:44:17]

BTW, is this check necessary?  Surely no pexes can be generated in a trusted env
anyway?  You could make this an assertion if you think having a check is useful.

[jvoung (off chromium), 2013/03/28 19:45:31]

There are a couple of CommandSelLdrTestNaCl() coming from one build.scons file,
where the nexe is a canned nexe (old_abi, negative_hole, etc.):

find src/ -name "build.scons" | xargs grep "CommandSelLdrTestNacl"
src/trusted/service_runtime/build.scons:node = env.CommandSelLdrTestNacl(
src/trusted/service_runtime/build.scons:node = env.CommandSelLdrTestNacl(
src/trusted/service_runtime/build.scons:  node = env.CommandSelLdrTestNacl(
src/trusted/service_runtime/build.scons:  node = env.CommandSelLdrTestNacl(
src/trusted/service_runtime/build.scons:node =
env.CommandSelLdrTestNacl('unaligned_data.out', unaligned_data_nexe)
src/trusted/service_runtime/build.scons:node = env.CommandSelLdrTestNacl(
src/trusted/service_runtime/build.scons:  node = env.CommandSelLdrTestNacl(
src/trusted/service_runtime/build.scons:  node = env.CommandSelLdrTestNacl(
src/trusted/service_runtime/build.scons:  node = env.CommandSelLdrTestNacl(

+  if env['NACL_BUILD_FAMILY'] == 'TRUSTED':
+    return pexe
+
+  # Otherwise, finalize during the build step, since there is no finalize tool
+  # that can run on triggered bots such as the ARM HW bots.
   pexe_name = pexe.abspath
-  nexe_name = pexe_name[:pexe_name.index('.pexe')] + '.nexe'
+  final_name = pexe_name[:pexe_name.index('.nonfinal.pexe')] + '.final.pexe'

[Mark Seaborn, 2013/03/28 18:44:17]

I think you missed this before:

On 2013/03/28 01:31:34, Mark Seaborn wrote:
Looking at this again, using .index() makes this a little safer, but dodgy in a
different way, because ".nonfinal.pexe" *could* appear somewhere other than the
end of the string.

[jvoung (off chromium), 2013/03/28 19:45:31]

Ah yes, the StripSuffix helper is safer -- using the asserting version now.

   # Make sure the pexe doesn't get removed by the fake builders when
   # built_elsewhere=1
   env.Precious(pexe)
-  node = env.Command(target=nexe_name, source=[pexe_name],
-                     action=[Action('${TRANSLATECOM}', '${TRANSLATECOMSTR}')])
+  node = env.Command(target=final_name, source=[pexe_name],
+                     action=[Action('${PNACLFINALIZECOM}',
+                                    '${PNACLFINALIZECOMSTR}')])
   assert len(node) == 1, node
   return node[0]
 
-pre_base_env.AddMethod(GetTranslatedNexe)
 
+# Translate the given pexe.
+def GetTranslatedNexe(env, pexe):
+  # First finalize the pexe.
+  pexe = GetFinalizedPexe(env, pexe)
 
-def ShouldTranslateToNexe(env, pexe):
-  """ Determine when we need to translate a Pexe to a Nexe.
-  """
+  # Then check if we need to translate.
   # Check if we started with a pexe, so there is actually a translation step.
   if not env.Bit('pnacl_generate_pexe'):
-    return False
+    return pexe
 
   # There is no bitcode for trusted code.
   if env['NACL_BUILD_FAMILY'] == 'TRUSTED':
-    return False
+    return pexe
 
   # Often there is a build step (do_not_run_tests=1) and a test step
   # (which is run with -j1). Normally we want to translate in the build step
@@ -1593,10 +1611,20 @@ def ShouldTranslateToNexe(env, pexe):
   # to force the translation to be done in the test step. Hence,
   # we check the bit 'translate_in_build_step' / check if we are
   # in the test step.
-  return (
-    env.Bit('translate_in_build_step') or not env.Bit('do_not_run_tests'))
+  if not env.Bit('translate_in_build_step') and env.Bit('do_not_run_tests'):
+    return pexe
 
-pre_base_env.AddMethod(ShouldTranslateToNexe)
+  pexe_name = pexe.abspath
+  nexe_name = pexe_name[:pexe_name.index('.pexe')] + '.nexe'

[Mark Seaborn, 2013/03/28 18:44:17]

Can you add an assertion here too, please?
i.e.
assert pexe_name.endswith('.pexe')

Also, should you be stripping off the full ".{non,}final.pexe" suffix?  That
would make the .nexe names more concise.

You could add a helper function:

def StripSuffix(string, suffix):
  assert string.endswith(suffix)
  return string[:-len(suffix)]

or

def StripSuffix(string, suffix):
  if string.endswith(suffix):
    return string[:-len(suffix)]
  return string

[jvoung (off chromium), 2013/03/28 19:45:31]

Done (using the asserting version), and simplified the nexe name.

+  # Make sure the pexe doesn't get removed by the fake builders when
+  # built_elsewhere=1
+  env.Precious(pexe)
+  node = env.Command(target=nexe_name, source=[pexe_name],
+                     action=[Action('${TRANSLATECOM}', '${TRANSLATECOMSTR}')])
+  assert len(node) == 1, node
+  return node[0]
+
+pre_base_env.AddMethod(GetTranslatedNexe)
 
 
 def CommandTestFileDumpCheck(env,
@@ -1614,12 +1642,9 @@ def CommandTestFileDumpCheck(env,
   # ARM objdump though... a TODO(jvoung) for when there is time.
   if env.Bit('built_elsewhere'):
     return []
-  if env.ShouldTranslateToNexe(target):
-    target_obj = env.GetTranslatedNexe(target)
-  else:
-    target_obj = target
+  target = env.GetTranslatedNexe(target)
   return env.CommandTestFileCheck(name,
-                                  ['${OBJDUMP}', objdump_flags, target_obj],
+                                  ['${OBJDUMP}', objdump_flags, target],
                                   check_file)
 
 pre_base_env.AddMethod(CommandTestFileDumpCheck)
@@ -1660,9 +1685,9 @@ def CommandSelLdrTestNacl(env, name, nexe,
       env['TRUSTED_ENV'].Bit('windows')):
     return []
 
-  if env.ShouldTranslateToNexe(nexe):
-    # The nexe is actually a pexe.  Translate it before we run it.
-    nexe = env.GetTranslatedNexe(nexe)
+  # The nexe might be a pexe that needs finalization, and translation.
+  nexe = env.GetTranslatedNexe(nexe)
+
   command = [nexe]
   if args is not None:
     command += args