Remove wasm offset converter #24963
Remove wasm offset converter #24963
Conversation
|
Don't have time to do a thorough review right now, but looks like a nice cleanup! |
| var match; | ||
| if (match = /^\s+at .*\.wasm\.(.*) \(.*\)$/.exec(frame)) { | ||
| name = match[1]; | ||
| } else if (match = /^\s+at (.*) \(.*\)$/.exec(frame)) { |
There was a problem hiding this comment.
Previously we used two completely different mechanism for getting the functions name:
- For JS functions to match against the line in the backtrace.
- For native functions we rely on wasmOffsetConverter.
However as @RReverser pointed out in #24587, there is no need to do that now that the v8 bug has been fixed, and we can use the same approach for both native and JS functions.
There was a problem hiding this comment.
Hmm, ok.
I spent a little time trying to figure out what these two lines (249 and 251) accept and reject, and I think I get it now. Perhaps add some comments though? I mean something like saying on the first, "this is the pattern emitted by [..], which looks like [example]" etc.
sbc100
force-pushed
the
remove_offset_coverter
branch
3 times, most recently
from
806478e to
b3a1d6f
Compare
sbc100
force-pushed
the
remove_offset_coverter
branch
2 times, most recently
from
d6cfa63 to
ab6ab74
Compare
| if (!wasmOffsetConverter.isSameFunc(offset, normalized)) { | ||
| return null; | ||
| } | ||
| #endif |
There was a problem hiding this comment.
I'm not sure what this bit of code was supposed to be doing. It looks like its an early exit if the "normalized" is not in the same function as the original offset.
It was added in #8711, but the code also has to work in the absence of USE_OFFSET_CONVERTER.. so I'm tempted to say that if the tests pass we should just land this.
The v8 bug that made this necessary was fixed in 2019: https://crbug.com/v8/9172 From running some tests I confirmed the node v12 and newer work fine without the offset converter. Older node versions such as v10 will show a warning about the legacy backtrace format and will not see symbol names in their backtraces (when using ASAN etc). Fixes: emscripten-core#24587
sbc100
force-pushed
the
remove_offset_coverter
branch
from
ab6ab74 to
a895413
Compare
3 participants
The v8 bug that made this necessary was fixed in 2019: https://crbug.com/v8/9172
From running some tests I confirmed the node v12 and newer work fine without the offset converter. Older node versions such as v10 will show a warning about the legacy backtrace format and will not see symbol names in their backtraces (when using ASAN etc).
Fixes: #24587