Handle ARM "ret void" and function alignment with proper padding.

Handle ARM "ret void" and function alignment with proper padding.

Modify run-pnacl-sz to pass in the correct assembler/disasembler flags
for ARM when not using the integrated assembler.

Model the "ret" pseudo instruction (special form of
"bx" inst). Separate from "bx" to allow epilogue
insertion to find the terminator.

Add a flag "--skip-unimplemented" to skip through all of the
"Not yet implemented" assertions, and use that in the test.

Set up a stack trace printer when ALLOW_DUMP so that the
UnimplementedError prints out some useful information of
*which* case is unimplemented.

Change the .type ...,@function from @function to %function.
ARM assembler seems to only like %function because
"@" is a comment character.

BUG= https://code.google.com/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=b2d5084c573ef5de15eb0e87bdde5dfba59e524a

[jvoung (off chromium), 2015-05-08 21:23:23]

jvoung@chromium.org changed reviewers:
+ jfb@chromium.org, kschimpf@google.com, stichnot@chromium.org

[jvoung (off chromium), 2015-05-08 21:31:25]

Decided to send out the simpler "ret void" first instead of stuff for add (which
would need lowerArguments, ldr, str, mov, etc).

[Jim Stichnoth, 2015-05-11 20:12:32]

https://codereview.chromium.org/1136793002/diff/40001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/1136793002/diff/40001/src/IceCfg.cpp#newcode467
src/IceCfg.cpp:467: Str << "\t.type\t" << MangledName << ",%function\n";
% instead of @ is also OK for x86-32?

https://codereview.chromium.org/1136793002/diff/40001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1136793002/diff/40001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:22: #include "IceOperand.h"
sort includes

https://codereview.chromium.org/1136793002/diff/40001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:66: assert(LR->hasReg() && LR->getRegNum() ==
RegARM32::Reg_lr);
Break into 2 asserts?  makes it easier to debug an assertion failure

https://codereview.chromium.org/1136793002/diff/40001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:68: Str << "\tbx\t";
Just to share an idle thought -- I was thinking of changing lines like this to
something like:
  Str << TABSTR << "bx" << TABSTR;
which would make it easier to "git grep -w" on these strings.

https://codereview.chromium.org/1136793002/diff/40001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1136793002/diff/40001/src/IceInstARM32.h#newc...
src/IceInstARM32.h:81: // Ret psuedo-instruction.  This is actually a "bx"
instruction with
pseudo

https://codereview.chromium.org/1136793002/diff/40001/src/IceInstARM32.h#newc...
src/IceInstARM32.h:82: // an "lr" register operand, but epilogue lowering will
search for a Ret
I'm not ecstatic about how the Ret marker stuff is done for x86-32, so if you
have ideas on a better cleaner approach, that would be great...

https://codereview.chromium.org/1136793002/diff/40001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1136793002/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:250: if (RegNum == RegARM32::Reg_sp) {
Maybe this instead?
  if (RegNum == RegARM32::Reg_sp || RegNum == RegARM32::Reg_lr) {

https://codereview.chromium.org/1136793002/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:276: if (Offset <= -(1 <<
InstARM32::kOffset12Bits) ||
Should this be "Offset < -(1 << InstARM32::kOffset12Bits"?

It seems like these comparisons should be either < / >=  or  <= / >  .

https://codereview.chromium.org/1136793002/diff/40001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/function_aligned.ll (right):

https://codereview.chromium.org/1136793002/diff/40001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/function_aligned.ll:23: ; ARM32-NEXT: 8: e7fedef0 udf
; ARM32-NEXT: c: e7fedef0 udf

?

[jvoung (off chromium), 2015-05-11 22:11:51]

https://codereview.chromium.org/1136793002/diff/40001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/1136793002/diff/40001/src/IceCfg.cpp#newcode467
src/IceCfg.cpp:467: Str << "\t.type\t" << MangledName << ",%function\n";
On 2015/05/11 20:12:31, stichnot wrote:
> % instead of @ is also OK for x86-32?

I think so.

Apparently there are 5 ways to state the type, because each target has different
comment characters:
https://sourceware.org/binutils/docs/as/Type.html#Type

It looks like "@" is a possible comment character for ARM. Couldn't find the
definitive source for that, but:

lib/Target/ARM/MCTargetDesc/ARMMCAsmInfo.cpp:  CommentString = "@";

http://stackoverflow.com/questions/15663280/gnu-assembler-comments/17168600#1...

Since this isn't "#" this should work for X86 too. Looks like Mips uses "#"
also.

https://codereview.chromium.org/1136793002/diff/40001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1136793002/diff/40001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:22: #include "IceOperand.h"
On 2015/05/11 20:12:31, stichnot wrote:
> sort includes

Done.

https://codereview.chromium.org/1136793002/diff/40001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:66: assert(LR->hasReg() && LR->getRegNum() ==
RegARM32::Reg_lr);
On 2015/05/11 20:12:31, stichnot wrote:
> Break into 2 asserts?  makes it easier to debug an assertion failure

Done.

https://codereview.chromium.org/1136793002/diff/40001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:68: Str << "\tbx\t";
On 2015/05/11 20:12:31, stichnot wrote:
> Just to share an idle thought -- I was thinking of changing lines like this to
> something like:
>   Str << TABSTR << "bx" << TABSTR;
> which would make it easier to "git grep -w" on these strings.

Hmm yeah that sounds good.

https://codereview.chromium.org/1136793002/diff/40001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1136793002/diff/40001/src/IceInstARM32.h#newc...
src/IceInstARM32.h:81: // Ret psuedo-instruction.  This is actually a "bx"
instruction with
On 2015/05/11 20:12:31, stichnot wrote:
> pseudo

Done.

https://codereview.chromium.org/1136793002/diff/40001/src/IceInstARM32.h#newc...
src/IceInstARM32.h:82: // an "lr" register operand, but epilogue lowering will
search for a Ret
On 2015/05/11 20:12:31, stichnot wrote:
> I'm not ecstatic about how the Ret marker stuff is done for x86-32, so if you
> have ideas on a better cleaner approach, that would be great...

I guess LLVM would have a flag on the instruction to indicate if it "isReturn"
which the epilogue insertion would search for:

  // Add epilogue to restore the callee-save registers in each exiting block
  for (MachineFunction::iterator I = Fn.begin(), E = Fn.end(); I != E; ++I) {
    // If last instruction is a return instruction, add an epilogue
    if (!I->empty() && I->back().isReturn())
      TFI.emitEpilogue(Fn, *I);
  }

There is the "back()" and there is getLastNonDebugInstr().

On the other hand, for LLVM, has a NACL_RET pseudo which has the isReturn flag,
so the sandboxing sequence is also not expanded until pretty late (if that is a
concern).

https://codereview.chromium.org/1136793002/diff/40001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1136793002/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:250: if (RegNum == RegARM32::Reg_sp) {
On 2015/05/11 20:12:32, stichnot wrote:
> Maybe this instead?
>   if (RegNum == RegARM32::Reg_sp || RegNum == RegARM32::Reg_lr) {

Done.

https://codereview.chromium.org/1136793002/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:276: if (Offset <= -(1 <<
InstARM32::kOffset12Bits) ||
On 2015/05/11 20:12:31, stichnot wrote:
> Should this be "Offset < -(1 << InstARM32::kOffset12Bits"?
> 
> It seems like these comparisons should be either < / >=  or  <= / >  .

Hmm, apparently ARM is sign-magnitude instead of two's complement for these
offsets.

ARM, word or byte [a]	-4095 to 4095

http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0489c/CIHGJHED...

I'll refactor this to be like the Dart check:
https://github.com/dart-lang/bleeding_edge/blob/master/dart/runtime/vm/assemb...

This might not be the right place to check for it, since we will need to handle
the offsets that don't fit, but plopping here for now.

https://codereview.chromium.org/1136793002/diff/40001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/function_aligned.ll (right):

https://codereview.chromium.org/1136793002/diff/40001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/function_aligned.ll:23: ; ARM32-NEXT: 8: e7fedef0 udf
On 2015/05/11 20:12:32, stichnot wrote:
> ; ARM32-NEXT: c: e7fedef0 udf
> 
> ?

Done.

[Jim Stichnoth, 2015-05-12 00:01:45]

lgtm

https://codereview.chromium.org/1136793002/diff/40001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1136793002/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:276: if (Offset <= -(1 <<
InstARM32::kOffset12Bits) ||
On 2015/05/11 22:11:51, jvoung wrote:
> On 2015/05/11 20:12:31, stichnot wrote:
> > Should this be "Offset < -(1 << InstARM32::kOffset12Bits"?
> > 
> > It seems like these comparisons should be either < / >=  or  <= / >  .
> 
> Hmm, apparently ARM is sign-magnitude instead of two's complement for these
> offsets.
> 
> ARM, word or byte [a]	-4095 to 4095
> 
>
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0489c/CIHGJHED...
> 
> I'll refactor this to be like the Dart check:
>
https://github.com/dart-lang/bleeding_edge/blob/master/dart/runtime/vm/assemb...
> 
> This might not be the right place to check for it, since we will need to
handle
> the offsets that don't fit, but plopping here for now.

Thanks.  I would also suggest a comment in canHoldOffset() about sign-magnitude
rather than two's complement, for the uninitiated (like me).

[jvoung (off chromium), 2015-05-12 16:38:09]

Thanks!

https://codereview.chromium.org/1136793002/diff/40001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1136793002/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:276: if (Offset <= -(1 <<
InstARM32::kOffset12Bits) ||
On 2015/05/12 00:01:45, stichnot wrote:
> On 2015/05/11 22:11:51, jvoung wrote:
> > On 2015/05/11 20:12:31, stichnot wrote:
> > > Should this be "Offset < -(1 << InstARM32::kOffset12Bits"?
> > > 
> > > It seems like these comparisons should be either < / >=  or  <= / >  .
> > 
> > Hmm, apparently ARM is sign-magnitude instead of two's complement for these
> > offsets.
> > 
> > ARM, word or byte [a]	-4095 to 4095
> > 
> >
>
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0489c/CIHGJHED...
> > 
> > I'll refactor this to be like the Dart check:
> >
>
https://github.com/dart-lang/bleeding_edge/blob/master/dart/runtime/vm/assemb...
> > 
> > This might not be the right place to check for it, since we will need to
> handle
> > the offsets that don't fit, but plopping here for now.
> 
> Thanks.  I would also suggest a comment in canHoldOffset() about
sign-magnitude
> rather than two's complement, for the uninitiated (like me).

Sounds good -- added a comment about sign-magnitude in the .cpp and some
comments about parameters in the .h.

[jvoung (off chromium), 2015-05-12 16:53:56]

Committed patchset #9 (id:150001) manually as
b2d5084c573ef5de15eb0e87bdde5dfba59e524a (tree was closed).