Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1832653002:
[interpreter] Fix memory leak in Bytecodes::Decode(). (Closed)
Description[interpreter] Fix memory leak in Bytecodes::Decode().
BUG=v8:4280
LOG=N
Committed: https://crrev.com/e718f49a762e7d3d3cb010df73da92f284dd2d56
Cr-Commit-Position: refs/heads/master@{#35054}
Patch Set 1 #
Total comments: 1
Patch Set 2 : iomanip variant. #
Total comments: 1
Messages
Total messages: 15 (5 generated)
oth@chromium.org changed reviewers: + rmcilroy@chromium.org
PTAL, thanks
Lgtm with an optional suggestion. Thanks https://codereview.chromium.org/1832653002/diff/1/src/interpreter/bytecodes.cc File src/interpreter/bytecodes.cc (right): https://codereview.chromium.org/1832653002/diff/1/src/interpreter/bytecodes.c... src/interpreter/bytecodes.cc:499: SNPrintF(buf, "%02x ", bytecode_start[i]); Could we just use something like: os << std::width(2) << std::hex() << bytecode_start[i]; Then we could get rid of buf entirely?
On 2016/03/24 08:51:27, rmcilroy wrote: > Lgtm with an optional suggestion. Thanks > > https://codereview.chromium.org/1832653002/diff/1/src/interpreter/bytecodes.cc > File src/interpreter/bytecodes.cc (right): > > https://codereview.chromium.org/1832653002/diff/1/src/interpreter/bytecodes.c... > src/interpreter/bytecodes.cc:499: SNPrintF(buf, "%02x ", bytecode_start[i]); > Could we just use something like: > os << std::width(2) << std::hex() << bytecode_start[i]; > > Then we could get rid of buf entirely? Well, this has always been my preferred way of doing this. It's written this way to fit in with the norm in v8. Will tweak it later.
The CQ bit was checked by oth@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1832653002/#ps20001 (title: "iomanip variant.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1832653002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1832653002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [interpreter] Fix memory leak in Bytecodes::Decode(). BUG=v8:4280 LOG=N ========== to ========== [interpreter] Fix memory leak in Bytecodes::Decode(). BUG=v8:4280 LOG=N Committed: https://crrev.com/e718f49a762e7d3d3cb010df73da92f284dd2d56 Cr-Commit-Position: refs/heads/master@{#35054} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e718f49a762e7d3d3cb010df73da92f284dd2d56 Cr-Commit-Position: refs/heads/master@{#35054}
Message was sent while issue was closed.
machenbach@chromium.org changed reviewers: + machenbach@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1832653002/diff/20001/src/interpreter/bytecod... File src/interpreter/bytecodes.cc (right): https://codereview.chromium.org/1832653002/diff/20001/src/interpreter/bytecod... src/interpreter/bytecodes.cc:494: operand_scale = Bytecodes::PrefixBytecodeToOperandScale(bytecode); FYI: The experimental code-coverage claims this code isn't executed on x64. Is that maybe the case or are we missing to run a test? In which scenario would this code path get exercised? I assumed it would get exercised through the test that is unskipped in this CL. But I'm not seeing that test in the output of any test step of the coverage bot. Is this test maybe skipped otherwise or reported under a different label? Or I need to track down an infrastructure bug.
Message was sent while issue was closed.
On 2016/03/24 13:35:29, Michael Achenbach wrote: > https://codereview.chromium.org/1832653002/diff/20001/src/interpreter/bytecod... > File src/interpreter/bytecodes.cc (right): > > https://codereview.chromium.org/1832653002/diff/20001/src/interpreter/bytecod... > src/interpreter/bytecodes.cc:494: operand_scale = > Bytecodes::PrefixBytecodeToOperandScale(bytecode); > FYI: The experimental code-coverage claims this code isn't executed on x64. Is > that maybe the case or are we missing to run a test? In which scenario would > this code path get exercised? > > I assumed it would get exercised through the test that is unskipped in this CL. > But I'm not seeing that test in the output of any test step of the coverage bot. > Is this test maybe skipped otherwise or reported under a different label? Or I > need to track down an infrastructure bug. This is curious - the test in unskipped CL is the only on there is for the code path. The test cases in Bytecodes.DecodeBytecodeAndOperands that include B(Wide) and B(ExtraWide) will go down this branch. Locally it exercises both paths. I don't see a way we could run one path and not another. Orion
Message was sent while issue was closed.
On 2016/03/24 14:03:19, oth wrote: > On 2016/03/24 13:35:29, Michael Achenbach wrote: > > > https://codereview.chromium.org/1832653002/diff/20001/src/interpreter/bytecod... > > File src/interpreter/bytecodes.cc (right): > > > > > https://codereview.chromium.org/1832653002/diff/20001/src/interpreter/bytecod... > > src/interpreter/bytecodes.cc:494: operand_scale = > > Bytecodes::PrefixBytecodeToOperandScale(bytecode); > > FYI: The experimental code-coverage claims this code isn't executed on x64. Is > > that maybe the case or are we missing to run a test? In which scenario would > > this code path get exercised? > > > > I assumed it would get exercised through the test that is unskipped in this > CL. > > But I'm not seeing that test in the output of any test step of the coverage > bot. > > Is this test maybe skipped otherwise or reported under a different label? Or I > > need to track down an infrastructure bug. > > This is curious - the test in unskipped CL is the only on there is for the code > path. > > The test cases in Bytecodes.DecodeBytecodeAndOperands that include B(Wide) and > B(ExtraWide) will go down this branch. > > Locally it exercises both paths. I don't see a way we could run one path and not > another. > > Orion I'll investigate more. It's maybe a strange infra bug. I searched manually for the appearance of DecodeBytecodeAndOperands in the stdio of the test runner. I found it here: https://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/... https://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds... But I didn't find it e.g. here: https://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/... https://build.chromium.org/p/tryserver.v8/builders/v8_linux64_sanitizer_cover...
Message was sent while issue was closed.
FYI: The random test coverage was an infra bug which is now fixed here: https://codereview.chromium.org/1842673002/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
