Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2493773003:
[inspector] Introduce translation of wasm frames (Closed)
|
Created:
4 years, 1 month ago by Clemens Hammacher Modified:
4 years, 1 month ago CC:
devtools-reviews_chromium.org, v8-reviews_googlegroups.com, Yang Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[inspector] Introduce translation of wasm frames
This allows to show wasm source (disassembled wasm code) in DevTools.
See design doc for details.
More tests for the disassembly will have to follow. Also, the text
format (generated by V8) will be changed.
BUG=chromium:659715
R=yangguo@chromium.org, kozyatinskiy@chromium.org, titzer@chromium.org, dgozman@chromium.org
Committed: https://crrev.com/d4a42a5f89b923a1f8cf0b09bbbd3f4a83049c5e
Cr-Commit-Position: refs/heads/master@{#41055}
Patch Set 1 #Patch Set 2 : Avoid usage of emplace (not available before gcc 4.8) #Patch Set 3 : Fix signed/unsigned issues #Patch Set 4 : More signed/unsigned #
Total comments: 12
Patch Set 5 : Address comments #
Total comments: 8
Patch Set 6 : Address Bens comments #Patch Set 7 : Fix last patch #
Total comments: 6
Patch Set 8 : Address Alexey's comments #
Total comments: 16
Patch Set 9 : Address Dmitry's comments #
Total comments: 2
Patch Set 10 : Very last-minute changes #
Messages
Total messages: 71 (50 generated)
The CQ bit was checked by clemensh@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_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) 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_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_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/15983)
The CQ bit was checked by clemensh@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_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/17690)
The CQ bit was checked by clemensh@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 checked by clemensh@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.
https://codereview.chromium.org/2493773003/diff/60001/src/debug/debug-interfa... File src/debug/debug-interface.h (right): https://codereview.chromium.org/2493773003/diff/60001/src/debug/debug-interfa... src/debug/debug-interface.h:206: static std::pair<std::string, std::vector<std::tuple<uint32_t, int, int>>> Can we introduce a struct for this type, instead of the std::tuple? https://codereview.chromium.org/2493773003/diff/60001/src/inspector/v8-debugg... File src/inspector/v8-debugger.cc (right): https://codereview.chromium.org/2493773003/diff/60001/src/inspector/v8-debugg... src/inspector/v8-debugger.cc:594: char sourceUrlStart[7]; Can you factor this code out into a "startsWith" helper? https://codereview.chromium.org/2493773003/diff/60001/src/inspector/wasm-tran... File src/inspector/wasm-translation.h (right): https://codereview.chromium.org/2493773003/diff/60001/src/inspector/wasm-tran... src/inspector/wasm-translation.h:41: */ Line comments instead of block comments?
dgozman@chromium.org changed reviewers: + dgozman@chromium.org
https://codereview.chromium.org/2493773003/diff/60001/src/debug/debug-interfa... File src/debug/debug-interface.h (right): https://codereview.chromium.org/2493773003/diff/60001/src/debug/debug-interfa... src/debug/debug-interface.h:206: static std::pair<std::string, std::vector<std::tuple<uint32_t, int, int>>> On 2016/11/11 10:59:02, titzer wrote: > Can we introduce a struct for this type, instead of the std::tuple? +1 It will also help with describing what this complex structure means. https://codereview.chromium.org/2493773003/diff/60001/src/inspector/v8-debugg... File src/inspector/v8-debugger.cc (right): https://codereview.chromium.org/2493773003/diff/60001/src/inspector/v8-debugg... src/inspector/v8-debugger.cc:584: // Determine if it is a wasm script by checking whether the source url Why not expose isWasmScript or similar? https://codereview.chromium.org/2493773003/diff/60001/src/inspector/wasm-tran... File src/inspector/wasm-translation.h (right): https://codereview.chromium.org/2493773003/diff/60001/src/inspector/wasm-tran... src/inspector/wasm-translation.h:42: bool Translate(protocol::Debugger::Location* location); What's the result of this function?
The CQ bit was checked by clemensh@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...
PTAL https://codereview.chromium.org/2493773003/diff/60001/src/debug/debug-interfa... File src/debug/debug-interface.h (right): https://codereview.chromium.org/2493773003/diff/60001/src/debug/debug-interfa... src/debug/debug-interface.h:206: static std::pair<std::string, std::vector<std::tuple<uint32_t, int, int>>> On 2016/11/11 at 21:50:34, dgozman wrote: > On 2016/11/11 10:59:02, titzer wrote: > > Can we introduce a struct for this type, instead of the std::tuple? > > +1 > It will also help with describing what this complex structure means. As discussed offline with Ben: Introducing a struct here requires us to either include the debug-interface.h in several wasm headers where this information is produced and passed. We therefore leave it a tuple for now. https://codereview.chromium.org/2493773003/diff/60001/src/inspector/v8-debugg... File src/inspector/v8-debugger.cc (right): https://codereview.chromium.org/2493773003/diff/60001/src/inspector/v8-debugg... src/inspector/v8-debugger.cc:584: // Determine if it is a wasm script by checking whether the source url On 2016/11/11 at 21:50:34, dgozman wrote: > Why not expose isWasmScript or similar? Added "isWasm" method to script wrapper. https://codereview.chromium.org/2493773003/diff/60001/src/inspector/v8-debugg... src/inspector/v8-debugger.cc:594: char sourceUrlStart[7]; On 2016/11/11 at 10:59:02, titzer wrote: > Can you factor this code out into a "startsWith" helper? Obsolete with the isWasm method. https://codereview.chromium.org/2493773003/diff/60001/src/inspector/wasm-tran... File src/inspector/wasm-translation.h (right): https://codereview.chromium.org/2493773003/diff/60001/src/inspector/wasm-tran... src/inspector/wasm-translation.h:41: */ On 2016/11/11 at 10:59:02, titzer wrote: > Line comments instead of block comments? Done. https://codereview.chromium.org/2493773003/diff/60001/src/inspector/wasm-tran... src/inspector/wasm-translation.h:42: bool Translate(protocol::Debugger::Location* location); On 2016/11/11 at 21:50:35, dgozman wrote: > What's the result of this function? Documented now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with nits https://codereview.chromium.org/2493773003/diff/80001/src/inspector/wasm-tran... File src/inspector/wasm-translation.h (right): https://codereview.chromium.org/2493773003/diff/80001/src/inspector/wasm-tran... src/inspector/wasm-translation.h:42: bool Translate(protocol::Debugger::Location* location); Should this be AddTranslation()? From what I can tell, it updates internal data structures. https://codereview.chromium.org/2493773003/diff/80001/src/inspector/wasm-tran... src/inspector/wasm-translation.h:52: bool TranslateBack(ScriptBreakpoint* breakpoint); Can you name this method closer to what the comment says (instead of just TranslateBack)? https://codereview.chromium.org/2493773003/diff/80001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2493773003/diff/80001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1908: DCHECK(ok); Bah, just make it a CHECK() so you don't need the USE() below. That does mean we will crash in production, though. https://codereview.chromium.org/2493773003/diff/80001/src/wasm/wasm-module.h File src/wasm/wasm-module.h (right): https://codereview.chromium.org/2493773003/diff/80001/src/wasm/wasm-module.h#... src/wasm/wasm-module.h:400: // column in the disassembly. Format isn't totally clear from the comment. Maybe: Returns the disassembly string and a list of (byte offset, line, column) entries.
The CQ bit was checked by clemensh@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...
PTAL https://codereview.chromium.org/2493773003/diff/80001/src/inspector/wasm-tran... File src/inspector/wasm-translation.h (right): https://codereview.chromium.org/2493773003/diff/80001/src/inspector/wasm-tran... src/inspector/wasm-translation.h:42: bool Translate(protocol::Debugger::Location* location); On 2016/11/16 at 14:23:59, titzer wrote: > Should this be AddTranslation()? > > From what I can tell, it updates internal data structures. Updated the documentation and renamed as discussed offline. https://codereview.chromium.org/2493773003/diff/80001/src/inspector/wasm-tran... src/inspector/wasm-translation.h:52: bool TranslateBack(ScriptBreakpoint* breakpoint); On 2016/11/16 at 14:23:59, titzer wrote: > Can you name this method closer to what the comment says (instead of just TranslateBack)? Changed as discussed offline. https://codereview.chromium.org/2493773003/diff/80001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2493773003/diff/80001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1908: DCHECK(ok); On 2016/11/16 at 14:23:59, titzer wrote: > Bah, just make it a CHECK() so you don't need the USE() below. That does mean we will crash in production, though. Done. We only call it for already compiled functions, so it should never crash anyway. But I guess the runtime overhead for a CHECK is acceptable ;) https://codereview.chromium.org/2493773003/diff/80001/src/wasm/wasm-module.h File src/wasm/wasm-module.h (right): https://codereview.chromium.org/2493773003/diff/80001/src/wasm/wasm-module.h#... src/wasm/wasm-module.h:400: // column in the disassembly. On 2016/11/16 at 14:23:59, titzer wrote: > Format isn't totally clear from the comment. > > Maybe: > > Returns the disassembly string and a list of (byte offset, line, column) entries. Done.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/28690)
The CQ bit was checked by clemensh@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...
https://codereview.chromium.org/2493773003/diff/60001/src/debug/debug-interfa... File src/debug/debug-interface.h (right): https://codereview.chromium.org/2493773003/diff/60001/src/debug/debug-interfa... src/debug/debug-interface.h:206: static std::pair<std::string, std::vector<std::tuple<uint32_t, int, int>>> On 2016/11/16 11:36:28, Clemens Hammacher wrote: > On 2016/11/11 at 21:50:34, dgozman wrote: > > On 2016/11/11 10:59:02, titzer wrote: > > > Can we introduce a struct for this type, instead of the std::tuple? > > > > +1 > > It will also help with describing what this complex structure means. > > As discussed offline with Ben: > Introducing a struct here requires us to either include the debug-interface.h in > several wasm headers where this information is produced and passed. > We therefore leave it a tuple for now. We already include debug-interface.h in debug.h and I think it's ok to include this file in all places where parts of interface implementation are defined. Tuple requires looking up what each field actually means each time when you see them in codebase.
On 2016/11/16 16:13:41, kozyatinskiy wrote: > https://codereview.chromium.org/2493773003/diff/60001/src/debug/debug-interfa... > File src/debug/debug-interface.h (right): > > https://codereview.chromium.org/2493773003/diff/60001/src/debug/debug-interfa... > src/debug/debug-interface.h:206: static std::pair<std::string, > std::vector<std::tuple<uint32_t, int, int>>> > On 2016/11/16 11:36:28, Clemens Hammacher wrote: > > On 2016/11/11 at 21:50:34, dgozman wrote: > > > On 2016/11/11 10:59:02, titzer wrote: > > > > Can we introduce a struct for this type, instead of the std::tuple? > > > > > > +1 > > > It will also help with describing what this complex structure means. > > > > As discussed offline with Ben: > > Introducing a struct here requires us to either include the debug-interface.h > in > > several wasm headers where this information is produced and passed. > > We therefore leave it a tuple for now. > > We already include debug-interface.h in debug.h and I think it's ok to include > this file in all places where parts of interface implementation are defined. > Tuple requires looking up what each field actually means each time when you see > them in codebase. Well I suggested the struct in the first place, but include dependencies changed my mind :-) But if this CL is OK otherwise, is it alright to do that in a followup CL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
As I understand provisional breakpoints support will be added later? (set breakpoint and then after page reload this breakpoint should work). I general: lgtm. Please wait for Dmitry review. https://codereview.chromium.org/2493773003/diff/120001/src/inspector/v8-debug... File src/inspector/v8-debugger-agent-impl.h (right): https://codereview.chromium.org/2493773003/diff/120001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.h:146: void newWasmScript(v8::Local<v8::Object> scriptWrapper); I prefer to expose WasmTranslation* wasmTranslation() { return &m_wasnTranslation; } and use it directly. https://codereview.chromium.org/2493773003/diff/120001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.h:225: friend class WasmTranslation; Why WasmTranslation is friend? It looks like WasmTranslation uses only public didParseSource method. https://codereview.chromium.org/2493773003/diff/120001/src/js/messages.js File src/js/messages.js (right): https://codereview.chromium.org/2493773003/diff/120001/src/js/messages.js#new... src/js/messages.js:68: function ScriptIsWasm() { We prefer to use debug-interface.h instead of introducing new mirror API.
The CQ bit was checked by clemensh@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...
https://codereview.chromium.org/2493773003/diff/120001/src/inspector/v8-debug... File src/inspector/v8-debugger-agent-impl.h (right): https://codereview.chromium.org/2493773003/diff/120001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.h:146: void newWasmScript(v8::Local<v8::Object> scriptWrapper); On 2016/11/16 at 16:44:53, kozyatinskiy wrote: > I prefer to expose WasmTranslation* wasmTranslation() { return &m_wasnTranslation; } and use it directly. Done. https://codereview.chromium.org/2493773003/diff/120001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.h:225: friend class WasmTranslation; On 2016/11/16 at 16:44:53, kozyatinskiy wrote: > Why WasmTranslation is friend? It looks like WasmTranslation uses only public didParseSource method. Good catch! It's not needed any more, I removed it. https://codereview.chromium.org/2493773003/diff/120001/src/js/messages.js File src/js/messages.js (right): https://codereview.chromium.org/2493773003/diff/120001/src/js/messages.js#new... src/js/messages.js:68: function ScriptIsWasm() { On 2016/11/16 at 16:44:53, kozyatinskiy wrote: > We prefer to use debug-interface.h instead of introducing new mirror API. This is script wrapper, but it will also go away soon. However, Yang approved this change, we will come up with a different implementation once this one is removed.
On 2016/11/16 at 16:44:53, kozyatinskiy wrote: > As I understand provisional breakpoints support will be added later? (set breakpoint and then after page reload this breakpoint should work). Yes, this is not about breakpoint support yet. Just showing meaningful stack traces and providing some kind of "source view" for wasm frames.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2493773003/diff/140001/src/debug/debug-interf... File src/debug/debug-interface.h (right): https://codereview.chromium.org/2493773003/diff/140001/src/debug/debug-interf... src/debug/debug-interface.h:206: static std::pair<std::string, std::vector<std::tuple<uint32_t, int, int>>> Let's have a comment describing this structure at least. https://codereview.chromium.org/2493773003/diff/140001/src/inspector/v8-debug... File src/inspector/v8-debugger-agent-impl.cc (right): https://codereview.chromium.org/2493773003/diff/140001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.cc:539: TranslateBackWasmScriptBreakpoint(breakpoint, &m_wasmTranslation); Why do we always do this? Let's check whether the script is wasm before calling into translation. https://codereview.chromium.org/2493773003/diff/140001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.cc:1045: TranslateWasmStackTraceLocations(result->get(), &m_wasmTranslation); Why do we need this? I thought code in V8StackTraceImpl takes care of this. https://codereview.chromium.org/2493773003/diff/140001/src/inspector/v8-debug... File src/inspector/v8-debugger-agent-impl.h (right): https://codereview.chromium.org/2493773003/diff/140001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.h:148: WasmTranslation* wasmTranslation() { return &m_wasmTranslation; } WasmTranslation should be a part of V8Debugger class, so that multiple debugger agents could translate same scripts into same fake scripts. https://codereview.chromium.org/2493773003/diff/140001/src/inspector/v8-debug... File src/inspector/v8-debugger.cc (right): https://codereview.chromium.org/2493773003/diff/140001/src/inspector/v8-debug... src/inspector/v8-debugger.cc:594: agent->newWasmScript(scriptWrapper.As<v8::Object>()); I think we should use v8::DebugInterface::Script instead of v8::Object. The only place to use objects coming from debugger context should be here just to call the Script::Wrap() method, and Alexey will remove that soon (switching to pure C++ api). https://codereview.chromium.org/2493773003/diff/140001/src/inspector/v8-stack... File src/inspector/v8-stack-trace-impl.cc (right): https://codereview.chromium.org/2493773003/diff/140001/src/inspector/v8-stack... src/inspector/v8-stack-trace-impl.cc:43: agent->wasmTranslation()->TranslateProtocolLocationToWasmScriptLocation( This is a wrong place for translation - it could be executed when debugger is not enabled yet (e.g. when exception occurs, and we open DevTools later). Instead, we should translate directly before sending this over protocol.
The CQ bit was checked by clemensh@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...
PTAL https://codereview.chromium.org/2493773003/diff/140001/src/debug/debug-interf... File src/debug/debug-interface.h (right): https://codereview.chromium.org/2493773003/diff/140001/src/debug/debug-interf... src/debug/debug-interface.h:206: static std::pair<std::string, std::vector<std::tuple<uint32_t, int, int>>> On 2016/11/16 at 19:00:09, dgozman wrote: > Let's have a comment describing this structure at least. Ups, I thought I did this, but it was only on the v8/wasm side. Done. https://codereview.chromium.org/2493773003/diff/140001/src/inspector/v8-debug... File src/inspector/v8-debugger-agent-impl.cc (right): https://codereview.chromium.org/2493773003/diff/140001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.cc:539: TranslateBackWasmScriptBreakpoint(breakpoint, &m_wasmTranslation); On 2016/11/16 at 19:00:09, dgozman wrote: > Why do we always do this? Let's check whether the script is wasm before calling into translation. The translation will only translate something for scripts that were previously added via AddScript, which is called by the debugger agend on newWasmScript. In order to additionally ensure that nothing goes wrong, I now only call to the translation if the script id is not in the m_scripts of the debugger agent. https://codereview.chromium.org/2493773003/diff/140001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.cc:1045: TranslateWasmStackTraceLocations(result->get(), &m_wasmTranslation); On 2016/11/16 at 19:00:10, dgozman wrote: > Why do we need this? I thought code in V8StackTraceImpl takes care of this. V8StackTraceImpl is only used to transfer materialized stack traces via the protocol. Here, an array of protocol::Debugger::CallFrame is built directly, so we also have to translate here. This code here is used to get the live stack trace while paused. https://codereview.chromium.org/2493773003/diff/140001/src/inspector/v8-debug... File src/inspector/v8-debugger-agent-impl.h (right): https://codereview.chromium.org/2493773003/diff/140001/src/inspector/v8-debug... src/inspector/v8-debugger-agent-impl.h:148: WasmTranslation* wasmTranslation() { return &m_wasmTranslation; } On 2016/11/16 at 19:00:10, dgozman wrote: > WasmTranslation should be a part of V8Debugger class, so that multiple debugger agents could translate same scripts into same fake scripts. Done. https://codereview.chromium.org/2493773003/diff/140001/src/inspector/v8-debug... File src/inspector/v8-debugger.cc (right): https://codereview.chromium.org/2493773003/diff/140001/src/inspector/v8-debug... src/inspector/v8-debugger.cc:594: agent->newWasmScript(scriptWrapper.As<v8::Object>()); On 2016/11/16 at 19:00:10, dgozman wrote: > I think we should use v8::DebugInterface::Script instead of v8::Object. The only place to use objects coming from debugger context should be here just to call the Script::Wrap() method, and Alexey will remove that soon (switching to pure C++ api). Done. https://codereview.chromium.org/2493773003/diff/140001/src/inspector/v8-stack... File src/inspector/v8-stack-trace-impl.cc (right): https://codereview.chromium.org/2493773003/diff/140001/src/inspector/v8-stack... src/inspector/v8-stack-trace-impl.cc:43: agent->wasmTranslation()->TranslateProtocolLocationToWasmScriptLocation( On 2016/11/16 at 19:00:10, dgozman wrote: > This is a wrong place for translation - it could be executed when debugger is not enabled yet (e.g. when exception occurs, and we open DevTools later). Instead, we should translate directly before sending this over protocol. Not sure if this is fixed with the switch from debugger agent to debugger. At least I am not seeing a path which leads to V8StackTraceImpl::Create without a debugger being in place.
lgtm https://codereview.chromium.org/2493773003/diff/140001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/2493773003/diff/140001/src/api.cc#newcode9115 src/api.cc:9115: std::pair<std::string, std::vector<std::tuple<uint32_t, int, int>>> I have absolutely no idea what type I'm looking at. Are we really doing this because of include rules? Can't we define this as a struct in a src/wasm/, and include it in debug-interface.h? https://codereview.chromium.org/2493773003/diff/160001/src/inspector/v8-debug... File src/inspector/v8-debugger.cc (right): https://codereview.chromium.org/2493773003/diff/160001/src/inspector/v8-debug... src/inspector/v8-debugger.cc:585: // starts with "wasm://". Comment is outdated.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with a comment https://codereview.chromium.org/2493773003/diff/140001/src/inspector/v8-stack... File src/inspector/v8-stack-trace-impl.cc (right): https://codereview.chromium.org/2493773003/diff/140001/src/inspector/v8-stack... src/inspector/v8-stack-trace-impl.cc:43: agent->wasmTranslation()->TranslateProtocolLocationToWasmScriptLocation( On 2016/11/16 20:31:30, Clemens Hammacher wrote: > On 2016/11/16 at 19:00:10, dgozman wrote: > > This is a wrong place for translation - it could be executed when debugger is > not enabled yet (e.g. when exception occurs, and we open DevTools later). > Instead, we should translate directly before sending this over protocol. > > Not sure if this is fixed with the switch from debugger agent to debugger. At > least I am not seeing a path which leads to V8StackTraceImpl::Create without a > debugger being in place. We can translate here when debugger is disabled, and no translation would happen. Later on, with enabled debugging, incorrect stack trace would be sent to client. Let's just move this code to buildInspectorObjectImpl().
The CQ bit was checked by clemensh@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 checked by clemensh@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...
Patchset #10 (id:180001) has been deleted
Patchset #10 (id:200001) has been deleted
Dry run: Try jobs failed on following builders: 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_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_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/28731) v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/17904) v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...) v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/18016)
The CQ bit was checked by clemensh@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...
https://codereview.chromium.org/2493773003/diff/140001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/2493773003/diff/140001/src/api.cc#newcode9115 src/api.cc:9115: std::pair<std::string, std::vector<std::tuple<uint32_t, int, int>>> On 2016/11/16 at 20:41:12, Yang wrote: > I have absolutely no idea what type I'm looking at. Are we really doing this because of include rules? Can't we define this as a struct in a src/wasm/, and include it in debug-interface.h? We can either do this, or clean it up a bit by typedefs/using-declarations in a follow-up commit. https://codereview.chromium.org/2493773003/diff/140001/src/inspector/v8-stack... File src/inspector/v8-stack-trace-impl.cc (right): https://codereview.chromium.org/2493773003/diff/140001/src/inspector/v8-stack... src/inspector/v8-stack-trace-impl.cc:43: agent->wasmTranslation()->TranslateProtocolLocationToWasmScriptLocation( On 2016/11/16 at 22:01:27, dgozman wrote: > On 2016/11/16 20:31:30, Clemens Hammacher wrote: > > On 2016/11/16 at 19:00:10, dgozman wrote: > > > This is a wrong place for translation - it could be executed when debugger is > > not enabled yet (e.g. when exception occurs, and we open DevTools later). > > Instead, we should translate directly before sending this over protocol. > > > > Not sure if this is fixed with the switch from debugger agent to debugger. At > > least I am not seeing a path which leads to V8StackTraceImpl::Create without a > > debugger being in place. > > We can translate here when debugger is disabled, and no translation would happen. Later on, with enabled debugging, incorrect stack trace would be sent to client. Let's just move this code to buildInspectorObjectImpl(). As discussed in chat: We postpone this, as it seems to require larger changes in the API. https://codereview.chromium.org/2493773003/diff/160001/src/inspector/v8-debug... File src/inspector/v8-debugger.cc (right): https://codereview.chromium.org/2493773003/diff/160001/src/inspector/v8-debug... src/inspector/v8-debugger.cc:585: // starts with "wasm://". On 2016/11/16 at 20:41:12, Yang wrote: > Comment is outdated. Fixed.
If the last patch set looks good to the bots, I will land it then. It seems that the general concepts are agreed upon. There are a lot of last-minute changes, which I consider TBR by the respective owners. Follow-up commits might be needed to clean up a few things. Thank you all for your outstanding cooperation and support!
Description was changed from ========== [inspector] Introduce translation of wasm frames This allows to show wasm source (disassembled wasm code) in DevTools. See design doc for details. More tests for the disassembly will have to follow. Also, the text format (generated by V8) will be changed. BUG=chromium:659715 R=yangguo@chromium.org, kozyatinskiy@chromium.org, titzer@chromium.org ========== to ========== [inspector] Introduce translation of wasm frames This allows to show wasm source (disassembled wasm code) in DevTools. See design doc for details. More tests for the disassembly will have to follow. Also, the text format (generated by V8) will be changed. BUG=chromium:659715 R=yangguo@chromium.org, kozyatinskiy@chromium.org, titzer@chromium.org, dgozman@chromium.org ==========
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 clemensh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org, kozyatinskiy@chromium.org, dgozman@chromium.org, yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/2493773003/#ps220001 (title: "Very last-minute changes")
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 #10 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== [inspector] Introduce translation of wasm frames This allows to show wasm source (disassembled wasm code) in DevTools. See design doc for details. More tests for the disassembly will have to follow. Also, the text format (generated by V8) will be changed. BUG=chromium:659715 R=yangguo@chromium.org, kozyatinskiy@chromium.org, titzer@chromium.org, dgozman@chromium.org ========== to ========== [inspector] Introduce translation of wasm frames This allows to show wasm source (disassembled wasm code) in DevTools. See design doc for details. More tests for the disassembly will have to follow. Also, the text format (generated by V8) will be changed. BUG=chromium:659715 R=yangguo@chromium.org, kozyatinskiy@chromium.org, titzer@chromium.org, dgozman@chromium.org Committed: https://crrev.com/d4a42a5f89b923a1f8cf0b09bbbd3f4a83049c5e Cr-Commit-Position: refs/heads/master@{#41055} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/d4a42a5f89b923a1f8cf0b09bbbd3f4a83049c5e Cr-Commit-Position: refs/heads/master@{#41055} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
