Chromium Code Reviews

Issue 2705293011: MIPS[64]: Fix unaligned arguments storage in Wasm-to-interpreter entry (Closed)

Created:
3 years, 10 months ago by ivica.bogosavljevic
Modified:
3 years, 9 months ago
Reviewers:
titzer
CC:
v8-mips-ports_googlegroups.com, v8-reviews_googlegroups.com
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

MIPS[64]: Fix unaligned arguments storage in Wasm-to-interpreter entry In Wasm-to-interpeter entry creation, arguments for the interpreter are stored in an argument buffer. Depending on the order of the arguments some arguments may be misaligned and this causes crashes on those architectures that do not support unaligned memory access. TEST=cctest/test-wasm-interpreter-entry/TestArgumentPassing_AllTypes BUG= Review-Url: https://codereview.chromium.org/2705293011 Cr-Commit-Position: refs/heads/master@{#43476} Committed: https://chromium.googlesource.com/v8/v8/+/84ff6e4c1997b63c01e95504c31ee6c5504430d5

Patch Set 1 #

Total comments: 2

Patch Set 2 : 8-byte data alignment in argument buffer #

Patch Set 3 : Make buildbots happy #

Unified diffs Side-by-side diffs Stats (+24 lines, -7 lines)
M src/compiler/wasm-compiler.cc View 4 chunks +15 lines, -3 lines 0 comments
M src/utils.h View 1 chunk +5 lines, -0 lines 0 comments
M src/wasm/wasm-debug.cc View 3 chunks +4 lines, -4 lines 0 comments

Messages

Total messages: 28 (13 generated)
ivica.bogosavljevic
PTAL
3 years, 10 months ago (2017-02-23 16:27:01 UTC) #2
titzer
https://codereview.chromium.org/2705293011/diff/1/src/compiler/wasm-compiler.cc File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2705293011/diff/1/src/compiler/wasm-compiler.cc#newcode2980 src/compiler/wasm-compiler.cc:2980: int alignment = offset % (1 << ElementSizeLog2Of(param_rep)); What ...
3 years, 10 months ago (2017-02-23 20:44:47 UTC) #3
ivica.bogosavljevic
https://codereview.chromium.org/2705293011/diff/1/src/compiler/wasm-compiler.cc File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2705293011/diff/1/src/compiler/wasm-compiler.cc#newcode2980 src/compiler/wasm-compiler.cc:2980: int alignment = offset % (1 << ElementSizeLog2Of(param_rep)); On ...
3 years, 10 months ago (2017-02-24 09:37:56 UTC) #4
ivica.bogosavljevic
Data in argument buffer aligned to 8 bytes, PTAL
3 years, 9 months ago (2017-02-27 14:22:30 UTC) #5
titzer
lgtm
3 years, 9 months ago (2017-02-27 14:26:56 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2705293011/40001
3 years, 9 months ago (2017-02-28 11:59:39 UTC) #17
commit-bot: I haz the power
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/84ff6e4c1997b63c01e95504c31ee6c5504430d5
3 years, 9 months ago (2017-02-28 12:01:20 UTC) #20
ivica.bogosavljevic
On 2017/02/27 14:26:56, titzer wrote: > lgtm @titzer: there is a problem with this patch ...
3 years, 9 months ago (2017-03-01 13:23:34 UTC) #21
titzer
On 2017/03/01 13:23:34, ivica.bogosavljevic wrote: > On 2017/02/27 14:26:56, titzer wrote: > > lgtm > ...
3 years, 9 months ago (2017-03-01 13:25:06 UTC) #22
Clemens Hammacher
On 2017/03/01 at 13:25:06, titzer wrote: > On 2017/03/01 13:23:34, ivica.bogosavljevic wrote: > > On ...
3 years, 9 months ago (2017-03-08 15:43:38 UTC) #23
ivica.bogosavljevic
On 2017/03/08 15:43:38, Clemens Hammacher wrote: > On 2017/03/01 at 13:25:06, titzer wrote: > > ...
3 years, 9 months ago (2017-03-09 09:12:14 UTC) #24
Clemens Hammacher
On 2017/03/09 at 09:12:14, ivica.bogosavljevic wrote: > On 2017/03/08 15:43:38, Clemens Hammacher wrote: > > ...
3 years, 9 months ago (2017-03-13 17:18:25 UTC) #25
ivica.bogosavljevic
On 2017/03/13 17:18:25, Clemens Hammacher wrote: > On 2017/03/09 at 09:12:14, ivica.bogosavljevic wrote: > > ...
3 years, 9 months ago (2017-03-14 08:35:16 UTC) #26
Clemens Hammacher
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2760603002/ by clemensh@chromium.org. ...
3 years, 9 months ago (2017-03-17 10:29:56 UTC) #27
Clemens Hammacher
3 years, 9 months ago (2017-03-17 11:27:59 UTC) #28
Message was sent while issue was closed.
> > If we agree that this CL does not fix anything, and instead increases code
> > complexity and stack memory usage, I vote for reverting it.
> 
> In that case revert it. When we have 8-byte aligned StackSlot ready, we will
reland it.

Done.
Feel free to reland or partially reland if this is still required after fixing
the real issue.
Please include me as a reviewer then. Thanks!

Powered by Google App Engine