439502 - Tamarin Tracing: functionReturnTypes acceptance test fails

Status: RESOLVED FIXED

(Tamarin Graveyard :: Tracing Virtual Machine, defect)

Description

[Jim Blandy :jimb]

Revision 79d0489893b0 from http://hg.mozilla.org/tamarin-tracing/ fails the acceptance test:
test/acceptance/as3/Definitions/Function/functionReturnTypes.as
Tamarin Tracing crashes with a segfault in avmplus::String::testSingleByte, as the processor attempts to use a MOVDQA instruction to save a 16-byte portion of the string into a stack slot which is not aligned on a 16-byte boundary, as that instruction requires.

The first question that arises: is this code even correct in the first place?  Can GCC ever correctly use MOVDQA to store a value in a stack slot?

Since the x86 ABI requires only a 4-byte stack alignment, technically MOVDQA is never correct unless the function takes explicit steps to align the stack slot.  (Although MOVDQU, corresponding to the _mm_loadu_si128 intrinsic, would probably work fine.)

However, starting in late 2006, GCC generates code to always preserve a 16-byte alignment:
http://gcc.gnu.org/ml/gcc-patches/2006-09/msg00298.html
And, for some reason I haven't been able to discern, a GCC-generated 'main' function aligns the stack on a 16-byte boundary, even when compiled with -Os.  So unless you're mixing non-GCC code with GCC code, you've always got a properly aligned stack for MOVDQU.

Which leads to the second question: if GCC is preserving this stack alignment, why does this instruction ever segfault?  As it turns out, it segfaults only when called from code generated by the TT JIT.

The TT JIT should follow GCC's lead and keep the stack aligned on 16-byte boundaries.  Edwin Smith offers the pointer:

<edwsmith> look in nanojit/Nativei386.cpp, genPrologue(), there's code that's
           ifndef'd DARWIN to dynamically align to 8bytes.

Comment 1

[Jim Blandy :jimb]

Attachment: x86 nanoJIT: preserve 16B stack alignment code on OSX and Linux; enforce on Win

x86 Darwin requires the stack to be aligned on a 16-byte boundary at
all times.  It's not officially required on x86 Linux, but at least
since 2006, GCC has aligned the stack to 16 bytes in main, and
preserved that much alignment in all other functions; code compiled
with -msse, -msse2, etc. depends on this.

This patch has two effects:
1) Preserve 16-byte alignment on all targets.
2) Assume 16-byte alignment on Linux as well as Darwin.

Comment 2

[Jason Orendorff [:jorendorff]]

Pushed http://hg.mozilla.org/tamarin-tracing/index.cgi/rev/e9e1af8a421c

Comment 3

[Brent Baker]

Changeset 445:e9e1af8a421c has caused some serious performance degradation in
the performance suite along with crashes in both the performance suite and the
acceptance suite.

http://hg.mozilla.org/tamarin-tracing/index.cgi/rev/e9e1af8a421c

Here is a run of the sunspider suite using 444 vs 445, values of 9999999
indicate a crash of the VM.

test                                                   avm    avm2     %sp

sunspider/bitops-3bit-bits-in-byte.as                 14.0    14.0     0.0
sunspider/crypto-sha1.as                              40.0   346.0  -765.0
sunspider/s3d-raytrace.as                            214.0 9999999 -4672796.7
sunspider/bitops-bits-in-byte.as                      39.0   199.0  -410.3
sunspider/math-cordic.as                              47.0   320.0  -580.9
sunspider/math-spectral-norm.as                       36.0   142.0  -294.4
sunspider/crypto-aes.as                              170.0   577.0  -239.4
sunspider/access-nbody.as                            163.0   265.0   -62.6
sunspider/s3d-morph.as                                94.0   741.0  -688.3
sunspider/access-fannkuch.as                         142.0   971.0  -583.8
sunspider/math-partial-sums.as                       225.0   984.0  -337.3
sunspider/access-nsieve.as                            63.0   115.0   -82.5
sunspider/s3d-cube.as                                166.0 9999999 -6023995.8
sunspider/bitops-bitwise-and.as                      198.0   206.0    -4.0
sunspider/access-binary-trees.as                      82.0 9999999 -12195020.7
sunspider/controlflow-recursive.as                    32.0 9999999 -31249896.9
sunspider/string-validate-input.as                   300.0   515.0   -71.7
sunspider/crypto-md5.as                              129.0   133.0    -3.1
sunspider/bitops-nsieve-bits.as                       58.0   111.0   -91.4
sunspider/string-fasta.as                            138.0   300.0  -117.4
sunspider/as3/bitops-3bit-bits-in-byte.as              8.0     8.0     0.0
sunspider/as3/crypto-sha1.as                          30.0   229.0  -663.3
sunspider/as3/s3d-raytrace.as                         67.0 9999999 -14925271.6
sunspider/as3/bitops-bits-in-byte.as                  18.0    80.0  -344.4
sunspider/as3/math-cordic.as                          33.0   222.0  -572.7
sunspider/as3/math-spectral-norm.as                   31.0    85.0  -174.2
sunspider/as3/crypto-aes.as                          144.0   320.0  -122.2
sunspider/as3/access-nbody.as                         26.0    30.0   -15.4
sunspider/as3/s3d-morph.as                            76.0    75.0     1.3
sunspider/as3/access-fannkuch.as                     110.0   479.0  -335.5
sunspider/as3/math-partial-sums.as                    96.0   363.0  -278.1
sunspider/as3/access-nsieve.as                        53.0    61.0   -15.1
sunspider/as3/s3d-cube.as                             77.0    70.0     9.1
sunspider/as3/bitops-bitwise-and.as                    5.0     6.0   -20.0
sunspider/as3/access-binary-trees.as                  24.0 9999999 -41666562.5
sunspider/as3/controlflow-recursive.as                36.0 9999999 -27777675.0
sunspider/as3/string-validate-input.as               283.0   474.0   -67.5
sunspider/as3/crypto-md5.as                           86.0    95.0   -10.5
sunspider/as3/bitops-nsieve-bits.as                   39.0    50.0   -28.2
sunspider/as3/string-fasta.as                         83.0   288.0  -247.0

There are also additional crashes in the other performance tests and also in
the acceptance suite.

Comment 5

[Jim Blandy :jimb]

Yes --- I noticed these when waterfall reported them; I'm trying to figure out what the problem is.

The problem goes away when TT is compiled without optimization, which I assume is why it affects Release but not Debug.

Comment 6

[Jim Blandy :jimb]

Attachment: TT x86 nanojit: Adjust epilogue code to match prologue code re: SP alignment

Changeset e9e1af8a421c made Tamarin Tracing prologues assume a
16B-aligned stack pointer under GCC, and omit code to force the
alignment.  The changeset neglected to make a corresponding adjustment
to the epilogue code.

This patch changes the epilogue generation code to work properly with
the prologue generation code under GCC.  It's meant to be applied in
addition to the changeset e9e1af8a421c patch.

Tested under Linux; no test suite regressions.

(Thanks to Dave Anderson, who tried to explain the problem to me,
unfortunately while I was still too confused to understand it.)

Comment 7

[Jim Blandy :jimb]

Comment on attachment 327497 [details] [diff] [review]
TT x86 nanojit: Adjust epilogue code to match prologue code re: SP alignment 

Eric Christopher (GCC developer at Apple) says that GCC does not always preserve 16B stack alignment, in his experience.  Since the prologue is only executed when we enter fragments from the interpreter, the alignment code is cheap; Edwin and I agreed it's better to just always align and be robust against changes in GCC's behavior.

Comment 8

[Jim Blandy :jimb]

Attachment: TT x86: establish a 16-byte stack alignment, on all platforms.

We hear rumors that GCC can't be trusted to establish and maintain a
16B alignment for the stack pointer.  Since the prologue and epilogue
don't run often, it's cheap to simply always establish the alignment
we need to be able to use SSE instructions, etc. there.  So do so on
all x86 platforms.

This runs happily on Linux, in both the debug and release configurations.  I'm having some difficulties with the test harness on Windows, which I'll get help with tomorrow.

Comment 9

[Brent Baker]

I tested the patch (id=327579) against linux, windows and mac and the builds all passed the acceptance suite.

Comment 11

[Jim Blandy :jimb]

Landed: 329ad622b0b9