Bug 45673 – Regression r65596 - Stack not 8 byte aligned

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[David Tapuska]

r65596 adjusted ENABLE_PROFILER_REFERENCE_OFFSET so that it was no longer divisible by 8. ENABLE_PROFILER_REFERENCE_OFFSET is used to adjust the stack pointer in the cti methods and 65596 will cause the stack pointer to no longer be 8 byte aligned. According to the ABI 8 byte stack alignment should be preserved.

[David Tapuska]

Created attachment 67411 [details]
Ensure ENABLE_PROFILER_REFERENCE_OFFSET is divisible by 8

This change adjusts the padding in the struct. In r65596 the padding that was there for JSVALUE32_64 was made useless because the object would be 8 byte aligned already so it can be removed. The additional padding that was essentially used for making sure the enabledProfilerReference offset was 8 byte aligned for JSVALUE32 was moved ahead so that we remove this padding for JSVALUE32_64 and have the correct field alignments for the rest. One benefit of this change (aside from fixing the bug) is that it reduces the JITStackFrame by 12 bytes by removing useless padding.

[WebKit Review Bot]

Attachment 67411 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/jit/JITStubs.h:151:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Tapuska]

Created attachment 67414 [details]
Fix style of previous patch - Ensure ENABLE_PROFILER_REFERENCE_OFFSET is divisible by 8

[Gavin Barraclough]

Looks fine, but please allow me to test this patch on Darwin before landing.  Will do so tomorrow, and review then.

[Gabor Loki]

Works fine here. Thanks for the fix!

[David Tapuska]

(In reply to comment #5)
> Works fine here. Thanks for the fix!

So who can give me a r+ and a commit-queue+?

[Gabor Loki]

> So who can give me a r+ and a commit-queue+?

Please wait for Gavin to test your patch on Darwin!

[Eric Seidel (no email)]

The commit-queue will "test" (run the layout tests and javascriptcore tests) on Darwin of course. :)  Depends on what you need to test.

[Gavin Barraclough]

Comment on attachment 67414 [details]
Fix style of previous patch - Ensure ENABLE_PROFILER_REFERENCE_OFFSET is divisible by 8

Sorry for holding this up.  My main dev machine has melted itself in a pretty catastrophic way today & will be unable to test for at least another day.  I don't think it would be reasonable for me to delay you any further so I'll get out of the way & r+.
If this does raise any issues we can sort this out afterwards.

[David Tapuska]

(In reply to comment #9)
> (From update of attachment 67414 [details])
> Sorry for holding this up.  My main dev machine has melted itself in a pretty catastrophic way today & will be unable to test for at least another day.  I don't think it would be reasonable for me to delay you any further so I'll get out of the way & r+.
> If this does raise any issues we can sort this out afterwards.

I don't think I attached a changelog so it should probably be r- until I do that.

[Gavin Barraclough]

Comment on attachment 67414 [details]
Fix style of previous patch - Ensure ENABLE_PROFILER_REFERENCE_OFFSET is divisible by 8

Ooops, good point. :-)

[David Tapuska]

Created attachment 68108 [details]
Same patch as 67414 but with prepare-ChangeLog run

[Csaba Osztrogonác]

Comment on attachment 68108 [details]
Same patch as 67414 but with prepare-ChangeLog run

This patch already contains the previously missing 
changelog, and doesn't make any other changes.

[WebKit Commit Bot]

Comment on attachment 68108 [details]
Same patch as 67414 but with prepare-ChangeLog run

Clearing flags on attachment: 68108

Committed r67943: <http://trac.webkit.org/changeset/67943>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.