Bug 45673 - Regression r65596 - Stack not 8 byte aligned
Summary: Regression r65596 - Stack not 8 byte aligned
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-13 07:51 PDT by David Tapuska
Modified: 2010-09-21 04:59 PDT (History)
7 users (show)

See Also:


Attachments
Ensure ENABLE_PROFILER_REFERENCE_OFFSET is divisible by 8 (4.25 KB, patch)
2010-09-13 08:12 PDT, David Tapuska
no flags Details | Formatted Diff | Diff
Fix style of previous patch - Ensure ENABLE_PROFILER_REFERENCE_OFFSET is divisible by 8 (4.24 KB, patch)
2010-09-13 08:29 PDT, David Tapuska
barraclough: review-
barraclough: commit-queue-
Details | Formatted Diff | Diff
Same patch as 67414 but with prepare-ChangeLog run (4.92 KB, patch)
2010-09-20 10:23 PDT, David Tapuska
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Tapuska 2010-09-13 07:51:45 PDT
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.
Comment 1 David Tapuska 2010-09-13 08:12:14 PDT
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.
Comment 2 WebKit Review Bot 2010-09-13 08:15:17 PDT
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.
Comment 3 David Tapuska 2010-09-13 08:29:59 PDT
Created attachment 67414 [details]
Fix style of previous patch - Ensure ENABLE_PROFILER_REFERENCE_OFFSET is divisible by 8
Comment 4 Gavin Barraclough 2010-09-13 22:34:02 PDT
Looks fine, but please allow me to test this patch on Darwin before landing.  Will do so tomorrow, and review then.
Comment 5 Gabor Loki 2010-09-14 01:55:56 PDT
Works fine here. Thanks for the fix!
Comment 6 David Tapuska 2010-09-14 06:29:30 PDT
(In reply to comment #5)
> Works fine here. Thanks for the fix!

So who can give me a r+ and a commit-queue+?
Comment 7 Gabor Loki 2010-09-14 09:28:17 PDT
> So who can give me a r+ and a commit-queue+?

Please wait for Gavin to test your patch on Darwin!
Comment 8 Eric Seidel (no email) 2010-09-14 23:52:17 PDT
The commit-queue will "test" (run the layout tests and javascriptcore tests) on Darwin of course. :)  Depends on what you need to test.
Comment 9 Gavin Barraclough 2010-09-16 16:58:09 PDT
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.
Comment 10 David Tapuska 2010-09-16 17:04:59 PDT
(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.
Comment 11 Gavin Barraclough 2010-09-16 17:39:48 PDT
Comment on attachment 67414 [details]
Fix style of previous patch - Ensure ENABLE_PROFILER_REFERENCE_OFFSET is divisible by 8

Ooops, good point. :-)
Comment 12 David Tapuska 2010-09-20 10:23:39 PDT
Created attachment 68108 [details]
Same patch as 67414 but with prepare-ChangeLog run
Comment 13 Csaba Osztrogonác 2010-09-21 03:13:44 PDT
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.
Comment 14 WebKit Commit Bot 2010-09-21 04:59:46 PDT
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>
Comment 15 WebKit Commit Bot 2010-09-21 04:59:55 PDT
All reviewed patches have been landed.  Closing bug.