Skip to content

fix: Backport heroic fixes made in #1559 #1568

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 8, 2020
Merged

fix: Backport heroic fixes made in #1559 #1568

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion cli/shim/process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ module.exports = {
umask() {
return 0;
},
hrtime
hrtime,
argv: []
};

// https://github.com/kumavis/browser-process-hrtime v1.0.0
Expand Down
251 changes: 149 additions & 102 deletions src/builtins.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,6 @@ import {
getConstValueI32,
getConstValueF32,
getConstValueF64,
Relooper,
RelooperBlockRef,
SIMDLoadOp,
getLocalGetIndex,
createType,
Expand All @@ -82,7 +80,6 @@ import {
Field,
Global,
DecoratorFlags,
Element,
ClassPrototype,
Class
} from "./program";
Expand Down Expand Up @@ -8691,134 +8688,184 @@ export function compileVisitGlobals(compiler: Compiler): void {
);
}

/** 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 {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

assert(instance.type.isManaged);
if (instance.visitRef) return;

var program = compiler.program;
var module = compiler.module;
var usizeType = program.options.usizeType;
var nativeSizeType = usizeType.toNativeType();
var nativeSizeSize = usizeType.byteSize;
var managedClasses = program.managedClasses;
var visitInstance = assert(program.visitInstance);
var blocks = new Array<RelooperBlockRef>();
var relooper = Relooper.create(module);

// this function is @lazy: make sure it exists
compiler.compileFunction(visitInstance, true);

var outer = relooper.addBlockWithSwitch(
module.nop(),
module.load(nativeSizeSize, false,
nativeSizeType == NativeType.I64
? module.binary(BinaryOp.SubI64,
module.local_get(0, nativeSizeType),
module.i64(8)
)
: module.binary(BinaryOp.SubI32,
module.local_get(0, nativeSizeType),
module.i32(8) // rtId is at -8
),
NativeType.I32,
0
)
);

var lastId = 0;
// TODO: for (let [instanceId, instance] of managedClasses) {
for (let _keys = Map_keys(managedClasses), i = 0, k = _keys.length; i < k; ++i) {
let instanceId = _keys[i];
let instance = assert(managedClasses.get(instanceId));
assert(instance.type.isManaged);
assert(instanceId == lastId++);

let visitImpl: Element | null;
let code = new Array<ExpressionRef>();
var body = new Array<ExpressionRef>();

// If the class has a base class, call its visitor first
var base = instance.base;
if (base) {
body.push(
module.call(base.internalName + "~visit", [
module.local_get(0, nativeSizeType), // this
module.local_get(1, NativeType.I32) // cookie
], NativeType.None)
);
}

// if a library element, check if it implements a custom traversal function
if (instance.isDeclaredInLibrary && (visitImpl = instance.lookupInSelf("__visit_impl")) !== null) {
assert(visitImpl.kind == ElementKind.FUNCTION_PROTOTYPE);
let visitFunc = program.resolver.resolveFunction(<FunctionPrototype>visitImpl, null);
if (!visitFunc || !compiler.compileFunction(visitFunc)) {
code.push(
// Some standard library components provide a custom visitor implementation,
// for example to visit all members of a collection, e.g. arrays and maps.
var hasVisitImpl = false;
if (instance.isDeclaredInLibrary) {
let visitPrototype = instance.lookupInSelf("__visit");
if (visitPrototype) {
assert(visitPrototype.kind == ElementKind.FUNCTION_PROTOTYPE);
let visitInstance = program.resolver.resolveFunction(<FunctionPrototype>visitPrototype, null);
if (!visitInstance || !compiler.compileFunction(visitInstance)) {
body.push(
module.unreachable()
);
} else {
let visitSig = visitFunc.signature;
let visitThisType = assert(visitSig.thisType);
let visitSignature = visitInstance.signature;
let visitThisType = assert(visitSignature.thisType);
assert(
visitSig.parameterTypes.length == 1 &&
visitSig.parameterTypes[0] == Type.u32 &&
visitSig.returnType == Type.void &&
visitSignature.parameterTypes.length == 1 &&
visitSignature.parameterTypes[0] == Type.u32 &&
visitSignature.returnType == Type.void &&
instance.type.isStrictlyAssignableTo(visitThisType) // incl. implemented on super
);
code.push(
module.call(visitFunc.internalName, [
module.local_get(0, nativeSizeType), // ref
body.push(
module.call(visitInstance.internalName, [
module.local_get(0, nativeSizeType), // this
module.local_get(1, NativeType.I32) // cookie
], NativeType.None)
);
}
hasVisitImpl = true;
}
}

// otherwise generate traversal logic for own fields
} else {
let members = instance.members;
if (members) {
// TODO: for (let member of members.values()) {
for (let _values = Map_values(members), j = 0, l = _values.length; j < l; ++j) {
let member = unchecked(_values[j]);
if (member.kind == ElementKind.FIELD) {
if ((<Field>member).parent === instance) {
let fieldType = (<Field>member).type;
if (fieldType.isManaged) {
let fieldOffset = (<Field>member).memoryOffset;
assert(fieldOffset >= 0);
code.push(
// if ($2 = value) FIELDCLASS~traverse($2)
module.if(
module.local_tee(2,
module.load(nativeSizeSize, false,
module.local_get(0, nativeSizeType),
nativeSizeType, fieldOffset
)
),
module.call(visitInstance.internalName, [
module.local_get(2, nativeSizeType), // ref
module.local_get(1, NativeType.I32) // cookie
], NativeType.None)
)
);
}
// Otherwise, if there is no custom visitor, generate a visitor function
// according to class layout, visiting all _own_ managed members.
var needsTempValue = false;
if (!hasVisitImpl) {
let members = instance.members;
if (members) {
// TODO: for (let member of members.values()) {
for (let _values = Map_values(members), j = 0, l = _values.length; j < l; ++j) {
let member = unchecked(_values[j]);
if (member.kind == ElementKind.FIELD) {
if ((<Field>member).parent === instance) {
let fieldType = (<Field>member).type;
if (fieldType.isManaged) {
let fieldOffset = (<Field>member).memoryOffset;
assert(fieldOffset >= 0);
needsTempValue = true;
body.push(
// if ($2 = value) __visit($2, $1)
module.if(
module.local_tee(2,
module.load(nativeSizeSize, false,
module.local_get(0, nativeSizeType),
nativeSizeType, fieldOffset
)
),
module.call(visitInstance.internalName, [
module.local_get(2, nativeSizeType), // value
module.local_get(1, NativeType.I32) // cookie
], NativeType.None)
)
);
}
}
}
}
}
if (!instance.base) code.push(module.return());
let block = relooper.addBlock(
module.flatten(code)
);
relooper.addBranchForSwitch(outer, block, [ instanceId ]);
blocks.push(block);
}
// TODO: for (let [instanceId, instance] of managedClasses) {

// Create the visitor function
instance.visitRef = module.addFunction(instance.internalName + "~visit",
createType([nativeSizeType, NativeType.I32]),
NativeType.None,
needsTempValue ? [ nativeSizeType ] : null,
module.flatten(body, NativeType.None)
);

// And make sure the base visitor function exists
if (base) ensureVisitMembersOf(compiler, base);
}

/** Compiles the `__visit_members` function. */
export function compileVisitMembers(compiler: Compiler): void {
var program = compiler.program;
var module = compiler.module;
var usizeType = program.options.usizeType;
var nativeSizeType = usizeType.toNativeType();
var managedClasses = program.managedClasses;
var visitInstance = assert(program.visitInstance);
compiler.compileFunction(visitInstance, true); // is lazy, make sure it is compiled

// Prepare a mapping of class names to visitor calls. Each name corresponds to
// the respective sequential (0..N) class id.
var names = new Array<string>();
var cases = new Array<ExpressionRef>();
var nextId = 0;
for (let _keys = Map_keys(managedClasses), i = 0, k = _keys.length; i < k; ++i) {
let instanceId = unchecked(_keys[i]);
let instanceId = _keys[i];
assert(instanceId == nextId++);
let instance = assert(managedClasses.get(instanceId));
let base = instance.base;
if (base) relooper.addBranch(blocks[instanceId], blocks[base.id]);
}
blocks.push(
relooper.addBlock(
module.unreachable()
names[i] = instance.internalName;
cases[i] = module.block(null, [
module.call(instance.internalName + "~visit", [
module.local_get(0, nativeSizeType), // this
module.local_get(1, NativeType.I32) // cookie
], NativeType.None),
module.return()
], NativeType.None);
ensureVisitMembersOf(compiler, instance);
}

// Make a br_table of the mapping, calling visitor functions by unique class id
var current = module.block(names[0], [
module.switch(names, "invalid",
// load<u32>(changetype<usize>(this) - 8)
module.load(4, false,
nativeSizeType == NativeType.I64
? module.binary(BinaryOp.SubI64,
module.local_get(0, nativeSizeType),
module.i64(8)
)
: module.binary(BinaryOp.SubI32,
module.local_get(0, nativeSizeType),
module.i32(8) // rtId is at -8
),
NativeType.I32, 0
)
)
);
relooper.addBranchForSwitch(outer, blocks[blocks.length - 1], []); // default
compiler.compileFunction(visitInstance);
], NativeType.None);

// Wrap blocks in order
for (let i = 0, k = names.length - 1; i < k; ++i) {
current = module.block(names[i + 1], [
current,
cases[i]
], NativeType.None);
}

// Wrap the last id in an 'invalid' block to break out of on invalid ids
current = module.block("invalid", [
current,
cases[names.length - 1]
], NativeType.None);

// Add the function, executing an unreachable if breaking to 'invalid'
module.addFunction(BuiltinNames.visit_members,
createType([ usizeType.toNativeType(), NativeType.I32 ]), // ref, cookie
createType([ nativeSizeType, NativeType.I32 ]), // this, cookie
NativeType.None, // => void
[ nativeSizeType ],
relooper.renderAndDispose(outer, 2)
null,
module.flatten([
current,
module.unreachable()
])
);
}

Expand Down
6 changes: 5 additions & 1 deletion src/compiler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10508,8 +10508,12 @@ export class Compiler extends DiagnosticEmitter {
}
}
var parameterTypes = signature.parameterTypes;
var parameterNodes = reportNode.parameters;
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];
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

else parameterReportNode = reportNode;
if (!this.checkTypeSupported(parameterTypes[i], parameterReportNode)) {
supported = false;
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/flow.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -612,7 +612,7 @@ export class Flow {
/** Tests if the specified `this` field has the specified flag or flags. */
isThisFieldFlag(field: Field, flag: FieldFlags): bool {
var fieldFlags = this.thisFieldFlags;
if (fieldFlags) {
if (fieldFlags != null && fieldFlags.has(field)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return (changetype<FieldFlags>(fieldFlags.get(field)) & flag) == flag;
}
return false;
Expand Down
2 changes: 1 addition & 1 deletion src/glue/wasm/i64.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ function i64_is<T>(value: T): bool {
// @ts-ignore: decorator
@global @inline
function i64_new(lo: i32, hi: i32 = 0): i64 {
return lo | (hi << 32);
return <i64><u32>lo | (<i64>hi << 32);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// @ts-ignore: decorator
Expand Down
7 changes: 4 additions & 3 deletions src/program.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4117,6 +4117,8 @@ export class Class extends TypedElement {
implementers: Set<Class> | null = null;
/** Whether the field initialization check has already been performed. */
didCheckFieldInitialization: bool = false;
/** Runtime visitor function reference. */
visitRef: FunctionRef = 0;

/** Gets the unique runtime id of this class. */
get id(): u32 {
Expand Down Expand Up @@ -4273,9 +4275,8 @@ export class Class extends TypedElement {
var instance: Class | null = this;
do {
let overloads = instance.overloads;
if (overloads) {
let overload = overloads.get(kind);
if (overload) return overload;
if (overloads != null && overloads.has(kind)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another fix within the Map#has before Map#get wasnt-even-conceived-by-someone category. No error in JS, entirely justified fireworks in Wasm.

return assert(overloads.get(kind));
}
instance = instance.base;
} while (instance);
Expand Down
2 changes: 1 addition & 1 deletion std/assembly/array.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,7 @@ export class Array<T> {

// RT integration

@unsafe private __visit_impl(cookie: u32): void {
@unsafe private __visit(cookie: u32): void {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfection is achieved not when there is nothing more to add, but when there is nothing left to take away.

if (isManaged<T>()) {
let cur = this.dataStart;
let end = cur + (<usize>this.length_ << alignof<T>());
Expand Down
2 changes: 1 addition & 1 deletion std/assembly/function.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ type auto = i32;

// RT integration

@unsafe private __visit_impl(cookie: u32): void {
@unsafe private __visit(cookie: u32): void {
// Env is either `null` (nop) or compiler-generated
__visit(this._env, cookie);
}
Expand Down
2 changes: 1 addition & 1 deletion std/assembly/map.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ export class Map<K,V> {

// RT integration

@unsafe private __visit_impl(cookie: u32): void {
@unsafe private __visit(cookie: u32): void {
__visit(changetype<usize>(this.buckets), cookie);
var entries = changetype<usize>(this.entries);
if (isManaged<K>() || isManaged<V>()) {
Expand Down
2 changes: 1 addition & 1 deletion std/assembly/set.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ export class Set<T> {

// RT integration

@unsafe private __visit_impl(cookie: u32): void {
@unsafe private __visit(cookie: u32): void {
__visit(changetype<usize>(this.buckets), cookie);
var entries = changetype<usize>(this.entries);
if (isManaged<T>()) {
Expand Down
Loading