Baldr: update to new name section format

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: luke, Assigned: mds)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Comment 1

2 years ago
Alon provided a test case:
  http://kripken.github.io/talks/demangle_stacks.html
which will currently print 'wasm-function[i]' but, once we have the fix, should print the names and we should confirm match Chrome's names (i.e., we're getting the indexes right).
(Reporter)

Comment 4

2 years ago
Comment on attachment 8861214 [details] [diff] [review]
WIP/Partial - New name section (function subsection only)

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

Good progress, thanks.  I guess the local section is coming next?  Also are you able to run the test in comment 1?

::: js/src/jit-test/lib/wasm-binary.js
@@ +30,5 @@
> +const newNameSectionName    = "name-utf8";
> +
> +// Name section name types
> +const nameTypeFunction = 1;
> +const nameTypeLocal    = 2;

Same comments as WasmBinaryConstants.h

::: js/src/jit-test/tests/wasm/binary.js
@@ +238,5 @@
> +    var index = 0;
> +    for (let n of elems) {
> +        var nenc = toU8(encodedString(n.name, n.nameLen));
> +        names.push(varU32(index));
> +        names.push(...nenc);

I think you can just write `names.push(...encodedString(n.name, n.nameLen))` (no need for intermediate typed array).

::: js/src/wasm/WasmBinaryConstants.h
@@ +27,5 @@
>  static const uint32_t MagicNumber        = 0x6d736100; // "\0asm"
>  static const uint32_t EncodingVersion    = 0x01;
>  
> +static const char LegacyNameSectionName[]= "name";
> +static const char NameSectionName[]      = "name-utf8";

The new names section is still called "name".  Since Chrome isn't either, let's not bother with trying to support the older name section and thus nix LegacyNameSectionName and support for the legacy names section in general.  That should simplify the patch when both the new and legacy name section have the same string name.

@@ +29,5 @@
>  
> +static const char LegacyNameSectionName[]= "name";
> +static const char NameSectionName[]      = "name-utf8";
> +
> +enum class NameSectionSubsection

nit: could you move this enum under SectionID?

::: js/src/wasm/WasmValidate.cpp
@@ +1563,5 @@
>  static void
>  MaybeDecodeNameSectionBody(Decoder& d, ModuleEnvironment* env)
>  {
> +    uint8_t subsectionType = UINT8_MAX;
> +    uint32_t name_payload_len = 0;

nit: SM style (https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style) is camelCase for locals.  Also, we try to push declarations down to right before their first use.  Also a few other variables names below

@@ +1585,5 @@
> +
> +    uint32_t name_payload_len_offset = d.currentOffset();
> +
> +    NameInBytecodeVector fncNames;
> +    if (!fncNames.resize(name_count))

nit: usual SM abbreviation for function is 'func'

@@ +1588,5 @@
> +    NameInBytecodeVector fncNames;
> +    if (!fncNames.resize(name_count))
> +        return;
> +
> +    for (uint32_t i = 0; i < name_count; ++i) {

It would be good to factor out this loop out for sharing with the local section.

@@ +1590,5 @@
> +        return;
> +
> +    for (uint32_t i = 0; i < name_count; ++i) {
> +        uint32_t funcidx = 0;
> +        if (!d.readVarU32(&funcidx))

nit: the canonical abbreviation we use for function index is 'funcIndex'

@@ +1594,5 @@
> +        if (!d.readVarU32(&funcidx))
> +            return;
> +
> +        uint32_t name_len = 0;
> +        if (!d.readVarU32(&name_len))

Can you check against MaxStringLength like the legacy name section does?

@@ +1602,5 @@
> +        func.offset = d.currentOffset();
> +        func.length = name_len;
> +
> +        const uint8_t* tbytes;
> +        if (!d.readBytes(name_len, &tbytes))

The second param has an optional second parameter, so you can just leave out tbytes here.

@@ +1608,5 @@
> +
> +        fncNames[funcidx] = func;
> +    }
> +
> +    MOZ_ASSERT(d.currentOffset() == name_payload_len + name_payload_len_offset, "Something has been missed?");

Since this is an untrusted binary, we can't assert this since we have to assume the binary is trash.  Rather, we should dynamically test this.
Attachment #8861214 - Attachment is obsolete: true
Comment hidden (mozreview-request)
As we're not using the locals anywhere the idea is to leave the code as simple as possible; that's why binary.js:466 has been scraped, along with the support for the old "legacy" name section.

Also, some of the identifiers in MaybeDecodeNameSectionBody are named like that to be as close as possible to the actual names in the spec.

And yes, the test in comment 1 looks good.:)
(Reporter)

Comment 7

2 years ago
mozreview-review
Comment on attachment 8862747 [details]
Bug 1357911 - [WASM] Update to new name section format.

https://reviewboard.mozilla.org/r/134612/#review143680

Since you're on PTO, I'll make changes and land.

Unfortunately, since the patch was written, a new Module name subsection was added which goes in front of the function subsection.  This requires a bit more disciplined approach to decoding subsections.

::: js/src/wasm/WasmValidate.cpp:1609
(Diff revision 1)
> +        func.length = nameLen;
> +
> +        if (!d.readBytes(nameLen))
> -                return;
> +            return;
> +
> +        funcNames[funcIndex] = func;

funcNames is resized above to nameCount but indexed here with funcIndex but funcIndex has no relation to nameCount.  Instead, funcNames must be lazily resized to grow on each funcIndex.  Also, function indices are supposed to be in ascending order in BinaryEncoding.md which should be checked.
Attachment #8862747 - Flags: review?(luke) → review+

Comment 8

2 years ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c67aff0a734d
Baldr: update names section format (r=luke)

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c67aff0a734d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1382040
Assignee: nobody → mdesimone
You need to log in before you can comment on or make changes to this bug.