Subzero. ARM32. Nonsfi.

Subzero. ARM32. Nonsfi.

Adds nonsfi support to the ARM32 backend.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4076
R=stichnot@chromium.org

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

[John, 2016-02-09 13:33:05]

jpp@chromium.org changed reviewers:
+ eholk@chromium.org, kschimpf@google.com, sehr@chromium.org,
stichnot@chromium.org

[Jim Stichnoth, 2016-02-10 06:36:00]

lgtm

https://codereview.chromium.org/1665263003/diff/280001/crosstest/test_cast_to...
File crosstest/test_cast_to_u1.ll (right):

https://codereview.chromium.org/1665263003/diff/280001/crosstest/test_cast_to...
crosstest/test_cast_to_u1.ll:80: %tobool.i1 = trunc i32 %tobool to i1
I'd like to understand more about this change.

I expect that one problem is that we are testing undefined behavior, i.e. in
most cases a double doesn't truncate "cleanly" to i1 and results in undefined
behavior.

But truncating (fptoui) a double to i32 also may have undefined behavior, so
this change isn't making the test bulletproof or anything.

In fact, when possible, Subzero's goal is to mimic llc's undefined behavior.  In
theory, this minimizes wasting time investigating bug reports that turn out to
be the program relying on undefined behavior.

All that said, what on earth does this have to do with nonsfi, and why didn't we
see it before?

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

https://codereview.chromium.org/1665263003/diff/280001/pydir/build-runtime.py...
pydir/build-runtime.py:26: '-arm-enable-dwarf-eh=1',
Just checking - is this flag also happening in the PNaCl target library build
scripts?

https://codereview.chromium.org/1665263003/diff/280001/runtime/szrt_asm_arm32.s
File runtime/szrt_asm_arm32.s (right):

https://codereview.chromium.org/1665263003/diff/280001/runtime/szrt_asm_arm32...
runtime/szrt_asm_arm32.s:19: b	__aeabi_read_tp
Optional: This tailcall is simple and all, but I wonder if there is some asm
magic that could tell the linker in this file just to substitute the value of
__aeabi_read_tp for __nacl_read_tp .

https://codereview.chromium.org/1665263003/diff/280001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/1665263003/diff/280001/src/IceCfg.h#newcode160
src/IceCfg.h:160: /// \name Manages the Globals used by this CFG.
s/Manages/Manage/ for consistency

Also, maybe s/CFG/function/ for consistency?

https://codereview.chromium.org/1665263003/diff/280001/src/IceELFObjectWriter...
File src/IceELFObjectWriter.cpp (right):

https://codereview.chromium.org/1665263003/diff/280001/src/IceELFObjectWriter...
src/IceELFObjectWriter.cpp:27: #include "llvm/Support/ELF.h"
alphabetical order

https://codereview.chromium.org/1665263003/diff/280001/src/IceELFObjectWriter...
src/IceELFObjectWriter.cpp:422: const auto *Reloc =
Cool.  If you think it's appropriate, there are a couple other fixes like this,
in a couple different files:

  git grep 'auto[^*&]*=$'

https://codereview.chromium.org/1665263003/diff/280001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1665263003/diff/280001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:753: // TODO(jpp): explain fake def.
yes, I'm curious...

edit: I see, reading ahead in materializeGotAddr().

So if I understand correctly, you're using a InstFakeDef in the same way
InstX86GetIP is used on the x86 side, with the advantage that you don't have to
implement all the instruction boilerplate?  Cool, maybe the x86 side should be
simplified the same way.

https://codereview.chromium.org/1665263003/diff/280001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:760: const IceString CRName = CR->getName();
maybe const IceString &CRName

https://codereview.chromium.org/1665263003/diff/280001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:5662: llvm::isa<ConstantFloat>(Opnd) ||
llvm::isa<ConstantDouble>(Opnd);
Can/should this be more precise?  I thought that +0.0 for float and/or double
was synthesized using xor.

[John, 2016-02-10 15:41:14]

not quite ready to land. please take another look.

https://codereview.chromium.org/1665263003/diff/280001/crosstest/test_cast_to...
File crosstest/test_cast_to_u1.ll (right):

https://codereview.chromium.org/1665263003/diff/280001/crosstest/test_cast_to...
crosstest/test_cast_to_u1.ll:80: %tobool.i1 = trunc i32 %tobool to i1
On 2016/02/10 06:35:59, stichnot wrote:
> I'd like to understand more about this change.
> 
> I expect that one problem is that we are testing undefined behavior, i.e. in
> most cases a double doesn't truncate "cleanly" to i1 and results in undefined
> behavior.
> 
> But truncating (fptoui) a double to i32 also may have undefined behavior, so
> this change isn't making the test bulletproof or anything.
> 
> In fact, when possible, Subzero's goal is to mimic llc's undefined behavior. 
In
> theory, this minimizes wasting time investigating bug reports that turn out to
> be the program relying on undefined behavior.
> 
> All that said, what on earth does this have to do with nonsfi, and why didn't
we
> see it before?

the nonsfi test_cast_main.cpp is ocmpiled with -O2, while {nacl,native} are both
compiled with -O0.

For both O0 and O2, pnacl-clang does the following

ret.llc = cast<float, bool>()
ret.llc.i1 = reg & 1
ret.sz = Subzero_::cast<float, bool>()
ret.sz.i1 = reg & 1

now, here's the  difference. For O0, pnacl-clang does

memcmp(&ret.llc.i1, &ret.sz.i1)

and, for O2,

cmp ret.llc, ret.sz


For X86, we see the same lowering for calling the cast functions, and the same
memcmp call for O0. For O2, it does

cmp.i32 = xor ret.llc, reg.sz
cmp.i1 = cmp.i32 & 1

In essence, arm32 nonsfi just exposed something that's "wrong" with subzero in
general. Even for x86, llvm does not & 1 when converting from from f?? to i1,
but subzero does.

What should we do here? I can see us leaving this change here, or we could
change the crosstest.py script, or we could even update the targets to not "& 1"
these fptoui conversions (this last approach would probably be better off if
implemented in a different CL, and we would still need a stop gap solution.)

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

https://codereview.chromium.org/1665263003/diff/280001/pydir/build-runtime.py...
pydir/build-runtime.py:26: '-arm-enable-dwarf-eh=1',
On 2016/02/10 06:35:59, stichnot wrote:
> Just checking - is this flag also happening in the PNaCl target library build
> scripts?

Are you asking if this is park of pnacl-translate.py? I just verified -- it is.
:)

https://codereview.chromium.org/1665263003/diff/280001/runtime/szrt_asm_arm32.s
File runtime/szrt_asm_arm32.s (right):

https://codereview.chromium.org/1665263003/diff/280001/runtime/szrt_asm_arm32...
runtime/szrt_asm_arm32.s:19: b	__aeabi_read_tp
On 2016/02/10 06:35:59, stichnot wrote:
> Optional: This tailcall is simple and all, but I wonder if there is some asm
> magic that could tell the linker in this file just to substitute the value of
> __aeabi_read_tp for __nacl_read_tp .

Not as far as I could find. I wonder, should the TargetLowering generate a call
to __aeabi_read_tp directly?

https://codereview.chromium.org/1665263003/diff/280001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/1665263003/diff/280001/src/IceCfg.h#newcode160
src/IceCfg.h:160: /// \name Manages the Globals used by this CFG.
On 2016/02/10 06:35:59, stichnot wrote:
> s/Manages/Manage/ for consistency
> 
> Also, maybe s/CFG/function/ for consistency?

Done.

https://codereview.chromium.org/1665263003/diff/280001/src/IceELFObjectWriter...
File src/IceELFObjectWriter.cpp (right):

https://codereview.chromium.org/1665263003/diff/280001/src/IceELFObjectWriter...
src/IceELFObjectWriter.cpp:27: #include "llvm/Support/ELF.h"
On 2016/02/10 06:36:00, stichnot wrote:
> alphabetical order

Done.

https://codereview.chromium.org/1665263003/diff/280001/src/IceELFObjectWriter...
src/IceELFObjectWriter.cpp:422: const auto *Reloc =
On 2016/02/10 06:36:00, stichnot wrote:
> Cool.  If you think it's appropriate, there are a couple other fixes like
this,
> in a couple different files:
> 
>   git grep 'auto[^*&]*=$'

Noted. Expect a cl later today with this fix.

https://codereview.chromium.org/1665263003/diff/280001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1665263003/diff/280001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:753: // TODO(jpp): explain fake def.
On 2016/02/10 06:36:00, stichnot wrote:
> yes, I'm curious...
> 
> edit: I see, reading ahead in materializeGotAddr().
> 
> So if I understand correctly, you're using a InstFakeDef in the same way
> InstX86GetIP is used on the x86 side, with the advantage that you don't have
to
> implement all the instruction boilerplate?  Cool, maybe the x86 side should be
> simplified the same way.

And I missed this comment. :-/

I wanted to leave a comment explaining why I needed two fake-defs here. Done.

https://codereview.chromium.org/1665263003/diff/280001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:760: const IceString CRName = CR->getName();
On 2016/02/10 06:36:00, stichnot wrote:
> maybe const IceString &CRName

In general I don't like saving a return value by reference. Most of the times,
methods should not return this classes by ref (e.g., when it returns a temporary
string), so adding the & here only adds a level of indirection that you did not
need to pay.

that being said, getName() does return a ref, so the & is better. done.

https://codereview.chromium.org/1665263003/diff/280001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:5662: llvm::isa<ConstantFloat>(Opnd) ||
llvm::isa<ConstantDouble>(Opnd);
On 2016/02/10 06:36:00, stichnot wrote:
> Can/should this be more precise?  I thought that +0.0 for float and/or double
> was synthesized using xor.

You spotted my laziness. Done.

[Jim Stichnoth, 2016-02-10 17:00:21]

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

https://codereview.chromium.org/1665263003/diff/280001/pydir/build-runtime.py...
pydir/build-runtime.py:26: '-arm-enable-dwarf-eh=1',
On 2016/02/10 15:41:13, John wrote:
> On 2016/02/10 06:35:59, stichnot wrote:
> > Just checking - is this flag also happening in the PNaCl target library
build
> > scripts?
> 
> Are you asking if this is park of pnacl-translate.py? I just verified -- it
is.
> :)

I actually meant toolchain_build/pnacl_targetlibs.py (I assume that's also true,
but I could be wrong).

https://codereview.chromium.org/1665263003/diff/280001/runtime/szrt_asm_arm32.s
File runtime/szrt_asm_arm32.s (right):

https://codereview.chromium.org/1665263003/diff/280001/runtime/szrt_asm_arm32...
runtime/szrt_asm_arm32.s:19: b	__aeabi_read_tp
On 2016/02/10 15:41:13, John wrote:
> On 2016/02/10 06:35:59, stichnot wrote:
> > Optional: This tailcall is simple and all, but I wonder if there is some asm
> > magic that could tell the linker in this file just to substitute the value
of
> > __aeabi_read_tp for __nacl_read_tp .
> 
> Not as far as I could find. I wonder, should the TargetLowering generate a
call
> to __aeabi_read_tp directly?

Yeah, I think we could directly call __aeabi_read_tp.

If you do, please document it in runtime/szrt.c .

https://codereview.chromium.org/1665263003/diff/300001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1665263003/diff/300001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:5696: if (F64 != nullptr) {
This doesn't look right.  If F64!=nullptr, it should have already returned
above.

[John, 2016-02-10 23:38:21]

https://codereview.chromium.org/1665263003/diff/280001/runtime/szrt_asm_arm32.s
File runtime/szrt_asm_arm32.s (right):

https://codereview.chromium.org/1665263003/diff/280001/runtime/szrt_asm_arm32...
runtime/szrt_asm_arm32.s:19: b	__aeabi_read_tp
On 2016/02/10 17:00:21, stichnot wrote:
> On 2016/02/10 15:41:13, John wrote:
> > On 2016/02/10 06:35:59, stichnot wrote:
> > > Optional: This tailcall is simple and all, but I wonder if there is some
asm
> > > magic that could tell the linker in this file just to substitute the value
> of
> > > __aeabi_read_tp for __nacl_read_tp .
> > 
> > Not as far as I could find. I wonder, should the TargetLowering generate a
> call
> > to __aeabi_read_tp directly?
> 
> Yeah, I think we could directly call __aeabi_read_tp.
> 
> If you do, please document it in runtime/szrt.c .

Done.

https://codereview.chromium.org/1665263003/diff/300001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1665263003/diff/300001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:5696: if (F64 != nullptr) {
On 2016/02/10 17:00:21, stichnot wrote:
> This doesn't look right.  If F64!=nullptr, it should have already returned
> above.

Done.

[John, 2016-02-10 23:57:20]

Description was changed from

==========
Subzero. ARM32. Nonsfi.

Adds nonsfi support to the ARM32 backend.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4076
==========

to

==========
Subzero. ARM32. Nonsfi.

Adds nonsfi support to the ARM32 backend.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4076
R=stichnot@chromium.org

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

[John, 2016-02-10 23:57:21]

Committed patchset #17 (id:320001) manually as
dc61925cf9de3cba7f310c9e8855c56b8e5b3edc (presubmit successful).