Closed Bug 1458029 Opened 6 years ago Closed 6 years ago

Baldr: update Error.stack to match spec display conventions

Categories

(Core :: JavaScript Engine, enhancement, P2)

enhancement

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)

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)
Attached patch tweak-function-display-name (obsolete) — Splinter Review
Attachment #8972134 - Flags: review?(bbouvier)
Attached patch update-urlSplinter Review
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)
Attached patch update-location (obsolete) — Splinter Review
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.
And this patch implements the changes to how function names (what's left of the @) are rendered.
Attachment #8972190 - Flags: review?(bbouvier)
Attached patch fix-testsSplinter Review
This patch has all the test changes required for the previous patches to pass.
Attachment #8972191 - Flags: review?(bbouvier)
Priority: -- → P2
Now with more rooting!
Attachment #8972134 - Attachment is obsolete: true
Attachment #8972134 - Flags: review?(bbouvier)
Attachment #8972348 - Flags: review?(bbouvier)
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 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+
Attachment #8972191 - Flags: review?(bbouvier) → review+
Comment on attachment 8972137 [details] [diff] [review]
update-url

Review of attachment 8972137 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good
Attachment #8972137 - Flags: review?(ydelendik) → review+
Attached patch update-locationSplinter Review
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 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+
(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.
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)
Blocks: 1460089
https://hg.mozilla.org/mozilla-central/rev/1791360323e9
https://hg.mozilla.org/mozilla-central/rev/fc3a8ce00c3f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: