Subzero, MIPS32: Cross-testing enabled for MIPS32

Subzero, MIPS32: Cross-testing enabled for MIPS32

Enables running crosstests for MIPS32 target.

R=stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=623f8ce32c8dd33f9c755dde229967dfddd3353b

[obucinac, 2016-06-22 15:05:16]

Description was changed from

==========
Subzero, MIPS32: Cross-testing enabled for MIPS32

This patch adds MIPS32 to crosstest test suite.
==========

to

==========
Subzero, MIPS32: Cross-testing enabled for MIPS32

This patch adds MIPS32 to crosstest test suite.
==========

[obucinac, 2016-06-22 15:05:16]

obucinac@gmail.com changed reviewers:
+ mohit.bhakkad@imgtec.com, sagar.thakur@imgtec.com, stichnot@chromium.org

[Jim Stichnoth, 2016-06-22 19:14:16]

This is really great that you're working on bringing up cross tests.  These are
immensely more valuable than the lit tests, because the cross tests basically
comprise millions of unit tests.

I think maybe I will try to add a trivial cross test just to allow the basic
build framework to be tested.

https://codereview.chromium.org/2085303002/diff/1/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/2085303002/diff/1/Makefile.standalone#newcode413
Makefile.standalone:413: exists-nonsfi-arm32 exists-nonsfi-mips32 exists-sbtc
exists-spec
I'm not sure we should bother with the "nonsfi" feature for MIPS.

We implemented "nonsfi" mode in Subzero for another group here, but they have
since moved on and I believe they are not using Subzero any more.

The main priority for Chrome is "sandbox" code generation, however for the
initial bring-up and testing and tuning, "native" code generation is much more
convenient.

https://codereview.chromium.org/2085303002/diff/1/Makefile.standalone#newcode552
Makefile.standalone:552: -i mips32 \
I don't think any of the MIPS cross tests are ready yet to be enabled, because
each cross test has some missing MIPS lowering functionality.

Also, cross tests use --filetype=obj by default unless FORCEASM is used in the
makefile, so the MIPS cross tests will all fail anyway until --filetype=obj is
ready.

Because of this, you should not add any "-i mips32" arguments here because "make
check-xtest" will fail.

Later, I would start with "simple_loop" which is the simplest of the cross
tests.  It currently fails because the "load" instruction is not yet
implemented, also O2 fails because some of the O2 machinery is not yet
implemented.  But after the load instruction is implemented:

./pydir/crosstest_generator.py -v --filetype=asm --toolchain-root
/path/to/native_client/toolchain/linux_x86 -i mips32,native,Om1,simple_loop

should work.

[obucinac, 2016-06-22 19:43:21]

On 2016/06/22 19:14:16, stichnot wrote:
> This is really great that you're working on bringing up cross tests.  These
are
> immensely more valuable than the lit tests, because the cross tests basically
> comprise millions of unit tests.
> 
> I think maybe I will try to add a trivial cross test just to allow the basic
> build framework to be tested.
> 
> https://codereview.chromium.org/2085303002/diff/1/Makefile.standalone
> File Makefile.standalone (right):
> 
>
https://codereview.chromium.org/2085303002/diff/1/Makefile.standalone#newcode413
> Makefile.standalone:413: exists-nonsfi-arm32 exists-nonsfi-mips32 exists-sbtc
> exists-spec
> I'm not sure we should bother with the "nonsfi" feature for MIPS.
> 
> We implemented "nonsfi" mode in Subzero for another group here, but they have
> since moved on and I believe they are not using Subzero any more.
> 
> The main priority for Chrome is "sandbox" code generation, however for the
> initial bring-up and testing and tuning, "native" code generation is much more
> convenient.
> 
>
https://codereview.chromium.org/2085303002/diff/1/Makefile.standalone#newcode552
> Makefile.standalone:552: -i mips32 \
> I don't think any of the MIPS cross tests are ready yet to be enabled, because
> each cross test has some missing MIPS lowering functionality.
> 
> Also, cross tests use --filetype=obj by default unless FORCEASM is used in the
> makefile, so the MIPS cross tests will all fail anyway until --filetype=obj is
> ready.
> 
> Because of this, you should not add any "-i mips32" arguments here because
"make
> check-xtest" will fail.
> 
> Later, I would start with "simple_loop" which is the simplest of the cross
> tests.  It currently fails because the "load" instruction is not yet
> implemented, also O2 fails because some of the O2 machinery is not yet
> implemented.  But after the load instruction is implemented:
> 
> ./pydir/crosstest_generator.py -v --filetype=asm --toolchain-root
> /path/to/native_client/toolchain/linux_x86 -i mips32,native,Om1,simple_loop
> 
> should work.

OK Jim. I am working on Load for some time now, and currently have many problems
because of existing mov implementation, which does not exist on MIPS.
Implementation is originally copied from ARM, and then expanded into whatever
looks like working at the moment. E.g it tries to handle all data types, and all
kind of data transfer, reg to reg, reg to mem, mem to reg, mem to mem, tries to
replace load/store instructions, and gives false impression of working by
printing correct output. It is used in many places across the code, and this
make it hard to remove it. I will upload changes for lowerLoad/Store as soon as
I get something working.

[obucinac, 2016-08-22 16:19:06]

Patchset 4 is combined work from Mohit, Jaydeep and me.

It uses MIPS toolchain available from Ubuntu 16.04.1 repository.

QEMU cpu is changed to mips32r5-generic, to avoid changes to existing PNaCl
toolchains (uses existing qemu, instead of adding new one).

lowerLoad implementation is copied from ARM implementation, to get tests
running.

Test for simple_loop shows 102 passes.

[obucinac, 2016-08-22 16:47:10]

For the reference, this is the list of all arm and mipsel toolchain related
packages on my computer:


$ dpkg -l '*' | grep '\-arm-' | grep 'ii ' | cut -c 1-40

ii  binutils-arm-linux-gnueabi          
ii  binutils-arm-linux-gnueabihf        
ii  cpp-5-arm-linux-gnueabi             
ii  cpp-5-arm-linux-gnueabihf           
ii  cpp-arm-linux-gnueabi               
ii  cpp-arm-linux-gnueabihf             
ii  g++-5-arm-linux-gnueabi             
ii  g++-5-arm-linux-gnueabihf           
ii  g++-arm-linux-gnueabi               
ii  g++-arm-linux-gnueabihf             
ii  gcc-5-arm-linux-gnueabi             
ii  gcc-5-arm-linux-gnueabi-base:amd64  
ii  gcc-5-arm-linux-gnueabihf           
ii  gcc-5-arm-linux-gnueabihf-base:amd64
ii  gcc-arm-linux-gnueabi               
ii  gcc-arm-linux-gnueabihf             

$ dpkg -l '*' | grep '\-mipsel-' | grep 'ii ' | cut -c 1-40

ii  binutils-mipsel-linux-gnu           
ii  cpp-5-mipsel-linux-gnu              
ii  cpp-mipsel-linux-gnu                
ii  g++-5-mipsel-linux-gnu              
ii  g++-mipsel-linux-gnu                
ii  gcc-5-mipsel-linux-gnu              
ii  gcc-5-mipsel-linux-gnu-base:amd64   
ii  gcc-mipsel-linux-gnu                
ii  lib64atomic1-mipsel-cross           
ii  lib64gcc-5-dev-mipsel-cross         
ii  lib64gcc1-mipsel-cross              
ii  lib64gomp1-mipsel-cross             
ii  libatomic1-mipsel-cross             
ii  libc6-dev-mips64-mipsel-cross       
ii  libc6-dev-mipsel-cross              
ii  libc6-dev-mipsn32-mipsel-cross      
ii  libc6-mips64-mipsel-cross           
ii  libc6-mipsel-cross                  
ii  libc6-mipsn32-mipsel-cross          
ii  libgcc-5-dev-mipsel-cross           
ii  libgcc1-mipsel-cross                
ii  libgomp1-mipsel-cross               
ii  libn32atomic1-mipsel-cross          
ii  libn32gcc-5-dev-mipsel-cross        
ii  libn32gcc1-mipsel-cross             
ii  libn32gomp1-mipsel-cross            
ii  libstdc++-5-dev-mipsel-cross        
ii  libstdc++6-mipsel-cross             
ii  linux-libc-dev-mipsel-cross

[obucinac, 2016-08-22 16:51:15]

More complete list:

$ dpkg -l '*' | grep '\-arm' | grep 'ii ' | cut -c 1-40
ii  binutils-arm-linux-gnueabi          
ii  binutils-arm-linux-gnueabihf        
ii  cpp-5-arm-linux-gnueabi             
ii  cpp-5-arm-linux-gnueabihf           
ii  cpp-arm-linux-gnueabi               
ii  cpp-arm-linux-gnueabihf             
ii  g++-5-arm-linux-gnueabi             
ii  g++-5-arm-linux-gnueabihf           
ii  g++-arm-linux-gnueabi               
ii  g++-arm-linux-gnueabihf             
ii  gcc-5-arm-linux-gnueabi             
ii  gcc-5-arm-linux-gnueabi-base:amd64  
ii  gcc-5-arm-linux-gnueabihf           
ii  gcc-5-arm-linux-gnueabihf-base:amd64
ii  gcc-arm-linux-gnueabi               
ii  gcc-arm-linux-gnueabihf             
ii  libasan2-armel-cross                
ii  libasan2-armhf-cross                
ii  libatomic1-armel-cross              
ii  libatomic1-armhf-cross              
ii  libc6-armel-cross                   
ii  libc6-armhf-cross                   
ii  libc6-dev-armel-cross               
ii  libc6-dev-armhf-cross               
ii  libgcc-5-dev-armel-cross            
ii  libgcc-5-dev-armhf-cross            
ii  libgcc1-armel-cross                 
ii  libgcc1-armhf-cross                 
ii  libgomp1-armel-cross                
ii  libgomp1-armhf-cross                
ii  libstdc++-5-dev-armel-cross         
ii  libstdc++-5-dev-armhf-cross         
ii  libstdc++6-armel-cross              
ii  libstdc++6-armhf-cross              
ii  libubsan0-armel-cross               
ii  libubsan0-armhf-cross               
ii  linux-libc-dev-armel-cross          
ii  linux-libc-dev-armhf-cross          
ii  qemu-system-arm                     

$ dpkg -l '*' | grep '\-mips' | grep 'ii ' | cut -c 1-40

ii  binutils-mipsel-linux-gnu           
ii  cpp-5-mipsel-linux-gnu              
ii  cpp-mipsel-linux-gnu                
ii  g++-5-mipsel-linux-gnu              
ii  g++-mipsel-linux-gnu                
ii  gcc-5-mipsel-linux-gnu              
ii  gcc-5-mipsel-linux-gnu-base:amd64   
ii  gcc-mipsel-linux-gnu                
ii  lib64atomic1-mipsel-cross           
ii  lib64gcc-5-dev-mipsel-cross         
ii  lib64gcc1-mipsel-cross              
ii  lib64gomp1-mipsel-cross             
ii  libatomic1-mipsel-cross             
ii  libc6-dev-mips64-mipsel-cross       
ii  libc6-dev-mipsel-cross              
ii  libc6-dev-mipsn32-mipsel-cross      
ii  libc6-mips64-mipsel-cross           
ii  libc6-mipsel-cross                  
ii  libc6-mipsn32-mipsel-cross          
ii  libgcc-5-dev-mipsel-cross           
ii  libgcc1-mipsel-cross                
ii  libgomp1-mipsel-cross               
ii  libn32atomic1-mipsel-cross          
ii  libn32gcc-5-dev-mipsel-cross        
ii  libn32gcc1-mipsel-cross             
ii  libn32gomp1-mipsel-cross            
ii  libstdc++-5-dev-mipsel-cross        
ii  libstdc++6-mipsel-cross             
ii  linux-libc-dev-mipsel-cross         
ii  qemu-system-mips

[obucinac, 2016-08-22 16:59:34]

obucinac@gmail.com changed reviewers:
+ jaydeep.patil@imgtec.com

[Jim Stichnoth, 2016-08-22 17:36:40]

On 2016/08/22 16:19:06, obucinac wrote:
> Patchset 4 is combined work from Mohit, Jaydeep and me.
> 
> It uses MIPS toolchain available from Ubuntu 16.04.1 repository.
> 
> QEMU cpu is changed to mips32r5-generic, to avoid changes to existing PNaCl
> toolchains (uses existing qemu, instead of adding new one).
> 
> lowerLoad implementation is copied from ARM implementation, to get tests
> running.
> 
> Test for simple_loop shows 102 passes.

I'm having some problems getting the mips toolchain installed on my corp
machine, but in the meantime, I hacked around it to make progress.

I'm seeing unimplemented errors in the MIPS target lowering.  Trying to run just
the simple_loop test:

make -f Makefile.standalone check-xtest
CHECK_XTEST_TESTS=crosstest/Output/simple_loop_mips32_native_O2_base.xtest
make -f Makefile.standalone check-xtest
CHECK_XTEST_TESTS=crosstest/Output/simple_loop_mips32_native_Om1_base.xtest

Both fail in pnacl-sz before getting to the link stage.

Are you using any special arguments or configurations to get this to work?

[obucinac, 2016-08-22 17:39:01]

This command works for me

/subzero/nacl/native_client/toolchain_build/src/subzero$
./pydir/crosstest_generator.py -v --filetype=asm --toolchain-root
/subzero/subzero/nacl/native_client/toolchain/linux_x86/ -i
mips32,native,Om1,simple_loop

[Jim Stichnoth, 2016-08-22 17:49:42]

On 2016/08/22 17:39:01, obucinac wrote:
> This command works for me
> 
> /subzero/nacl/native_client/toolchain_build/src/subzero$
> ./pydir/crosstest_generator.py -v --filetype=asm --toolchain-root
> /subzero/subzero/nacl/native_client/toolchain/linux_x86/ -i
> mips32,native,Om1,simple_loop

Ah, thanks.  I forgot that the default "make check-xtest" uses filetype=obj.

This would be the way to invoke it through the Makefile.standalone:

FORCEASM=1 make -f Makefile.standalone check-xtest
CHECK_XTEST_TESTS=crosstest/Output/simple_loop_mips32_native_Om1_base.xtest

The next thing I run into is the following:

$ .../native_client/toolchain/linux_x86/pnacl_newlib_raw/bin/clang
simple_loop_main.c
/.../native_client/toolchain_build/src/subzero/crosstest/Output/simple_loop.nat.Om1.base.mips32.sz.o
/.../native_client/toolchain_build/src/subzero/crosstest/Output/simple_loop.nat.Om1.base.mips32.llc.o
-o
/.../native_client/toolchain_build/src/subzero/crosstest/Output/simple_loop_mips32_native_Om1_base
-g -target=mipsel-linux-gnu -lm -lpthread -Wl,--defsym=__Sz_AbsoluteZero=0
Assembler messages:
Fatal error: invalid -march= option: `mips32r2'
clang: error: assembler command failed with exit code 1 (use -v to see
invocation)

This seems to be an issue with the PNaCl version of clang and/or llvm-mc.  Did
you do anything special in your native_client tree to make this work?

[obucinac, 2016-08-22 18:16:01]

On 2016/08/22 17:49:42, stichnot wrote:
> On 2016/08/22 17:39:01, obucinac wrote:
> > This command works for me
> > 
> > /subzero/nacl/native_client/toolchain_build/src/subzero$
> > ./pydir/crosstest_generator.py -v --filetype=asm --toolchain-root
> > /subzero/subzero/nacl/native_client/toolchain/linux_x86/ -i
> > mips32,native,Om1,simple_loop
> 
> Ah, thanks.  I forgot that the default "make check-xtest" uses filetype=obj.
> 
> This would be the way to invoke it through the Makefile.standalone:
> 
> FORCEASM=1 make -f Makefile.standalone check-xtest
> CHECK_XTEST_TESTS=crosstest/Output/simple_loop_mips32_native_Om1_base.xtest
> 
> The next thing I run into is the following:
> 
> $ .../native_client/toolchain/linux_x86/pnacl_newlib_raw/bin/clang
> simple_loop_main.c
>
/.../native_client/toolchain_build/src/subzero/crosstest/Output/simple_loop.nat.Om1.base.mips32.sz.o
>
/.../native_client/toolchain_build/src/subzero/crosstest/Output/simple_loop.nat.Om1.base.mips32.llc.o
> -o
>
/.../native_client/toolchain_build/src/subzero/crosstest/Output/simple_loop_mips32_native_Om1_base
> -g -target=mipsel-linux-gnu -lm -lpthread -Wl,--defsym=__Sz_AbsoluteZero=0
> Assembler messages:
> Fatal error: invalid -march= option: `mips32r2'
> clang: error: assembler command failed with exit code 1 (use -v to see
> invocation)
> 
> This seems to be an issue with the PNaCl version of clang and/or llvm-mc.  Did
> you do anything special in your native_client tree to make this work?

This was the root cause for me getting into system update to get to the 16.04.1
which has mips toolchain in its repositories.

Basically, this means that you did not properly replaced clang with the
toolchain installed from repositories.

If you tried to use clang from pnacl toolchains, it does not work. Thats why we
needed external toolchain.

[obucinac, 2016-08-22 18:20:13]

On 2016/08/22 18:16:01, obucinac wrote:
> On 2016/08/22 17:49:42, stichnot wrote:
> > On 2016/08/22 17:39:01, obucinac wrote:
> > > This command works for me
> > > 
> > > /subzero/nacl/native_client/toolchain_build/src/subzero$
> > > ./pydir/crosstest_generator.py -v --filetype=asm --toolchain-root
> > > /subzero/subzero/nacl/native_client/toolchain/linux_x86/ -i
> > > mips32,native,Om1,simple_loop
> > 
> > Ah, thanks.  I forgot that the default "make check-xtest" uses filetype=obj.
> > 
> > This would be the way to invoke it through the Makefile.standalone:
> > 
> > FORCEASM=1 make -f Makefile.standalone check-xtest
> > CHECK_XTEST_TESTS=crosstest/Output/simple_loop_mips32_native_Om1_base.xtest
> > 
> > The next thing I run into is the following:
> > 
> > $ .../native_client/toolchain/linux_x86/pnacl_newlib_raw/bin/clang
> > simple_loop_main.c
> >
>
/.../native_client/toolchain_build/src/subzero/crosstest/Output/simple_loop.nat.Om1.base.mips32.sz.o
> >
>
/.../native_client/toolchain_build/src/subzero/crosstest/Output/simple_loop.nat.Om1.base.mips32.llc.o
> > -o
> >
>
/.../native_client/toolchain_build/src/subzero/crosstest/Output/simple_loop_mips32_native_Om1_base
> > -g -target=mipsel-linux-gnu -lm -lpthread -Wl,--defsym=__Sz_AbsoluteZero=0
> > Assembler messages:
> > Fatal error: invalid -march= option: `mips32r2'
> > clang: error: assembler command failed with exit code 1 (use -v to see
> > invocation)
> > 
> > This seems to be an issue with the PNaCl version of clang and/or llvm-mc. 
Did
> > you do anything special in your native_client tree to make this work?
> 
> This was the root cause for me getting into system update to get to the
16.04.1
> which has mips toolchain in its repositories.
> 
> Basically, this means that you did not properly replaced clang with the
> toolchain installed from repositories.
> 
> If you tried to use clang from pnacl toolchains, it does not work. Thats why
we
> needed external toolchain.


No changes in native_client tree, except what is in the patch.

I just updated my Ubuntu 14.04 to 16.04.1 and installed MIPS toolchain from
Ubuntu repositories.

[Jim Stichnoth, 2016-08-23 14:56:49]

https://codereview.chromium.org/2085303002/diff/60001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/2085303002/diff/60001/Makefile.standalone#new...
Makefile.standalone:566: -i mips32 \
Change these two mips32 lines to this single line:
  -i mips32,native,Om1,simple_loop

This restricts the mips32 cross tests to the single currently working test, when
"make -f Makefile.standalone check-xtest" is run.

https://codereview.chromium.org/2085303002/diff/60001/Makefile.standalone#new...
Makefile.standalone:623: ifeq ($(TARGET),mips32)
Should probably add a comment noting that
native_client/tests/spec2k/{Makefile.common,run_all.sh} do not currently have
MIPS configs, so those would need to be added for proper Subzero testing here.

https://codereview.chromium.org/2085303002/diff/60001/Makefile.standalone#new...
Makefile.standalone:626: ;SPEC := --filetype=asm
Remove the ';'.

(Not sure if that was meant as a comment character which is actually '#', but
this line without the ';' should be correct.)

https://codereview.chromium.org/2085303002/diff/60001/pydir/build-runtime.py
File pydir/build-runtime.py (right):

https://codereview.chromium.org/2085303002/diff/60001/pydir/build-runtime.py#...
pydir/build-runtime.py:67: ], echo=verbose)
revert this whitespace change

https://codereview.chromium.org/2085303002/diff/60001/pydir/build-runtime.py#...
pydir/build-runtime.py:83: shellcmd([GetObjcopyCmd(target=target_info.target),
Can you remove "target=" ?

here and below

https://codereview.chromium.org/2085303002/diff/60001/pydir/crosstest.py
File pydir/crosstest.py (right):

https://codereview.chromium.org/2085303002/diff/60001/pydir/crosstest.py#newc...
pydir/crosstest.py:173: shellcmd(['{bin}/{objcopy}'.format(bin=bindir,
objcopy=GetObjcopyCmd(args.target)),
80-col

https://codereview.chromium.org/2085303002/diff/60001/pydir/crosstest.py#newc...
pydir/crosstest.py:187: shellcmd(['{bin}/{objcopy}'.format(bin=bindir,
objcopy=GetObjcopyCmd(args.target)),
80-col

https://codereview.chromium.org/2085303002/diff/60001/pydir/crosstest.py#newc...
pydir/crosstest.py:255: shellcmd(['{bin}/{objcopy}'.format(bin=bindir,
objcopy=GetObjcopyCmd(args.target)),
80-col

https://codereview.chromium.org/2085303002/diff/60001/pydir/crosstest_generat...
File pydir/crosstest_generator.py (right):

https://codereview.chromium.org/2085303002/diff/60001/pydir/crosstest_generat...
pydir/crosstest_generator.py:79: 'mips32': ['base'] }
[ 'base' ]
for consistency

https://codereview.chromium.org/2085303002/diff/60001/pydir/crosstest_generat...
pydir/crosstest_generator.py:86: 'mips32': []}
Add back in the space before the closing brace?
  [] }

https://codereview.chromium.org/2085303002/diff/60001/pydir/run-pnacl-sz.py
File pydir/run-pnacl-sz.py (right):

https://codereview.chromium.org/2085303002/diff/60001/pydir/run-pnacl-sz.py#n...
pydir/run-pnacl-sz.py:23: 'mips32': ['-triple=%s' % ('mipsel-nacl' if sandboxed
else 'mipsel'),
80-col

https://codereview.chromium.org/2085303002/diff/60001/pydir/targets.py
File pydir/targets.py (right):

https://codereview.chromium.org/2085303002/diff/60001/pydir/targets.py#newcode58
pydir/targets.py:58: cross_headers=['-isystem', FindMIPSCrossInclude()])
fix alignment

https://codereview.chromium.org/2085303002/diff/60001/runtime/szrt_asm_mips32.s
File runtime/szrt_asm_mips32.s (right):

https://codereview.chromium.org/2085303002/diff/60001/runtime/szrt_asm_mips32...
runtime/szrt_asm_mips32.s:16: .p2alignl 4,0xE7FEDEF0
This is the encoding for an ARM trap instruction.  See
https://codereview.chromium.org/1649053002/ .

What is the right value for MIPS32?

[Jim Stichnoth, 2016-08-23 17:22:36]

> I just updated my Ubuntu 14.04 to 16.04.1 and installed MIPS toolchain from
> Ubuntu repositories.

Unfortunately we are not really allowed to update our distro, so I need to find
a good process for getting the proper tools installed.
(Also so that I can try this out myself.)

[obucinac, 2016-08-24 13:22:30]

On 2016/08/23 17:22:36, stichnot wrote:
> > I just updated my Ubuntu 14.04 to 16.04.1 and installed MIPS toolchain from
> > Ubuntu repositories.
> 
> Unfortunately we are not really allowed to update our distro, so I need to
find
> a good process for getting the proper tools installed.
> (Also so that I can try this out myself.)

The first approach that Mohit took was to download one of the available
toolchains, and put it somewhere in the path. There was a problem with linker,
which he avoided with additional -L parameter.

Making this process easy for other developers, required changes to native client
scripts, to automatically download and install toolchain. To avoid this, I
changed initial approach. Since we have apt-get prerequisites installation
anyway, I decided to go with OS update, because 16.04 has available MIPS
toolchains in its repositories, so adding MIPS toolchain is just addition to
existing "apt-get install" command.

Another way may be to expand toolchain building scripts, to build proper
toolchains from the downloaded source.

[Jim Stichnoth, 2016-08-24 22:38:21]

On 2016/08/24 13:22:30, obucinac wrote:
> On 2016/08/23 17:22:36, stichnot wrote:
> > > I just updated my Ubuntu 14.04 to 16.04.1 and installed MIPS toolchain
from
> > > Ubuntu repositories.
> > 
> > Unfortunately we are not really allowed to update our distro, so I need to
> find
> > a good process for getting the proper tools installed.
> > (Also so that I can try this out myself.)
> 
> The first approach that Mohit took was to download one of the available
> toolchains, and put it somewhere in the path. There was a problem with linker,
> which he avoided with additional -L parameter.
> 
> Making this process easy for other developers, required changes to native
client
> scripts, to automatically download and install toolchain. To avoid this, I
> changed initial approach. Since we have apt-get prerequisites installation
> anyway, I decided to go with OS update, because 16.04 has available MIPS
> toolchains in its repositories, so adding MIPS toolchain is just addition to
> existing "apt-get install" command.
> 
> Another way may be to expand toolchain building scripts, to build proper
> toolchains from the downloaded source.

I'm thinking of taking a somewhat different approach, for now.

The idea would be to add a variable like MIPSTEST to the Makefile.standalone,
which would provide two configurations of testing.  Differences might be:

check-xtest: add the mips32 tests for MIPSTEST=1; no mips32 tests by default

check-lit: no difference

check-spec: don't run for MIPSTEST=1 (since I'm guessing you don't have the
spec2k package?)

check-presubmit: don't bother running so many configurations for MIPSTEST=1

Then, before sending out a CL, you'd be expected to verify that it passes
prescribed tests:
  make -f Makefile.standalone MIPSTEST=1 check-presubmit

(This is similar to the expectation over here that presubmit tests have already
passed.)

More specifically, I would only be testing that the mips32-related code builds
under all the configurations, and that the mips32 lit tests pass, but I wouldn't
be checking mips32 cross tests or (eventually) mips32 spec2k results.

I would start right away on a CL that does this.

Does that sound reasonable?

[obucinac, 2016-08-30 12:26:28]

On 2016/08/24 22:38:21, stichnot wrote:
> On 2016/08/24 13:22:30, obucinac wrote:
> > On 2016/08/23 17:22:36, stichnot wrote:
> > > > I just updated my Ubuntu 14.04 to 16.04.1 and installed MIPS toolchain
> from
> > > > Ubuntu repositories.
> > > 
> > > Unfortunately we are not really allowed to update our distro, so I need to
> > find
> > > a good process for getting the proper tools installed.
> > > (Also so that I can try this out myself.)
> > 
> > The first approach that Mohit took was to download one of the available
> > toolchains, and put it somewhere in the path. There was a problem with
linker,
> > which he avoided with additional -L parameter.
> > 
> > Making this process easy for other developers, required changes to native
> client
> > scripts, to automatically download and install toolchain. To avoid this, I
> > changed initial approach. Since we have apt-get prerequisites installation
> > anyway, I decided to go with OS update, because 16.04 has available MIPS
> > toolchains in its repositories, so adding MIPS toolchain is just addition to
> > existing "apt-get install" command.
> > 
> > Another way may be to expand toolchain building scripts, to build proper
> > toolchains from the downloaded source.
> 
> I'm thinking of taking a somewhat different approach, for now.
> 
> The idea would be to add a variable like MIPSTEST to the Makefile.standalone,
> which would provide two configurations of testing.  Differences might be:
> 
> check-xtest: add the mips32 tests for MIPSTEST=1; no mips32 tests by default
> 
> check-lit: no difference
> 
> check-spec: don't run for MIPSTEST=1 (since I'm guessing you don't have the
> spec2k package?)
> 
> check-presubmit: don't bother running so many configurations for MIPSTEST=1
> 
> Then, before sending out a CL, you'd be expected to verify that it passes
> prescribed tests:
>   make -f Makefile.standalone MIPSTEST=1 check-presubmit
> 
> (This is similar to the expectation over here that presubmit tests have
already
> passed.)
> 
> More specifically, I would only be testing that the mips32-related code builds
> under all the configurations, and that the mips32 lit tests pass, but I
wouldn't
> be checking mips32 cross tests or (eventually) mips32 spec2k results.
> 
> I would start right away on a CL that does this.
> 
> Does that sound reasonable?

Hello Jim,

Yes, its fine. We had some internal discussion about this, and regardless of
everything, the most important thing is that we can run tests and get the
results, with this patch merged or not, or with initial approach with
downloading third party toolchain. Update to 16.04 for everybody will happen
sooner or later.

Best regards

[Jim Stichnoth, 2016-08-30 13:23:32]

On 2016/08/30 12:26:28, obucinac wrote:
> > I would start right away on a CL that does this.
> > 
> > Does that sound reasonable?
> 
> Hello Jim,
> 
> Yes, its fine. We had some internal discussion about this, and regardless of
> everything, the most important thing is that we can run tests and get the
> results, with this patch merged or not, or with initial approach with
> downloading third party toolchain. Update to 16.04 for everybody will happen
> sooner or later.
> 
> Best regards

Thanks.  In that case, could you try out
https://codereview.chromium.org/2271053006/ and give me l-g-t-m and/or comments
on the CL?

[Jim Stichnoth, 2016-09-02 15:49:31]

https://codereview.chromium.org/2085303002/diff/60001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/2085303002/diff/60001/Makefile.standalone#new...
Makefile.standalone:566: -i mips32 \
On 2016/08/23 14:56:48, stichnot wrote:
> Change these two mips32 lines to this single line:
>   -i mips32,native,Om1,simple_loop
> 
> This restricts the mips32 cross tests to the single currently working test,
when
> "make -f Makefile.standalone check-xtest" is run.

Update: After you "git pull --rebase" to pull in
https://codereview.chromium.org/2271053006/ , put "-i
mips32,native,Om1,simple_loop" in the "ifdef MIPS" section.

https://codereview.chromium.org/2085303002/diff/60001/pydir/build-runtime.py
File pydir/build-runtime.py (right):

https://codereview.chromium.org/2085303002/diff/60001/pydir/build-runtime.py#...
pydir/build-runtime.py:213: MakeRuntimesForTarget(targets.MIPS32Target,
ll_files,
We need a way to not run this in a non-MIPS build environment.

It's probably best to control that via a command-line flag in this script.

Maybe add something like this above:

    argparser.add_argument('--exclude-target', dest='exclude_target',
                           default=[], action='append',
                           help='Target whose runtime should not be built')

And then pass args.exclude_target as an additional argument to
MakeRuntimesForTarget().  And at the top of MakeRuntimesForTarget(), do an early
exit:

  if target_info.target in excluded_targets:
    return

Then the Makefile.standalone needs some special MIPS controls for RT_SRC and
RT_OBJ.

I think this could be effectively tested for non-MIPS by temporarily defining
FindMIPSCrossInclude() to glob a nonexistent directory.

https://codereview.chromium.org/2085303002/diff/60001/pydir/targets.py
File pydir/targets.py (right):

https://codereview.chromium.org/2085303002/diff/60001/pydir/targets.py#newcode19
pydir/targets.py:19: return glob.glob(
This fails on a system without MIPS libraries, e.g. my system. :)

One possibility is this implementation:

def FindMIPSCrossInclude():
  globs = glob.glob('/usr/mipsel-linux-gnu/include/c++/*/mipsel-linux-gnu')
  return globs[-1] if globs else '/invalid/mips/include/path'

[obucinac, 2016-09-05 16:56:00]

https://codereview.chromium.org/2085303002/diff/60001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/2085303002/diff/60001/Makefile.standalone#new...
Makefile.standalone:566: -i mips32 \
On 2016/09/02 15:49:31, stichnot wrote:
> On 2016/08/23 14:56:48, stichnot wrote:
> > Change these two mips32 lines to this single line:
> >   -i mips32,native,Om1,simple_loop
> > 
> > This restricts the mips32 cross tests to the single currently working test,
> when
> > "make -f Makefile.standalone check-xtest" is run.
> 
> Update: After you "git pull --rebase" to pull in
> https://codereview.chromium.org/2271053006/ , put "-i
> mips32,native,Om1,simple_loop" in the "ifdef MIPS" section.

Done.

https://codereview.chromium.org/2085303002/diff/60001/Makefile.standalone#new...
Makefile.standalone:623: ifeq ($(TARGET),mips32)
On 2016/08/23 14:56:48, stichnot wrote:
> Should probably add a comment noting that
> native_client/tests/spec2k/{Makefile.common,run_all.sh} do not currently have
> MIPS configs, so those would need to be added for proper Subzero testing here.

Done.

https://codereview.chromium.org/2085303002/diff/60001/Makefile.standalone#new...
Makefile.standalone:626: ;SPEC := --filetype=asm
On 2016/08/23 14:56:48, stichnot wrote:
> Remove the ';'.
> 
> (Not sure if that was meant as a comment character which is actually '#', but
> this line without the ';' should be correct.)

Done.

https://codereview.chromium.org/2085303002/diff/60001/pydir/build-runtime.py
File pydir/build-runtime.py (right):

https://codereview.chromium.org/2085303002/diff/60001/pydir/build-runtime.py#...
pydir/build-runtime.py:67: ], echo=verbose)
On 2016/08/23 14:56:48, stichnot wrote:
> revert this whitespace change

Done.

https://codereview.chromium.org/2085303002/diff/60001/pydir/build-runtime.py#...
pydir/build-runtime.py:83: shellcmd([GetObjcopyCmd(target=target_info.target),
On 2016/08/23 14:56:48, stichnot wrote:
> Can you remove "target=" ?
> 
> here and below

Done.

https://codereview.chromium.org/2085303002/diff/60001/pydir/crosstest.py
File pydir/crosstest.py (right):

https://codereview.chromium.org/2085303002/diff/60001/pydir/crosstest.py#newc...
pydir/crosstest.py:173: shellcmd(['{bin}/{objcopy}'.format(bin=bindir,
objcopy=GetObjcopyCmd(args.target)),
On 2016/08/23 14:56:49, stichnot wrote:
> 80-col

Done.

https://codereview.chromium.org/2085303002/diff/60001/pydir/crosstest.py#newc...
pydir/crosstest.py:187: shellcmd(['{bin}/{objcopy}'.format(bin=bindir,
objcopy=GetObjcopyCmd(args.target)),
On 2016/08/23 14:56:49, stichnot wrote:
> 80-col

Done.

https://codereview.chromium.org/2085303002/diff/60001/pydir/crosstest.py#newc...
pydir/crosstest.py:255: shellcmd(['{bin}/{objcopy}'.format(bin=bindir,
objcopy=GetObjcopyCmd(args.target)),
On 2016/08/23 14:56:49, stichnot wrote:
> 80-col

Done.

https://codereview.chromium.org/2085303002/diff/60001/pydir/crosstest_generat...
File pydir/crosstest_generator.py (right):

https://codereview.chromium.org/2085303002/diff/60001/pydir/crosstest_generat...
pydir/crosstest_generator.py:79: 'mips32': ['base'] }
On 2016/08/23 14:56:49, stichnot wrote:
> [ 'base' ]
> for consistency

Done.

https://codereview.chromium.org/2085303002/diff/60001/pydir/crosstest_generat...
pydir/crosstest_generator.py:86: 'mips32': []}
On 2016/08/23 14:56:49, stichnot wrote:
> Add back in the space before the closing brace?
>   [] }

Done.

https://codereview.chromium.org/2085303002/diff/60001/pydir/run-pnacl-sz.py
File pydir/run-pnacl-sz.py (right):

https://codereview.chromium.org/2085303002/diff/60001/pydir/run-pnacl-sz.py#n...
pydir/run-pnacl-sz.py:23: 'mips32': ['-triple=%s' % ('mipsel-nacl' if sandboxed
else 'mipsel'),
On 2016/08/23 14:56:49, stichnot wrote:
> 80-col

Done.

https://codereview.chromium.org/2085303002/diff/60001/pydir/targets.py
File pydir/targets.py (right):

https://codereview.chromium.org/2085303002/diff/60001/pydir/targets.py#newcode19
pydir/targets.py:19: return glob.glob(
On 2016/09/02 15:49:31, stichnot wrote:
> This fails on a system without MIPS libraries, e.g. my system. :)
> 
> One possibility is this implementation:
> 
> def FindMIPSCrossInclude():
>   globs = glob.glob('/usr/mipsel-linux-gnu/include/c++/*/mipsel-linux-gnu')
>   return globs[-1] if globs else '/invalid/mips/include/path'

Done.

https://codereview.chromium.org/2085303002/diff/60001/pydir/targets.py#newcode58
pydir/targets.py:58: cross_headers=['-isystem', FindMIPSCrossInclude()])
On 2016/08/23 14:56:49, stichnot wrote:
> fix alignment

Done.

https://codereview.chromium.org/2085303002/diff/60001/runtime/szrt_asm_mips32.s
File runtime/szrt_asm_mips32.s (right):

https://codereview.chromium.org/2085303002/diff/60001/runtime/szrt_asm_mips32...
runtime/szrt_asm_mips32.s:16: .p2alignl 4,0xE7FEDEF0
On 2016/08/23 14:56:49, stichnot wrote:
> This is the encoding for an ARM trap instruction.  See
> https://codereview.chromium.org/1649053002/ .
> 
> What is the right value for MIPS32?

Not sure with which instruction to match this one.
I will try BREAK.

[obucinac, 2016-09-05 17:02:08]

https://codereview.chromium.org/2085303002/diff/80001/pydir/crosstest.py
File pydir/crosstest.py (right):

https://codereview.chromium.org/2085303002/diff/80001/pydir/crosstest.py#newc...
pydir/crosstest.py:190: objcopy=GetObjcopyCmd(args.target)),
These symbols can not be stripped from obj file. The current solution is to
remove stripping, but we are yet to see how this will affect our tests.

[Jim Stichnoth, 2016-09-06 19:04:44]

There are still problems with this patchset, with respect to running tests in a
non-MIPS environment without the MIPS toolchain properly installed.  With the
patch below, applied against the latest patchset, "make -f Makefile.standalone
check-xtest" works correctly again.

(If the paste below is corrupted or unusable, I can send it directly by email.)


diff --git a/Makefile.standalone b/Makefile.standalone
index f972ccf..81742db 100644
--- a/Makefile.standalone
+++ b/Makefile.standalone
@@ -514,7 +514,6 @@ $(OBJDIR)/unittest/AssemblerX8664: $(OBJDIR)/unittest
 RT_SRC := runtime/szrt.c runtime/szrt_ll.ll runtime/szrt_profiler.c \
           runtime/szrt_asm_x8632.s runtime/szrt_asm_x8664.s \
           runtime/szrt_asm_arm32.s \
-          runtime/szrt_asm_mips32.s \
           runtime/szrt_asan.c
 RT_OBJ := build/runtime/szrt_native_x8632.o build/runtime/szrt_sb_x8632.o \
           build/runtime/szrt_nonsfi_x8632.o \
@@ -522,10 +521,17 @@ RT_OBJ := build/runtime/szrt_native_x8632.o
build/runtime/szrt_sb_x8632.o \
           build/runtime/szrt_nonsfi_x8664.o \
           build/runtime/szrt_native_arm32.o build/runtime/szrt_sb_arm32.o \
           build/runtime/szrt_nonsfi_arm32.o \
-          build/runtime/szrt_native_mips32.o build/runtime/szrt_sb_mips32.o \
           build/runtime/szrt_asan_x8632.o build/runtime/szrt_asan_x8664.o \
           build/runtime/szrt_asan_arm32.o
 
+EXCLUDED_RT :=
+ifdef MIPS
+RT_SRC += runtime/szrt_asm_mips32.s
+RT_OBJ += build/runtime/szrt_native_mips32.o build/runtime/szrt_sb_mips32.o
+else
+EXCLUDED_RT += --exclude-target=mips32
+endif
+
 runtime: $(RT_OBJ)
 
 # Use runtime.is.built so that build-runtime.py is invoked only once
@@ -534,7 +540,8 @@ runtime: $(RT_OBJ)
 $(RT_OBJ): runtime.is.built
 runtime.is.built: $(RT_SRC) pydir/build-runtime.py
        @echo ================ Building Subzero runtime ================
-       ./pydir/build-runtime.py -v --pnacl-root $(PNACL_TOOLCHAIN_ROOT)
+       ./pydir/build-runtime.py -v --pnacl-root $(PNACL_TOOLCHAIN_ROOT) \
+          $(EXCLUDED_RT)
 
 check-lit: $(OBJDIR)/pnacl-sz make_symlink runtime
        PNACL_BIN_PATH=$(PNACL_BIN_PATH) \
diff --git a/pydir/build-runtime.py b/pydir/build-runtime.py
index d22dc87..4239190 100755
--- a/pydir/build-runtime.py
+++ b/pydir/build-runtime.py
@@ -17,6 +17,7 @@ def Translate(ll_files, extra_args, obj, verbose, target):
   """
   shellcmd(['cat'] + ll_files + ['|',
             'pnacl-llc',
+            '-externalize',
             '-function-sections',
             '-O2',
             '-filetype=obj',
@@ -40,8 +41,10 @@ def PartialLink(obj_files, extra_args, lib, verbose):
 
 
 def MakeRuntimesForTarget(target_info, ll_files,
-                          srcdir, tempdir, rtdir, verbose):
+                          srcdir, tempdir, rtdir, verbose, excluded_targets):
   """Builds native, sandboxed, and nonsfi runtimes for the given target."""
+  if target_info.target in excluded_targets:
+    return
   # File-mangling helper functions.
   def TmpFile(template):
     return template.format(dir=tempdir, target=target_info.target)
@@ -55,7 +58,7 @@ def MakeRuntimesForTarget(target_info, ll_files,
     Translate(ll_files,
               ['-mtriple=' + target_info.triple] + target_info.llc_flags,
               TmpFile('{dir}/szrt_native_{target}.tmp.o'),
-              verbose,target_info.target)
+              verbose, target_info.target)
     # Compile srcdir/szrt_profiler.c to
     # tempdir/szrt_profiler_native_{target}.o.
     shellcmd(['clang',
@@ -171,6 +174,9 @@ def main():
                              '{root}/toolchain/linux_x86/pnacl_newlib_raw'
                            ).format(root=nacl_root),
                            help='Path to PNaCl toolchain binaries.')
+    argparser.add_argument('--exclude-target', dest='excluded_targets',
+                           default=[], action='append',
+                           help='Target whose runtime should not be built')
     args = argparser.parse_args()
     os.environ['PATH'] = ('{root}/bin{sep}{path}'
         ).format(root=args.pnacl_root, sep=os.pathsep, path=os.environ['PATH'])
@@ -205,13 +211,17 @@ def main():
                     '{srcdir}/szrt_ll.ll'.format(srcdir=srcdir)]
 
         MakeRuntimesForTarget(targets.X8632Target, ll_files,
-                              srcdir, tempdir, rtdir, args.verbose)
+                              srcdir, tempdir, rtdir, args.verbose,
+                              args.excluded_targets)
         MakeRuntimesForTarget(targets.X8664Target, ll_files,
-                              srcdir, tempdir, rtdir, args.verbose)
+                              srcdir, tempdir, rtdir, args.verbose,
+                              args.excluded_targets)
         MakeRuntimesForTarget(targets.ARM32Target, ll_files,
-                              srcdir, tempdir, rtdir, args.verbose)
+                              srcdir, tempdir, rtdir, args.verbose,
+                              args.excluded_targets)
         MakeRuntimesForTarget(targets.MIPS32Target, ll_files,
-                              srcdir, tempdir, rtdir, args.verbose)
+                              srcdir, tempdir, rtdir, args.verbose,
+                              args.excluded_targets)
 
     finally:
         try:
diff --git a/pydir/crosstest.py b/pydir/crosstest.py
index 956957c..e0a6198 100755
--- a/pydir/crosstest.py
+++ b/pydir/crosstest.py
@@ -186,9 +186,13 @@ def main():
                   '-bitcode-format=llvm',
                   '-o=' + obj_llc,
                   bitcode] + llc_flags)
+        strip_syms = []
+        if args.target != 'mips32':
+          strip_syms += ['nacl_tp_tdb_offset', 'nacl_tp_tls_offset']
         shellcmd(['{bin}/{objcopy}'.format(bin=bindir,
                   objcopy=GetObjcopyCmd(args.target)),
-                  obj_llc])
+                  obj_llc] +
+                 [('--strip-symbol=' + sym) for sym in strip_syms])
         objs.append(obj_llc)
 
     # Add szrt_sb_${target}.o or szrt_native_${target}.o.

https://codereview.chromium.org/2085303002/diff/80001/pydir/build-runtime.py
File pydir/build-runtime.py (left):

https://codereview.chromium.org/2085303002/diff/80001/pydir/build-runtime.py#...
pydir/build-runtime.py:20: '-externalize',
Why is -externalize removed?  This actually breaks many of the non-MIPS cross
tests.

https://codereview.chromium.org/2085303002/diff/80001/pydir/crosstest.py
File pydir/crosstest.py (right):

https://codereview.chromium.org/2085303002/diff/80001/pydir/crosstest.py#newc...
pydir/crosstest.py:190: objcopy=GetObjcopyCmd(args.target)),
On 2016/09/05 17:02:08, obucinac wrote:
> These symbols can not be stripped from obj file. The current solution is to
> remove stripping, but we are yet to see how this will affect our tests.

When the symbols are not stripped, the sandbox and nonsfi tests all fail with a
multiply defined symbol error.

The patch I supplied handle this, though I would like to understand why the MIPS
objcopy command fails.

[obucinac, 2016-09-07 13:46:02]

This is the stripping error message:

[cmd]
cat /tmp/tmpeuji82/szrt.ll
/scratch/workareas/subzero/subzero/nacl/native_client/toolchain_build/src/subzero/runtime/szrt_ll.ll
| pnacl-llc -externalize -function-sections -O2 -filetype=obj
-bitcode-format=llvm -arm-enable-dwarf-eh=1 -o
/tmp/tmpeuji82/szrt_native_mips32.tmp.o -mtriple=mipsel-linux-gnu

[cmd]
mipsel-nacl-objcopy --strip-symbol=nacl_tp_tdb_offset
--strip-symbol=nacl_tp_tls_offset /tmp/tmpeuji82/szrt_native_mips32.tmp.o

mipsel-nacl-objcopy: not stripping symbol `nacl_tp_tdb_offset' because it is
named in a relocation
mipsel-nacl-objcopy: not stripping symbol `nacl_tp_tls_offset' because it is
named in a relocation

I guess that, when '-externalize' is removed, the symbol visibility is
different, and linking for MIPS passes without problem.
I encountered this problem with other toolchains also, at the beginning of
creating these changes.

[obucinac, 2016-09-22 14:02:43]

I maded a workaround for externalize and now I am able to run crosstests with
two test passed, simple_loop and test_strengthreduce, with a command line

make -j12 -f Makefile.standalone DEBUG=1 MIPS=1 FORCEASM=1 check-xtest

[Jim Stichnoth, 2016-09-23 05:29:11]

Very nice!  "make -f Makefile.standalone presubmit" works perfectly in my
environment that does not contain the MIPS toolchain.

In the proper MIPS environment, does this work?
  make -f Makefile.standalone -j12 MIPS=1 presubmit
(using a suitable -j value for your system)

https://codereview.chromium.org/2085303002/diff/140001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/2085303002/diff/140001/Makefile.standalone#ne...
Makefile.standalone:516: runtime/szrt_asm_arm32.s \
Revert this change?  (i.e. put them on the same line)

https://codereview.chromium.org/2085303002/diff/140001/pydir/build-runtime.py
File pydir/build-runtime.py (right):

https://codereview.chromium.org/2085303002/diff/140001/pydir/build-runtime.py...
pydir/build-runtime.py:18: externalize = []
externalize = [] if target == 'mips32' else ['-externalize']

https://codereview.chromium.org/2085303002/diff/140001/pydir/build-runtime.py...
pydir/build-runtime.py:29: strip_syms = []
strip_syms = [] if target == 'mips32' else ['nacl_tp_tdb_offset',
'nacl_tp_tls_offset']

(formatted to 80-col)

https://codereview.chromium.org/2085303002/diff/140001/pydir/crosstest.py
File pydir/crosstest.py (right):

https://codereview.chromium.org/2085303002/diff/140001/pydir/crosstest.py#new...
pydir/crosstest.py:189: strip_syms = []
strip_syms = [] if args.target == 'mips32' else [ ... ]

[obucinac, 2016-09-23 13:22:03]

Jim, for some reason, I was never able to build sbtc.

./toolchain_build/toolchain_build_pnacl.py --build-sbtc

does not work for me.

As presubmit includes sbtc, the command

make -f Makefile.standalone -j12 MIPS=1 presubmit

Consider running 'toolchain_build_pnacl.py --build-sbtc'.
Makefile.standalone:691: recipe for target 'exists-sbtc' failed
make: *** [exists-sbtc] Error 1

https://codereview.chromium.org/2085303002/diff/140001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/2085303002/diff/140001/Makefile.standalone#ne...
Makefile.standalone:516: runtime/szrt_asm_arm32.s \
On 2016/09/23 05:29:11, stichnot wrote:
> Revert this change?  (i.e. put them on the same line)

Done.

https://codereview.chromium.org/2085303002/diff/140001/pydir/build-runtime.py
File pydir/build-runtime.py (right):

https://codereview.chromium.org/2085303002/diff/140001/pydir/build-runtime.py...
pydir/build-runtime.py:18: externalize = []
On 2016/09/23 05:29:11, stichnot wrote:
> externalize = [] if target == 'mips32' else ['-externalize']

Done.

https://codereview.chromium.org/2085303002/diff/140001/pydir/build-runtime.py...
pydir/build-runtime.py:29: strip_syms = []
On 2016/09/23 05:29:11, stichnot wrote:
> strip_syms = [] if target == 'mips32' else ['nacl_tp_tdb_offset',
> 'nacl_tp_tls_offset']
> 
> (formatted to 80-col)

Done.

https://codereview.chromium.org/2085303002/diff/140001/pydir/crosstest.py
File pydir/crosstest.py (right):

https://codereview.chromium.org/2085303002/diff/140001/pydir/crosstest.py#new...
pydir/crosstest.py:189: strip_syms = []
On 2016/09/23 05:29:11, stichnot wrote:
> strip_syms = [] if args.target == 'mips32' else [ ... ]

Done.

[obucinac, 2016-09-23 13:25:15]

obucinac@gmail.com changed reviewers:
+ eholk@chromium.org, jpp@chromium.org, kschimpf@google.com
- mohit.bhakkad@imgtec.com

[Jim Stichnoth, 2016-09-23 18:05:47]

On 2016/09/23 13:22:03, obucinac wrote:
> Jim, for some reason, I was never able to build sbtc.
> 
> ./toolchain_build/toolchain_build_pnacl.py --build-sbtc
> 
> does not work for me.
> 
> As presubmit includes sbtc, the command
> 
> make -f Makefile.standalone -j12 MIPS=1 presubmit
> 
> Consider running 'toolchain_build_pnacl.py --build-sbtc'.
> Makefile.standalone:691: recipe for target 'exists-sbtc' failed
> make: *** [exists-sbtc] Error 1

OK. For now I guess we can disable that part of presubmit, but it's good to test
as many compilers as practical for build errors (currently clang++,
pnacl-clang++, and g++, and someone else has recently been using Visual Studio).

In a separate email thread, could you send me the errors for
"./toolchain_build/toolchain_build_pnacl.py --build-sbtc"?

[obucinac, 2016-09-25 19:14:41]

Messed up. I had crosstest patch applied to intrinsic calls branch for testing
purposes, and wrong patch landed here.

[obucinac, 2016-09-25 21:50:02]

Jim, tested with the commands You sent, sbtc can be built, and I can
successfully run presubmit tests for test_fcmp with disabled vector cases.
simple_loop and test_strengthreduce are failing because of unimplemented
instruction encodigs.
With FORCEASM, X86* reports that in many tests only filetype=obj can be used.

Now, I am not sure what tests to put there, because every one of them is missing
something to sucessfully run.

Add more IFs for special cases?

[Jim Stichnoth, 2016-09-26 05:20:04]

https://codereview.chromium.org/2085303002/diff/220001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/2085303002/diff/220001/Makefile.standalone#ne...
Makefile.standalone:716: # Run lit tests, cross tests, and unit tests.
I forgot about the fact that MIPS32 cross tests currently require FORCEASM.

Does it work if this sub-make command is replace with the following two
commands?

+make -f Makefile.standalone check-lit check-unit
+make -f Makefile.standalone FORCEASM=1 check-xtest

Specifically, I want "make -f Makefile.standalone -j12 MIPS=1 presubmit" to work
correctly.

[obucinac, 2016-09-26 20:53:03]

Hello,

I finally got it working. I added some missing instruction encodings, and
simple_loop is running.

make -j12 -f Makefile.standalone MIPS=1 presubmit

[obucinac, 2016-09-26 20:59:02]

Description was changed from

==========
Subzero, MIPS32: Cross-testing enabled for MIPS32

This patch adds MIPS32 to crosstest test suite.
==========

to

==========
Subzero, MIPS32: Cross-testing enabled for MIPS32

Enables running crosstests for MIPS32 target.
==========

[Jim Stichnoth, 2016-09-27 03:01:32]

LGTM!!!

[obucinac, 2016-09-27 03:03:19]

YEAH!!! MERGE!!! MERGE!!!

[Jim Stichnoth, 2016-09-27 03:03:24]

Description was changed from

==========
Subzero, MIPS32: Cross-testing enabled for MIPS32

Enables running crosstests for MIPS32 target.
==========

to

==========
Subzero, MIPS32: Cross-testing enabled for MIPS32

Enables running crosstests for MIPS32 target.

R=stichnot@chromium.org

Committed:
https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a...
==========

[Jim Stichnoth, 2016-09-27 03:03:26]

Committed patchset #15 (id:280001) manually as
623f8ce32c8dd33f9c755dde229967dfddd3353b (presubmit successful).