fix: Backport heroic fixes made in #1559 - #1568
Conversation
| /** Compiles the `visit_members` function. */ | ||
| export function compileVisitMembers(compiler: Compiler): void { | ||
| /** Ensures that the visitor function of the specified class is compiled. */ | ||
| function ensureVisitMembersOf(compiler: Compiler, instance: Class): void { |
There was a problem hiding this comment.
A new runtime visitor implementation that generates per-class functions instead of a huge unwieldy switch. Increases binary size somewhat, but is much easier to reason about.
| for (let i = 0, k = parameterTypes.length; i < k; ++i) { | ||
| if (!this.checkTypeSupported(parameterTypes[i], reportNode.parameters[i])) { | ||
| let parameterReportNode: Node; | ||
| if (parameterNodes.length > i) parameterReportNode = parameterNodes[i]; |
There was a problem hiding this comment.
Here, the function may come from a function expression with an omitted argument being compiled in context of a function type with fewer argument expressions, implicitly dropping excess arguments due to being irrelevant. Doesn't error when compiling to JS due to undefined, but produced a fine runtime index-out-of-bounds in Wasm.
| isThisFieldFlag(field: Field, flag: FieldFlags): bool { | ||
| var fieldFlags = this.thisFieldFlags; | ||
| if (fieldFlags) { | ||
| if (fieldFlags != null && fieldFlags.has(field)) { |
There was a problem hiding this comment.
This is old code apparently, written before the requirement for a Map#has before a Map#get was conceived. Produced a wholesome key-does-not-exist error in Wasm.
| @global @inline | ||
| function i64_new(lo: i32, hi: i32 = 0): i64 { | ||
| return lo | (hi << 32); | ||
| return <i64><u32>lo | (<i64>hi << 32); |
There was a problem hiding this comment.
A spectacular piece of code that was completely wrong before, and looks absolutely bogus now. Apart from the missing cast to i64 to ensure proper bit size, the <i64><u32> is necessary so the compiler doesn't attempt to sign-extend due to the cast value being signed. Somewhat strange, and made me wonder whether we can do better, but without any deeper changes that's the awesome fix fixing things.
| if (overloads) { | ||
| let overload = overloads.get(kind); | ||
| if (overload) return overload; | ||
| if (overloads != null && overloads.has(kind)) { |
There was a problem hiding this comment.
Another fix within the Map#has before Map#get wasnt-even-conceived-by-someone category. No error in JS, entirely justified fireworks in Wasm.
| // RT integration | ||
|
|
||
| @unsafe private __visit_impl(cookie: u32): void { | ||
| @unsafe private __visit(cookie: u32): void { |
There was a problem hiding this comment.
Perfection is achieved not when there is nothing more to add, but when there is nothing left to take away.
|
Btw, in case you are disappointed, a more impressive fix was in #1567 already. Finding that without source maps support running Wasm in node was quite the achievement. Thanks to Max for helping me to figure this one out :) |
This PR backports the series of herculean fixes made in the tracing GC branch to master, in the hope that these also benefit ARC.