1913111 - Verify that call stacks for inlined functions are correct

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, task, P3)

Description

[Ryan Hunt [:rhunt]]

We should double check that frame iteration and error stack reporting correctly reports the 'source level' call stack, not the 'inlined' call stack.

We can get an error stack with new Error().stack.

Comment 1

[Ryan Hunt [:rhunt]]

Inlined call stacks are indeed not correct with inlining.

Comment 2

[Ryan Hunt [:rhunt]]

Attachment: Bug 1913111 - mfbt: Add HashTable::swap. r?glandium

I'm replacing a usage of Vector with HashMap, and I need the
equivalent of Vector::swap for HashMap.

Comment 3

[Ryan Hunt [:rhunt]]

Attachment: Bug 1913111 - wasm: Add CallSites class for storing CallSite. r?jseward

CallSite currently is conceptually three fields:

1. 'Kind'
2. 'BytecodeOffset'
3. 'ReturnAddressOffset'

These are bitpacked into 8 bytes by combining the bytecode offset and
kind. However this bitpacking is not truly valid and our static
assert here is wrong. With very large modules (which are very difficult
to write tests of) we could overflow the bytecode offset limit.

In addition, for inlining we will need a CallSite to be able to represent
not just a single bytecode offset but a stack of bytecode offsets for all
of the 'inlined callers'.

Call sites are extremely common, so minimizing space usage is important.

This commit reworks the storage to fix both of these issues with several
changes:

1. Switch from using an 'array of structs' to 'struct of arrays' approach
   for CallSite storage. The new class is called 'CallSites'.
   • This allows us to replace the bitpacking with just storing 'Kind' in
     a separate vector. The overhead for each call site goes from 8 bytes to
     to 9 bytes. If we kept the array of structs, the overhead would have
     gone to 12 bytes due to padding.
2. Add a reference to a stack of bytecode offsets 'inlinedCallerOffsets'
   to CallSite.
   • This is created by RootCompiler which maintains a stack. The stack is
     shared for every call site within a function.
3. Add a sparse hashmap to CallSites that tracks the 'inlinedCallerOffsets'
   for each call site. This reduces storage because many call sites will
   not be in an inlined function. For baseline, none of them will be.

The 'inlinedCallerOffsets' data is not used yet. That will require changes
to WasmFrameIter.

Depends on D231455

Comment 4

[Ryan Hunt [:rhunt]]

Attachment: Bug 1913111 - wasm: Add test. r?jseward

Depends on D231456

Comment 5

[Ryan Hunt [:rhunt]]

Attachment: Bug 1913111 - wasm: Refactor WasmFrameIter. r?jseward

The state on WasmFrameIter is difficult to reason about.

• Group the state by 'kind' and note when they are
  safe to access.
• Add more assertions.
• Reset more state to default values in popFrame.
• Switch from "Mode" to a boolean flag. This helps
  when inlining is added as that's another independent
  mode.

Depends on D231457

Comment 6

[Ryan Hunt [:rhunt]]

Attachment: Bug 1913111 - wasm: Add mode to WasmFrameIter that visits inlined frames. r?jseward

Add a mode to WasmFrameIter that will read the CallSite data prepared
by the earlier patch and use it. This mode disables access to physical
frame data. This mode is used only by FrameIter which is used when
generating stack traces.

Depends on D231458

Comment 7

[Ryan Hunt [:rhunt]]

Attachment: Bug 1913111 - wasm: Refactor trap site storage. r?jseward

Similar to CallSites in the previous patch, move to a struct of arrays
approach for storing TrapSites. This will also make it easier to have
a sparse inlineCallerOffsets map (in the next patch).

Depends on D231459

Comment 8

[Ryan Hunt [:rhunt]]

Attachment: Bug 1913111 - wasm: Store inlined caller offset information on TrapSite. r?jseward

Add a sparse map of inlineCallerOffsets to TrapSites, similar to
CallSites. This map lives on the inner TrapSitesForKind class as
it is indexed by an entry in the vector.

A later patch will use this information and fill it in.

Depends on D231765

Comment 9

[Ryan Hunt [:rhunt]]

Attachment: Bug 1913111 - wasm: Use inlining information for traps in WasmFrameIter. r?jseward

Signal handling looks up a trap and gives that information to the
JitActivation using startWasmTrap. WasmFrameIter then reads that
information. We need to plumb the inlining information all the
way through that.

Depends on D231766

Comment 10

[Ryan Hunt [:rhunt]]

Attachment: Bug 1913111 - wasm: Report inlining context for traps in all codegen. r?jseward

Update all previous code which just passed around a BytecodeOffset when
identifying a trap to pass around a TrapSiteDesc instead. There are a lot
of users here, but they're all very mechanical.

Depends on D231767

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57d526dae6a5
mfbt: Add HashTable::swap. r=glandium
https://hg.mozilla.org/integration/autoland/rev/9c71b0d81cfc
wasm: Add CallSites class for storing CallSite. r=jseward
https://hg.mozilla.org/integration/autoland/rev/c58b73ad7046
wasm: Add test. r=jseward
https://hg.mozilla.org/integration/autoland/rev/8057555f112a
wasm: Refactor WasmFrameIter. r=jseward
https://hg.mozilla.org/integration/autoland/rev/2acdf2b0a944
wasm: Add mode to WasmFrameIter that visits inlined frames. r=jseward
https://hg.mozilla.org/integration/autoland/rev/ca09273b036e
wasm: Refactor trap site storage. r=jseward
https://hg.mozilla.org/integration/autoland/rev/5a3a452002e3
wasm: Store inlined caller offset information on TrapSite. r=jseward
https://hg.mozilla.org/integration/autoland/rev/91e4ff936bf3
wasm: Use inlining information for traps in WasmFrameIter. r=jseward
https://hg.mozilla.org/integration/autoland/rev/6c15f9170882
wasm: Report inlining context for traps in all codegen. r=jseward
https://hg.mozilla.org/integration/autoland/rev/0ca5c9901c10
apply code formatting via Lando

Comment 12

[Atila Butkovits]

Backed out for causing SM bustages.

Backout link: https://hg.mozilla.org/integration/autoland/rev/4dc4b3c1d36a39b285603238e0d622abe45e48da

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=0ca5c9901c105eeb1ebabe0a66e500f9460b5380&selectedTaskRun=ZLMog2KVT9OPzmNygZpUnw.0

Failure log:
https://treeherder.mozilla.org/logviewer?job_id=486895817&repo=autoland&lineNumber=20576
https://treeherder.mozilla.org/logviewer?job_id=486901971&repo=autoland&lineNumber=3702

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a51b5c91d67
mfbt: Add HashTable::swap. r=glandium
https://hg.mozilla.org/integration/autoland/rev/899a84b48df0
wasm: Add CallSites class for storing CallSite. r=jseward
https://hg.mozilla.org/integration/autoland/rev/bbaa4b84e861
wasm: Add test. r=jseward
https://hg.mozilla.org/integration/autoland/rev/db490f6445cf
wasm: Refactor WasmFrameIter. r=jseward
https://hg.mozilla.org/integration/autoland/rev/b52637fa0a96
wasm: Add mode to WasmFrameIter that visits inlined frames. r=jseward
https://hg.mozilla.org/integration/autoland/rev/8641cf19826f
wasm: Refactor trap site storage. r=jseward
https://hg.mozilla.org/integration/autoland/rev/cf1c7b54383d
wasm: Store inlined caller offset information on TrapSite. r=jseward
https://hg.mozilla.org/integration/autoland/rev/6046eda1f062
wasm: Use inlining information for traps in WasmFrameIter. r=jseward
https://hg.mozilla.org/integration/autoland/rev/7913ff2dd3b3
wasm: Report inlining context for traps in all codegen. r=jseward
https://hg.mozilla.org/integration/autoland/rev/f23c58fbde51
apply code formatting via Lando

Comment 14

[amarc]

https://hg.mozilla.org/mozilla-central/rev/0a51b5c91d67
https://hg.mozilla.org/mozilla-central/rev/899a84b48df0
https://hg.mozilla.org/mozilla-central/rev/bbaa4b84e861
https://hg.mozilla.org/mozilla-central/rev/db490f6445cf
https://hg.mozilla.org/mozilla-central/rev/b52637fa0a96
https://hg.mozilla.org/mozilla-central/rev/8641cf19826f
https://hg.mozilla.org/mozilla-central/rev/cf1c7b54383d
https://hg.mozilla.org/mozilla-central/rev/6046eda1f062
https://hg.mozilla.org/mozilla-central/rev/7913ff2dd3b3
https://hg.mozilla.org/mozilla-central/rev/f23c58fbde51