Fix problems with sandboxing and the ARM integrated assembler.

Fix problems with sandboxing and the ARM integrated assembler.

Fixes (at least) the obvious problems with sandboxing and the
integrated assembler. This includes:

Added assembly instruction Nop.

Fixed implementation of padWithNop.

Fixed linking to local label to only fire when persistent (i.e.  last
pass of assembly generation).

Removed restriction on single register push/pop, since the ARM
integrated assembler converts it to a corresopnding str/ldr.

Fixed OperandARM32FlexImm to use smallest rotation value, so that
external assemblers and the ARM integrated assembler will agree on
encoding.

Fixed ARM sandboxing requires test in sandboxing.ll

BUG=None
R=stichnot@chromium.org

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

[Karl, 2015-12-08 18:57:23]

Description was changed from

==========
Fix problems with sandboxing and the ARM integrated assembler.

Fixes (at least) the obvious problems with sandboxing and the
integrated assembler. This includes:

Added assembly instruction Nop.

Fixed implementation of padWithNop.

Fixed linking to local label to only fire when persistent (i.e.  last
pass of assembly generation).

Removed restriction on single register push/pop, since the ARM
integrated assembler converts it to a corresopnding str/ldr.

Fixed OperandARM32FlexImm to use smallest rotation value, so that
external assemblers and the ARM integrated assembler will agree on
encoding.

Fixed ARM sandboxing requires test in sandboxing.ll

BUG=None
==========

to

==========
Fix problems with sandboxing and the ARM integrated assembler.

Fixes (at least) the obvious problems with sandboxing and the
integrated assembler. This includes:

Added assembly instruction Nop.

Fixed implementation of padWithNop.

Fixed linking to local label to only fire when persistent (i.e.  last
pass of assembly generation).

Removed restriction on single register push/pop, since the ARM
integrated assembler converts it to a corresopnding str/ldr.

Fixed OperandARM32FlexImm to use smallest rotation value, so that
external assemblers and the ARM integrated assembler will agree on
encoding.

Fixed ARM sandboxing requires test in sandboxing.ll

BUG=None
==========

[Karl, 2015-12-08 18:57:23]

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

[John, 2015-12-08 19:45:04]

https://codereview.chromium.org/1511653002/diff/1/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1511653002/diff/1/src/IceAssemblerARM32.cpp#n...
src/IceAssemblerARM32.cpp:1386: return emitMemOp(Cond, IsLoad, IsByte, Rt,
OpAddress, TInfo, StrName);
Optional: I know the language allows to return a void expression expression, but
would you mind doing:

  emitMemOp(...);
  return;

instead?

https://codereview.chromium.org/1511653002/diff/1/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1511653002/diff/1/src/IceInstARM32.cpp#newcod...
src/IceInstARM32.cpp:1767: // The assembler wants the smallest rotation. Rotate
if needed. Note: Imm is
Why is this needed?

[Jim Stichnoth, 2015-12-08 19:54:59]

https://codereview.chromium.org/1511653002/diff/1/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1511653002/diff/1/src/IceAssemblerARM32.cpp#n...
src/IceAssemblerARM32.cpp:466: "Padding not mulitple of instruction size");
multiple

https://codereview.chromium.org/1511653002/diff/1/src/IceAssemblerARM32.cpp#n...
src/IceAssemblerARM32.cpp:1386: return emitMemOp(Cond, IsLoad, IsByte, Rt,
OpAddress, TInfo, StrName);
On 2015/12/08 19:45:04, John wrote:
> Optional: I know the language allows to return a void expression expression,
but
> would you mind doing:
> 
>   emitMemOp(...);
>   return;
> 
> instead?

This pattern of returning a void expression was suggested (by me) and first
added in:

https://chromiumcodereview.appspot.com/1418523002/#msg4

After reacquainting myself with the issue, I agree that this pattern should
probably only be used with setNeedsTextFixup(), and would ultimately disappear
when full iasm support is there.

https://codereview.chromium.org/1511653002/diff/1/src/IceAssemblerARM32.h
File src/IceAssemblerARM32.h (right):

https://codereview.chromium.org/1511653002/diff/1/src/IceAssemblerARM32.h#new...
src/IceAssemblerARM32.h:239: void nop(const CondARM32::Cond = CondARM32::AL);
Does nop *really* need to be predicated?

https://codereview.chromium.org/1511653002/diff/1/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1511653002/diff/1/src/IceInstARM32.cpp#newcod...
src/IceInstARM32.cpp:1768: // an 8-bit value.
Maybe validate Imm?  e.g.
  assert(Utils::IsUint(8, Imm));

[Karl, 2015-12-08 19:58:35]

https://codereview.chromium.org/1511653002/diff/1/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1511653002/diff/1/src/IceAssemblerARM32.cpp#n...
src/IceAssemblerARM32.cpp:1386: return emitMemOp(Cond, IsLoad, IsByte, Rt,
OpAddress, TInfo, StrName);
On 2015/12/08 19:45:04, John wrote:
> Optional: I know the language allows to return a void expression expression,
but
> would you mind doing:
> 
>   emitMemOp(...);
>   return;
> 
> instead?

I mainly started doing this because it was much easier to write:

  if (...)
    return setNeedsTextFixup();

rather than:

  if (...) {
    setNeedsTextFixup();
    return;
  }

To be consistent, I started doing it whenever the last element was a call. I can
change it none the less.

https://codereview.chromium.org/1511653002/diff/1/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1511653002/diff/1/src/IceInstARM32.cpp#newcod...
src/IceInstARM32.cpp:1767: // The assembler wants the smallest rotation. Rotate
if needed. Note: Imm is
On 2015/12/08 19:45:04, John wrote:
> Why is this needed?

If I don't do this, then values like rotate=2, Imm8=0xc will compile differently
by the two assemblers. I didn't discover this issue until I started compiling
with sandboxed examples (which appear to use the pair of constants quite often).

If the outputs are different, it is hard to compare an external assembler (when
using -filetype=asm) with the integrated assembler.

Hence, I added this to match what ARM states should be used if multiple
encodings are defined. I could have alternatively fixed all computations to not
do this, but it seemed harder.

[Karl, 2015-12-08 20:49:36]

I also updated DART source to show that we have implemented nop() in the ARM
integrated assembler.

https://codereview.chromium.org/1511653002/diff/1/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1511653002/diff/1/src/IceAssemblerARM32.cpp#n...
src/IceAssemblerARM32.cpp:466: "Padding not mulitple of instruction size");
On 2015/12/08 19:54:58, stichnot wrote:
> multiple

Done.

https://codereview.chromium.org/1511653002/diff/1/src/IceAssemblerARM32.cpp#n...
src/IceAssemblerARM32.cpp:1386: return emitMemOp(Cond, IsLoad, IsByte, Rt,
OpAddress, TInfo, StrName);
On 2015/12/08 19:58:35, Karl wrote:
> On 2015/12/08 19:45:04, John wrote:
> > Optional: I know the language allows to return a void expression expression,
> but
> > would you mind doing:
> > 
> >   emitMemOp(...);
> >   return;
> > 
> > instead?
> 
> I mainly started doing this because it was much easier to write:
> 
>   if (...)
>     return setNeedsTextFixup();
> 
> rather than:
> 
>   if (...) {
>     setNeedsTextFixup();
>     return;
>   }
> 
> To be consistent, I started doing it whenever the last element was a call. I
can
> change it none the less.

Removing returns of this form.

https://codereview.chromium.org/1511653002/diff/1/src/IceAssemblerARM32.h
File src/IceAssemblerARM32.h (right):

https://codereview.chromium.org/1511653002/diff/1/src/IceAssemblerARM32.h#new...
src/IceAssemblerARM32.h:239: void nop(const CondARM32::Cond = CondARM32::AL);
On 2015/12/08 19:54:58, stichnot wrote:
> Does nop *really* need to be predicated?

Was following DART code. However, I agree that I can't think of any reason.
Removing until need is found.

https://codereview.chromium.org/1511653002/diff/1/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1511653002/diff/1/src/IceInstARM32.cpp#newcod...
src/IceInstARM32.cpp:1768: // an 8-bit value.
On 2015/12/08 19:54:58, stichnot wrote:
> Maybe validate Imm?  e.g.
>   assert(Utils::IsUint(8, Imm));

Added assert.

[Jim Stichnoth, 2015-12-08 20:52:28]

https://codereview.chromium.org/1511653002/diff/1/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1511653002/diff/1/src/IceAssemblerARM32.cpp#n...
src/IceAssemblerARM32.cpp:662: if (!needsTextFixup() && !getPreliminary())
This is clearly a problem that affects all assemblers, and so it would be great
to factor it up to the base Assembler class.

Can you try modifying Label::linkTo() to also take a const Assembler * argument,
and then do an early return if Assembler::getPreliminary() is true?  Then the
getPreliminary() checks are unneeded for the target-specific assemblers.

The same change should be applied to the x86 nearLinkTo() method.

I think this is simple enough that it could be part of this CL, or it could be a
separate CL if you like.

[Karl, 2015-12-08 21:38:11]

https://codereview.chromium.org/1511653002/diff/1/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1511653002/diff/1/src/IceAssemblerARM32.cpp#n...
src/IceAssemblerARM32.cpp:662: if (!needsTextFixup() && !getPreliminary())
On 2015/12/08 20:52:27, stichnot wrote:
> This is clearly a problem that affects all assemblers, and so it would be
great
> to factor it up to the base Assembler class.
> 
> Can you try modifying Label::linkTo() to also take a const Assembler *
argument,
> and then do an early return if Assembler::getPreliminary() is true?  Then the
> getPreliminary() checks are unneeded for the target-specific assemblers.
> 
> The same change should be applied to the x86 nearLinkTo() method.
> 
> I think this is simple enough that it could be part of this CL, or it could be
a
> separate CL if you like.

Moved check inside linkTo() and nearLinkTo(). Code now reads like there is a
single pass, even if there isn't.

[Jim Stichnoth, 2015-12-08 22:20:00]

https://codereview.chromium.org/1511653002/diff/40001/src/IceAssembler.h
File src/IceAssembler.h (right):

https://codereview.chromium.org/1511653002/diff/40001/src/IceAssembler.h#newc...
src/IceAssembler.h:87: inline void linkTo(const Assembler &Asm, intptr_t
position);
Does the "inline" really help here?

Also, we may be a bit inconsistent about whether to use Assembler & versus
Assembler *.  This version is probably reasonable, but just wondering if you
considered it.

https://codereview.chromium.org/1511653002/diff/40001/src/IceAssembler.h#newc...
src/IceAssembler.h:355: void Label::linkTo(const Assembler &Asm, intptr_t
position) {
I think this should be moved into the .cpp file.

Also, "position" should be capitalized (and probably also renamed to avoid
shadowing).

https://codereview.chromium.org/1511653002/diff/40001/src/IceAssembler.h#newc...
src/IceAssembler.h:356: if (Asm.getPreliminary() || Asm.needsTextFixup())
I think it's important to leave some documentation here.  Suggestion:

// We must not set the link until the Position is absolutely known.  This means
not during the preliminary (sandboxing) pass, and not when the instruction needs
a text fixup (hybrid iasm mode).

https://codereview.chromium.org/1511653002/diff/40001/src/IceAssemblerX86Base...
File src/IceAssemblerX86BaseImpl.h (right):

https://codereview.chromium.org/1511653002/diff/40001/src/IceAssemblerX86Base...
src/IceAssemblerX86BaseImpl.h:3411: void
AssemblerX86Base<Machine>::emitNearLabelLink(Label *label) {
Change "label" to "Label", and "position" to "Position", as above.

[Karl, 2015-12-08 22:39:32]

https://codereview.chromium.org/1511653002/diff/40001/src/IceAssembler.h
File src/IceAssembler.h (right):

https://codereview.chromium.org/1511653002/diff/40001/src/IceAssembler.h#newc...
src/IceAssembler.h:87: inline void linkTo(const Assembler &Asm, intptr_t
position);
On 2015/12/08 22:20:00, stichnot wrote:
> Does the "inline" really help here?
> 
> Also, we may be a bit inconsistent about whether to use Assembler & versus
> Assembler *.  This version is probably reasonable, but just wondering if you
> considered it.

The "inline" is needed if we don't want the function to be compiled into every
source, but maintain the property that it is still inlined. However, since you
suggest moving the implementation to the cpp file, this is mute. Removing the
inline.

As far as why I used "const Assembler &Asm" rather than "const Assembler *Asm",
there are two (correlated) reasons:

1) The const has different meanings. The first is that the Assembler instance is
const. The second is that the pointer is const (not the instance).

2) The compiler insists on a cast on derived classes for the second form, but
not the first.

Hence, I used "const Assembler &Asm".

https://codereview.chromium.org/1511653002/diff/40001/src/IceAssembler.h#newc...
src/IceAssembler.h:355: void Label::linkTo(const Assembler &Asm, intptr_t
position) {
On 2015/12/08 22:20:00, stichnot wrote:
> I think this should be moved into the .cpp file.
> 
> Also, "position" should be capitalized (and probably also renamed to avoid
> shadowing).

Done.

https://codereview.chromium.org/1511653002/diff/40001/src/IceAssembler.h#newc...
src/IceAssembler.h:356: if (Asm.getPreliminary() || Asm.needsTextFixup())
On 2015/12/08 22:20:00, stichnot wrote:
> I think it's important to leave some documentation here.  Suggestion:
> 
> // We must not set the link until the Position is absolutely known.  This
means
> not during the preliminary (sandboxing) pass, and not when the instruction
needs
> a text fixup (hybrid iasm mode).

Done.

https://codereview.chromium.org/1511653002/diff/40001/src/IceAssemblerX86Base...
File src/IceAssemblerX86BaseImpl.h (right):

https://codereview.chromium.org/1511653002/diff/40001/src/IceAssemblerX86Base...
src/IceAssemblerX86BaseImpl.h:3411: void
AssemblerX86Base<Machine>::emitNearLabelLink(Label *label) {
On 2015/12/08 22:20:00, stichnot wrote:
> Change "label" to "Label", and "position" to "Position", as above.

Done.

[Jim Stichnoth, 2015-12-08 23:06:44]

lgtm

https://codereview.chromium.org/1511653002/diff/40001/src/IceAssembler.h
File src/IceAssembler.h (right):

https://codereview.chromium.org/1511653002/diff/40001/src/IceAssembler.h#newc...
src/IceAssembler.h:87: inline void linkTo(const Assembler &Asm, intptr_t
position);
On 2015/12/08 22:39:32, Karl wrote:
> As far as why I used "const Assembler &Asm" rather than "const Assembler
*Asm",
> there are two (correlated) reasons:
> 
> 1) The const has different meanings. The first is that the Assembler instance
is
> const. The second is that the pointer is const (not the instance).

No - "const Assembler *Asm" means that the pointed-to object is constant - "Asm
= Asm2;" is fine but "Asm->some_field++;" is not.  What you are describing is
"Assembler *const Asm".

> 
> 2) The compiler insists on a cast on derived classes for the second form, but
> not the first.

Not sure I understand this.  Upcasts shouldn't require an explicit case.  For
example,
  Variable *Var = ...;
  Operand *Opnd = Var;
is fine, and similarly a Variable* can be passed as an Operand* parameter.

> 
> Hence, I used "const Assembler &Asm".

No big deal, because I think a stylistic argument could be made for your choice.

https://codereview.chromium.org/1511653002/diff/60001/src/IceAssembler.cpp
File src/IceAssembler.cpp (right):

https://codereview.chromium.org/1511653002/diff/60001/src/IceAssembler.cpp#ne...
src/IceAssembler.cpp:41: // needs a text fixup (hygrid iasm mode).
hybrid

[Karl, 2015-12-08 23:37:02]

Description was changed from

==========
Fix problems with sandboxing and the ARM integrated assembler.

Fixes (at least) the obvious problems with sandboxing and the
integrated assembler. This includes:

Added assembly instruction Nop.

Fixed implementation of padWithNop.

Fixed linking to local label to only fire when persistent (i.e.  last
pass of assembly generation).

Removed restriction on single register push/pop, since the ARM
integrated assembler converts it to a corresopnding str/ldr.

Fixed OperandARM32FlexImm to use smallest rotation value, so that
external assemblers and the ARM integrated assembler will agree on
encoding.

Fixed ARM sandboxing requires test in sandboxing.ll

BUG=None
==========

to

==========
Fix problems with sandboxing and the ARM integrated assembler.

Fixes (at least) the obvious problems with sandboxing and the
integrated assembler. This includes:

Added assembly instruction Nop.

Fixed implementation of padWithNop.

Fixed linking to local label to only fire when persistent (i.e.  last
pass of assembly generation).

Removed restriction on single register push/pop, since the ARM
integrated assembler converts it to a corresopnding str/ldr.

Fixed OperandARM32FlexImm to use smallest rotation value, so that
external assemblers and the ARM integrated assembler will agree on
encoding.

Fixed ARM sandboxing requires test in sandboxing.ll

BUG=None
R=stichnot@chromium.org

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

[Karl, 2015-12-08 23:37:04]

Committed patchset #5 (id:80001) manually as
67574d83d05f5bde748843a73adcab6247736601 (presubmit successful).