Add a scons pnacl finalize step for pnacl_generate_pexe tests.

Add a scons pnacl finalize step for pnacl_generate_pexe tests.

Most of the ABI stability steps run as part of pnacl-ld.
However, some steps such as stripping nonstable debug
metadata can make debugging hard.  Thus, we add one more
step to finalize the ABI.

A new scons Bit 'nonstable_bitcode' can be set by individual tests
to opt out of this prep step if the metadata is needed
(e.g., gdb tests).  The test will still run. Add a
commandline flag 'skip_nonstable_bitcode' if it is required
to skip building and running such tests.

An alternative way to opt out is to make the test part of
the nonpexe_test suite.

We need to roll the archived frontend rev too, since finalize
is run during the build portion to properly work with
triggered bots such as the ARM hw bot.


BUG= https://code.google.com/p/nativeclient/issues/detail?id=3348
TEST= trybots + buildbot/buildbot_pnacl_toolchain_tests.sh archived-frontend-test x86-64

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

[jvoung (off chromium), 2013-03-28 00:08:46]

I could separate the roll, but that is how I tested it =)

[Mark Seaborn, 2013-03-28 01:31:33]

> I could separate the roll, but that is how I tested it =)

Fine to combine them for the trybots, though I'd be inclined to commit the roll
separately (as TBR if necessary), but it's up to you.

https://codereview.chromium.org/12594036/diff/15001/SConstruct
File SConstruct (left):

https://codereview.chromium.org/12594036/diff/15001/SConstruct#oldcode1577
SConstruct:1577: 
Nit: Can you leave 2 empty lines between top-level defs please?  That's the
usual Python style.

https://codereview.chromium.org/12594036/diff/15001/SConstruct
File SConstruct (right):

https://codereview.chromium.org/12594036/diff/15001/SConstruct#newcode1607
SConstruct:1607: final_name = pexe_name[:pexe_name.index('.pexe')] +
'.final.pexe'
You should assert that pexe_name ends with ".pexe" so that this doesn't mangle
the name if it doesn't.

Could we use a slightly different naming convention?  e.g. ".final-pexe", so
that globbing for "*.pexe" and "*.final-pexe" match disjoint sets of files (for
when copying around sets of these manually).

I'd suggest reversing the naming actually: how about ".nonfinal-pexe" or
".unstripped-pexe" and ".pexe"?  We'd want to use a briefer name for the file
that's intended to be stable and published.

https://codereview.chromium.org/12594036/diff/15001/SConstruct#newcode1619
SConstruct:1619: # Bit to be set by individual test/nacl.scons files that need
to opt-out.
Nit: "opt out", no hyphen :-)

https://codereview.chromium.org/12594036/diff/15001/SConstruct#newcode1656
SConstruct:1656: target = env.GetFinalizedPexe(target)
GetFinalizedPexe() is always called after ShouldFinalizePexe(), so why not merge
the two functions?

(The same goes for GetTranslatedNexe() but I'm not asking you to refactor that
now. :-) )

Is there a reason not to have GetTranslatedNexe() call ShouldFinalizePexe() for
you?  There are a few other calls to GetTranslatedNexe() in nacl.scons files --
should these be calling ShouldFinalizePexe()?

https://codereview.chromium.org/12594036/diff/15001/tests/barebones/nacl.scons
File tests/barebones/nacl.scons (right):

https://codereview.chromium.org/12594036/diff/15001/tests/barebones/nacl.scon...
tests/barebones/nacl.scons:26: # a stub function for something that normally
comes from the libraries.
What stub function does barebones_reloc provide that it needs symbols preserved?
 I don't see anything in barebones_reloc.c that should break.

[eliben, 2013-03-28 02:55:38]

> https://codereview.chromium.org/12594036/diff/15001/SConstruct#newcode1607
> SConstruct:1607: final_name = pexe_name[:pexe_name.index('.pexe')] +
> '.final.pexe'
> You should assert that pexe_name ends with ".pexe" so that this doesn't mangle
> the name if it doesn't.
> 
> Could we use a slightly different naming convention?  e.g. ".final-pexe", so
> that globbing for "*.pexe" and "*.final-pexe" match disjoint sets of files
(for
> when copying around sets of these manually).
> 
> I'd suggest reversing the naming actually: how about ".nonfinal-pexe" or
> ".unstripped-pexe" and ".pexe"?  We'd want to use a briefer name for the file
> that's intended to be stable and published.

+1. *If* we plan to have separate extension names, the final one should just be
.pexe since the other option is just burdening the developer with implementation
details. ISTM the name "pexe" is fairly ingrained for us as well, so it would be
nice for it to keep referring to files actually ending with ".pexe".

We should discuss whether a separate extension is required at all, however. In
native code, there's no differentiation between build artifacts (.o, .so and
executables) that are stripped or unstripped, and those with/without debug info
also have the same extension.

[jvoung - send to chromium..., 2013-03-28 15:50:29]

just responding to some comments, will send out a revised CL later

https://codereview.chromium.org/12594036/diff/15001/SConstruct
File SConstruct (left):

https://codereview.chromium.org/12594036/diff/15001/SConstruct#oldcode1577
SConstruct:1577: 
On 2013/03/28 01:31:34, Mark Seaborn wrote:
> Nit: Can you leave 2 empty lines between top-level defs please?  That's the
> usual Python style.

Done.

https://codereview.chromium.org/12594036/diff/15001/SConstruct
File SConstruct (right):

https://codereview.chromium.org/12594036/diff/15001/SConstruct#newcode1619
SConstruct:1619: # Bit to be set by individual test/nacl.scons files that need
to opt-out.
On 2013/03/28 01:31:34, Mark Seaborn wrote:
> Nit: "opt out", no hyphen :-)

Done.

https://codereview.chromium.org/12594036/diff/15001/SConstruct#newcode1656
SConstruct:1656: target = env.GetFinalizedPexe(target)
On 2013/03/28 01:31:34, Mark Seaborn wrote:
> GetFinalizedPexe() is always called after ShouldFinalizePexe(), so why not
merge
> the two functions?

Yes, I think we can merge these.

> 
> (The same goes for GetTranslatedNexe() but I'm not asking you to refactor that
> now. :-) )
> 
> Is there a reason not to have GetTranslatedNexe() call ShouldFinalizePexe()
for
> you?  There are a few other calls to GetTranslatedNexe() in nacl.scons files
--
> should these be calling ShouldFinalizePexe()?

I'd kept them separate because of the ShouldTranslateToNexe() condition is
different, so I didn't want to accidentally leave out a call to
GetFinalizedPexe, just because ShouldTranslateToNexe() is false.

We probably do want to have the finalized pexe for those other nacl.scons files
too, for consistency, so it might make sense to fuse all the Should* with the
Get*, and have one Get...() that does both.

With this file renaming, I'm also a bit worried now about nacl integration tests
making an URL for the non-finalized pexe in the autogenerated NMF files... but
that can probably be fixed in a later CL.

https://codereview.chromium.org/12594036/diff/15001/tests/barebones/nacl.scons
File tests/barebones/nacl.scons (right):

https://codereview.chromium.org/12594036/diff/15001/tests/barebones/nacl.scon...
tests/barebones/nacl.scons:26: # a stub function for something that normally
comes from the libraries.
On 2013/03/28 01:31:34, Mark Seaborn wrote:
> What stub function does barebones_reloc provide that it needs symbols
preserved?
>  I don't see anything in barebones_reloc.c that should break.

barebones_reloc.c stores to a few tls variables.  The -nostdlib clause prevents
gcc / pnacl-translate from linking the native crtbegin, crtend, etc. files, so
it won't have __nacl_read_tp.

[Derek Schuff, 2013-03-28 16:37:18]

> +1. *If* we plan to have separate extension names, the final one should just
be
> .pexe since the other option is just burdening the developer with
implementation
> details. ISTM the name "pexe" is fairly ingrained for us as well, so it would
be
> nice for it to keep referring to files actually ending with ".pexe".
> 
> We should discuss whether a separate extension is required at all, however. In
> native code, there's no differentiation between build artifacts (.o, .so and
> executables) that are stripped or unstripped, and those with/without debug
info
> also have the same extension.

Yes, although the difference there is that both flavors of .o or.so conform to
the relevant system ABIs and will work, which isn't necessarily the same here.
However let's not forget that scons is a tool for our tests and not for
developers, and that we and our tools expect pexes to have pexe extensions. I
think it's sufficient to keep the pexe extension with a known convention
(although making it globbable would be nice. maybe *.unstripped.pexe or
*.nonfinal.pexe and *.final.pexe or *.stable.pexe?). But I don't have a strong
opinion  there.

[Mark Seaborn, 2013-03-28 16:44:32]

https://codereview.chromium.org/12594036/diff/15001/tests/barebones/nacl.scons
File tests/barebones/nacl.scons (right):

https://codereview.chromium.org/12594036/diff/15001/tests/barebones/nacl.scon...
tests/barebones/nacl.scons:26: # a stub function for something that normally
comes from the libraries.
On 2013/03/28 15:50:29, jvoung wrote:
> On 2013/03/28 01:31:34, Mark Seaborn wrote:
> > What stub function does barebones_reloc provide that it needs symbols
> > preserved?  I don't see anything in barebones_reloc.c that should break.
> 
> barebones_reloc.c stores to a few tls variables.  The -nostdlib clause
> prevents gcc / pnacl-translate from linking the native crtbegin, crtend,
> etc. files, so it won't have __nacl_read_tp.

Do you get a link error here?

I'm not sure why stripping should affect this.  barebones_reloc.c defines its
own __nacl_read_tp, but after recent changes that definition should get hidden
and not used by the __nacl_read_tp references that codegen produces.

Having a -nostdlib option in the translator is arguably a bug (see
https://code.google.com/p/nativeclient/issues/detail?id=3069), so maybe you
could add a TODO here?  Or does dropping TRANSLATEFLAGS=['-nostdlib'] fix the
link error?

[jvoung - send to chromium..., 2013-03-28 17:12:12]

On 2013/03/28 16:37:18, Derek Schuff wrote:
> > +1. *If* we plan to have separate extension names, the final one should just
> be
> > .pexe since the other option is just burdening the developer with
> implementation
> > details. ISTM the name "pexe" is fairly ingrained for us as well, so it
would
> be
> > nice for it to keep referring to files actually ending with ".pexe".
> > 
> > We should discuss whether a separate extension is required at all, however.
In
> > native code, there's no differentiation between build artifacts (.o, .so and
> > executables) that are stripped or unstripped, and those with/without debug
> info
> > also have the same extension.
> 
> Yes, although the difference there is that both flavors of .o or.so conform to
> the relevant system ABIs and will work, which isn't necessarily the same here.
> However let's not forget that scons is a tool for our tests and not for
> developers, and that we and our tools expect pexes to have pexe extensions. I
> think it's sufficient to keep the pexe extension with a known convention
> (although making it globbable would be nice. maybe *.unstripped.pexe or
> *.nonfinal.pexe and *.final.pexe or *.stable.pexe?). But I don't have a strong
> opinion  there.


I'm leaning toward having .nonfinal.pexe and .final.pexe.  Then we have a
different suffix for globbing, it's clear which is which, and the extension is
.pexe in both cases.

It would be nice if the final thing was short, but I don't know if it really
matters that much, since it just for SCons as Derek points out.

Just throwing this out there... but we could also add a flag to pnacl-ld that
would run the finalizer during linking, and if you needed the non-final file,
you would run with -save-temps to keep that file, much like how you would use
-save-temps to keep the unoptimized file or the pre-expand files.

That would be the inverse of the -g proposal earlier.

Then we don't need to have two files sitting around which are copied between
bots, etc, and we wouldn't need to add a GetFinalizedPexe() function.  For debug
tests, you would filter out that flag from the scons scripts.

[eliben, 2013-03-28 17:16:29]

On 2013/03/28 17:12:12, jvoung wrote:
> On 2013/03/28 16:37:18, Derek Schuff wrote:
> > > +1. *If* we plan to have separate extension names, the final one should
just
> > be
> > > .pexe since the other option is just burdening the developer with
> > implementation
> > > details. ISTM the name "pexe" is fairly ingrained for us as well, so it
> would
> > be
> > > nice for it to keep referring to files actually ending with ".pexe".
> > > 
> > > We should discuss whether a separate extension is required at all,
however.
> In
> > > native code, there's no differentiation between build artifacts (.o, .so
and
> > > executables) that are stripped or unstripped, and those with/without debug
> > info
> > > also have the same extension.
> > 
> > Yes, although the difference there is that both flavors of .o or.so conform
to
> > the relevant system ABIs and will work, which isn't necessarily the same
here.
> > However let's not forget that scons is a tool for our tests and not for
> > developers, and that we and our tools expect pexes to have pexe extensions.
I
> > think it's sufficient to keep the pexe extension with a known convention
> > (although making it globbable would be nice. maybe *.unstripped.pexe or
> > *.nonfinal.pexe and *.final.pexe or *.stable.pexe?). But I don't have a
strong
> > opinion  there.
> 
> 
> I'm leaning toward having .nonfinal.pexe and .final.pexe.  Then we have a
> different suffix for globbing, it's clear which is which, and the extension is
> .pexe in both cases.
> 
> It would be nice if the final thing was short, but I don't know if it really
> matters that much, since it just for SCons as Derek points out.
> 
> Just throwing this out there... but we could also add a flag to pnacl-ld that
> would run the finalizer during linking, and if you needed the non-final file,
> you would run with -save-temps to keep that file, much like how you would use
> -save-temps to keep the unoptimized file or the pre-expand files.
> 
> That would be the inverse of the -g proposal earlier.
> 
> Then we don't need to have two files sitting around which are copied between
> bots, etc, and we wouldn't need to add a GetFinalizedPexe() function.  For
debug
> tests, you would filter out that flag from the scons scripts.

SGTM. For our scons workflows explicit is best and filename length isn't very
important. How does this affect the developers' flow though?

[jvoung (off chromium), 2013-03-28 17:54:50]

https://codereview.chromium.org/12594036/diff/15001/tests/barebones/nacl.scons
File tests/barebones/nacl.scons (right):

https://codereview.chromium.org/12594036/diff/15001/tests/barebones/nacl.scon...
tests/barebones/nacl.scons:26: # a stub function for something that normally
comes from the libraries.
On 2013/03/28 16:44:33, Mark Seaborn wrote:
> On 2013/03/28 15:50:29, jvoung wrote:
> > On 2013/03/28 01:31:34, Mark Seaborn wrote:
> > > What stub function does barebones_reloc provide that it needs symbols
> > > preserved?  I don't see anything in barebones_reloc.c that should break.
> > 
> > barebones_reloc.c stores to a few tls variables.  The -nostdlib clause
> > prevents gcc / pnacl-translate from linking the native crtbegin, crtend,
> > etc. files, so it won't have __nacl_read_tp.
> 
> Do you get a link error here?

Yes it's a link error, trying to find __nacl_read_tp.


> 
> I'm not sure why stripping should affect this.  barebones_reloc.c defines its
> own __nacl_read_tp, but after recent changes that definition should get hidden
> and not used by the __nacl_read_tp references that codegen produces.

The definition in barebones_reloc.c will get hidden by stripping yes.  However
-nostdlib in translate flags makes this not link in crtbegin, so it won't find
the version from crtbegin after codegen runs?


> 
> Having a -nostdlib option in the translator is arguably a bug (see
> https://code.google.com/p/nativeclient/issues/detail?id=3069), so maybe you
> could add a TODO here?

Well, I guess there is a difference between what these barebones tests are
trying to do, vs trying to match gcc's -nostdlib behavior with PNaCl.  For the
tests, we probably only cared that -nostdlib prevented libc from being linked in
to minimize the code that we need to translate.  For matching gcc, it also means
not linking in crtbegin, libgcc, libgcc_eh, etc.  That part probably doesn't
matter as much for debugging LLVM.


> Or does dropping TRANSLATEFLAGS=['-nostdlib'] fix the
> link error?

No it doesn't fix the link error because of 3069.  If I take out
TRANSLATEFLAGS=['-nostdlib'], then libgcc_eh gets linked in, and I end up with 
problems finding malloc.  I think libgcc_eh gets pulled in because crtbegin
depends on __register_frame_info.

[jvoung (off chromium), 2013-03-28 18:27:38]

On 2013/03/28 17:16:29, eliben wrote:
> On 2013/03/28 17:12:12, jvoung wrote:
> > On 2013/03/28 16:37:18, Derek Schuff wrote:
> > > > +1. *If* we plan to have separate extension names, the final one should
> just
> > > be
> > > > .pexe since the other option is just burdening the developer with
> > > implementation
> > > > details. ISTM the name "pexe" is fairly ingrained for us as well, so it
> > would
> > > be
> > > > nice for it to keep referring to files actually ending with ".pexe".
> > > > 
> > > > We should discuss whether a separate extension is required at all,
> however.
> > In
> > > > native code, there's no differentiation between build artifacts (.o, .so
> and
> > > > executables) that are stripped or unstripped, and those with/without
debug
> > > info
> > > > also have the same extension.
> > > 
> > > Yes, although the difference there is that both flavors of .o or.so
conform
> to
> > > the relevant system ABIs and will work, which isn't necessarily the same
> here.
> > > However let's not forget that scons is a tool for our tests and not for
> > > developers, and that we and our tools expect pexes to have pexe
extensions.
> I
> > > think it's sufficient to keep the pexe extension with a known convention
> > > (although making it globbable would be nice. maybe *.unstripped.pexe or
> > > *.nonfinal.pexe and *.final.pexe or *.stable.pexe?). But I don't have a
> strong
> > > opinion  there.
> > 
> > 
> > I'm leaning toward having .nonfinal.pexe and .final.pexe.  Then we have a
> > different suffix for globbing, it's clear which is which, and the extension
is
> > .pexe in both cases.
> > 
> > It would be nice if the final thing was short, but I don't know if it really
> > matters that much, since it just for SCons as Derek points out.
> > 
> > Just throwing this out there... but we could also add a flag to pnacl-ld
that
> > would run the finalizer during linking, and if you needed the non-final
file,
> > you would run with -save-temps to keep that file, much like how you would
use
> > -save-temps to keep the unoptimized file or the pre-expand files.
> > 
> > That would be the inverse of the -g proposal earlier.
> > 
> > Then we don't need to have two files sitting around which are copied between
> > bots, etc, and we wouldn't need to add a GetFinalizedPexe() function.  For
> debug
> > tests, you would filter out that flag from the scons scripts.
> 
> SGTM. For our scons workflows explicit is best and filename length isn't very
> important. How does this affect the developers' flow though?


For developers' flow, I expect this will mean another build step.  They can name
the files however they like, e.g., foo.nonfinal.pexe and foo.pexe?

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

LGTM with changes below

https://codereview.chromium.org/12594036/diff/46001/SConstruct
File SConstruct (right):

https://codereview.chromium.org/12594036/diff/46001/SConstruct#newcode1569
SConstruct:1569: def GetFinalizedPexe(env, pexe):
Nit: 2 empty lines before this to separate top-level decls

https://codereview.chromium.org/12594036/diff/46001/SConstruct#newcode1575
SConstruct:1575: # There is no bitcode for trusted code.
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.

https://codereview.chromium.org/12594036/diff/46001/SConstruct#newcode1582
SConstruct:1582: final_name = pexe_name[:pexe_name.index('.nonfinal.pexe')] +
'.final.pexe'
I think you missed this before:

On 2013/03/28 01:31:34, Mark Seaborn wrote:
> You should assert that pexe_name ends with ".pexe" so that this
> doesn't mangle the name if it doesn't.

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.

https://codereview.chromium.org/12594036/diff/46001/SConstruct#newcode1618
SConstruct:1618: nexe_name = pexe_name[:pexe_name.index('.pexe')] + '.nexe'
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]

https://codereview.chromium.org/12594036/diff/46001/SConstruct
File SConstruct (right):

https://codereview.chromium.org/12594036/diff/46001/SConstruct#newcode1569
SConstruct:1569: def GetFinalizedPexe(env, pexe):
On 2013/03/28 18:44:17, Mark Seaborn wrote:
> Nit: 2 empty lines before this to separate top-level decls

Done.

https://codereview.chromium.org/12594036/diff/46001/SConstruct#newcode1575
SConstruct:1575: # There is no bitcode for trusted code.
On 2013/03/28 18:44:17, Mark Seaborn wrote:
> 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.


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(

https://codereview.chromium.org/12594036/diff/46001/SConstruct#newcode1582
SConstruct:1582: final_name = pexe_name[:pexe_name.index('.nonfinal.pexe')] +
'.final.pexe'
On 2013/03/28 18:44:17, Mark Seaborn wrote:
> I think you missed this before:
> 
> On 2013/03/28 01:31:34, Mark Seaborn wrote:
> > You should assert that pexe_name ends with ".pexe" so that this
> > doesn't mangle the name if it doesn't.
> 
> 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.

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

https://codereview.chromium.org/12594036/diff/46001/SConstruct#newcode1618
SConstruct:1618: nexe_name = pexe_name[:pexe_name.index('.pexe')] + '.nexe'
On 2013/03/28 18:44:17, Mark Seaborn wrote:
> 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

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

[Derek Schuff, 2013-03-28 19:52:07]

lgtm

[jvoung (off chromium), 2013-03-28 21:11:15]

Committed patchset #4 manually as r11090 (presubmit successful).

[Mark Seaborn, 2013-03-29 15:05:34]

This appears to break the PNaCl tests in nacl_integration when rolled into
Chromium:

scons-out/nacl-x86-32-pnacl-pexe-clang/obj/tests/breakpad_crash_test/breakpad_untrusted_crash_test.out.tmp/crash_test.nmf
failed: Source `scons-out/nacl-x86-32-pnacl-pexe-clang/staging/crash_test.pexe'
not found, needed by target
`scons-out/nacl-x86-32-pnacl-pexe-clang/obj/tests/breakpad_crash_test/breakpad_untrusted_crash_test.out.tmp/crash_test.nmf'.

scons-out/nacl-x86-32-pnacl-pexe-clang/obj/tests/breakpad_crash_test/breakpad_crash_in_syscall_test.out.tmp/crash_in_syscall.nmf
failed: Source
`scons-out/nacl-x86-32-pnacl-pexe-clang/staging/crash_in_syscall.pexe' not
found, needed by target
`scons-out/nacl-x86-32-pnacl-pexe-clang/obj/tests/breakpad_crash_test/breakpad_crash_in_syscall_test.out.tmp/crash_in_syscall.nmf'.
...

See
http://build.chromium.org/p/tryserver.chromium/builders/win_rel_naclmore/buil...
https://codereview.chromium.org/13236006/

Do you want to do a Chromium-side roll + fix?

[jvoung (off chromium), 2013-03-29 15:09:29]

Yes, the renaming will break things.  I'll try to do ta chrome-roll + fix.


On Fri, Mar 29, 2013 at 8:05 AM, <mseaborn@chromium.org> wrote:

> This appears to break the PNaCl tests in nacl_integration when rolled into
> Chromium:
>
> scons-out/nacl-x86-32-pnacl-**pexe-clang/obj/tests/breakpad_**
> crash_test/breakpad_untrusted_**crash_test.out.tmp/crash_test.**nmf
> failed: Source `scons-out/nacl-x86-32-pnacl-**
> pexe-clang/staging/crash_test.**pexe'
> not found, needed by target
> `scons-out/nacl-x86-32-pnacl-**pexe-clang/obj/tests/breakpad_**
> crash_test/breakpad_untrusted_**crash_test.out.tmp/crash_test.**nmf'.
>
> scons-out/nacl-x86-32-pnacl-**pexe-clang/obj/tests/breakpad_**
> crash_test/breakpad_crash_in_**syscall_test.out.tmp/crash_in_**syscall.nmf
> failed: Source
> `scons-out/nacl-x86-32-pnacl-**pexe-clang/staging/crash_in_**syscall.pexe'
> not
> found, needed by target
> `scons-out/nacl-x86-32-pnacl-**pexe-clang/obj/tests/breakpad_**
> crash_test/breakpad_crash_in_**syscall_test.out.tmp/crash_in_**
> syscall.nmf'.
> ...
>
> See
> http://build.chromium.org/p/**tryserver.chromium/builders/**
>
win_rel_naclmore/builds/511<http://build.chromium.org/p/tryserver.chromium/builders/win_rel_naclmore/builds/511>
>
https://codereview.chromium.**org/13236006/<https://codereview.chromium.org/1...
>
> Do you want to do a Chromium-side roll + fix?
>
>
https://codereview.chromium.**org/12594036/<https://codereview.chromium.org/1...
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

[jvoung (off chromium), 2013-03-29 22:13:52]

Hmm the problem is that the nacl_integration tests (indirectly) refer to
published files in the STAGING directory.  So, if we wanted to run the
integration tests over the .final.pexe, we would need to (a) generate those from
the .nonfinal.pexe and (b) publish those.


It looks to that env.Publish() for the pexe/nexes happen as part of
env.ComponentProgram().


So, one idea was to inject a "GetFinalizedPexe()" inside env.ComponentProgram(),
but that seems to break complaining about the same target being created by
multiple environments ("Two different environments were specified for
target...")


We could get GetFinalizedPexe() to env.Publish() the result, but:

(a) We'll need to be sure that it publishes under the same key as the key used
by env.ComponentProgram() in the first place.

(b) We'll need to add calls to env.GetFinalizedPexe() for the browser test
pexes, somewhere, since those tests use PPAPIBrowsererTester().  We have to be
careful where that is called, otherwise we also get the error "Two different
environments were specified for target..."

I'm not sure where is a good place to do that, since some of the browser tests
in chrome borrow a pexe built from nacl, and assume it was published to the
staging directory.  Even if it wasn't cross repo, it's possible that the same
pexe can be used for two different tests.


Temporary fix is: https://codereview.chromium.org/13136004/, which just tests
the .nonfinal.pexe.  


====

What do you guys think about rolling this back, avoiding having both
.nonfinal.pexe vs .final.pexe, and doing something like the other proposal:


"""
Just throwing this out there... but we could also add a flag (-finalize) to
pnacl-ld by default (part of default scons LINKFLAGS) that
would run the finalizer during linking, and if you needed the non-final file,
you would run with -save-temps to keep that file, much like how you would use
-save-temps to keep the unoptimized file or the pre-expand files.

That would be the inverse of the -g proposal earlier.
"""


Otherwise, I'll think more about what else we can do.


On 2013/03/29 15:09:29, jvoung (cr) wrote:
> Yes, the renaming will break things.  I'll try to do ta chrome-roll + fix.
> 
> 
> On Fri, Mar 29, 2013 at 8:05 AM, <mailto:mseaborn@chromium.org> wrote:
> 
> > This appears to break the PNaCl tests in nacl_integration when rolled into
> > Chromium:
> >
> > scons-out/nacl-x86-32-pnacl-**pexe-clang/obj/tests/breakpad_**
> > crash_test/breakpad_untrusted_**crash_test.out.tmp/crash_test.**nmf
> > failed: Source `scons-out/nacl-x86-32-pnacl-**
> > pexe-clang/staging/crash_test.**pexe'
> > not found, needed by target
> > `scons-out/nacl-x86-32-pnacl-**pexe-clang/obj/tests/breakpad_**
> > crash_test/breakpad_untrusted_**crash_test.out.tmp/crash_test.**nmf'.
> >
> > scons-out/nacl-x86-32-pnacl-**pexe-clang/obj/tests/breakpad_**
> > crash_test/breakpad_crash_in_**syscall_test.out.tmp/crash_in_**syscall.nmf
> > failed: Source
> > `scons-out/nacl-x86-32-pnacl-**pexe-clang/staging/crash_in_**syscall.pexe'
> > not
> > found, needed by target
> > `scons-out/nacl-x86-32-pnacl-**pexe-clang/obj/tests/breakpad_**
> > crash_test/breakpad_crash_in_**syscall_test.out.tmp/crash_in_**
> > syscall.nmf'.
> > ...
> >
> > See
> > http://build.chromium.org/p/**tryserver.chromium/builders/**
> >
>
win_rel_naclmore/builds/511<http://build.chromium.org/p/tryserver.chromium/builders/win_rel_naclmore/builds/511>
> >
>
https://codereview.chromium.**org/13236006/%3Chttps://codereview.chromium.org...>
> >
> > Do you want to do a Chromium-side roll + fix?
> >
> >
>
https://codereview.chromium.**org/12594036/%3Chttps://codereview.chromium.org...>
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Native-Client-Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:native-client-reviews+unsubscribe@googlegroups.com.
> To post to this group, send email to
mailto:native-client-reviews@googlegroups.com.
> Visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.
> For more options, visit https://groups.google.com/groups/opt_out.
> 
>