Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2738683003:
[wasm][arm] Emit MaybeCheckConstPool in the trap code generation (Closed)
Description[wasm][arm] Emit MaybeCheckConstPool in the trap code generation
Without the check it happened that the builtin call in the trap code
was too far away from the constant pool and therefore crashed.
BUG=v8:6054
R=bmeurer@chromium.org, v8-arm-ports@googlegroups.com
Review-Url: https://codereview.chromium.org/2738683003
Cr-Commit-Position: refs/heads/master@{#43928}
Committed: https://chromium.googlesource.com/v8/v8/+/ab97fd76effb4c2075b35f61a7806311c21a5bea
Patch Set 1 #
Total comments: 1
Patch Set 2 : Change the comment. #Patch Set 3 : Check the constant pool in the macro assembler #Patch Set 4 : Test added #Patch Set 5 : Move MaybeCheckConstPool and always call it. #Messages
Total messages: 42 (27 generated)
The CQ bit was checked by ahaas@chromium.org 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...
lgtm
jacob.bramley@arm.com changed reviewers: + jacob.bramley@arm.com
https://codereview.chromium.org/2738683003/diff/1/src/compiler/arm/code-gener... File src/compiler/arm/code-generator-arm.cc (right): https://codereview.chromium.org/2738683003/diff/1/src/compiler/arm/code-gener... src/compiler/arm/code-generator-arm.cc:2139: __ MaybeCheckConstPool(); What is special about this location? I would have thought that the subsequent code generation would trigger this check. (The pools aren't blocked.) If the subsequent code _doesn't_ emit the pool properly, we should work out why not, because otherwise this crash is probably just a symptom of a different underlying problem.
On 2017/03/07 at 14:24:08, jacob.bramley wrote: > https://codereview.chromium.org/2738683003/diff/1/src/compiler/arm/code-gener... > File src/compiler/arm/code-generator-arm.cc (right): > > https://codereview.chromium.org/2738683003/diff/1/src/compiler/arm/code-gener... > src/compiler/arm/code-generator-arm.cc:2139: __ MaybeCheckConstPool(); > What is special about this location? I would have thought that the subsequent code generation would trigger this check. (The pools aren't blocked.) > > If the subsequent code _doesn't_ emit the pool properly, we should work out why not, because otherwise this crash is probably just a symptom of a different underlying problem. I think what's special about this location is that it is within out-of-line code which references the constant pool. At the moment MaybeCheckConstPool() is called at the beginning of AssembleArchInstruction, but AssembleArchInstruction is not called anymore when the out-of-line code is generated. When there is too much out-of-line code, we can get away too far from the constant pool.
On 2017/03/07 at 14:36:04, ahaas wrote: > On 2017/03/07 at 14:24:08, jacob.bramley wrote: > > https://codereview.chromium.org/2738683003/diff/1/src/compiler/arm/code-gener... > > File src/compiler/arm/code-generator-arm.cc (right): > > > > https://codereview.chromium.org/2738683003/diff/1/src/compiler/arm/code-gener... > > src/compiler/arm/code-generator-arm.cc:2139: __ MaybeCheckConstPool(); > > What is special about this location? I would have thought that the subsequent code generation would trigger this check. (The pools aren't blocked.) > > > > If the subsequent code _doesn't_ emit the pool properly, we should work out why not, because otherwise this crash is probably just a symptom of a different underlying problem. > > I think what's special about this location is that it is within out-of-line code which references the constant pool. At the moment MaybeCheckConstPool() is called at the beginning of AssembleArchInstruction, but AssembleArchInstruction is not called anymore when the out-of-line code is generated. When there is too much out-of-line code, we can get away too far from the constant pool. As far as I can tell, other out-of-line code does not reference the constant pool.
On 2017/03/07 14:36:04, ahaas wrote: > On 2017/03/07 at 14:24:08, jacob.bramley wrote: > > > https://codereview.chromium.org/2738683003/diff/1/src/compiler/arm/code-gener... > > File src/compiler/arm/code-generator-arm.cc (right): > > > > > https://codereview.chromium.org/2738683003/diff/1/src/compiler/arm/code-gener... > > src/compiler/arm/code-generator-arm.cc:2139: __ MaybeCheckConstPool(); > > What is special about this location? I would have thought that the subsequent > code generation would trigger this check. (The pools aren't blocked.) > > > > If the subsequent code _doesn't_ emit the pool properly, we should work out > why not, because otherwise this crash is probably just a symptom of a different > underlying problem. > > I think what's special about this location is that it is within out-of-line code > which references the constant pool. At the moment MaybeCheckConstPool() is > called at the beginning of AssembleArchInstruction, but AssembleArchInstruction > is not called anymore when the out-of-line code is generated. When there is too > much out-of-line code, we can get away too far from the constant pool. Right, but the low-level Assembler::emit(Instr) calls it so the code generation should trigger it. The block at the start of AssembleArchInstruction exists because some instructions block the pool from start to finish, so we have to explicitly check between instructions. I don't think this case is similar, is it?
On 2017/03/07 at 14:43:10, jacob.bramley wrote: > On 2017/03/07 14:36:04, ahaas wrote: > > On 2017/03/07 at 14:24:08, jacob.bramley wrote: > > > > > https://codereview.chromium.org/2738683003/diff/1/src/compiler/arm/code-gener... > > > File src/compiler/arm/code-generator-arm.cc (right): > > > > > > > > https://codereview.chromium.org/2738683003/diff/1/src/compiler/arm/code-gener... > > > src/compiler/arm/code-generator-arm.cc:2139: __ MaybeCheckConstPool(); > > > What is special about this location? I would have thought that the subsequent > > code generation would trigger this check. (The pools aren't blocked.) > > > > > > If the subsequent code _doesn't_ emit the pool properly, we should work out > > why not, because otherwise this crash is probably just a symptom of a different > > underlying problem. > > > > I think what's special about this location is that it is within out-of-line code > > which references the constant pool. At the moment MaybeCheckConstPool() is > > called at the beginning of AssembleArchInstruction, but AssembleArchInstruction > > is not called anymore when the out-of-line code is generated. When there is too > > much out-of-line code, we can get away too far from the constant pool. > > Right, but the low-level Assembler::emit(Instr) calls it so the code generation should trigger it. > > The block at the start of AssembleArchInstruction exists because some instructions block the pool from start to finish, so we have to explicitly check between instructions. I don't think this case is similar, is it? I think the constant pool is blocked here: https://cs.chromium.org/chromium/src/v8/src/arm/macro-assembler-arm.cc?q=macr.... The comment says: "Block constant pool for the call instruction sequence."
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/07 14:47:27, ahaas wrote: > I think the constant pool is blocked here: > https://cs.chromium.org/chromium/src/v8/src/arm/macro-assembler-arm.cc?q=macr.... > The comment says: "Block constant pool for the call instruction sequence." Ah, yes, and I think that exists because we expect (somewhere) to be able to examine and patch the sequence. On ARM64, blocking the pools first checks the pools, so we avoid a lot of situations like this, but it wasn't easy to modify the ARM back-end to behave in the same way. I would argue for putting the MaybeCheck inside `Call`, but that might have undesirable side-effects. In this case, though, I think it would be worth adding a comment to explain why it's necessary, since it is somewhat surprising by itself. Thanks, Jacob
The CQ bit was checked by ahaas@chromium.org 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 ahaas@chromium.org 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.
On 2017/03/07 at 15:01:33, jacob.bramley wrote: > On 2017/03/07 14:47:27, ahaas wrote: > > I think the constant pool is blocked here: > > https://cs.chromium.org/chromium/src/v8/src/arm/macro-assembler-arm.cc?q=macr.... > > The comment says: "Block constant pool for the call instruction sequence." > > Ah, yes, and I think that exists because we expect (somewhere) to be able to examine and patch the sequence. On ARM64, blocking the pools first checks the pools, so we avoid a lot of situations like this, but it wasn't easy to modify the ARM back-end to behave in the same way. > > I would argue for putting the MaybeCheck inside `Call`, but that might have undesirable side-effects. In this case, though, I think it would be worth adding a comment to explain why it's necessary, since it is somewhat surprising by itself. > > Thanks, > Jacob I put the MaybeCheck into the macro-assembler now. I had to add a flag because the macro-assembler function was also used in a PredictableCodeSizeScope. PTAL
The CQ bit was checked by ahaas@chromium.org 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.
Hmm. The operation at the call site is now less clear because we just pass 'true' in with no obvious meaning, and it still just fixes this specific failure. I'm worried about other dangerous uses of Call which might fail in the same way. Bugs like this often manifest themselves only after a particular sequence of code so the tests might not hit them even if they can be hit with real code. In decreasing order of preference: 1. Make BlockPools call MaybeCheck when it starts, to fix this problem in the general case. I tried this some time last year but it conflicted too often with the way that we use BlockPools. 2. Make Call always call MaybeCheck. (You've already determined that this isn't possible.) That leaves: 3. Make Call call MaybeCheck by default, but turn it off explicitly when needed. (This may be quite troublesome in practice.) 4. Just call MaybeCheck in the trap code generator, as you did in your first patch, but add a comment to explain why it's needed. I was hoping that it could be unconditional, so that all uses of Call were guaranteed to be correct. Thanks, Jacob
The CQ bit was checked by ahaas@chromium.org 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.
On 2017/03/08 at 11:00:30, jacob.bramley wrote: > Hmm. The operation at the call site is now less clear because we just pass 'true' in with no obvious meaning, and it still just fixes this specific failure. > > I'm worried about other dangerous uses of Call which might fail in the same way. Bugs like this often manifest themselves only after a particular sequence of code so the tests might not hit them even if they can be hit with real code. > > In decreasing order of preference: > > 1. Make BlockPools call MaybeCheck when it starts, to fix this problem in the general case. I tried this some time last year but it conflicted too often with the way that we use BlockPools. > 2. Make Call always call MaybeCheck. (You've already determined that this isn't possible.) > > That leaves: > > 3. Make Call call MaybeCheck by default, but turn it off explicitly when needed. (This may be quite troublesome in practice.) > 4. Just call MaybeCheck in the trap code generator, as you did in your first patch, but add a comment to explain why it's needed. > > I was hoping that it could be unconditional, so that all uses of Call were guaranteed to be correct. > > Thanks, > Jacob Hi Jacob, PTAL. I was not able to reproduce my problems with the PredictableCodeSizeScope anymore, so I put the MaybeCheck unconditionally into the inner call function. Cheers, Andreas
Perfect, thanks! LGTM
The CQ bit was checked by ahaas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org Link to the patchset: https://codereview.chromium.org/2738683003/#ps80001 (title: "Move MaybeCheckConstPool and always call it.")
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 ahaas@chromium.org
The CQ bit was checked by ahaas@chromium.org
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": 80001, "attempt_start_ts": 1490001064942410,
"parent_rev": "086ec2bd8d0ff97e0ea8f423d40fd13a5238038f", "commit_rev":
"ab97fd76effb4c2075b35f61a7806311c21a5bea"}
Message was sent while issue was closed.
Description was changed from ========== [wasm][arm] Emit MaybeCheckConstPool in the trap code generation Without the check it happened that the builtin call in the trap code was too far away from the constant pool and therefore crashed. BUG=v8:6054 R=bmeurer@chromium.org, v8-arm-ports@googlegroups.com ========== to ========== [wasm][arm] Emit MaybeCheckConstPool in the trap code generation Without the check it happened that the builtin call in the trap code was too far away from the constant pool and therefore crashed. BUG=v8:6054 R=bmeurer@chromium.org, v8-arm-ports@googlegroups.com Review-Url: https://codereview.chromium.org/2738683003 Cr-Commit-Position: refs/heads/master@{#43928} Committed: https://chromium.googlesource.com/v8/v8/+/ab97fd76effb4c2075b35f61a7806311c21... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/v8/v8/+/ab97fd76effb4c2075b35f61a7806311c21... |
