|
|
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. |
DescriptionMIPS[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 #
Created: 3 years, 9 months ago
Messages
Total messages: 28 (13 generated)
ivica.bogosavljevic@imgtec.com changed reviewers: + titzer@chromium.org
PTAL
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.... src/compiler/wasm-compiler.cc:2980: int alignment = offset % (1 << ElementSizeLog2Of(param_rep)); What about just aligning all arguments to 64-bits?
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.... src/compiler/wasm-compiler.cc:2980: int alignment = offset % (1 << ElementSizeLog2Of(param_rep)); On 2017/02/23 20:44:46, titzer wrote: > What about just aligning all arguments to 64-bits? Pfiffig!
Data in argument buffer aligned to 8 bytes, PTAL
lgtm
The CQ bit was checked by ivica.bogosavljevic@imgtec.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/23276)
The CQ bit was checked by ivica.bogosavljevic@imgtec.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ivica.bogosavljevic@imgtec.com
The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org Link to the patchset: https://codereview.chromium.org/2705293011/#ps40001 (title: "Make buildbots happy")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488283172202460, "parent_rev": "7467f16d737be89d9cac573a07026e477cff85db", "commit_rev": "84ff6e4c1997b63c01e95504c31ee6c5504430d5"}
Message was sent while issue was closed.
Description was changed from ========== 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= ========== to ========== 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/+/84ff6e4c1997b63c01e95504c31ee6c5504... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/84ff6e4c1997b63c01e95504c31ee6c5504...
Message was sent while issue was closed.
On 2017/02/27 14:26:56, titzer wrote: > lgtm @titzer: there is a problem with this patch after it's landed. We aligned the arguments in the argument buffer, but alas the argument buffer is a StackSlot which is 4-bytes aligned, so this does nothing. The tests were passing at the time when the patch landed, and I believe this happened by accident. I am still investigating what happened and I will upload the CL that makes all the tests pass in all configuration (simulator/target without address fixups/big-endian).
Message was sent while issue was closed.
On 2017/03/01 13:23:34, ivica.bogosavljevic wrote: > On 2017/02/27 14:26:56, titzer wrote: > > lgtm > > @titzer: there is a problem with this patch after it's landed. We aligned the > arguments in the argument buffer, but alas the argument buffer is a StackSlot > which is 4-bytes aligned, so this does nothing. The tests were passing at the > time when the patch landed, and I believe this happened by accident. I am still > investigating what happened and I will upload the CL that makes all the tests > pass in all configuration (simulator/target without address fixups/big-endian). Perhaps we should add an alignment parameter to the StackSlot mechanism in TF.
Message was sent while issue was closed.
On 2017/03/01 at 13:25:06, titzer wrote: > On 2017/03/01 13:23:34, ivica.bogosavljevic wrote: > > On 2017/02/27 14:26:56, titzer wrote: > > > lgtm > > > > @titzer: there is a problem with this patch after it's landed. We aligned the > > arguments in the argument buffer, but alas the argument buffer is a StackSlot > > which is 4-bytes aligned, so this does nothing. The tests were passing at the > > time when the patch landed, and I believe this happened by accident. I am still > > investigating what happened and I will upload the CL that makes all the tests > > pass in all configuration (simulator/target without address fixups/big-endian). > > Perhaps we should add an alignment parameter to the StackSlot mechanism in TF. I am including this CL in a backmerge of https://codereview.chromium.org/2721053002/, since there is a dependency between those two CLs. Bug for the backmerge: https://bugs.chromium.org/p/v8/issues/detail?id=6043 Backmerge CL: https://codereview.chromium.org/2734253003 Can you please follow up on the concerns raised above? I am actually having difficulties spotting the original issue, as we were using ReadUnalignedValue and WriteUnalignedValue when reading back values from the buffer. Was this about leaving the whole stack pointer unaligned? This would indeed be a problem to fix in TF.
Message was sent while issue was closed.
On 2017/03/08 15:43:38, Clemens Hammacher wrote: > On 2017/03/01 at 13:25:06, titzer wrote: > > On 2017/03/01 13:23:34, ivica.bogosavljevic wrote: > > > On 2017/02/27 14:26:56, titzer wrote: > > > > lgtm > > > > > > @titzer: there is a problem with this patch after it's landed. We aligned > the > > > arguments in the argument buffer, but alas the argument buffer is a > StackSlot > > > which is 4-bytes aligned, so this does nothing. The tests were passing at > the > > > time when the patch landed, and I believe this happened by accident. I am > still > > > investigating what happened and I will upload the CL that makes all the > tests > > > pass in all configuration (simulator/target without address > fixups/big-endian). > > > > Perhaps we should add an alignment parameter to the StackSlot mechanism in TF. > > I am including this CL in a backmerge of > https://codereview.chromium.org/2721053002/, since there is a dependency between > those two CLs. > Bug for the backmerge: https://bugs.chromium.org/p/v8/issues/detail?id=6043 > Backmerge CL: https://codereview.chromium.org/2734253003 > > Can you please follow up on the concerns raised above? > I am actually having difficulties spotting the original issue, as we were using > ReadUnalignedValue and WriteUnalignedValue when reading back values from the > buffer. Was this about leaving the whole stack pointer unaligned? This would > indeed be a problem to fix in TF. The big-endian fix https://codereview.chromium.org/2721053002/ fixes problems with failures on BE that are unrelated to misalignment problems. This CL tries to fix problems with misaligned access (but does it only partially because StackSlot is not 8-bytes aligned). Just by accident they are in this order, it could have been vice versa.
Message was sent while issue was closed.
On 2017/03/09 at 09:12:14, ivica.bogosavljevic wrote: > On 2017/03/08 15:43:38, Clemens Hammacher wrote: > > On 2017/03/01 at 13:25:06, titzer wrote: > > > On 2017/03/01 13:23:34, ivica.bogosavljevic wrote: > > > > On 2017/02/27 14:26:56, titzer wrote: > > > > > lgtm > > > > > > > > @titzer: there is a problem with this patch after it's landed. We aligned > > the > > > > arguments in the argument buffer, but alas the argument buffer is a > > StackSlot > > > > which is 4-bytes aligned, so this does nothing. The tests were passing at > > the > > > > time when the patch landed, and I believe this happened by accident. I am > > still > > > > investigating what happened and I will upload the CL that makes all the > > tests > > > > pass in all configuration (simulator/target without address > > fixups/big-endian). > > > > > > Perhaps we should add an alignment parameter to the StackSlot mechanism in TF. > > > > I am including this CL in a backmerge of > > https://codereview.chromium.org/2721053002/, since there is a dependency between > > those two CLs. > > Bug for the backmerge: https://bugs.chromium.org/p/v8/issues/detail?id=6043 > > Backmerge CL: https://codereview.chromium.org/2734253003 > > > > Can you please follow up on the concerns raised above? > > I am actually having difficulties spotting the original issue, as we were using > > ReadUnalignedValue and WriteUnalignedValue when reading back values from the > > buffer. Was this about leaving the whole stack pointer unaligned? This would > > indeed be a problem to fix in TF. > > The big-endian fix https://codereview.chromium.org/2721053002/ fixes problems with failures > on BE that are unrelated to misalignment problems. > This CL tries to fix problems with misaligned access (but does it only partially > because StackSlot is not 8-bytes aligned). > Just by accident they are in this order, it could have been vice versa. I see how the other CL fixes things. I don't see what this CL should fix. It might rather make things worse by using a direct memory load/store instead of using Read/WriteUnalignedValue. Can you point to the precise location of a (previously) unaligned memory access? If we agree that this CL does not fix anything, and instead increases code complexity and stack memory usage, I vote for reverting it.
Message was sent while issue was closed.
On 2017/03/13 17:18:25, Clemens Hammacher wrote: > On 2017/03/09 at 09:12:14, ivica.bogosavljevic wrote: > > On 2017/03/08 15:43:38, Clemens Hammacher wrote: > > > On 2017/03/01 at 13:25:06, titzer wrote: > > > > On 2017/03/01 13:23:34, ivica.bogosavljevic wrote: > > > > > On 2017/02/27 14:26:56, titzer wrote: > > > > > > lgtm > > > > > > > > > > @titzer: there is a problem with this patch after it's landed. We > aligned > > > the > > > > > arguments in the argument buffer, but alas the argument buffer is a > > > StackSlot > > > > > which is 4-bytes aligned, so this does nothing. The tests were passing > at > > > the > > > > > time when the patch landed, and I believe this happened by accident. I > am > > > still > > > > > investigating what happened and I will upload the CL that makes all the > > > tests > > > > > pass in all configuration (simulator/target without address > > > fixups/big-endian). > > > > > > > > Perhaps we should add an alignment parameter to the StackSlot mechanism in > TF. > > > > > > I am including this CL in a backmerge of > > > https://codereview.chromium.org/2721053002/, since there is a dependency > between > > > those two CLs. > > > Bug for the backmerge: https://bugs.chromium.org/p/v8/issues/detail?id=6043 > > > Backmerge CL: https://codereview.chromium.org/2734253003 > > > > > > Can you please follow up on the concerns raised above? > > > I am actually having difficulties spotting the original issue, as we were > using > > > ReadUnalignedValue and WriteUnalignedValue when reading back values from the > > > buffer. Was this about leaving the whole stack pointer unaligned? This would > > > indeed be a problem to fix in TF. > > > > The big-endian fix https://codereview.chromium.org/2721053002/ fixes problems > with failures > > on BE that are unrelated to misalignment problems. > > This CL tries to fix problems with misaligned access (but does it only > partially > > because StackSlot is not 8-bytes aligned). > > Just by accident they are in this order, it could have been vice versa. > > I see how the other CL fixes things. > I don't see what this CL should fix. It might rather make things worse by using > a direct memory load/store instead of using Read/WriteUnalignedValue. > Can you point to the precise location of a (previously) unaligned memory access? > > 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.
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2760603002/ by clemensh@chromium.org. The reason for reverting is: Did not fix the issue..
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! |