Yarr Interpreter custom allocator may misalign ParenthesesDisjunctionContext

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: martin, Unassigned)

Tracking

Trunk
mozilla24
Sun
NetBSD
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

ParenthesesDisjunctionContext contains a pointer (next) so it may need proper alignment, for example on sparc64. Simple fix is to keep alignment with all allocations (see attached patch).

If the small space wasted is considered inappropriate, we should create a macro like

  JS_REQUIRED_ALIGNMENT_OF()

which could return 0 on all archs that might align, but do not necessarily require it, and is identical to JS_ALIGNMENT_OF() on archs requiring strict alignment.

[teaser: this bug report brought to you via aurora running on NetBSD/current sparc64]
Comment on attachment 748684 [details] [diff] [review]
Roundup allocation size to keep required alignment

Let's try to get feedback on this too, or even review!
Attachment #748684 - Flags: review?(luke)
Comment on attachment 748684 [details] [diff] [review]
Roundup allocation size to keep required alignment

Fwiw, this helps fixing firefox on sparc64 for me too, as per bug #840242 comment 15.

Pushed to try in https://tbpl.mozilla.org/?tree=Try&rev=e62d4aee3e38
Attachment #748684 - Flags: feedback+
Comment on attachment 748684 [details] [diff] [review]
Roundup allocation size to keep required alignment

Forwarding to Sean since this is in Yarr.
Attachment #748684 - Flags: review?(luke) → review?(sstangl)
Comment on attachment 748684 [details] [diff] [review]
Roundup allocation size to keep required alignment

Review of attachment 748684 [details] [diff] [review]:
-----------------------------------------------------------------

A small increase in interpreter memory is acceptable to keep platforms consistent and has little effect since Yarr code nearly exclusively executes in the Yarr JIT on supported platforms. Thanks for fixing this!
Attachment #748684 - Flags: review?(sstangl) → review+
I just noticed that the patch uses a tab instead of spaces, but I'll fix that when committing.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c001dce06445
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.