Closed
Bug 1458029
Opened 7 years ago
Closed 7 years ago
Baldr: update Error.stack to match spec display conventions
Categories
(Core :: JavaScript Engine, enhancement, P2)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: luke, Assigned: luke)
References
(Blocks 1 open bug)
Details
Attachments
(5 files, 2 obsolete files)
(deleted),
patch
|
yury
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
fitzgen
:
review+
|
Details | Diff | Splinter Review |
Currently we'll display either:
wasm-function[func-index]@location:bytecode-in-base-10:1
or
funcName@location:bytecode-in-base-10:1
(depending on presence of the name section)
To match https://webassembly.github.io/spec/web-api/index.html#conventions, we should display:
moduleName.funcName@location:wasm-function[func-index]:bytecode-in-base-16
or
@wasm-function[func-index]:bytecode-in-base-16
(depending on presence of the name section)
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8972134 -
Flags: review?(bbouvier)
Assignee | ||
Comment 2•7 years ago
|
||
This patch makes two changes:
- updates the URL to have the "> WebAssembly.compile" (et al), as described in https://webassembly.github.io/spec/web-api/index.html#conventions which makes it clear that the JS file/line info are showing where WebAssembly.compile (et al) was called, just like with eval.
- use the Response URL (set by noteResponseURLs), instead of the above "who compile" location, when present. This folds the responseURLs.baseURL with filename, allowing that field to be removed, in which case it doesn't make sense to have the separate ResponseURLs struct. The filenameIsURL flag is added to maintain existing debugger URL behavior.
The patch also removes the dead 'column' field.
Attachment #8972137 -
Flags: review?(ydelendik)
Assignee | ||
Comment 3•7 years ago
|
||
Here's one way to change the line/column to be func-index/bytecode-offset. However, I'm wondering whether this is the best solution. Will investigate.
Assignee | ||
Comment 4•7 years ago
|
||
And this patch implements the changes to how function names (what's left of the @) are rendered.
Attachment #8972190 -
Flags: review?(bbouvier)
Assignee | ||
Comment 5•7 years ago
|
||
This patch has all the test changes required for the previous patches to pass.
Attachment #8972191 -
Flags: review?(bbouvier)
Updated•7 years ago
|
Priority: -- → P2
Assignee | ||
Comment 6•7 years ago
|
||
Now with more rooting!
Attachment #8972134 -
Attachment is obsolete: true
Attachment #8972134 -
Flags: review?(bbouvier)
Attachment #8972348 -
Flags: review?(bbouvier)
Comment 7•7 years ago
|
||
Comment on attachment 8972348 [details] [diff] [review]
tweak-function-display-name
Review of attachment 8972348 [details] [diff] [review]:
-----------------------------------------------------------------
Nice.
Attachment #8972348 -
Flags: review?(bbouvier) → review+
Comment 8•7 years ago
|
||
Comment on attachment 8972190 [details] [diff] [review]
update-function-names
Review of attachment 8972190 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good to me, thanks!
::: js/src/wasm/WasmCode.cpp
@@ +968,3 @@
> {
> + MOZ_RELEASE_ASSERT(maybeBytecode, "NameInBytecode requires preserved bytecode");
> + MOZ_RELEASE_ASSERT(name.offset + name.length <= maybeBytecode->length());
Could this overflow? Maybe also assert that name.length <= maybeBytecode->length() and use subtract in this assertion instead?
@@ +989,5 @@
> +bool
> +Metadata::getFuncName(NameContext ctx, const Bytes* maybeBytecode, uint32_t funcIndex,
> + UTF8Bytes* name) const
> +{
> + if (moduleName) {
nit: && moduleName->length != 0
Attachment #8972190 -
Flags: review?(bbouvier) → review+
Updated•7 years ago
|
Attachment #8972191 -
Flags: review?(bbouvier) → review+
Comment 9•7 years ago
|
||
Comment on attachment 8972137 [details] [diff] [review]
update-url
Review of attachment 8972137 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good
Updated•7 years ago
|
Attachment #8972137 -
Flags: review?(ydelendik) → review+
Assignee | ||
Comment 10•7 years ago
|
||
As discussed earlier, this minimal patch stuffs the func-index and bytecode-offset into the column and line, resp, setting the high bit of the uint32 column to indicate the frame is wasm.
Attachment #8972189 -
Attachment is obsolete: true
Attachment #8974074 -
Flags: review?(nfitzgerald)
Comment 11•7 years ago
|
||
Comment on attachment 8974074 [details] [diff] [review]
update-location
Review of attachment 8974074 [details] [diff] [review]:
-----------------------------------------------------------------
Great! Thanks for consolidating all that column++ gunk too :-/
Just a couple nitpicks below :)
::: js/src/vm/SavedFrame.h
@@ +50,5 @@
> JSPrincipals* getPrincipals();
> bool isSelfHosted(JSContext* cx);
> + bool isWasm();
> + uint32_t wasmFuncIndex();
> + uint32_t wasmBytecodeOffset();
These functions should have a comment noting that they are only valid to call when `isWasm()`.
::: js/src/vm/SavedStacks.cpp
@@ +1026,5 @@
> +{
> + if (frame->isWasm()) {
> + // According to the Developer-Facing Display Conventions in WebAssembly
> + // Web API spec, the "column" component of the location string triplet
> + // is really the bytecode offset, rendered in hex.
It would be nice for future readers if there was a link to the conventions here.
@@ +1080,2 @@
> && sb.append(':')
> + && FormatStackFrameColumn(cx, sb, frame)
Does V8 actually follow these conventions too? How much do we care that our V8-style stack formatting matches V8 exactly? We added this for spider node, so I suspect the answer is "we don't really care anymore" and that we could probably even remove this...
Attachment #8974074 -
Flags: review?(nfitzgerald) → review+
Assignee | ||
Comment 12•7 years ago
|
||
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #11)
> These functions should have a comment noting that they are only valid to
> call when `isWasm()`.
Done
> It would be nice for future readers if there was a link to the conventions
> here.
Done (putting the URL in WasmFrameIter::computeLine(), which describes the overall hack, and having the two SavedStacks.cpp comments point to computeLine() instead)
> Does V8 actually follow these conventions too? How much do we care that our
> V8-style stack formatting matches V8 exactly? We added this for spider node,
> so I suspect the answer is "we don't really care anymore" and that we could
> probably even remove this...
I think V8 will do this change before too long; I was just talking to some of the devs in the last toolchain meeting. Maybe we don't even care, as you say, but I'll leave that decision to a separate bug.
Comment 13•7 years ago
|
||
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1791360323e9
Baldr: tweak function display name interface (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc3a8ce00c3f
Baldr: update wasm frame stack format string to match WebAssembly Web API spec (r=yury,bbouvier,fitzgen)
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1791360323e9
https://hg.mozilla.org/mozilla-central/rev/fc3a8ce00c3f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in
before you can comment on or make changes to this bug.
Description
•