Subzero, MIPS32: Sandbox initial patch

Subzero, MIPS32: Sandbox initial patch

Initial patch regarding the Subzero MIPS32 sandboxing stage.
At the moment, the results of the crosstests with vector tests
disabled are as follows:

ASM mode:
19 passing / 5 failing
test_bitmanip: O2
test_calling_conv: Om1, O2
test_sync_atomic: Om1, O2

ELF mode:
15 passing / 9 failing
test_bitmanip: O2
test_calling_conv: Om1, O2
test_global: Om1, O2
test_stacksave: Om1, O2
test_sync_atomic: Om1, O2

R=stichnot@chromium.org

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

[Stefan Maksimovic, 2016-11-08 08:56:17]

Description was changed from

==========
Subzero, MIPS32: Sandbox initial patch

Initial patch regarding the Subzero MIPS32 sandboxing stage.
At the moment, the results of the crosstests with vector tests
disabled are as follows:

ASM mode:
19 passing / 5 failing
test_bitmanip: O2
test_calling_conv: Om1, O2
test_sync_atomic: Om1, O2

ELF mode:
15 passing / 9 failing
test_bitmanip: O2
test_calling_conv: Om1, O2
test_global: Om1, O2
test_stacksave: Om1, O2
test_sync_atomic: Om1, O2
==========

to

==========
Subzero, MIPS32: Sandbox initial patch

Initial patch regarding the Subzero MIPS32 sandboxing stage.
At the moment, the results of the crosstests with vector tests
disabled are as follows:

ASM mode:
19 passing / 5 failing
test_bitmanip: O2
test_calling_conv: Om1, O2
test_sync_atomic: Om1, O2

ELF mode:
15 passing / 9 failing
test_bitmanip: O2
test_calling_conv: Om1, O2
test_global: Om1, O2
test_stacksave: Om1, O2
test_sync_atomic: Om1, O2
==========

[Stefan Maksimovic, 2016-11-08 08:56:17]

makdstefan@gmail.com changed reviewers:
+ jaydeep.patil@imgtec.com, sagar.thakur@imgtec.com, stichnot@chromium.org

[Jim Stichnoth, 2016-11-08 13:36:32]

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

https://codereview.chromium.org/2482123002/diff/1/pydir/build-runtime.py#newc...
pydir/build-runtime.py:29: if target != 'mips32':
Is this diff actually needed?  As I understand it, "objcopy -L foo" is short for
"objcopy --localize-symbol=foo", and shouldn't that be unnecessary when
--strip-symbol is already being applied to those symbols?

https://codereview.chromium.org/2482123002/diff/1/src/IceTargetLoweringMIPS32...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2482123002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:1658: return;
Remove this unnecessary return statement.

https://codereview.chromium.org/2482123002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:5464: return;
Remove this return statement

[Stefan Maksimovic, 2016-11-08 14:59:56]

As a side note, this is still WIP as I am trying to make the remaining cross
tests to work, and we'll probably need to hear from Jaydeep on when to accept
the change if all goes fine and well.

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

https://codereview.chromium.org/2482123002/diff/1/pydir/build-runtime.py#newc...
pydir/build-runtime.py:29: if target != 'mips32':
On 2016/11/08 13:36:31, Jim Stichnoth wrote:
> Is this diff actually needed?  As I understand it, "objcopy -L foo" is short
for
> "objcopy --localize-symbol=foo", and shouldn't that be unnecessary when
> --strip-symbol is already being applied to those symbols?

Initially I got the undefined reference errors pointing to the _Sz_* functions
from szrt.c when trying to run the cross tests, which was due to them being
local symbols.
I added the -externalize flag, making them global, which also made
nacl_tp_tdb_offset and nacl_tp_tls_offset global as well.

If those two symbols were not stripped, I'd get an error complaining about them
being defined multiple times.
If I tried to strip them, it would fail as well with the message that they are
named in a relocation, thus cannot be stripped.

The solution which worked for me was to keep them there, but to make them local,
which enabled me to run the cross tests successfully.

Please advise if there is anything faulty with this approach.

https://codereview.chromium.org/2482123002/diff/1/src/IceTargetLoweringMIPS32...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2482123002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:1658: return;
On 2016/11/08 13:36:31, Jim Stichnoth wrote:
> Remove this unnecessary return statement.

Done.

https://codereview.chromium.org/2482123002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:5464: return;
On 2016/11/08 13:36:31, Jim Stichnoth wrote:
> Remove this return statement

Done.

[Jim Stichnoth, 2016-11-08 15:34:08]

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

https://codereview.chromium.org/2482123002/diff/1/pydir/build-runtime.py#newc...
pydir/build-runtime.py:29: if target != 'mips32':
On 2016/11/08 14:59:55, Stefan Maksimovic wrote:
> On 2016/11/08 13:36:31, Jim Stichnoth wrote:
> > Is this diff actually needed?  As I understand it, "objcopy -L foo" is short
> for
> > "objcopy --localize-symbol=foo", and shouldn't that be unnecessary when
> > --strip-symbol is already being applied to those symbols?
> 
> Initially I got the undefined reference errors pointing to the _Sz_* functions
> from szrt.c when trying to run the cross tests, which was due to them being
> local symbols.
> I added the -externalize flag, making them global, which also made
> nacl_tp_tdb_offset and nacl_tp_tls_offset global as well.
> 
> If those two symbols were not stripped, I'd get an error complaining about
them
> being defined multiple times.
> If I tried to strip them, it would fail as well with the message that they are
> named in a relocation, thus cannot be stripped.
> 
> The solution which worked for me was to keep them there, but to make them
local,
> which enabled me to run the cross tests successfully.
> 
> Please advise if there is anything faulty with this approach.

Ah sorry!  I misread the logic in the new code.  I thought you were both
localizing and stripping those symbols.  Instead, I see you are localizing them
for mips and stripping them for non-mips.

It looks like the non-mips targets also work if I change --strip-symbol to
--localize-symbol.  So I think we can just make that change globally.  Rename
strip_syms to localize_syms, initialize it unconditionally to
nacl_tp_{tdb,tls}_offset, and do:

  shellcmd([GetObjcopyCmd(target), obj] + [('--localize-symbol=' + sym) for sym
in localize_syms])

[jaydeep.patil, 2016-11-09 03:50:26]

On 2016/11/08 14:59:56, Stefan Maksimovic wrote:
> As a side note, this is still WIP as I am trying to make the remaining cross
> tests to work, and we'll probably need to hear from Jaydeep on when to accept
> the change if all goes fine and well.

I think it is fine to submit the patch with few known issues.


> https://codereview.chromium.org/2482123002/diff/1/pydir/build-runtime.py
> File pydir/build-runtime.py (right):
> 
>
https://codereview.chromium.org/2482123002/diff/1/pydir/build-runtime.py#newc...
> pydir/build-runtime.py:29: if target != 'mips32':
> On 2016/11/08 13:36:31, Jim Stichnoth wrote:
> > Is this diff actually needed?  As I understand it, "objcopy -L foo" is short
> for
> > "objcopy --localize-symbol=foo", and shouldn't that be unnecessary when
> > --strip-symbol is already being applied to those symbols?
> 
> Initially I got the undefined reference errors pointing to the _Sz_* functions
> from szrt.c when trying to run the cross tests, which was due to them being
> local symbols.
> I added the -externalize flag, making them global, which also made
> nacl_tp_tdb_offset and nacl_tp_tls_offset global as well.
> 
> If those two symbols were not stripped, I'd get an error complaining about
them
> being defined multiple times.
> If I tried to strip them, it would fail as well with the message that they are
> named in a relocation, thus cannot be stripped.
> 
> The solution which worked for me was to keep them there, but to make them
local,
> which enabled me to run the cross tests successfully.
> 
> Please advise if there is anything faulty with this approach.
> 
>
https://codereview.chromium.org/2482123002/diff/1/src/IceTargetLoweringMIPS32...
> File src/IceTargetLoweringMIPS32.cpp (right):
> 
>
https://codereview.chromium.org/2482123002/diff/1/src/IceTargetLoweringMIPS32...
> src/IceTargetLoweringMIPS32.cpp:1658: return;
> On 2016/11/08 13:36:31, Jim Stichnoth wrote:
> > Remove this unnecessary return statement.
> 
> Done.
> 
>
https://codereview.chromium.org/2482123002/diff/1/src/IceTargetLoweringMIPS32...
> src/IceTargetLoweringMIPS32.cpp:5464: return;
> On 2016/11/08 13:36:31, Jim Stichnoth wrote:
> > Remove this return statement
> 
> Done.

[Jim Stichnoth, 2016-11-09 14:02:16]

LGTM.

If I don't hear anything in the next few hours, I'll land this CL.

If any new changes are needed, please also run "make format". (though formatting
changes to IceInstX86BaseImpl.h should be manually reverted since that mistake
is currently being fixed in a separate CL)

[Stefan Maksimovic, 2016-11-09 14:45:12]

Patchset #4 (id:60001) has been deleted

[Stefan Maksimovic, 2016-11-09 14:57:09]

With these changes, we're able to run the same amount of cross tests as in
native ELF mode.
However, now the stacksave test fails in ASM mode, with the message that the
fragment can't be larger than a bundle size, which LLVM reports.

I guess it would be okay to address that issue in a separate patch, if anything
comes to mind in the meantime.

[Jim Stichnoth, 2016-11-10 01:31:54]

Description was changed from

==========
Subzero, MIPS32: Sandbox initial patch

Initial patch regarding the Subzero MIPS32 sandboxing stage.
At the moment, the results of the crosstests with vector tests
disabled are as follows:

ASM mode:
19 passing / 5 failing
test_bitmanip: O2
test_calling_conv: Om1, O2
test_sync_atomic: Om1, O2

ELF mode:
15 passing / 9 failing
test_bitmanip: O2
test_calling_conv: Om1, O2
test_global: Om1, O2
test_stacksave: Om1, O2
test_sync_atomic: Om1, O2
==========

to

==========
Subzero, MIPS32: Sandbox initial patch

Initial patch regarding the Subzero MIPS32 sandboxing stage.
At the moment, the results of the crosstests with vector tests
disabled are as follows:

ASM mode:
19 passing / 5 failing
test_bitmanip: O2
test_calling_conv: Om1, O2
test_sync_atomic: Om1, O2

ELF mode:
15 passing / 9 failing
test_bitmanip: O2
test_calling_conv: Om1, O2
test_global: Om1, O2
test_stacksave: Om1, O2
test_sync_atomic: Om1, O2

R=stichnot@chromium.org

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

[Jim Stichnoth, 2016-11-10 01:31:55]

Committed patchset #4 (id:80001) manually as
6e03343bc3076bc76e994a5027c11b014c0af839 (tree was closed).