Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2446073002:
[stubs] Add more assertions in the CodeStubAssembler (Closed)
|
Created:
4 years, 1 month ago by Camillo Bruni Modified:
4 years, 1 month ago CC:
rmcilroy, v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[stubs] Add more assertions in the CodeStubAssembler
BUG=
Committed: https://crrev.com/feb96acecd0ed98579949907b970a39b88274cd1
Cr-Commit-Position: refs/heads/master@{#40638}
Patch Set 1 #Patch Set 2 : fix merge conflicts #Patch Set 3 : Not using DebugBreak() might help running the code #Patch Set 4 : g cl try #
Total comments: 5
Patch Set 5 : adding slow asserts #
Total comments: 3
Patch Set 6 : fixing CSA_SLOW_ASSERT #Patch Set 7 : rebase #
Messages
Total messages: 33 (21 generated)
The CQ bit was checked by cbruni@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: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/15155) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/15176) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/11428) v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/27225)
The CQ bit was checked by cbruni@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: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
cbruni@chromium.org changed reviewers: + ishell@chromium.org
PTAL Maybe the IsMap() checks are over the top and should just be put in LoadMap() to avoid excessive slowdowns.
Maybe it's time to introduce CSA_SLOW_ASSERT for IsMap? lgtm once you address the comments: https://codereview.chromium.org/2446073002/diff/60001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2446073002/diff/60001/src/code-stub-assembler... src/code-stub-assembler.cc:1161: Node* CodeStubAssembler::LoadWeakCellValue(Node* weak_cell, Label* if_cleared) { CSA_ASSERT(IsWeakCell(weak_cell)); https://codereview.chromium.org/2446073002/diff/60001/src/code-stub-assembler... src/code-stub-assembler.cc:1887: CSA_ASSERT(IntPtrGreaterThan(capacity_node, IntPtrConstant(0))); SmiOrIntPtrConstant(0, mode) to be consistent. https://codereview.chromium.org/2446073002/diff/60001/src/code-stub-assembler... src/code-stub-assembler.cc:2720: Node* CodeStubAssembler::IsSeededNumberDictionary(Node* object) { IsUnseededNumberDictionary
cbruni@chromium.org changed reviewers: + mythria@chromium.org
mythria@ PTAL src/interpreter/* ishell@ PTAL again, added SLOW_CSA_ASSERT https://codereview.chromium.org/2446073002/diff/60001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2446073002/diff/60001/src/code-stub-assembler... src/code-stub-assembler.cc:1887: CSA_ASSERT(IntPtrGreaterThan(capacity_node, IntPtrConstant(0))); On 2016/10/26 at 08:58:51, Igor Sheludko wrote: > SmiOrIntPtrConstant(0, mode) to be consistent. done. https://codereview.chromium.org/2446073002/diff/60001/src/code-stub-assembler... src/code-stub-assembler.cc:2720: Node* CodeStubAssembler::IsSeededNumberDictionary(Node* object) { On 2016/10/26 at 08:58:51, Igor Sheludko wrote: > IsUnseededNumberDictionary done.
https://codereview.chromium.org/2446073002/diff/80001/src/code-stub-assembler.h File src/code-stub-assembler.h (right): https://codereview.chromium.org/2446073002/diff/80001/src/code-stub-assembler... src/code-stub-assembler.h:166: void SlowAssert(compiler::Node* condition, const char* string = nullptr, See next comment. https://codereview.chromium.org/2446073002/diff/80001/src/code-stub-assembler... src/code-stub-assembler.h:1188: #define CSA_SLOW_ASSERT(x) SlowAssert((x), #x, __FILE__, __LINE__) I think we don't need SlowAssert function as we are tending to use CSA_ASSERT macros instead. Plus, the flag is available only when ENABLE_SLOW_DCHECKS is defined. You need something like: #ifdef ENABLE_SLOW_DCHECKS #define CSA_SLOW_ASSERT(x) if (FLAG_...) { Assert(x, ...); } #else #define CSA_SLOW_ASSERT(x) #endif
lgtm on interpreter changes.
The CQ bit was checked by cbruni@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: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/15267) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/11541) v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/27366)
The CQ bit was checked by cbruni@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 cbruni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org, mythria@chromium.org Link to the patchset: https://codereview.chromium.org/2446073002/#ps120001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
https://codereview.chromium.org/2446073002/diff/80001/src/code-stub-assembler.h File src/code-stub-assembler.h (right): https://codereview.chromium.org/2446073002/diff/80001/src/code-stub-assembler... src/code-stub-assembler.h:1188: #define CSA_SLOW_ASSERT(x) SlowAssert((x), #x, __FILE__, __LINE__) On 2016/10/26 at 13:24:27, Igor Sheludko wrote: > I think we don't need SlowAssert function as we are tending to use CSA_ASSERT macros instead. Plus, the flag is available only when ENABLE_SLOW_DCHECKS is defined. You need something like: > > #ifdef ENABLE_SLOW_DCHECKS > #define CSA_SLOW_ASSERT(x) if (FLAG_...) { Assert(x, ...); } > #else > #define CSA_SLOW_ASSERT(x) > #endif ok, can do it this way.
Message was sent while issue was closed.
FYI: This seems to introduce a crash in arm64 nosnap debug: https://build.chromium.org/p/client.v8.ports/builders/V8%20Linux%20-%20arm64%...
Message was sent while issue was closed.
On 2016/11/02 at 10:47:07, machenbach wrote: > FYI: This seems to introduce a crash in arm64 nosnap debug: > https://build.chromium.org/p/client.v8.ports/builders/V8%20Linux%20-%20arm64%... Thanks for the heads up, looking into this.
Message was sent while issue was closed.
On 2016/11/02 at 19:02:25, Camillo Bruni wrote: > On 2016/11/02 at 10:47:07, machenbach wrote: > > FYI: This seems to introduce a crash in arm64 nosnap debug: > > https://build.chromium.org/p/client.v8.ports/builders/V8%20Linux%20-%20arm64%... > > Thanks for the heads up, looking into this. Being fixed here: https://codereview.chromium.org/2475913002
Message was sent while issue was closed.
Description was changed from ========== [stubs] Add more assertions in the CodeStubAssembler BUG= ========== to ========== [stubs] Add more assertions in the CodeStubAssembler BUG= Committed: https://crrev.com/feb96acecd0ed98579949907b970a39b88274cd1 Cr-Commit-Position: refs/heads/master@{#40638} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/feb96acecd0ed98579949907b970a39b88274cd1 Cr-Commit-Position: refs/heads/master@{#40638} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
