Add nacl-clang testing to SCons

Add nacl-clang testing to SCons

This CL adds a nacl_clang flag to SCons to use nacl-clang.

It adds an implicit dependence on -lpthread to nexes because nacl-clang
always links with it (because libc++ requires it). It's also enabled
for PNaCl; if this ends up working out well for nacl-clang maybe we can
remove the really ugly special-casing for pthread_private in pnacl-ld.

It modifies or disables a few tests, so that everything now build with
nacl-clang (or has a bug filed for why not).

It also ifdefs out the .init and .fini sections from crti/crtn, because
we do not use them with clang.

R=jvoung@chromium.org, mcgrathr@chromium.org, ncbray@chromium.org
BUG= https://code.google.com/p/nativeclient/issues/detail?id=3946

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

[Derek Schuff, 2014-10-08 20:09:50]

dschuff@chromium.org changed reviewers:
+ mcgrathr@chromium.org

[Derek Schuff, 2014-10-08 20:09:51]

+mcgrathr

[jvoung (off chromium), 2014-10-08 22:40:23]

https://codereview.chromium.org/639113003/diff/1/tests/threads/race_test.c
File tests/threads/race_test.c (right):

https://codereview.chromium.org/639113003/diff/1/tests/threads/race_test.c#ne...
tests/threads/race_test.c:53: foo = 0;
Could just make this one case w/out __asm__?

https://codereview.chromium.org/639113003/diff/1/tests/toolchain/synchronizat...
File tests/toolchain/synchronization_cpp11.cc (right):

https://codereview.chromium.org/639113003/diff/1/tests/toolchain/synchronizat...
tests/toolchain/synchronization_cpp11.cc:70:
static_assert(ATOMIC_POINTER_LOCK_FREE == 2, "should be compile-time 2");
#else ...

// TODO other architectures like mips.

Eventually this could just #error so people know to fill it in?

[Derek Schuff, 2014-10-09 16:55:42]

PTAL at the diff from pachset 1 in SConstruct. Most of the remaining test
failures are fixed by that (i.e. everything that uses libc++). Do let me know if
the comment isn't self-explanatory enough.

https://codereview.chromium.org/639113003/diff/1/tests/threads/race_test.c
File tests/threads/race_test.c (right):

https://codereview.chromium.org/639113003/diff/1/tests/threads/race_test.c#ne...
tests/threads/race_test.c:53: foo = 0;
On 2014/10/08 22:40:23, jvoung wrote:
> Could just make this one case w/out __asm__?

I dunno, an asm seems like a lot more reliable way to defeat the optimizer if
you can have it.
Oh and gcc 4.8 warns on this that foo is set but not used. So we should just
keep it I think.

https://codereview.chromium.org/639113003/diff/1/tests/toolchain/synchronizat...
File tests/toolchain/synchronization_cpp11.cc (right):

https://codereview.chromium.org/639113003/diff/1/tests/toolchain/synchronizat...
tests/toolchain/synchronization_cpp11.cc:70:
static_assert(ATOMIC_POINTER_LOCK_FREE == 2, "should be compile-time 2");
On 2014/10/08 22:40:23, jvoung wrote:
> #else ...
> 
> // TODO other architectures like mips.
> 
> Eventually this could just #error so people know to fill it in?
Done.
Yeah there may well never be a non-pnacl mips nacl-clang since the pnacl calling
conventions, ABI etc is the canonical one.

[jvoung (off chromium), 2014-10-09 17:35:01]

https://codereview.chromium.org/639113003/diff/20001/SConstruct
File SConstruct (right):

https://codereview.chromium.org/639113003/diff/20001/SConstruct#newcode2974
SConstruct:2974: # stil get satisfied from libpthread instead of
libpthread_private (and
"still get"

https://codereview.chromium.org/639113003/diff/20001/SConstruct#newcode2976
SConstruct:2976: # user link line before libpthread_private to ensure that its
references
I guess this is the actual hack that was in pnacl-translate... so this part
could help remove the hack later (though it hard-codes libc++, so might affect
the libstdc++ pnacl EH test variants... which are going to be removed
eventually).

https://codereview.chromium.org/639113003/diff/20001/SConstruct#newcode3178
SConstruct:3178: nacl_env.Append(CCFLAGS=['-Wno-format-security'])
Why is Wno-format-security needed for nacl_clang? I don't see that used for any
other clang variant?

Could the untrusted code be fixed?

https://codereview.chromium.org/639113003/diff/20001/SConstruct#newcode3562
SConstruct:3562: # libc++) pthread.
This was another hack, I think, from the libc++ dependency on pthread that could
be re-investigated.

https://codereview.chromium.org/639113003/diff/20001/SConstruct#newcode3682
SConstruct:3682: if ((env.Bit('nacl_clang') or env.Bit('bitcode')) and
Can use "needs_pthread" here too

https://codereview.chromium.org/639113003/diff/20001/tests/fixedfeaturecpu/na...
File tests/fixedfeaturecpu/nacl.scons (right):

https://codereview.chromium.org/639113003/diff/20001/tests/fixedfeaturecpu/na...
tests/fixedfeaturecpu/nacl.scons:66: # are uknown to me because ncval is broken
and ncval_new doesn't support
"unknown to me"

Who is "me", when someone reads this in the future? TODO(...) instead?

[Derek Schuff, 2014-10-09 18:40:40]

One more change added to disable regs_step_test for now. With these changes and
the to-be-rolled newlib change, small_tests all pass.

https://codereview.chromium.org/639113003/diff/20001/SConstruct
File SConstruct (right):

https://codereview.chromium.org/639113003/diff/20001/SConstruct#newcode2974
SConstruct:2974: # stil get satisfied from libpthread instead of
libpthread_private (and
On 2014/10/09 17:35:01, jvoung wrote:
> "still get"

Done.

https://codereview.chromium.org/639113003/diff/20001/SConstruct#newcode2976
SConstruct:2976: # user link line before libpthread_private to ensure that its
references
On 2014/10/09 17:35:01, jvoung wrote:
> I guess this is the actual hack that was in pnacl-translate... so this part
> could help remove the hack later (though it hard-codes libc++, so might affect
> the libstdc++ pnacl EH test variants... which are going to be removed
> eventually).

Yeah the idea was to allow this to remove the hack in pnacl-ld also... but I had
forgotten about the libstdc++ EH tests. maybe i'll just use this for nacl_clang
for now and look at pnacl itself later.

https://codereview.chromium.org/639113003/diff/20001/SConstruct#newcode3178
SConstruct:3178: nacl_env.Append(CCFLAGS=['-Wno-format-security'])
On 2014/10/09 17:35:01, jvoung wrote:
> Why is Wno-format-security needed for nacl_clang? I don't see that used for
any
> other clang variant?
> 
> Could the untrusted code be fixed?

Hm, i had it in there during bootstrapping for some reason but now it seems to
be working without it.

https://codereview.chromium.org/639113003/diff/20001/SConstruct#newcode3562
SConstruct:3562: # libc++) pthread.
On 2014/10/09 17:35:00, jvoung wrote:
> This was another hack, I think, from the libc++ dependency on pthread that
could
> be re-investigated.

It looks like we still want it to avoid depending on libc++, no? even if we do
always link with pthread?

https://codereview.chromium.org/639113003/diff/20001/SConstruct#newcode3682
SConstruct:3682: if ((env.Bit('nacl_clang') or env.Bit('bitcode')) and
On 2014/10/09 17:35:01, jvoung wrote:
> Can use "needs_pthread" here too

Done, and renamed.

https://codereview.chromium.org/639113003/diff/20001/tests/fixedfeaturecpu/na...
File tests/fixedfeaturecpu/nacl.scons (right):

https://codereview.chromium.org/639113003/diff/20001/tests/fixedfeaturecpu/na...
tests/fixedfeaturecpu/nacl.scons:66: # are uknown to me because ncval is broken
and ncval_new doesn't support
On 2014/10/09 17:35:01, jvoung wrote:
> "unknown to me"
> 
> Who is "me", when someone reads this in the future? TODO(...) instead?

I explicitly do not want a TODO to fix ncval :)

[jvoung (off chromium), 2014-10-09 19:05:02]

lgtm

https://codereview.chromium.org/639113003/diff/20001/SConstruct
File SConstruct (right):

https://codereview.chromium.org/639113003/diff/20001/SConstruct#newcode3562
SConstruct:3562: # libc++) pthread.
On 2014/10/09 18:40:40, Derek Schuff wrote:
> On 2014/10/09 17:35:00, jvoung wrote:
> > This was another hack, I think, from the libc++ dependency on pthread that
> could
> > be re-investigated.
> 
> It looks like we still want it to avoid depending on libc++, no? even if we do
> always link with pthread?

Okay, and now it doesn't apply to plain bitcode=1 either.

https://codereview.chromium.org/639113003/diff/320001/tests/fixedfeaturecpu/n...
File tests/fixedfeaturecpu/nacl.scons (right):

https://codereview.chromium.org/639113003/diff/320001/tests/fixedfeaturecpu/n...
tests/fixedfeaturecpu/nacl.scons:66: # are uknown because ncval is broken and
ncval_new doesn't support
uknown -> unknown

[Derek Schuff, 2014-10-09 21:19:50]

Roland, can you look at the crti/crtn files (and anything else you'd care to)?

[Roland McGrath, 2014-10-09 22:01:37]

lgtm

https://codereview.chromium.org/639113003/diff/320001/SConstruct
File SConstruct (right):

https://codereview.chromium.org/639113003/diff/320001/SConstruct#newcode2916
SConstruct:2916: using_nacl_libcxx = nacl_env.Bit('nacl_clang')
Drop this variable.  All it seems to be doing is making one line shorter at the
expense of adding this line and making the code harder to follow.

https://codereview.chromium.org/639113003/diff/320001/SConstruct#newcode3179
SConstruct:3179: # third_party/valgrind/nacl_valgrind.h uses asm instead of
__asm__
File a bug and a TODO for just fixing that header.

[Derek Schuff, 2014-10-09 22:22:00]

Jan: I went back to including pnacl and nacl-clang in  using_nacl_libcxx and
instead filtered lc++ out of implicit libs for the one test we actually have for
libstdc++. see what you think.

also now that the newlib change has landed I can actually test this on bots.

https://codereview.chromium.org/639113003/diff/320001/SConstruct
File SConstruct (right):

https://codereview.chromium.org/639113003/diff/320001/SConstruct#newcode2916
SConstruct:2916: using_nacl_libcxx = nacl_env.Bit('nacl_clang')
On 2014/10/09 22:01:37, Roland McGrath wrote:
> Drop this variable.  All it seems to be doing is making one line shorter at
the
> expense of adding this line and making the code harder to follow.

In the original version of this CL, this variable was 
nacl_env.Bit('nacl_clang') or nacl_env.Bit('bitcode')
and will be again shortly. Unfortunately that expression can't be used in place
of the single variable on line 2980. So yeah I agree that it's bad, but that's
why it's there. Maybe I could instead make the list that will eventually become
NONIRT_LIBS the variable up here, to at least keep the logic and comment all in
one place?

https://codereview.chromium.org/639113003/diff/320001/SConstruct#newcode3179
SConstruct:3179: # third_party/valgrind/nacl_valgrind.h uses asm instead of
__asm__
On 2014/10/09 22:01:37, Roland McGrath wrote:
> File a bug and a TODO for just fixing that header.

Done.

https://codereview.chromium.org/639113003/diff/320001/tests/fixedfeaturecpu/n...
File tests/fixedfeaturecpu/nacl.scons (right):

https://codereview.chromium.org/639113003/diff/320001/tests/fixedfeaturecpu/n...
tests/fixedfeaturecpu/nacl.scons:66: # are uknown because ncval is broken and
ncval_new doesn't support
On 2014/10/09 19:05:01, jvoung wrote:
> uknown -> unknown

Done.

[jvoung (off chromium), 2014-10-10 00:51:43]

On 2014/10/09 22:22:00, Derek Schuff wrote:
> Jan: I went back to including pnacl and nacl-clang in  using_nacl_libcxx and
> instead filtered lc++ out of implicit libs for the one test we actually have
for
> libstdc++. see what you think.

Okay, that seems fine.

Do you know what's going on with:

Function _ZNSt15__exception_ptr13exception_ptr4swapERS0_ is declared but not
defined (disallowed)
_ZNSt15__exception_ptr13exception_ptr4swapERS0_ is not a valid external symbol
(disallowed)
LLVM ERROR: PNaCl ABI verification failed

???

> 
> also now that the newlib change has landed I can actually test this on bots.
> 
> https://codereview.chromium.org/639113003/diff/320001/SConstruct
> File SConstruct (right):
> 
> https://codereview.chromium.org/639113003/diff/320001/SConstruct#newcode2916
> SConstruct:2916: using_nacl_libcxx = nacl_env.Bit('nacl_clang')
> On 2014/10/09 22:01:37, Roland McGrath wrote:
> > Drop this variable.  All it seems to be doing is making one line shorter at
> the
> > expense of adding this line and making the code harder to follow.
> 
> In the original version of this CL, this variable was 
> nacl_env.Bit('nacl_clang') or nacl_env.Bit('bitcode')
> and will be again shortly. Unfortunately that expression can't be used in
place
> of the single variable on line 2980. So yeah I agree that it's bad, but that's
> why it's there. Maybe I could instead make the list that will eventually
become
> NONIRT_LIBS the variable up here, to at least keep the logic and comment all
in
> one place?
> 
> https://codereview.chromium.org/639113003/diff/320001/SConstruct#newcode3179
> SConstruct:3179: # third_party/valgrind/nacl_valgrind.h uses asm instead of
> __asm__
> On 2014/10/09 22:01:37, Roland McGrath wrote:
> > File a bug and a TODO for just fixing that header.
> 
> Done.
> 
>
https://codereview.chromium.org/639113003/diff/320001/tests/fixedfeaturecpu/n...
> File tests/fixedfeaturecpu/nacl.scons (right):
> 
>
https://codereview.chromium.org/639113003/diff/320001/tests/fixedfeaturecpu/n...
> tests/fixedfeaturecpu/nacl.scons:66: # are uknown because ncval is broken and
> ncval_new doesn't support
> On 2014/10/09 19:05:01, jvoung wrote:
> > uknown -> unknown
> 
> Done.

[Derek Schuff, 2014-10-10 16:01:32]

On 2014/10/10 00:51:43, jvoung wrote:
> On 2014/10/09 22:22:00, Derek Schuff wrote:
> > Jan: I went back to including pnacl and nacl-clang in  using_nacl_libcxx and
> > instead filtered lc++ out of implicit libs for the one test we actually have
> for
> > libstdc++. see what you think.
> 
> Okay, that seems fine.
> 
> Do you know what's going on with:
> 
> Function _ZNSt15__exception_ptr13exception_ptr4swapERS0_ is declared but not
> defined (disallowed)
> _ZNSt15__exception_ptr13exception_ptr4swapERS0_ is not a valid external symbol
> (disallowed)
> LLVM ERROR: PNaCl ABI verification failed
> 
> ???


Yes, that was because I missed one other set of libstdc++ tests (see the diff
between PS3 and PS4)

[jvoung (off chromium), 2014-10-10 16:28:57]

On 2014/10/10 16:01:32, Derek Schuff wrote:
> On 2014/10/10 00:51:43, jvoung wrote:
> > On 2014/10/09 22:22:00, Derek Schuff wrote:
> > > Jan: I went back to including pnacl and nacl-clang in  using_nacl_libcxx
and
> > > instead filtered lc++ out of implicit libs for the one test we actually
have
> > for
> > > libstdc++. see what you think.
> > 
> > Okay, that seems fine.
> > 
> > Do you know what's going on with:
> > 
> > Function _ZNSt15__exception_ptr13exception_ptr4swapERS0_ is declared but not
> > defined (disallowed)
> > _ZNSt15__exception_ptr13exception_ptr4swapERS0_ is not a valid external
symbol
> > (disallowed)
> > LLVM ERROR: PNaCl ABI verification failed
> > 
> > ???
> 
> 
> Yes, that was because I missed one other set of libstdc++ tests (see the diff
> between PS3 and PS4)

Okay, diff looks fine.

[Derek Schuff, 2014-10-10 17:02:39]

dschuff@chromium.org changed reviewers:
+ ncbray@chromium.org

[Derek Schuff, 2014-10-10 17:02:39]

ncbray, can-has review for src/trusted/validator/nacl.scons?

[Nick Bray (chromium), 2014-10-10 19:53:25]

LGTM

https://codereview.chromium.org/639113003/diff/910001/SConstruct
File SConstruct (right):

https://codereview.chromium.org/639113003/diff/910001/SConstruct#newcode265
SConstruct:265: desc='Use native nacl-clang compiler')
Bad description.  Used when doing what?  Instead of what?  It's unclear to be
how this affects the glibc build, etc.

https://codereview.chromium.org/639113003/diff/910001/SConstruct#newcode2965
SConstruct:2965: (['c++','pthread_private'] if using_nacl_libcxx else [])),
As a matter of curiosity, I'd like to talk about what's going on here in person.

https://codereview.chromium.org/639113003/diff/910001/src/trusted/validator/n...
File src/trusted/validator/nacl.scons (right):

https://codereview.chromium.org/639113003/diff/910001/src/trusted/validator/n...
src/trusted/validator/nacl.scons:35: is_broken=env.Bit('bitcode') or
env.Bit('nacl_clang'))
Asking around, it may be possible to unhack this?  Pass in the compiler?  "This
would work fine with the other compiler." - Roland.

https://codereview.chromium.org/639113003/diff/910001/tests/fixedfeaturecpu/n...
File tests/fixedfeaturecpu/nacl.scons (right):

https://codereview.chromium.org/639113003/diff/910001/tests/fixedfeaturecpu/n...
tests/fixedfeaturecpu/nacl.scons:64: # are unknown because ncval is broken and
ncval_new doesn't support
https://groups.google.com/forum/#!msg/native-client-discuss/7ORdpmZL0RI/p8GFB...

[Derek Schuff, 2014-10-10 20:43:50]

Committed patchset #6 (id:1060001) manually as 13888 (presubmit successful).

[Derek Schuff, 2014-10-10 20:56:24]

https://codereview.chromium.org/639113003/diff/910001/SConstruct
File SConstruct (right):

https://codereview.chromium.org/639113003/diff/910001/SConstruct#newcode265
SConstruct:265: desc='Use native nacl-clang compiler')
On 2014/10/10 19:53:24, Nick Bray (chromium) wrote:
> Bad description.  Used when doing what?  Instead of what?  It's unclear to be
> how this affects the glibc build, etc.

Done.

https://codereview.chromium.org/639113003/diff/910001/SConstruct#newcode2965
SConstruct:2965: (['c++','pthread_private'] if using_nacl_libcxx else [])),
On 2014/10/10 19:53:24, Nick Bray (chromium) wrote:
> As a matter of curiosity, I'd like to talk about what's going on here in
person.

Acknowledged.

https://codereview.chromium.org/639113003/diff/910001/src/trusted/validator/n...
File src/trusted/validator/nacl.scons (right):

https://codereview.chromium.org/639113003/diff/910001/src/trusted/validator/n...
src/trusted/validator/nacl.scons:35: is_broken=env.Bit('bitcode') or
env.Bit('nacl_clang'))
On 2014/10/10 19:53:25, Nick Bray (chromium) wrote:
> Asking around, it may be possible to unhack this?  Pass in the compiler? 
"This
> would work fine with the other compiler." - Roland.

and in fact it did work when i changed the hardcoding. so yeah it can be
unhacked, but probably beyond the scope of this CL.

https://codereview.chromium.org/639113003/diff/910001/tests/fixedfeaturecpu/n...
File tests/fixedfeaturecpu/nacl.scons (right):

https://codereview.chromium.org/639113003/diff/910001/tests/fixedfeaturecpu/n...
tests/fixedfeaturecpu/nacl.scons:64: # are unknown because ncval is broken and
ncval_new doesn't support
On 2014/10/10 19:53:25, Nick Bray (chromium) wrote:
>
https://groups.google.com/forum/#!msg/native-client-discuss/7ORdpmZL0RI/p8GFB...

OK. but it doesn't help me in finding out the reason validation fails (i.e. what
is linked into the nexe that is not supported by the fixed feature mode)