Expose LLC's -force-align-stack flag to pnacl-translate

Expose LLC's -force-align-stack flag to pnacl-translate

x86-32 glibc's ld.so is built to maintain 4-byte stack alignment,
instead of 16 bytes that we use everywhere else. This means the IRT
must be built to tolerate 4-byte alignment when it is called. To build
the IRT with pnacl-clang, we can use LLC's -force-align-stack flag which
causes the stack to be realigned on function entry (it has always
maintained 16 byte stack alignment calling out, but has assumed
that the stack was aligned on entry).

R=mseaborn@chromium.org
BUG= https://code.google.com/p/nativeclient/issues/detail?id=3910

Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=13672

[Derek Schuff, 2014-08-27 20:23:49]

On 2014/08/27 20:04:03, Derek Schuff wrote:

actually hold off, i'll also roll a test into this CL using FEATURE_VERSION

[Derek Schuff, 2014-08-28 00:31:36]

added tests, ptal

[Mark Seaborn, 2014-08-28 19:13:14]

LGTM

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/call_wit...
File tests/toolchain/call_with_misaligned_stack.S (right):

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/call_wit...
tests/toolchain/call_with_misaligned_stack.S:8: Call the argument with a stack
pointer 4 bytes below its value on entry.
Nit: Use the NaCl style for multiline comments, with "*" prefixing each line:
/*
 * Blah...
 * blah.
 */

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/nacl.scons
File tests/toolchain/nacl.scons (right):

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/nacl.sco...
tests/toolchain/nacl.scons:584: obj1 =
mixedlink_env.ComponentObject('call_with_misaligned_stack.S')
I suspect you don't need ComponentObject.  Can these go directly in the
ComponentProgram call?

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/nacl.sco...
tests/toolchain/nacl.scons:590: 'toolchain_tests'],
Nit: fix indentation alignment

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/nacl.sco...
tests/toolchain/nacl.scons:591: 'run_stackalign_test')
Nit: fix indentation alignment here too

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/stackali...
File tests/toolchain/stackalign_test.c (right):

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/stackali...
tests/toolchain/stackalign_test.c:2: * Copyright (c) 2011 The Native Client
Authors. All rights reserved.
Nit: 2014

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/stackali...
tests/toolchain/stackalign_test.c:10: void call_with_misaligned_stack(void
(*func)());
"()" -> "(void)"

This is supposed to be getting compiled with -Wstrict-prototypes.

Apparently -Wstrict-prototypes is broken in Clang.

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/stackali...
tests/toolchain/stackalign_test.c:15: Test that locals are properly aligned,
even if functions are called with a
Use NaCl comment style

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/stackali...
tests/toolchain/stackalign_test.c:20: void testfunc() {
"(void)"

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/stackali...
tests/toolchain/stackalign_test.c:22: /* Smart compiler will optimize away the
test (it assumes the alignment is
Nit: Use the NaCl style for multiline comments, with "/*" and "*/" on separate
lines

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/stackali...
tests/toolchain/stackalign_test.c:25: if(addrf % 8 != 0) abort();
Add space after "if", or better, use ASSERT_EQ(addrf % 8, 0) (and #include
nacl_assert.h)

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/stackali...
tests/toolchain/stackalign_test.c:28: int main() {
"(void)"

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/stackali...
tests/toolchain/stackalign_test.c:33: }
Add "return 0", otherwise the return value in undefined in C.

[Derek Schuff, 2014-08-28 22:07:25]

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/call_wit...
File tests/toolchain/call_with_misaligned_stack.S (right):

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/call_wit...
tests/toolchain/call_with_misaligned_stack.S:8: Call the argument with a stack
pointer 4 bytes below its value on entry.
On 2014/08/28 19:13:13, Mark Seaborn wrote:
> Nit: Use the NaCl style for multiline comments, with "*" prefixing each line:
> /*
>  * Blah...
>  * blah.
>  */

Done.

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/nacl.scons
File tests/toolchain/nacl.scons (right):

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/nacl.sco...
tests/toolchain/nacl.scons:584: obj1 =
mixedlink_env.ComponentObject('call_with_misaligned_stack.S')
On 2014/08/28 19:13:13, Mark Seaborn wrote:
> I suspect you don't need ComponentObject.  Can these go directly in the
> ComponentProgram call?

Done.

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/nacl.sco...
tests/toolchain/nacl.scons:590: 'toolchain_tests'],
On 2014/08/28 19:13:13, Mark Seaborn wrote:
> Nit: fix indentation alignment

Done.

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/nacl.sco...
tests/toolchain/nacl.scons:591: 'run_stackalign_test')
On 2014/08/28 19:13:13, Mark Seaborn wrote:
> Nit: fix indentation alignment here too

Done.

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/stackali...
File tests/toolchain/stackalign_test.c (right):

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/stackali...
tests/toolchain/stackalign_test.c:2: * Copyright (c) 2011 The Native Client
Authors. All rights reserved.
On 2014/08/28 19:13:14, Mark Seaborn wrote:
> Nit: 2014

Done.

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/stackali...
tests/toolchain/stackalign_test.c:10: void call_with_misaligned_stack(void
(*func)());
On 2014/08/28 19:13:13, Mark Seaborn wrote:
> "()" -> "(void)"
> 
> This is supposed to be getting compiled with -Wstrict-prototypes.
> 
> Apparently -Wstrict-prototypes is broken in Clang.

Done.

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/stackali...
tests/toolchain/stackalign_test.c:15: Test that locals are properly aligned,
even if functions are called with a
On 2014/08/28 19:13:13, Mark Seaborn wrote:
> Use NaCl comment style

Done.

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/stackali...
tests/toolchain/stackalign_test.c:20: void testfunc() {
On 2014/08/28 19:13:13, Mark Seaborn wrote:
> "(void)"

Done.

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/stackali...
tests/toolchain/stackalign_test.c:22: /* Smart compiler will optimize away the
test (it assumes the alignment is
On 2014/08/28 19:13:14, Mark Seaborn wrote:
> Nit: Use the NaCl style for multiline comments, with "/*" and "*/" on separate
> lines

Done.

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/stackali...
tests/toolchain/stackalign_test.c:25: if(addrf % 8 != 0) abort();
On 2014/08/28 19:13:13, Mark Seaborn wrote:
> Add space after "if", or better, use ASSERT_EQ(addrf % 8, 0) (and #include
> nacl_assert.h)

Done.

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/stackali...
tests/toolchain/stackalign_test.c:28: int main() {
On 2014/08/28 19:13:13, Mark Seaborn wrote:
> "(void)"

Done.

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/stackali...
tests/toolchain/stackalign_test.c:33: }
On 2014/08/28 19:13:13, Mark Seaborn wrote:
> Add "return 0", otherwise the return value in undefined in C.

Done.

[Derek Schuff, 2014-08-28 22:30:47]

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

[Nico, 2014-08-29 04:34:09]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2014-08-29 04:34:09]

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/stackali...
File tests/toolchain/stackalign_test.c (right):

https://codereview.chromium.org/508823007/diff/20001/tests/toolchain/stackali...
tests/toolchain/stackalign_test.c:10: void call_with_misaligned_stack(void
(*func)());
On 2014/08/28 19:13:13, Mark Seaborn wrote:
> "()" -> "(void)"
> 
> This is supposed to be getting compiled with -Wstrict-prototypes.
> 
> Apparently -Wstrict-prototypes is broken in Clang.

If you find warnings that don't work right in clang, please file a bug at
http://llvm.org/bugs and send me a link to it. (Or just send me an email, if you
don't want to create a llvm bugzilla account.) We might be able to fix things.

[Mark Seaborn, 2014-08-29 04:50:24]

On 28 August 2014 21:34, <thakis@chromium.org> wrote:

>
> https://codereview.chromium.org/508823007/diff/20001/
> tests/toolchain/stackalign_test.c
> File tests/toolchain/stackalign_test.c (right):
>
> https://codereview.chromium.org/508823007/diff/20001/
> tests/toolchain/stackalign_test.c#newcode10
> tests/toolchain/stackalign_test.c:10: void
> call_with_misaligned_stack(void (*func)());
> On 2014/08/28 19:13:13, Mark Seaborn wrote:
>
>> "()" -> "(void)"
>>
>
>  This is supposed to be getting compiled with -Wstrict-prototypes.
>>
>
>  Apparently -Wstrict-prototypes is broken in Clang.
>>
>
> If you find warnings that don't work right in clang, please file a bug
> at http://llvm.org/bugs and send me a link to it. (Or just send me an
> email, if you don't want to create a llvm bugzilla account.) We might be
> able to fix things.


Sure:  http://llvm.org/bugs/show_bug.cgi?id=20796

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.