871444 - Yarr Interpreter custom allocator may misalign ParenthesesDisjunctionContext

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Martin Husemann]

Attachment: Roundup allocation size to keep required alignment

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 1

[Landry Breuil (:gaston)]

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!

Comment 2

[Landry Breuil (:gaston)]

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

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 748684 [details] [diff] [review]
Roundup allocation size to keep required alignment

Forwarding to Sean since this is in Yarr.

Comment 4

[Sean Stangl [: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!

Comment 5

[Sean Stangl [:sstangl]]

I just noticed that the patch uses a tab instead of spaces, but I'll fix that when committing.

Comment 6

[Sean Stangl [:sstangl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c001dce06445

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c001dce06445