Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years ago by sigurds Modified:
5 years ago Reviewers:
Benedikt Meurer CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[turbofan] Fix ASAN bug in escape analysis
BUG=566253
LOG=n
Committed: https://crrev.com/07cc8d598b155e5c59585f544e190c38f4ba8b2e
Cr-Commit-Position: refs/heads/master@{#32942}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Better solution #
Total comments: 2
Patch Set 3 : Don't write code in DCHECK :) #
Created: 5 years ago
Dependent Patchsets: Messages
Total messages: 18 (9 generated)
Description was changed from ========== [turbofan] Fix ASAN bug in escape analysis BUG=566253 LOG=n ========== to ========== [turbofan] Fix ASAN bug in escape analysis BUG=566253 LOG=n ==========
sigurds@chromium.org changed reviewers: + mstarzinger@chromium.org
sigurds@chromium.org changed reviewers: + bmeurer@chromium.org - mstarzinger@chromium.org
PTAL, this fixes a clusterfuzz issue.
https://codereview.chromium.org/1530143002/diff/1/src/compiler/escape-analysi... File src/compiler/escape-analysis.cc (right): https://codereview.chromium.org/1530143002/diff/1/src/compiler/escape-analysi... src/compiler/escape-analysis.cc:457: info_.resize(graph()->NodeCount()); How about if (node->id() >= info_.size()) return false; as in IsVirtual below? https://codereview.chromium.org/1530143002/diff/1/src/compiler/escape-analysi... src/compiler/escape-analysis.cc:471: return info_[node->id()] == kEscaped; Isn't this subject to the same issue?
Please take a look again :) https://codereview.chromium.org/1530143002/diff/1/src/compiler/escape-analysi... File src/compiler/escape-analysis.cc (right): https://codereview.chromium.org/1530143002/diff/1/src/compiler/escape-analysi... src/compiler/escape-analysis.cc:457: info_.resize(graph()->NodeCount()); On 2015/12/16 17:53:56, Benedikt Meurer wrote: > How about > > if (node->id() >= info_.size()) return false; > > as in IsVirtual below? I want to get rid of the checks here, see comments in new patchset. https://codereview.chromium.org/1530143002/diff/1/src/compiler/escape-analysi... src/compiler/escape-analysis.cc:471: return info_[node->id()] == kEscaped; On 2015/12/16 17:53:56, Benedikt Meurer wrote: > Isn't this subject to the same issue? See comments in new patch set. https://codereview.chromium.org/1530143002/diff/20001/src/compiler/escape-ana... File src/compiler/escape-analysis.cc (left): https://codereview.chromium.org/1530143002/diff/20001/src/compiler/escape-ana... src/compiler/escape-analysis.cc:463: return false; I'm removing this check here, because the class has the invariant that info_ has entries for all nodes in the graph (even phis that get created by the analysis). https://codereview.chromium.org/1530143002/diff/20001/src/compiler/escape-ana... File src/compiler/escape-analysis.cc (right): https://codereview.chromium.org/1530143002/diff/20001/src/compiler/escape-ana... src/compiler/escape-analysis.cc:1079: return false; IsVirtual and IsEscaped below are public methods that may get called after the graph has been further modified, hence the check is needed here.
LGTM
The CQ bit was checked by sigurds@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1530143002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1530143002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)
The CQ bit was checked by sigurds@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/1530143002/#ps40001 (title: "Don't write code in DCHECK :)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1530143002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1530143002/40001
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Fix ASAN bug in escape analysis BUG=566253 LOG=n ========== to ========== [turbofan] Fix ASAN bug in escape analysis BUG=566253 LOG=n ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Fix ASAN bug in escape analysis BUG=566253 LOG=n ========== to ========== [turbofan] Fix ASAN bug in escape analysis BUG=566253 LOG=n Committed: https://crrev.com/07cc8d598b155e5c59585f544e190c38f4ba8b2e Cr-Commit-Position: refs/heads/master@{#32942} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/07cc8d598b155e5c59585f544e190c38f4ba8b2e Cr-Commit-Position: refs/heads/master@{#32942} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
