Skip to content
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

Jump to bottom

fix #2730 compound assignment with post increment in lhs miscompiles #2774

Closed
wants to merge 1 commit into from
Closed

fix #2730 compound assignment with post increment in lhs miscompiles #2774

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/ast.ts
View file
Open in desktop
Expand Up @@ -1292,6 +1292,7 @@ export class ConstructorExpression extends IdentifierExpression {

/** Represents an element access expression, e.g., array access. */
export class ElementAccessExpression extends Expression {
private elementExpressionCompiledCache:Map<string, Expression>;
constructor(
/** Expression being accessed. */
public expression: Expression,
Expand All @@ -1301,6 +1302,19 @@ export class ElementAccessExpression extends Expression {
range: Range
) {
super(NodeKind.ElementAccess, range);
this.elementExpressionCompiledCache = new Map<string, Expression>();
}

public addCached(key: string, value:Expression): void {
this.elementExpressionCompiledCache.set(key, value);
}

public getElementExpression(key: string): Expression {
if(this.elementExpressionCompiledCache.has(key)){
return this.elementExpressionCompiledCache.get(key) as Expression;
}else{
return this.elementExpression;
}
}
}

Expand Down
77 changes: 49 additions & 28 deletions src/compiler.ts
View file
Open in desktop
Expand Up @@ -220,6 +220,7 @@ import {
liftRequiresExportRuntime,
lowerRequiresExportRuntime
} from "./bindings/js";
import { _BinaryenCallGetOperandAt, _BinaryenCallSetOperandAt } from "./glue/binaryen";

/** Features enabled by default. */
export const defaultFeatures = Feature.MutableGlobals
Expand Down Expand Up @@ -4716,6 +4717,11 @@ export class Compiler extends DiagnosticEmitter {
}
}
if (!compound) return expr;

let exprTempLocal = this.currentFlow.getTempLocal(this.currentType);
let calculationResRef = module.local_set(exprTempLocal.index, expr, false);

let valueExprRef = module.local_get(exprTempLocal.index, this.currentType.toRef());
let resolver = this.resolver;
let target = resolver.lookupExpression(left, this.currentFlow);
if (!target) return module.unreachable();
Expand All @@ -4728,15 +4734,18 @@ export class Compiler extends DiagnosticEmitter {
);
return module.unreachable();
}
return this.makeAssignment(

let assignmentExprRef = this.makeAssignment(
target,
expr,
valueExprRef,
this.currentType,
right,
resolver.currentThisExpression,
resolver.currentElementExpression,
contextualType != Type.void
);

return module.block(null, [calculationResRef, assignmentExprRef], this.currentType.toRef());
}

makeLt(leftExpr: ExpressionRef, rightExpr: ExpressionRef, type: Type): ExpressionRef {
Expand Down Expand Up @@ -6295,7 +6304,7 @@ export class Compiler extends DiagnosticEmitter {
}

// Inline if explicitly requested
if (instance.hasDecorator(DecoratorFlags.Inline) && (!instance.is(CommonFlags.Overridden) || reportNode.isAccessOnSuper)) {
if (instance.canInline() && (!instance.is(CommonFlags.Overridden) || reportNode.isAccessOnSuper)) {
assert(!instance.is(CommonFlags.Stub)); // doesn't make sense
let inlineStack = this.inlineStack;
if (inlineStack.includes(instance)) {
Expand Down Expand Up @@ -6757,7 +6766,7 @@ export class Compiler extends DiagnosticEmitter {
reportNode: Node,
immediatelyDropped: bool = false
): ExpressionRef {
if (instance.hasDecorator(DecoratorFlags.Inline)) {
if (instance.canInline()) {
if (!instance.is(CommonFlags.Overridden)) {
assert(!instance.is(CommonFlags.Stub)); // doesn't make sense
let inlineStack = this.inlineStack;
Expand Down Expand Up @@ -7022,9 +7031,27 @@ export class Compiler extends DiagnosticEmitter {
expression.range
);
}
return this.compileCallDirect(indexedGet, [
expression.elementExpression

let indexLocal = this.currentFlow.getTempLocal(Type.i32);
let indexExpression = expression.getElementExpression(this.currentFlow.targetFunction.internalName);

indexedGet.setNoInline();

let callExprRef = this.compileCallDirect(indexedGet, [
indexExpression
], expression, thisArg, constraints);

if(indexExpression === expression.elementExpression){
let indexType = indexedGet.signature.parameterTypes[0];
let indexExpressionRef = _BinaryenCallGetOperandAt(callExprRef, 1);
let teeExprRef = module.local_tee(indexLocal.index, indexExpressionRef, false, TypeRef.I32);
_BinaryenCallSetOperandAt(callExprRef, 1, teeExprRef);
let indexGetExpr = module.local_get(indexLocal.index, TypeRef.I32);

expression.addCached(this.currentFlow.targetFunction.internalName, Node.createCompiledExpression(indexGetExpr, indexType, expression.elementExpression.range));
}

return callExprRef;
}
}
this.error(
Expand Down Expand Up @@ -8727,7 +8754,7 @@ export class Compiler extends DiagnosticEmitter {
if (!classInstance) return module.unreachable();
if (contextualType == Type.void) constraints |= Constraints.WillDrop;
let ctor = this.ensureConstructor(classInstance, expression);
if (!ctor.hasDecorator(DecoratorFlags.Inline)) {
if (!ctor.canInline()) {
// Inlined ctors haven't been compiled yet and are checked upon inline
// compilation of their body instead.
this.checkFieldInitialization(classInstance, expression);
Expand All @@ -8747,7 +8774,7 @@ export class Compiler extends DiagnosticEmitter {
// shortcut if already compiled
if (instance.is(CommonFlags.Compiled)) return instance;
// do not attempt to compile if inlined anyway
if (!instance.hasDecorator(DecoratorFlags.Inline)) this.compileFunction(instance);
if (!instance.canInline()) this.compileFunction(instance);
} else {
// clone base constructor if a derived class. note that we cannot just
// call the base ctor since the derived class may have additional fields.
Expand Down Expand Up @@ -9355,37 +9382,31 @@ export class Compiler extends DiagnosticEmitter {
return module.unreachable();
}

// simplify if dropped anyway
if (!tempLocal) {
return this.makeAssignment(
target,
expr,
this.currentType,
expression.operand,
resolver.currentThisExpression,
resolver.currentElementExpression,
false
);
}
let exprTempLocal = this.currentFlow.getTempLocal(this.currentType);
let calculationResRef = module.local_set(exprTempLocal.index, expr, false);

let valueExprRef = module.local_get(exprTempLocal.index, this.currentType.toRef());

// otherwise use the temp. local for the intermediate value (always possibly overflows)
let setValue = this.makeAssignment(
target,
expr, // includes a tee of getValue to tempLocal
valueExprRef, // includes a tee of getValue to tempLocal
this.currentType,
expression.operand,
resolver.currentThisExpression,
resolver.currentElementExpression,
false
);

this.currentType = tempLocal.type;
let typeRef = tempLocal.type.toRef();
let blockExpressions = [calculationResRef, setValue,];

return module.block(null, [
setValue,
module.local_get(tempLocal.index, typeRef)
], typeRef); // result of 'x++' / 'x--' might overflow
// otherwise use the temp. local for the intermediate value (always possibly overflows)
if(tempLocal){
this.currentType = tempLocal.type;
let typeRef = tempLocal.type.toRef();
blockExpressions.push(module.local_get(tempLocal.index, typeRef));
}

return module.block(null, blockExpressions, this.currentType.toRef()); // result of 'x++' / 'x--' might overflow
}

private compileUnaryPrefixExpression(
Expand Down
10 changes: 10 additions & 0 deletions src/program.ts
View file
Open in desktop
Expand Up @@ -3766,6 +3766,8 @@ export class Function extends TypedElement {
memorySegment: MemorySegment | null = null;
/** Original function, if a stub. Otherwise `this`. */
original!: Function;
/** force the function to be compiled as no inline */
noInline: bool = false;

/** Counting id of inline operations involving this function. */
nextInlineId: i32 = 0;
Expand Down Expand Up @@ -3949,6 +3951,14 @@ export class Function extends TypedElement {
}
}
}

setNoInline(): void {
this.noInline = true;
}

canInline(): bool {
return this.hasDecorator(DecoratorFlags.Inline) && (!this.noInline);
}
}

/** A property comprised of a getter and a setter function. */
Expand Down
2 changes: 1 addition & 1 deletion src/resolver.ts
View file
Open in desktop
Expand Up @@ -1553,7 +1553,7 @@ export class Resolver extends DiagnosticEmitter {
let indexSignature = classReference.indexSignature;
if (indexSignature) {
this.currentThisExpression = targetExpression;
this.currentElementExpression = node.elementExpression;
this.currentElementExpression = node.getElementExpression(ctxFlow.targetFunction.internalName);
return indexSignature;
}
classReference = classReference.base;
Expand Down
22 changes: 20 additions & 2 deletions tests/compiler/NonNullable.debug.wat
View file
Open in desktop
Expand Up @@ -42,8 +42,14 @@
(local $ptr1 i32)
(local $ptr2 i32)
(local $7 i32)
(local $8 i32)
(local $9 i32)
(local $10 i32)
(local $11 i32)
(local $a i32)
(local $b i32)
(local $14 i32)
(local $15 i32)
local.get $str1
local.get $index1
i32.const 1
Expand Down Expand Up @@ -89,14 +95,20 @@
local.get $ptr1
i32.const 8
i32.add
local.set $7
local.get $7
local.set $ptr1
local.get $ptr2
i32.const 8
i32.add
local.set $8
local.get $8
local.set $ptr2
local.get $len
i32.const 4
i32.sub
local.set $9
local.get $9
local.set $len
local.get $len
i32.const 4
Expand All @@ -107,11 +119,13 @@
end
loop $while-continue|1
local.get $len
local.tee $7
local.tee $10
i32.const 1
i32.sub
local.set $11
local.get $11
local.set $len
local.get $7
local.get $10
if
local.get $ptr1
i32.load16_u $0
Expand All @@ -131,10 +145,14 @@
local.get $ptr1
i32.const 2
i32.add
local.set $14
local.get $14
local.set $ptr1
local.get $ptr2
i32.const 2
i32.add
local.set $15
local.get $15
local.set $ptr2
br $while-continue|1
end
Expand Down