Add trap (an UDF instruction) to the ARM integrated Assembler.

Add trap (an UDF instruction) to the ARM integrated Assembler.

Uses the same UDF instruction as used for function alignment.

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

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

[Karl, 2015-12-11 22:13:08]

Description was changed from

==========
Add trap (an UDF instruction) to the ARM integrated Assembler.

Uses the same UDF instruction as used for function alignment.

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

to

==========
Add trap (an UDF instruction) to the ARM integrated Assembler.

Uses the same UDF instruction as used for function alignment.

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

[Karl, 2015-12-11 22:13:08]

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

[Jim Stichnoth, 2015-12-11 23:31:08]

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

https://codereview.chromium.org/1519873003/diff/1/src/IceAssemblerARM32.cpp#n...
src/IceAssemblerARM32.cpp:1721: const uint8_t AssemblerARM32::TrapInst[4] =
{0xE7, 0xFE, 0xDE, 0xF0};
I think you can omit the "4".

https://codereview.chromium.org/1519873003/diff/1/src/IceAssemblerARM32.cpp#n...
src/IceAssemblerARM32.cpp:1725: for (size_t i = 1, len =
llvm::array_lengthof(TrapInst); i <= len; ++i)
Hmm, I would use a "more familiar" form of the loop:

  for (i=0; i<len; ++i) { TrapInst[len-i-1] }

  for (i=len; i>0; --i) { TrapInst[i-1] }

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

https://codereview.chromium.org/1519873003/diff/1/src/IceAssemblerARM32.h#new...
src/IceAssemblerARM32.h:130: return llvm::ArrayRef<uint8_t>(TrapInst, 4);
Can you move the implementation into the .cpp file so that the magic constant 4
can go away?

https://codereview.chromium.org/1519873003/diff/1/src/IceAssemblerARM32.h#new...
src/IceAssemblerARM32.h:313: // Trap instruction (defined in textual byte
order).
"textual"?  Maybe "little-endian" instead?

[Karl, 2015-12-17 16:23:09]

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

https://codereview.chromium.org/1519873003/diff/1/src/IceAssemblerARM32.cpp#n...
src/IceAssemblerARM32.cpp:1721: const uint8_t AssemblerARM32::TrapInst[4] =
{0xE7, 0xFE, 0xDE, 0xF0};
On 2015/12/11 23:31:08, stichnot wrote:
> I think you can omit the "4".

Done.

https://codereview.chromium.org/1519873003/diff/1/src/IceAssemblerARM32.cpp#n...
src/IceAssemblerARM32.cpp:1725: for (size_t i = 1, len =
llvm::array_lengthof(TrapInst); i <= len; ++i)
On 2015/12/11 23:31:08, stichnot wrote:
> Hmm, I would use a "more familiar" form of the loop:
> 
>   for (i=0; i<len; ++i) { TrapInst[len-i-1] }
> 
>   for (i=len; i>0; --i) { TrapInst[i-1] }

Done.

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

https://codereview.chromium.org/1519873003/diff/1/src/IceAssemblerARM32.h#new...
src/IceAssemblerARM32.h:130: return llvm::ArrayRef<uint8_t>(TrapInst, 4);
On 2015/12/11 23:31:08, stichnot wrote:
> Can you move the implementation into the .cpp file so that the magic constant
4
> can go away?

Done.

https://codereview.chromium.org/1519873003/diff/1/src/IceAssemblerARM32.h#new...
src/IceAssemblerARM32.h:313: // Trap instruction (defined in textual byte
order).
On 2015/12/11 23:31:08, stichnot wrote:
> "textual"?  Maybe "little-endian" instead?

Done.

[Jim Stichnoth, 2015-12-17 16:41:00]

lgtm

https://codereview.chromium.org/1519873003/diff/20001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1519873003/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:1886: for (size_t i = llvm::array_lengthof(TrapInst);
i > 0; --i) {
Actually, one more (optional) idea that adds a lot more clarity, I think (though
at the cost of minor efficiency):

  auto Bytes = getNonExecBundlePadding();
  for (const uint8_t &Byte : reverse_range(Bytes)) {
    Buffer.emit<uint8_t>(Byte);
  }

To address the efficiency issue, here's another version that could probably use
a bit more cleanup:

namespace {
const uint8_t TrapBytesRaw[] = {0xE7, 0xFE, 0xDE, 0xF0};
const auto TrapBytes =
  llvm::ArrayRef<uint8_t>(TrapBytesRaw, llvm::array_lengthof(TrapBytesRaw));
} // end of anonymous namespace

llvm::ArrayRef<uint8_t> AssemblerARM32::getNonExecBundlePadding() const {
  return TrapBytes;
}

void AssemblerARM32::trap() {
  AssemblerBuffer::EnsureCapacity ensured(&Buffer);
  for (const uint8_t &Byte : reverse_range(TrapBytes)) {
    Buffer.emit<uint8_t>(Byte);
  }
}

[Karl, 2015-12-17 16:55:13]

Description was changed from

==========
Add trap (an UDF instruction) to the ARM integrated Assembler.

Uses the same UDF instruction as used for function alignment.

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

to

==========
Add trap (an UDF instruction) to the ARM integrated Assembler.

Uses the same UDF instruction as used for function alignment.

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

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

[Karl, 2015-12-17 16:55:14]

Committed patchset #3 (id:40001) manually as
7de2435074e6f1c29089034e0ae537a9b1f01897 (presubmit successful).

[Karl, 2015-12-17 16:55:28]

https://codereview.chromium.org/1519873003/diff/20001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1519873003/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:1886: for (size_t i = llvm::array_lengthof(TrapInst);
i > 0; --i) {
On 2015/12/17 16:41:00, stichnot wrote:
> Actually, one more (optional) idea that adds a lot more clarity, I think
(though
> at the cost of minor efficiency):
> 
>   auto Bytes = getNonExecBundlePadding();
>   for (const uint8_t &Byte : reverse_range(Bytes)) {
>     Buffer.emit<uint8_t>(Byte);
>   }
> 
> To address the efficiency issue, here's another version that could probably
use
> a bit more cleanup:
> 
> namespace {
> const uint8_t TrapBytesRaw[] = {0xE7, 0xFE, 0xDE, 0xF0};
> const auto TrapBytes =
>   llvm::ArrayRef<uint8_t>(TrapBytesRaw, llvm::array_lengthof(TrapBytesRaw));
> } // end of anonymous namespace
> 
> llvm::ArrayRef<uint8_t> AssemblerARM32::getNonExecBundlePadding() const {
>   return TrapBytes;
> }
> 
> void AssemblerARM32::trap() {
>   AssemblerBuffer::EnsureCapacity ensured(&Buffer);
>   for (const uint8_t &Byte : reverse_range(TrapBytes)) {
>     Buffer.emit<uint8_t>(Byte);
>   }
> }

Done.