Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1712003003:
Add WasmFrame, backtraces reflect wasm's presence (Closed)
DescriptionAdd WasmFrame, backtraces reflect wasm's presence
For now WasmFrame doesn't summarize the wasm frames. That'll require adding the
metadata in wasm-compiler similar to DeoptimizationInputData.
Teach the basic backtrace to iterate over stack frames instead of JS frames.
Update the wasm stack test.
`git cl format` touches random lines in files I touch.
R=titzer@chromium.org
TEST=d8 --test --expose-wasm test/mjsunit/mjsunit.js test/mjsunit/wasm/stack.js
Committed: https://crrev.com/aeca945786dcccad3efecfddbf2c07aefa524a56
Cr-Commit-Position: refs/heads/master@{#34220}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Review comments #Patch Set 3 : Undefined. #
Total comments: 16
Patch Set 4 : Nits. #Patch Set 5 : Use StackFrameIterator in Isolate::CaptureSimpleStackTrace #Patch Set 6 : Rip out VisibleFrame and the Iterator it rode in on. #Patch Set 7 : Fix recv. #
Total comments: 6
Patch Set 8 : Nits. #Patch Set 9 : Add missing test change. #
Total comments: 2
Messages
Total messages: 42 (14 generated)
https://codereview.chromium.org/1712003003/diff/1/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/1712003003/diff/1/src/isolate.cc#newcode345 src/isolate.cc:345: Handle<FixedArray> new_e = I guess it's ok to spell out new_elements here, since the old code did as well. https://codereview.chromium.org/1712003003/diff/1/src/isolate.cc#newcode428 src/isolate.cc:428: elements->set(cursor++, *caller); I'm not sure it's OK to put in the caller as the receiver here. Probably undefined is best. In the future we will probably want to put in the module object, but currently there is no way to get to it. https://codereview.chromium.org/1712003003/diff/1/src/isolate.cc#newcode438 src/isolate.cc:438: DCHECK(false && "unknown frame type"); We usually just use UNREACHABLE(); https://codereview.chromium.org/1712003003/diff/1/test/mjsunit/wasm/stack.js File test/mjsunit/wasm/stack.js (right): https://codereview.chromium.org/1712003003/diff/1/test/mjsunit/wasm/stack.js#... test/mjsunit/wasm/stack.js:65: " at Function.<WASM> (<anonymous>)\n" + // -- This shows the two wrappers (into and out of JS), which is OK for now, but we'll need to add a TODO somewhere to filter them out.
https://codereview.chromium.org/1712003003/diff/1/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/1712003003/diff/1/src/isolate.cc#newcode345 src/isolate.cc:345: Handle<FixedArray> new_e = On 2016/02/19 10:50:30, titzer wrote: > I guess it's ok to spell out new_elements here, since the old code did as well. Done. https://codereview.chromium.org/1712003003/diff/1/src/isolate.cc#newcode428 src/isolate.cc:428: elements->set(cursor++, *caller); On 2016/02/19 10:50:30, titzer wrote: > I'm not sure it's OK to put in the caller as the receiver here. Probably > undefined is best. In the future we will probably want to put in the module > object, but currently there is no way to get to it. I wasn't sure about that :-) I left as-is for now, but added a TODO. https://codereview.chromium.org/1712003003/diff/1/src/isolate.cc#newcode438 src/isolate.cc:438: DCHECK(false && "unknown frame type"); On 2016/02/19 10:50:30, titzer wrote: > We usually just use UNREACHABLE(); Done. https://codereview.chromium.org/1712003003/diff/1/test/mjsunit/wasm/stack.js File test/mjsunit/wasm/stack.js (right): https://codereview.chromium.org/1712003003/diff/1/test/mjsunit/wasm/stack.js#... test/mjsunit/wasm/stack.js:65: " at Function.<WASM> (<anonymous>)\n" + // -- On 2016/02/19 10:50:30, titzer wrote: > This shows the two wrappers (into and out of JS), which is OK for now, but we'll > need to add a TODO somewhere to filter them out. Yeah the JS frame handling does: // Filter out internal frames that we do not want to show. if (!IsVisibleInStackTrace(*fun, *caller, *recv, &seen_caller)) continue; Once I pipe the metadata through that will be obvious :-)
lgtm with one detail https://codereview.chromium.org/1712003003/diff/1/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/1712003003/diff/1/src/isolate.cc#newcode428 src/isolate.cc:428: elements->set(cursor++, *caller); On 2016/02/19 16:53:58, JF wrote: > On 2016/02/19 10:50:30, titzer wrote: > > I'm not sure it's OK to put in the caller as the receiver here. Probably > > undefined is best. In the future we will probably want to put in the module > > object, but currently there is no way to get to it. > > I wasn't sure about that :-) > I left as-is for now, but added a TODO. Please make it undefined; it will explode more readily under misuse.
https://codereview.chromium.org/1712003003/diff/1/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/1712003003/diff/1/src/isolate.cc#newcode428 src/isolate.cc:428: elements->set(cursor++, *caller); On 2016/02/19 17:00:41, titzer wrote: > On 2016/02/19 16:53:58, JF wrote: > > On 2016/02/19 10:50:30, titzer wrote: > > > I'm not sure it's OK to put in the caller as the receiver here. Probably > > > undefined is best. In the future we will probably want to put in the module > > > object, but currently there is no way to get to it. > > > > I wasn't sure about that :-) > > I left as-is for now, but added a TODO. > > Please make it undefined; it will explode more readily under misuse. Ah I had misunderstood what you meant by "undefined" earlier. Done.
The CQ bit was checked by jfb@chromium.org
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/1712003003/#ps40001 (title: "Undefined.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1712003003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1712003003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...) v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
https://codereview.chromium.org/1712003003/diff/40001/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/1712003003/diff/40001/src/isolate.cc#newcode341 src/isolate.cc:341: static Handle<FixedArray> maybeGrow(Isolate* isolate, nit: Function name is not following V8 style. https://codereview.chromium.org/1712003003/diff/40001/src/isolate.cc#newcode399 src/isolate.cc:399: continue; nit: Please put curly braces around body if conditional statement spans more than one line. https://codereview.chromium.org/1712003003/diff/40001/src/isolate.cc#newcode402 src/isolate.cc:402: continue; nit: Please put curly braces around body if conditional statement spans more than one line. https://codereview.chromium.org/1712003003/diff/40001/src/isolate.cc#newcode419 src/isolate.cc:419: elements->set(cursor++, *factory()->undefined_value()); I don't think it is OK to pass undefined as the receiver here, this is accessible through CallSiteGetTypeName and CallSiteGetThis in the messages.js file.
https://codereview.chromium.org/1712003003/diff/40001/src/frames.h File src/frames.h (right): https://codereview.chromium.org/1712003003/diff/40001/src/frames.h#newcode626 src/frames.h:626: class VisibleFrame : public StandardFrame { It seems that we are already special-casing the WASM frame type in the one iteration where the VisibleFrame class is being used anyways. Would it be possible to not introduce this abstraction yet, just switch to a normal StackFrameIterator in Isolate::CaptureSimpleStackTrace and treat WASM frames specially there for now until the implementation is settled?
I've been testing with: ./tools/run-tests.py -j8 --variants=default --timeout=10 --arch=x64 --mode=debug --no-presubmit $(cd test/; ls cctest/wasm/test-*.cc | sed -es/wasm\\///g | sed -es/[.]cc/\\/\\*/g) ./tools/run-tests.py -j8 --variants=default --timeout=10 --arch=x64 --mode=debug --no-presubmit unittests ./tools/run-tests.py -j8 --variants=default --timeout=10 --arch=x64 --mode=debug --no-presubmit mjsunit/wasm/* I guess that's not enough :-) https://codereview.chromium.org/1712003003/diff/40001/src/frames.h File src/frames.h (right): https://codereview.chromium.org/1712003003/diff/40001/src/frames.h#newcode626 src/frames.h:626: class VisibleFrame : public StandardFrame { On 2016/02/19 21:23:13, Michael Starzinger wrote: > It seems that we are already special-casing the WASM frame type in the one > iteration where the VisibleFrame class is being used anyways. Would it be > possible to not introduce this abstraction yet, just switch to a normal > StackFrameIterator in Isolate::CaptureSimpleStackTrace and treat WASM frames > specially there for now until the implementation is settled? Possible yes, but the current approach is what titzer recommended :-) https://codereview.chromium.org/1712003003/diff/40001/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/1712003003/diff/40001/src/isolate.cc#newcode341 src/isolate.cc:341: static Handle<FixedArray> maybeGrow(Isolate* isolate, On 2016/02/19 21:15:27, Michael Starzinger wrote: > nit: Function name is not following V8 style. Done. https://codereview.chromium.org/1712003003/diff/40001/src/isolate.cc#newcode399 src/isolate.cc:399: continue; On 2016/02/19 21:15:27, Michael Starzinger wrote: > nit: Please put curly braces around body if conditional statement spans more > than one line. Done. Auto-format doesn't auto-curl :-( https://codereview.chromium.org/1712003003/diff/40001/src/isolate.cc#newcode402 src/isolate.cc:402: continue; On 2016/02/19 21:15:27, Michael Starzinger wrote: > nit: Please put curly braces around body if conditional statement spans more > than one line. Done. https://codereview.chromium.org/1712003003/diff/40001/src/isolate.cc#newcode419 src/isolate.cc:419: elements->set(cursor++, *factory()->undefined_value()); On 2016/02/19 21:15:27, Michael Starzinger wrote: > I don't think it is OK to pass undefined as the receiver here, this is > accessible through CallSiteGetTypeName and CallSiteGetThis in the messages.js > file. What would be OK instead?
mvstanton@chromium.org changed reviewers: + mvstanton@chromium.org
https://codereview.chromium.org/1712003003/diff/40001/src/frames.h File src/frames.h (right): https://codereview.chromium.org/1712003003/diff/40001/src/frames.h#newcode626 src/frames.h:626: class VisibleFrame : public StandardFrame { On 2016/02/19 21:43:51, JF wrote: > On 2016/02/19 21:23:13, Michael Starzinger wrote: > > It seems that we are already special-casing the WASM frame type in the one > > iteration where the VisibleFrame class is being used anyways. Would it be > > possible to not introduce this abstraction yet, just switch to a normal > > StackFrameIterator in Isolate::CaptureSimpleStackTrace and treat WASM frames > > specially there for now until the implementation is settled? > > Possible yes, but the current approach is what titzer recommended :-) Maybe Ben can chime in, it does seem a big change, thx.
mstarzinger@chromium.org changed reviewers: + mstarzinger@chromium.org
https://codereview.chromium.org/1712003003/diff/40001/src/frames.h File src/frames.h (right): https://codereview.chromium.org/1712003003/diff/40001/src/frames.h#newcode626 src/frames.h:626: class VisibleFrame : public StandardFrame { On 2016/02/19 21:43:51, JF wrote: > On 2016/02/19 21:23:13, Michael Starzinger wrote: > > It seems that we are already special-casing the WASM frame type in the one > > iteration where the VisibleFrame class is being used anyways. Would it be > > possible to not introduce this abstraction yet, just switch to a normal > > StackFrameIterator in Isolate::CaptureSimpleStackTrace and treat WASM frames > > specially there for now until the implementation is settled? > > Possible yes, but the current approach is what titzer recommended :-) Just looking at this CL (i.e. the change in Isolate::CaptureSimpleStackTrace) I don't see the advantage of the VisibleFrame abstraction, because WasmFrame::Summarize isn't even being called yet. So maybe I am missing the necessary context of what is yet to come. I am happy to discuss with Ben next week. https://codereview.chromium.org/1712003003/diff/40001/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/1712003003/diff/40001/src/isolate.cc#newcode419 src/isolate.cc:419: elements->set(cursor++, *factory()->undefined_value()); On 2016/02/19 21:43:51, JF wrote: > On 2016/02/19 21:15:27, Michael Starzinger wrote: > > I don't think it is OK to pass undefined as the receiver here, this is > > accessible through CallSiteGetTypeName and CallSiteGetThis in the messages.js > > file. > > What would be OK instead? The "recv" value, as it was before. I am on referring to the "normal" JavaScript frames (i.e. StackFrame::JAVA_SCRIPT, OPTIMIZED and INTERPRETED) here.
https://codereview.chromium.org/1712003003/diff/40001/src/frames.h File src/frames.h (right): https://codereview.chromium.org/1712003003/diff/40001/src/frames.h#newcode626 src/frames.h:626: class VisibleFrame : public StandardFrame { On 2016/02/19 21:54:04, mvstanton wrote: > On 2016/02/19 21:43:51, JF wrote: > > On 2016/02/19 21:23:13, Michael Starzinger wrote: > > > It seems that we are already special-casing the WASM frame type in the one > > > iteration where the VisibleFrame class is being used anyways. Would it be > > > possible to not introduce this abstraction yet, just switch to a normal > > > StackFrameIterator in Isolate::CaptureSimpleStackTrace and treat WASM frames > > > specially there for now until the implementation is settled? > > > > Possible yes, but the current approach is what titzer recommended :-) > > Maybe Ben can chime in, it does seem a big change, thx. Yup, you all know the codebase and I don't so I'll let you fight it out and do whatever the winner says.
On 2016/02/19 21:55:23, Michael Starzinger wrote: > https://codereview.chromium.org/1712003003/diff/40001/src/frames.h > File src/frames.h (right): > > https://codereview.chromium.org/1712003003/diff/40001/src/frames.h#newcode626 > src/frames.h:626: class VisibleFrame : public StandardFrame { > On 2016/02/19 21:43:51, JF wrote: > > On 2016/02/19 21:23:13, Michael Starzinger wrote: > > > It seems that we are already special-casing the WASM frame type in the one > > > iteration where the VisibleFrame class is being used anyways. Would it be > > > possible to not introduce this abstraction yet, just switch to a normal > > > StackFrameIterator in Isolate::CaptureSimpleStackTrace and treat WASM frames > > > specially there for now until the implementation is settled? > > > > Possible yes, but the current approach is what titzer recommended :-) > > Just looking at this CL (i.e. the change in Isolate::CaptureSimpleStackTrace) I > don't see the advantage of the VisibleFrame abstraction, because > WasmFrame::Summarize isn't even being called yet. So maybe I am missing the > necessary context of what is yet to come. I am happy to discuss with Ben next > week. The hope was that existing clients of JavascriptFrameIterator would automatically have the WASM frames filtered out. Maybe the VisibleFrameIterator is too much abstraction if it's only going to be used to create stack traces here in the isolate. (and why are we doing that in the isolate? oh nevermind :-)) So I am OK if we just use a regular stack frame iterator here and beef up the filtering. > > https://codereview.chromium.org/1712003003/diff/40001/src/isolate.cc > File src/isolate.cc (right): > > https://codereview.chromium.org/1712003003/diff/40001/src/isolate.cc#newcode419 > src/isolate.cc:419: elements->set(cursor++, *factory()->undefined_value()); > On 2016/02/19 21:43:51, JF wrote: > > On 2016/02/19 21:15:27, Michael Starzinger wrote: > > > I don't think it is OK to pass undefined as the receiver here, this is > > > accessible through CallSiteGetTypeName and CallSiteGetThis in the > messages.js > > > file. > > > > What would be OK instead? > > The "recv" value, as it was before. I am on referring to the "normal" JavaScript > frames (i.e. StackFrame::JAVA_SCRIPT, OPTIMIZED and INTERPRETED) here.
https://codereview.chromium.org/1712003003/diff/40001/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/1712003003/diff/40001/src/isolate.cc#newcode419 src/isolate.cc:419: elements->set(cursor++, *factory()->undefined_value()); On 2016/02/19 21:55:23, Michael Starzinger wrote: > On 2016/02/19 21:43:51, JF wrote: > > On 2016/02/19 21:15:27, Michael Starzinger wrote: > > > I don't think it is OK to pass undefined as the receiver here, this is > > > accessible through CallSiteGetTypeName and CallSiteGetThis in the > messages.js > > > file. > > > > What would be OK instead? > > The "recv" value, as it was before. I am on referring to the "normal" JavaScript > frames (i.e. StackFrame::JAVA_SCRIPT, OPTIMIZED and INTERPRETED) here. In a previous comment I had advised passing "undefined" but I meant the below, WASM case, not this case.
On 2016/02/21 09:01:44, titzer wrote: > On 2016/02/19 21:55:23, Michael Starzinger wrote: > > https://codereview.chromium.org/1712003003/diff/40001/src/frames.h > > File src/frames.h (right): > > > > https://codereview.chromium.org/1712003003/diff/40001/src/frames.h#newcode626 > > src/frames.h:626: class VisibleFrame : public StandardFrame { > > On 2016/02/19 21:43:51, JF wrote: > > > On 2016/02/19 21:23:13, Michael Starzinger wrote: > > > > It seems that we are already special-casing the WASM frame type in the one > > > > iteration where the VisibleFrame class is being used anyways. Would it be > > > > possible to not introduce this abstraction yet, just switch to a normal > > > > StackFrameIterator in Isolate::CaptureSimpleStackTrace and treat WASM > frames > > > > specially there for now until the implementation is settled? > > > > > > Possible yes, but the current approach is what titzer recommended :-) > > > > Just looking at this CL (i.e. the change in Isolate::CaptureSimpleStackTrace) > I > > don't see the advantage of the VisibleFrame abstraction, because > > WasmFrame::Summarize isn't even being called yet. So maybe I am missing the > > necessary context of what is yet to come. I am happy to discuss with Ben next > > week. > > The hope was that existing clients of JavascriptFrameIterator would > automatically have the WASM frames filtered out. > Maybe the VisibleFrameIterator is too much abstraction if it's only going to be > used to create stack traces here in the > isolate. (and why are we doing that in the isolate? oh nevermind :-)) > > So I am OK if we just use a regular stack frame iterator here and beef up the > filtering. Thanks! OK, so let's make class WasmFrame inherit from StandardFrame directly. And also the implementation of WasmFrame::Summarize can be dropped until it turns out that we actually need it. I am fine with that variant of the CL. > > > > > https://codereview.chromium.org/1712003003/diff/40001/src/isolate.cc > > File src/isolate.cc (right): > > > > > https://codereview.chromium.org/1712003003/diff/40001/src/isolate.cc#newcode419 > > src/isolate.cc:419: elements->set(cursor++, *factory()->undefined_value()); > > On 2016/02/19 21:43:51, JF wrote: > > > On 2016/02/19 21:15:27, Michael Starzinger wrote: > > > > I don't think it is OK to pass undefined as the receiver here, this is > > > > accessible through CallSiteGetTypeName and CallSiteGetThis in the > > messages.js > > > > file. > > > > > > What would be OK instead? > > > > The "recv" value, as it was before. I am on referring to the "normal" > JavaScript > > frames (i.e. StackFrame::JAVA_SCRIPT, OPTIMIZED and INTERPRETED) here.
On 2016/02/21 09:01:54, titzer wrote: > https://codereview.chromium.org/1712003003/diff/40001/src/isolate.cc > File src/isolate.cc (right): > > https://codereview.chromium.org/1712003003/diff/40001/src/isolate.cc#newcode419 > src/isolate.cc:419: elements->set(cursor++, *factory()->undefined_value()); > On 2016/02/19 21:55:23, Michael Starzinger wrote: > > On 2016/02/19 21:43:51, JF wrote: > > > On 2016/02/19 21:15:27, Michael Starzinger wrote: > > > > I don't think it is OK to pass undefined as the receiver here, this is > > > > accessible through CallSiteGetTypeName and CallSiteGetThis in the > > messages.js > > > > file. > > > > > > What would be OK instead? > > > > The "recv" value, as it was before. I am on referring to the "normal" > JavaScript > > frames (i.e. StackFrame::JAVA_SCRIPT, OPTIMIZED and INTERPRETED) here. > > In a previous comment I had advised passing "undefined" but I meant the below, > WASM case, not this case. Yep, below is fine, here we should preserve existing semantics.
OK this should now be good to go. https://codereview.chromium.org/1712003003/diff/40001/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/1712003003/diff/40001/src/isolate.cc#newcode419 src/isolate.cc:419: elements->set(cursor++, *factory()->undefined_value()); On 2016/02/21 09:01:54, titzer wrote: > On 2016/02/19 21:55:23, Michael Starzinger wrote: > > On 2016/02/19 21:43:51, JF wrote: > > > On 2016/02/19 21:15:27, Michael Starzinger wrote: > > > > I don't think it is OK to pass undefined as the receiver here, this is > > > > accessible through CallSiteGetTypeName and CallSiteGetThis in the > > messages.js > > > > file. > > > > > > What would be OK instead? > > > > The "recv" value, as it was before. I am on referring to the "normal" > JavaScript > > frames (i.e. StackFrame::JAVA_SCRIPT, OPTIMIZED and INTERPRETED) here. > > In a previous comment I had advised passing "undefined" but I meant the below, > WASM case, not this case. Oh right, sorry about that! Too much look-alike code. Fixed now.
Description was changed from ========== Add WasmFrame, backtraces reflect wasm's presence For now WasmFrame doesn't summarize the wasm frames. That'll require adding the metadata in wasm-compiler similar to DeoptimizationInputData. Add VisibleFrame which is either a JS frame (which can also be intrp or optimized), or a wasm frame. Teach the basic backtrace to iterate over visible frames instead of JS frames. Update the wasm stack test. `git cl format` touches random lines in files I touch. R=titzer@chromium.org TEST=d8 --test --expose-wasm test/mjsunit/mjsunit.js test/mjsunit/wasm/stack.js ========== to ========== Add WasmFrame, backtraces reflect wasm's presence For now WasmFrame doesn't summarize the wasm frames. That'll require adding the metadata in wasm-compiler similar to DeoptimizationInputData. Teach the basic backtrace to iterate over stack frames instead of JS frames. Update the wasm stack test. `git cl format` touches random lines in files I touch. R=titzer@chromium.org TEST=d8 --test --expose-wasm test/mjsunit/mjsunit.js test/mjsunit/wasm/stack.js ==========
LGTM. Thanks! Just nits left to address. https://codereview.chromium.org/1712003003/diff/120001/src/frames.cc File src/frames.cc (left): https://codereview.chromium.org/1712003003/diff/120001/src/frames.cc#oldcode138 src/frames.cc:138: // ------------------------------------------------------------------------- nit: Please bring back the separator comment. https://codereview.chromium.org/1712003003/diff/120001/src/frames.cc File src/frames.cc (right): https://codereview.chromium.org/1712003003/diff/120001/src/frames.cc#newcode1222 src/frames.cc:1222: int index) const {} nit: Can we print something here? Just one single line saying "Wasm frame" or something like would be fine. It would just be nice to not have it silently fly under the radar during debugging. https://codereview.chromium.org/1712003003/diff/120001/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/1712003003/diff/120001/src/isolate.cc#newcode435 src/isolate.cc:435: *factory()->NewFunction( Just for posterity: This creates a new one-shot function identity for each frame. Functions in JavaScript do have identity (just like any other JSObject), so if this object ever leaks back into JavaScript (which it does through the API in messages.js), then this would result in "weird" semantics. I am fine with this for now, but this is probably not what you want in the long run.
https://codereview.chromium.org/1712003003/diff/120001/src/frames.cc File src/frames.cc (left): https://codereview.chromium.org/1712003003/diff/120001/src/frames.cc#oldcode138 src/frames.cc:138: // ------------------------------------------------------------------------- On 2016/02/23 09:36:25, Michael Starzinger wrote: > nit: Please bring back the separator comment. Done. https://codereview.chromium.org/1712003003/diff/120001/src/frames.cc File src/frames.cc (right): https://codereview.chromium.org/1712003003/diff/120001/src/frames.cc#newcode1222 src/frames.cc:1222: int index) const {} On 2016/02/23 09:36:25, Michael Starzinger wrote: > nit: Can we print something here? Just one single line saying "Wasm frame" or > something like would be fine. It would just be nice to not have it silently fly > under the radar during debugging. Done. https://codereview.chromium.org/1712003003/diff/120001/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/1712003003/diff/120001/src/isolate.cc#newcode435 src/isolate.cc:435: *factory()->NewFunction( On 2016/02/23 09:36:25, Michael Starzinger wrote: > Just for posterity: This creates a new one-shot function identity for each > frame. Functions in JavaScript do have identity (just like any other JSObject), > so if this object ever leaks back into JavaScript (which it does through the API > in messages.js), then this would result in "weird" semantics. I am fine with > this for now, but this is probably not what you want in the long run. Right, it'll come from the metadata we're not yet generating per wasm function instead, and be extracted by the Summarize function or something like that once it's implemented.
The CQ bit was checked by jfb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org, mstarzinger@chromium.org Link to the patchset: https://codereview.chromium.org/1712003003/#ps140001 (title: "Nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1712003003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1712003003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/1784) v8_linux64_rel_ng_triggered on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)
The CQ bit was checked by jfb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org, mstarzinger@chromium.org Link to the patchset: https://codereview.chromium.org/1712003003/#ps160001 (title: "Add missing test change.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1712003003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1712003003/160001
Message was sent while issue was closed.
Description was changed from ========== Add WasmFrame, backtraces reflect wasm's presence For now WasmFrame doesn't summarize the wasm frames. That'll require adding the metadata in wasm-compiler similar to DeoptimizationInputData. Teach the basic backtrace to iterate over stack frames instead of JS frames. Update the wasm stack test. `git cl format` touches random lines in files I touch. R=titzer@chromium.org TEST=d8 --test --expose-wasm test/mjsunit/mjsunit.js test/mjsunit/wasm/stack.js ========== to ========== Add WasmFrame, backtraces reflect wasm's presence For now WasmFrame doesn't summarize the wasm frames. That'll require adding the metadata in wasm-compiler similar to DeoptimizationInputData. Teach the basic backtrace to iterate over stack frames instead of JS frames. Update the wasm stack test. `git cl format` touches random lines in files I touch. R=titzer@chromium.org TEST=d8 --test --expose-wasm test/mjsunit/mjsunit.js test/mjsunit/wasm/stack.js ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Add WasmFrame, backtraces reflect wasm's presence For now WasmFrame doesn't summarize the wasm frames. That'll require adding the metadata in wasm-compiler similar to DeoptimizationInputData. Teach the basic backtrace to iterate over stack frames instead of JS frames. Update the wasm stack test. `git cl format` touches random lines in files I touch. R=titzer@chromium.org TEST=d8 --test --expose-wasm test/mjsunit/mjsunit.js test/mjsunit/wasm/stack.js ========== to ========== Add WasmFrame, backtraces reflect wasm's presence For now WasmFrame doesn't summarize the wasm frames. That'll require adding the metadata in wasm-compiler similar to DeoptimizationInputData. Teach the basic backtrace to iterate over stack frames instead of JS frames. Update the wasm stack test. `git cl format` touches random lines in files I touch. R=titzer@chromium.org TEST=d8 --test --expose-wasm test/mjsunit/mjsunit.js test/mjsunit/wasm/stack.js Committed: https://crrev.com/aeca945786dcccad3efecfddbf2c07aefa524a56 Cr-Commit-Position: refs/heads/master@{#34220} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/aeca945786dcccad3efecfddbf2c07aefa524a56 Cr-Commit-Position: refs/heads/master@{#34220}
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.chromium.org/1730673002/ by machenbach@chromium.org. The reason for reverting is: [Sheriff] Seems to break gcmole: https://build.chromium.org/p/client.v8/builders/V8%20Linux/builds/8295.
Message was sent while issue was closed.
adamk@chromium.org changed reviewers: + adamk@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1712003003/diff/160001/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/1712003003/diff/160001/src/isolate.cc#newcode434 src/isolate.cc:434: elements->set(cursor++, This line is causing a GCMole failure (https://build.chromium.org/p/client.v8/builders/V8%20Linux/builds/8295). The return values of the factory methods need to be stored on the stack before being set in elements.
Message was sent while issue was closed.
https://codereview.chromium.org/1712003003/diff/160001/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/1712003003/diff/160001/src/isolate.cc#newcode434 src/isolate.cc:434: elements->set(cursor++, On 2016/02/23 18:58:04, adamk wrote: > This line is causing a GCMole failure > (https://build.chromium.org/p/client.v8/builders/V8%20Linux/builds/8295). The > return values of the factory methods need to be stored on the stack before being > set in elements. Fixed in: https://codereview.chromium.org/1724063002 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
